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

On Tue, Aug 18, 2026 at 5:35 AM Akihiko Odaki
<[email protected]> wrote:
>
>
> A whitespace problem remains here.
>

Got it!  This is fixed now

> >   } IsolationModeCtx;
> >
> >   struct vhost_user {
> > @@ -1128,15 +1130,38 @@ static int vhost_user_set_mem_table_postcopy(struct vhost_dev *dev,
> >       return 0;
> >   }
> >
> > -static void cleanup_isolation_regions(struct vhost_dev *dev)
> > +static void vhost_user_svq_cleanup(struct vhost_user *u, bool reset)
> > +{
> > +    VhostShadowVirtqueue *svq;
> > +    for (int i = 0; i < u->iso_mem_ctx.shadow_vqs->len; i++) {
> > +        svq = g_ptr_array_index(u->iso_mem_ctx.shadow_vqs, i);
> > +        vhost_svq_stop(svq);
> > +        event_notifier_cleanup(&svq->hdev_call);
> > +        event_notifier_cleanup(&svq->hdev_kick);
> > +    }
> > +
> > +    if (!reset) {
> > +        g_ptr_array_free(u->iso_mem_ctx.shadow_vqs, true);
> > +    }
> > +}
> > +
> > +static void cleanup_isolation_regions(struct vhost_dev *dev, bool reset)
> >   {
> >       struct vhost_user *u = dev->opaque;
> >       if (u->iso_mem_ctx.shared_mem_addr) {
>
> If vhost_dev_init() fails after vhost_user_init_svq(), shadow_vqs is
> present but shared_mem_addr is not because init_isolation_regions() is
> not called yet. shadow_vqs will be leaked then.
>

Good catch!  The call to vhost_user_svq_cleanup will be moved out of
the conditional statement so it always runs during cleanup.  I also
realized that it is possible for an error to occur after the call to
qemu_memfd_alloc but before the iova tree is allocated, so I am adding
a check before calling vhost_iova_tree_delete.

> > +        vhost_user_svq_cleanup(u, reset);
> >           vhost_iova_tree_delete(u->iso_mem_ctx.tree);
> >           qemu_memfd_free(u->iso_mem_ctx.shared_mem_addr,
> >                           u->iso_mem_ctx.size,
> >                           u->iso_mem_ctx.fd);
> > +
> > +        GPtrArray *temp = u->iso_mem_ctx.shadow_vqs;
>
> This is a mixed declaration prohibited in docs/devel/style.rst

Got it!  I will fix this.

>
> >           memset(&u->iso_mem_ctx, 0, sizeof(IsolationModeCtx));
> > +
> > +        if (!reset) {
>
> This condition is inverted. When reset is true, vhost_user_svq_cleanup()
> retains the array so the pointer to it should be kept.
>

You are right!  This is now fixed

...

> >   static int vhost_user_set_vring_kick(struct vhost_dev *dev,
> >                                        struct vhost_vring_file *file)
> >   {
> > -    int ret = vhost_set_vring_file(dev, VHOST_USER_SET_VRING_KICK, file);
> > +    struct vhost_user *u = dev->opaque;
> > +    int svq_idx = file->index - dev->vq_index;
> > +    VhostShadowVirtqueue *svq = NULL;
> > +    struct vhost_vring_file vr_file = *file;
> > +    int ret;
> > +
> > +    vhost_user_get_vq_index(dev, file->index); /* bounds checking */
> > +
> > +    if (u->user->memory_isolation) {
> > +        svq = g_ptr_array_index(u->iso_mem_ctx.shadow_vqs, svq_idx);
> > +        vhost_svq_set_svq_kick_fd(svq, file->fd);
>
> The svq->svq_kick EventNotifier is left set in the error paths.
>

I did not want to close the svq_kick notifier, since it shares its fd
with the host notifier in the underlying vq, and closing that from
here might be unexpected.  However, a safer alternative is probably to
call vhost_svq_set_svq_kck_fd with -1 to simply unbind the notifier,
which I suspect is what you are actually recommending here.

> Regards,
> Akihiko Odaki
>
...

> > @@ -1769,7 +1835,35 @@ static int vhost_user_set_vring_kick(struct vhost_dev *dev,
> >   static int vhost_user_set_vring_call(struct vhost_dev *dev,
> >                                        struct vhost_vring_file *file)
> >   {
> > -    return vhost_set_vring_file(dev, VHOST_USER_SET_VRING_CALL, file);
> > +    struct vhost_user *u = dev->opaque;
> > +    int svq_idx = file->index - dev->vq_index;
> > +    VhostShadowVirtqueue *svq = NULL;
> > +    struct vhost_vring_file vr_file = *file;
> > +    int ret;
> > +
> > +    vhost_user_get_vq_index(dev, file->index); /* bounds checking */
> > +
> > +    if (u->user->memory_isolation) {
> > +        svq = g_ptr_array_index(u->iso_mem_ctx.shadow_vqs, svq_idx);
> > +        vhost_svq_set_svq_call_fd(svq, file->fd);
> > +
> > +        if (file->fd != -1) {
> > +            if (!svq->hdev_call.initialized) {
> > +                ret = event_notifier_init(&svq->hdev_call, 0);
> > +                if (ret < 0) {
> > +                    event_notifier_cleanup(&svq->hdev_call);
> > +                    error_report("Failed to create call event notifier");
> > +                    return ret;
> > +                }
> > +            }
> > +
> > +            vr_file.fd = event_notifier_get_fd(&svq->hdev_call);
> > +        } else {
> > +            event_notifier_cleanup(&svq->hdev_call);
> > +        }
> > +    }
> > +
> > +    return vhost_set_vring_file(dev, VHOST_USER_SET_VRING_CALL, &vr_file);
>
> Clean up svq->hdev_call when this fails.
>

Did you mean cleanup svq->svq_call, similar to your feedback on
svq->svq_kick?  I believe svq->hdev_call already gets cleaned up on
failure.  I'll go ahead and add a call to vhost_svq_set_svq_call_fd
with fd=-1 here as well.

Thank you!
Connor