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

On Mon, Aug 3, 2026 at 7:10 AM Hanna Czenczek <[email protected]> wrote:
>
> On 24.07.26 00:30, Connor Kite wrote:
> > - Add start logic for shadow virtqueues, which sets vring addresses.
> > - Update logic for sending vring addresses to backend to point
> >    to the shadow vrings when isolation mode is active.
> > - Implement handlers for intercepted avail and used descriptors. These
> >    handlers copy buffer contents between bounce buffers in the isolation
> >    region and the buffers made available by the guest
> > - Implement logic to stop svqs
> >
> > Signed-off-by: Connor Kite <[email protected]>
> > ---
> >   hw/virtio/vhost-user.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 166 insertions(+), 1 deletion(-)
>
> In general, I find it strange to use two completely separate data
> structures for the address translation in either direction (avail/used).
> I suppose we could have a map of in-flight translations, adding entries
> in avail, and looking for them in used, removing them from the map.
>
> I mean, sure, right now, we can calculate the addresses, because we
> “duplicate” the whole range of guest memory for each device, but that is
> clearly a waste and it would be better to use a smaller size in the
> future; and then each request will need to allocate from that smaller
> device memory, get a random address, and that will need such a map for
> tracking.
>

Definitely agreed that the mapping could be done smarter to avoid
allocating such
a large region.  I think this would a reasonable optimization for when
the project is
more mature.

For your first sentence, are you talking about the svq and vhost_iova_tree data
structures?
...

> > +static int vhost_user_svq_handle_used(VhostShadowVirtqueue *svq,
> > +                                      VirtQueueElement *elem,
> > +                                      void *opaque)
> > +{
> > +    hwaddr hva;
> > +    int r;
> > +    struct vhost_dev *dev = opaque;
> > +
> > +    for (int i = 0; i < elem->in_num; i++) {
> > +        r = vhost_user_memory_lookup(dev, elem->in_addr[i], &hva);
>
> This function only looks for the starting address, so it is not
> guaranteed that the whole buffer is within one memory region.
>

For the next rev, both handlers will be looking at whether the last address
in the buffer fits in a single memory memory region and will copy from/to
multiple memory regions if necessary.  See below

> > +        if (r < 0) {
> > +            return r;
> > +        }
> > +
> > +        memcpy((void *) hva, elem->in_sg[i].iov_base, elem->in_sg[i].iov_len);
> > +        elem->in_sg[i].iov_base = (void *) hva;
> > +    }
> > +
> > +    for (int i = 0; i < elem->out_num; i++) {
> > +        r = vhost_user_memory_lookup(dev, elem->out_addr[i], &hva);
> > +        if (r < 0) {
> > +            return r;
> > +        }
> > +
> > +        memcpy((void *) hva, elem->out_sg[i].iov_base, elem->out_sg[i].iov_len);
>
> I don’t think we need this `memcpy()` here, as the device may only write
> to IN buffers, not OUT buffers.
>

You are right! This is removed now.

> > +        elem->out_sg[i].iov_base = (void *) hva;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int vhost_user_svq_handle_avail(VhostShadowVirtqueue *svq,
> > +                                       VirtQueueElement *elem,
> > +                                       void *opaque)
> > +{
> > +    hwaddr offset;
> > +    const DMAMap *map;
> > +    DMAMap needle;
> > +    hwaddr *iova_base;
> > +
> > +    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);
>
> Is it guaranteed that the returned `map` will fully contain `needle`?
>

It is not guaranteed.  If the needle range covers multiple regions, the returned
map would only represent the lowest matching addresses. Care also needs to be
taken in copying back to buffers made available by the guest. While the IOVA and
iso region hva spaces are all allocated with no gaps, there is no
guarantee the guest
buffers reside in a single region.

The next version I have implemented copies to multiple regions if
necessary, and also uses bounds checking to confirm that:

    1. bounce buffers are fully encompassed by the isolation region
    2. hva addresses in the element actually back the gpa addresses in same

> > +        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);
> > +    }
> > +
> > +    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);
>
> I don’t think we need this `memcpy()` here as the device is to strictly
> write to IN buffers, not read from them.
>

You are correct.  This is removed now.

> > +    }
> > +
> > +    vhost_svq_add(svq, elem->out_sg, elem->out_num, elem->out_addr,
> > +                  elem->in_sg, elem->in_num, elem->in_addr, elem);
>
> Might make sense to make vhost_svq_add_element() public and use it here.
>

Done!  Definitely easier to read.  I guess this adds the overhead of another
function call, but that's probably small relative to overhead due to the extra
interrupts and copying.  We could probably make use of "inline" if trying
to really optimize.

...

> > @@ -1779,6 +1872,25 @@ static int vhost_user_set_vring_addr(struct vhost_dev *dev,
> >           .hdr.size = sizeof(msg.payload.addr),
> >       };
> >
> > +    if (u->user->memory_isolation) {
> > +        if (!u->svqs_allocated) {
> > +            return 0;
> > +        }
> > +
> > +        int svq_idx = addr->index - dev->vq_index;
> > +        VhostShadowVirtqueue *svq = g_ptr_array_index(u->shadow_vqs,
> > +                                                      svq_idx);
>
> Again, bounds checking via `vhost_user_get_vq_index()` would be good.
>

This will be added for next rev.

...

> >
> > +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;
>
> I don’t like setting this before it actually being the case very much. I
> understand that it is necessary for how you decided to set up
> `vhost_user_set_vring_addr()`, but there should at least be a comment
> (because reading this code, there is the obvious question: Why is this
> set when it is not true?); or the code should be different. I.e. having
> a `vhost_user_set_shadow_vring_addr()` function, maybe, which would not
> even take an `addr` argument and could be used both below and in
> `vhost_user_set_vring_addr()`.
>

svqs_allocated should be going away as svq vring addresses will get
mapped earlier
to intercept when vhost allocates vrings.

> > +
> > +    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;
>
> I really do not like the lack of bounds checking here again. There
> should be something, somewhere (i.e. in the “Isolated memory data”
> portion, maybe in `struct IsolationRegion`), that tells how much memory
> has been allocated for the shadow virtqueues, and we need to check that
> we do not overflow.
>

Got it!  From now on, the total size of the vring allocation is
stored, and bounds
checking occurs as each svq's vrings are allocated.

> > +        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)
>
> All values set here are effectively ignored, right? Because
> `vhost_user_set_vring_addr()` just overwrites them. (Having a
> `vhost_user_set_shadow_vring_addr()` could maybe save us from having to
> set this up.)
>
> Not that it’s currently wrong to set them this way, just asking.
>
> Hanna
>

You are right about overwriting the values.  I will remove those statements from
vhost_user_svqs_start.