Re: [PATCH RFC 14/15] hw/virtio/vhost-user: handle data movement with shadow vqs

On Fri, Jul 24, 2026 at 8:20 AM Akihiko Odaki
<[email protected]> wrote:
>
>
> This copies data in the reversed direction.
>

Good catch!  This will be fixed going forward.

...


> > +    DMAMap needle;
> > +    hwaddr *iova_base;
>
> Please make it void *.
>

Got it!

> > +
> > +    for (int i = 0; i < elem->out_num; i++) {
> > +        needle.translated_addr = elem->out_addr[i];
> > +        needle.size = elem->out_sg[i].iov_len - 1;
> > +        map = vhost_iova_tree_find_gpa(svq->iova_tree, &needle);
>
> An error check is missing here. This translation does not seem to
> support IOMMU.
>

I have added in the error check.

Looking at vhost-vdpa, there is an
error when IOMMU is enabled at the same time as shadow virtqueues.
I'll need to dig into what, if any, work is needed to make svqs compatible
with IOMMU enabled.  For now, I can implement a similar check in
vhost_user_dev_start to that in vhost-vdpa.

> > +        offset = needle.translated_addr - map->translated_addr;
> > +        iova_base = (void *)(map->iova + offset);
> > +
> > +        elem->out_sg[i].iov_base = iova_base;
>
> elem->out_sg shouldn't be overwritten since virtqueue_unpop() will use
> it to unmap the element.
>
> > +        memcpy(iova_base, elem->out_sg[i].iov_base, needle.size + 1);
>
> This memcpy() is no-op. elem->out_sg[i].iov_base is already set to
> iova_base, so it's copying from iova_base to iova_base.
>

Roger!  There were a number of unnecessary adjustments to elem in
both handlers which should now be fixed.  As the IOVA and hva of the
isolation region are no longer the same (see Patch 11), there are some
additional changes required here, but the memcpy issues should also
be fixed for the next rev.

> > +    }
> > +
> > +    for (int i = 0; i < elem->in_num; i++) {
> > +        needle.translated_addr = elem->in_addr[i];
> > +        needle.size = elem->in_sg[i].iov_len - 1;
> > +        map = vhost_iova_tree_find_gpa(svq->iova_tree, &needle);
> > +        offset = needle.translated_addr - map->translated_addr;
> > +        iova_base = (void *)(map->iova + offset);
> > +
> > +        elem->in_sg[i].iov_base = iova_base;
> > +        memcpy(iova_base, elem->in_sg[i].iov_base, needle.size + 1);
> > +    }
> > +
> > +    vhost_svq_add(svq, elem->out_sg, elem->out_num, elem->out_addr,
> > +                  elem->in_sg, elem->in_num, elem->in_addr, elem);
>
> An error is ignored here too.
>

This is added now.

> > +
> > +    return 0;
> > +}
> > +
> > +

...

> > +    for (int i = 0; i < u->shadow_vqs->len; i++) {
> > +        VirtQueue *vq = virtio_get_queue(dev->vdev, dev->vq_index + i);
> > +        VhostShadowVirtqueue *svq = g_ptr_array_index(u->shadow_vqs, i);
> > +        svq->base_addr = (hwaddr *) vring_base;
> > +        vhost_svq_start(svq, dev->vdev, vq, u->iso_iova_tree);
>
> Calling vhost_svq_start() here overwrites the shadow vring index
> configured in vhost_virtqueue_start().
>

I might not understand what you are referring to here. If I understand
correctly,
vhost_virtqueue_start() does not touch any shadow vring indexes, so I
don't think
vhost_svq_start() could overwrite something configured there.

However, I see that the assignment to addr.index followed by
vhost_user_set_vring_addr
might create a disconnect between the vq index and that sent to the
backend.  It's possible
my ordering is just right as is, but I will confirm by assigning
virtio_get_queue_index(vq) to
addr.index

> > +
> > +        struct vhost_vring_addr addr = {
> > +            .index = dev->vq_index + i,
> > +            .desc_user_addr = vring_base,
> > +            .avail_user_addr = vring_base + sizeof(vring_desc_t) *
> > +                svq->vring.num,
>
> This assumes the ring is split, but VIRTIO_F_RING_PACKED is not rejected.
>

Currently, shadow virtqueue does not support packed vrings, so I can't here.  It
is a good idea to add a check and reject VIRTIO_F_RING_PACKED.

> > +            .used_user_addr = vring_base + vhost_svq_driver_area_size(svq)
> > +        };
> > +
> > +        vhost_user_set_vring_addr(dev, &addr);
>
>
> The initial ring startup is ordered incorrectly. Generic startup
> suppresses SET_VRING_ADDR, installs SET_VRING_KICK, and injects a kick;
> only the later backend-start hook creates the shadow ring and sends its
> address. SET_VRING_KICK starts the ring, so the backend can process
> before receiving valid addresses. The late SET_VRING_ADDR result is also
> ignored.

I think to avoid this issue, I need to assign svq vring locations when the mem
table is getting set, rather than on device start, as the svq
implementation seems to
expect.

It is likely that the isolation mode implementation will end up moving
up to vhost rather than
vhost-user.  If that's the case, some of the complexity around
tracking order of operations should
be simplified.

Best,
Connor