Re: [PATCH RFC v2 05/13] hw/virtio/vhost-shadow-virtqueue: specified vring placement

[Hanna Czenczek <hreitz@redhat.com>]

On 18.08.26 07:12, Connor Kite wrote:
> By default svq vrings are placed in an anonymous memory map. As svqs
> will be leveraged to enable memory isolation in vhost-user, it is useful
> to be able to place the vrings in a shared isolation memory region.
>
> Adds the option to specify vring placement by providing a vring base
> address before starting the svq.
>
> Signed-off-by: Connor Kite <connorkite@gmail.com>
> ---
>   hw/virtio/vhost-shadow-virtqueue.c | 69 ++++++++++++++++++++++++++++++++------
>   hw/virtio/vhost-shadow-virtqueue.h |  8 ++++-
>   2 files changed, 66 insertions(+), 11 deletions(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> index 496e7e58a3..f54a61439a 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -812,6 +812,13 @@ size_t vhost_svq_device_area_size(const VhostShadowVirtqueue *svq)
>       return ROUND_UP(used_size, qemu_real_host_page_size());
>   }
>   
> +size_t vhost_svq_vring_total_size(VirtIODevice *vdev, VirtQueue *vq)

If this function is not used in this patch, it probably should not be 
introduced here.

> +{
> +    VhostShadowVirtqueue svq;
> +    svq.vring.num = virtio_queue_get_num(vdev, virtio_get_queue_index(vq));
> +    return vhost_svq_driver_area_size(&svq) + vhost_svq_device_area_size(&svq);

I’m not 100 % happy about this (initializing a pseudo svq), might be 
better to make it so there are vhost_svq_driver/device_area_size() 
variants that take the num directly, and then have 
vhost_svq_driver/device_area_size() be wrappers around it, but, well. At 
least the implementations are right above it so one can verify that 
nothing else in VhostShadowVirtqueue is used. (It’s just that one should 
not *have* to verify.)

> +}
> +
>   /**
>    * Set a new file descriptor for the guest to kick the SVQ and notify for avail
>    *
> @@ -842,6 +849,19 @@ void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd)
>       }
>   }
>   
> +/**
> + * Set vring base address if using fixed locations
> + *
> + * @svq: Shadow Virtqueue
> + * @addr: Points to new base address
> + */
> +
> + void vhost_svq_set_base_addr(VhostShadowVirtqueue *svq, void *addr)
> + {
> +    svq->base_addr = addr;
> + }
> +
> +
>   /**
>    * Start the shadow virtqueue operation.
>    *
> @@ -849,8 +869,10 @@ void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd)
>    * @vdev: VirtIO device
>    * @vq: Virtqueue to shadow
>    * @iova_tree: Tree to perform descriptors translations
> + *
> + * Return 0 on success, -errno on failure
>    */
> -void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
> +int vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
>                        VirtQueue *vq, VhostIOVATree *iova_tree)
>   {
>       size_t desc_size;
> @@ -868,14 +890,27 @@ void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
>   
>       svq->vring.num = virtio_queue_get_num(vdev, virtio_get_queue_index(vq));
>       svq->num_free = svq->vring.num;
> -    svq->vring.desc = mmap(NULL, vhost_svq_driver_area_size(svq),
> -                           PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS,
> -                           -1, 0);
>       desc_size = sizeof(vring_desc_t) * svq->vring.num;
> -    svq->vring.avail = (void *)((char *)svq->vring.desc + desc_size);
> -    svq->vring.used = mmap(NULL, vhost_svq_device_area_size(svq),
> -                           PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS,
> -                           -1, 0);
> +    if (svq->base_addr == NULL) {
> +        svq->vring.desc = mmap(NULL, vhost_svq_driver_area_size(svq),
> +                            PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS,
> +                            -1, 0);
> +        svq->vring.avail = (void *)((char *)svq->vring.desc + desc_size);
> +        svq->vring.used = mmap(NULL, vhost_svq_device_area_size(svq),
> +                            PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS,
> +                            -1, 0);
> +    } else {
> +        svq->vring.desc = (void *)svq->base_addr;
> +        svq->vring.avail = (void *)((char *)svq->vring.desc + desc_size);
> +        svq->vring.used = (void *)((char *)svq->base_addr +
> +                          vhost_svq_driver_area_size(svq));
> +
> +        if ((uint64_t)svq->vring.used + vhost_svq_device_area_size(svq) - 1 <
> +           (uint64_t)svq->vring.desc) {
> +                error_report("Invalid shadow vring location");
> +                return -ENOMEM;
> +        }

Why does this check exist?

What needs to be done here is proper bounds checking. That either `.used 
+ device_area_size()` or `desc + vhost_svq_vring_total_size()` does not 
exceed the end of the memory region that has been allocated for the 
vrings. It is clear that such an allocation would never wrap around the 
end of memory because no allocation ever does.

> +    }
>       svq->desc_state = g_new0(SVQDescState, svq->vring.num);
>       if (virtio_vdev_has_feature(svq->vdev, VIRTIO_F_IN_ORDER)) {
>           svq->batch_last.id = VIRTIO_RING_NOT_IN_BATCH;
> @@ -884,6 +919,8 @@ void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
>               svq->desc_state[i].next = i + 1;
>           }
>       }
> +
> +    return 0;
>   }
>   
>   /**
> @@ -920,8 +957,19 @@ void vhost_svq_stop(VhostShadowVirtqueue *svq)
>       }
>       svq->vq = NULL;
>       g_free(svq->desc_state);
> -    munmap(svq->vring.desc, vhost_svq_driver_area_size(svq));
> -    munmap(svq->vring.used, vhost_svq_device_area_size(svq));
> +
> +    if (!svq->base_addr) {
> +        munmap(svq->vring.desc, vhost_svq_driver_area_size(svq));
> +        munmap(svq->vring.used, vhost_svq_device_area_size(svq));
> +    } else{
> +        if (svq->vring.desc) {
> +            memset(svq->vring.desc, 0, vhost_svq_driver_area_size(svq));
> +        }
> +        if (svq->vring.used) {
> +            memset(svq->vring.used, 0, vhost_svq_device_area_size(svq));
> +        }

Why the memset()?

Hanna

> +    }
> +
>       event_notifier_set_handler(&svq->hdev_call, NULL);
>   }
>   
> @@ -940,6 +988,7 @@ VhostShadowVirtqueue *vhost_svq_new(const VhostShadowVirtqueueOps *ops,
>       event_notifier_init_fd(&svq->svq_kick, VHOST_FILE_UNBIND);
>       svq->ops = ops;
>       svq->ops_opaque = ops_opaque;
> +    svq->base_addr = NULL;
>       return svq;
>   }
>   
> diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
> index fd68319fb7..1e0cc9e5e4 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.h
> +++ b/hw/virtio/vhost-shadow-virtqueue.h
> @@ -150,6 +150,9 @@ typedef struct VhostShadowVirtqueue {
>   
>       /* Size of SVQ vring free descriptors */
>       uint16_t num_free;
> +
> +    /* Location assigned to vrings if not in default anon memory map */
> +    void *base_addr;
>   } VhostShadowVirtqueue;
>   
>   bool vhost_svq_valid_features(uint64_t features, Error **errp);
> @@ -169,8 +172,9 @@ void vhost_svq_get_vring_addr(const VhostShadowVirtqueue *svq,
>                                 struct vhost_vring_addr *addr);
>   size_t vhost_svq_driver_area_size(const VhostShadowVirtqueue *svq);
>   size_t vhost_svq_device_area_size(const VhostShadowVirtqueue *svq);
> +size_t vhost_svq_vring_total_size(VirtIODevice *vdev, VirtQueue *vq);
>   
> -void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
> +int vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
>                        VirtQueue *vq, VhostIOVATree *iova_tree);
>   void vhost_svq_stop(VhostShadowVirtqueue *svq);
>   
> @@ -178,6 +182,8 @@ VhostShadowVirtqueue *vhost_svq_new(const VhostShadowVirtqueueOps *ops,
>                                       void *ops_opaque);
>   
>   void vhost_svq_free(gpointer vq);
> +void vhost_svq_set_base_addr(VhostShadowVirtqueue *svq, void *addr);
> +
>   G_DEFINE_AUTOPTR_CLEANUP_FUNC(VhostShadowVirtqueue, vhost_svq_free);
>   
>   #endif
>