Re: [PATCH RFC 13/15] hw/virtio/vhost-user: add shadow virtqueues and eventfd intercepts

On 11.08.26 04:32, Connor Kite wrote:
> On Mon, Aug 3, 2026 at 6:52 AM Hanna Czenczek <[email protected]> wrote:
>>> +    struct vhost_user *u = dev->opaque;
>>> +    int svq_idx = file->index - dev->vq_index;
>>> +    if (u->user->memory_isolation) {
>>> +        VhostShadowVirtqueue *svq = g_ptr_array_index(u->shadow_vqs,
>>> +                                                      svq_idx);
>> Bounds checking via `vhost_user_get_vq_index()` would be nice. (Same below.)
>>
> Adding the bounds checking outside of the memory-isolation only code path, since
> file->index wasn't being checked previously.
>
>>> +        if (svq->hdev_kick.initialized == false) {
>>> +            int r = event_notifier_init(&svq->hdev_kick, 0);
>>> +            if (r) {
>>> +                error_report("Failed to create kick event notifier");
>>> +                return r;
>>> +            }
>>> +        }
>>> +
>>> +        file->fd = event_notifier_get_fd(&svq->hdev_kick);
>> What if `file->fd` was -1, i.e. `VHOST_FILE_UNBIND`? Should we put a
>> real FD here then or just continue with -1?
>>
> Good point!  I think we need to propagate the -1 to the backend device, and
> run event_notifier_cleanup on the existing svq->hdev_kick.
>
> ...
>
>>> +
>>> +        if (svq->hdev_call.initialized == false) {
>>> +            int r = event_notifier_init(&svq->hdev_call, 0);
>> Do we need to check if the call event FD is already active?
>>
> Do you mean, do we need to call event_notifier_test_and_clear to see if it's
> been set and potentially run the call handler at this stage?

Yes, that is what I’m wondering. `vhost_svq_set_svq_kick_fd()` has this 
comment:

 > event_notifier_set_handler already checks for guest's notifications if
 > they arrive at the new file descriptor in the switch, so there is no
 > need to explicitly check for them.

So it sounds like there is some mechanism that will ensure that when the 
kick FD is set on a shadow virtqueue, it is checked for activity 
(notification), and if it is active, this is… handled? Which I suppose 
means the installed handler will be called.

But it’s entirely possible I misunderstand something because the same 
code in `vhost_svq_set_svq_kick_fd()` also just calls 
`event_notifier_set(svq_kick)`, so… kinda always emulates a kick, I assume?

In any case, there seems to be worry about pending checking kicks from 
the guest in this switch, and so I wonder if the same needs to be done 
for the call FD.

Hanna

> If so, this might be where some of the intricacies of when things happen in
> vhost-user vs in vhost would require some extra care, unless isolation mode
> implementation moved up into vhost.
>
> ...
>
>>> +    /*Modified from vhost-vdpa*/
>>> +    u->shadow_vqs = g_ptr_array_new_full(dev->nvqs, vhost_svq_free);
>>> +    for (int i = 0; i < dev->nvqs; i++) {
>>> +        VhostShadowVirtqueue *svq;
>>> +        svq = vhost_svq_new(NULL, NULL);
>> Patch 15 adds clean-up for this; I would squash it into here, as far as
>> possible. Ideally, a patch series can be stopped at any step and not
>> break anything, so if an allocation is added, the accompanying freeing
>> should come with it.
>>
>> Hanna
>>
> In hindsight I shouldn't have considered cleanup a separate stage in the
> development.  Too easy to introduce bugs that way.  I'll move cleanup up
> and comment in the Patch 15 thread.
>