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
