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

On Tue, Jul 28, 2026 at 1:57 PM Stefan Hajnoczi <[email protected]> wrote:
>
> > +
> > +    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);
>
> Mapping failure must be handled.
>

Got it!  This will be fixed.

> > +        offset = needle.translated_addr - map->translated_addr;
> > +        iova_base = (void *)(map->iova + offset);
> > +
> > +        elem->out_sg[i].iov_base = iova_base;
> > +        memcpy(iova_base, elem->out_sg[i].iov_base, needle.size + 1);
>
> iova_base is an iova, not QEMU memory (HVA). Memcpy cannot be used with
> IOVAs. The address of the mapped shared memory is needed as the
> destination instead.
>

This is a relic of how the IOVA space was originally really just the
hva of the shared
memory.   However, as you noted, that wasn't great for security.  Now that those
address spaces are separate, the call is adjusted.

> > +    }
> > +
> > +    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);
>
> Mapping failure must be handled.
>

Fixed as above!

> > +        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);
>
> No memcpy is necessary before vhost_svq_add() since this is data that
> will be read from the device upon I/O completion.
>

Copy! It is removed.

> > +    }
>
> elem->out_sg[] and elem->in_sg[] are modified in this function. Have we
> lost the original I/O buffer memory address from the guest virtqueue?
> This is a problem because they will be needed when completing the
> request.
>

I don't believe they were lost.  I had complimentary operations on elem
in the two handlers.  However, my thought that these needed to be adjusted
was incorrect, so address fields in elem are no longer modified in either
handler.

...

> >
> > +    if (u->user->memory_isolation) {
> > +        if (!u->svqs_allocated) {
> > +            return 0;
> > +        }
>
> Is the idea that this returns silently when called before
> vhost_user_dev_start()? State make code harder to understand. It would
> be cleaner to set up the vring addresses without relying on
> svqs_allocated. What is the reason for deferring the vring address
> setup to vhost_user_dev_start()?
>

The current implementation follows a similar order of operations as
vhost-vdpa, in which vhost_svq_start is called by vhost_dev_start.
vhost_svq_start is where vrings get assigned their locations, so there
is no valid address to send before then.

As we discussed offline, this could potentially be fixed by calling
vhost_svq_start when the mem table is being set, though this
complexity among others is a good reason to move implementation
of the isolation mode up to vhost.

...

> >
> > +static bool vhost_user_svqs_start(struct vhost_dev *dev)
> > +{
> > +    struct vhost_user *u = dev->opaque;
> > +    uint64_t vring_base = u->iso_memory.vring_base_addr;
> > +    u->svqs_allocated = true;
>
> Where is this field cleared to false on reset?
>

Nowhere originally, but I have added it in vhost_user_svqs_stop.

> > +
> > +    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;
>
> Does this support multiple virtqueues, it looks like they will all use
> the same base_addr?
>

It does!  vring_base is incremented after the assignment is made
for each svq.  Having said that, I don't think this works with virtio-net
where the device is implemented as multiple vhost-devs.  Since
multiple vhost-devs would need to share the iso memory, I will need
to adjust the vring assignment accordingly.

> > +        vhost_svq_start(svq, dev->vdev, vq, u->iso_iova_tree);
> > +
> > +        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,
> > +            .used_user_addr = vring_base + vhost_svq_driver_area_size(svq)
> > +        };
> > +
> > +        vhost_user_set_vring_addr(dev, &addr);
>
> Also mentioned above in vhost_user_set_vring_addr():
>
> Sending a vhost-user message here is strange since that should already
> be done by vhost.c:vhost_virtqueue_start() ->
> vhost_user_set_vring_addr().
>

Same response as above.