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

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?

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.