On Fri, Aug 2, 2024 at 1:22 PM Sahil Siddiq <icegambi...@gmail.com> wrote:
>
> Allocate memory for the packed vq format and support
> packed vq in the SVQ "start" and "stop" operations.
>
> Signed-off-by: Sahil Siddiq <sahil...@proton.me>
> ---
> Changes v2 -> v3:
> * vhost-shadow-virtqueue.c
> (vhost_svq_memory_packed): New function
> (vhost_svq_start):
> - Remove common variables out of if-else branch.
> (vhost_svq_stop):
> - Add support for packed vq.
> (vhost_svq_get_vring_addr): Revert changes
> (vhost_svq_get_vring_addr_packed): Likwise.
> * vhost-shadow-virtqueue.h
> - Revert changes made to "vhost_svq_get_vring_addr*"
> functions.
> * vhost-vdpa.c: Revert changes.
>
> hw/virtio/vhost-shadow-virtqueue.c | 56 +++++++++++++++++++++++-------
> hw/virtio/vhost-shadow-virtqueue.h | 4 +++
> 2 files changed, 47 insertions(+), 13 deletions(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c
> b/hw/virtio/vhost-shadow-virtqueue.c
> index 4c308ee53d..f4285db2b4 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -645,6 +645,8 @@ void vhost_svq_set_svq_call_fd(VhostShadowVirtqueue *svq,
> int call_fd)
>
> /**
> * Get the shadow vq vring address.
> + * This is used irrespective of whether the
> + * split or packed vq format is used.
> * @svq: Shadow virtqueue
> * @addr: Destination to store address
> */
> @@ -672,6 +674,16 @@ size_t vhost_svq_device_area_size(const
> VhostShadowVirtqueue *svq)
> return ROUND_UP(used_size, qemu_real_host_page_size());
> }
>
> +size_t vhost_svq_memory_packed(const VhostShadowVirtqueue *svq)
> +{
> + size_t desc_size = sizeof(struct vring_packed_desc) * svq->num_free;
> + size_t driver_event_suppression = sizeof(struct vring_packed_desc_event);
> + size_t device_event_suppression = sizeof(struct vring_packed_desc_event);
> +
> + return ROUND_UP(desc_size + driver_event_suppression +
> device_event_suppression,
> + qemu_real_host_page_size());
> +}
> +
> /**
> * Set a new file descriptor for the guest to kick the SVQ and notify for
> avail
> *
> @@ -726,17 +738,30 @@ 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);
> - svq->desc_state = g_new0(SVQDescState, svq->vring.num);
> - svq->desc_next = g_new0(uint16_t, svq->vring.num);
> - for (unsigned i = 0; i < svq->vring.num - 1; i++) {
> + svq->is_packed = virtio_vdev_has_feature(svq->vdev,
> VIRTIO_F_RING_PACKED);
> +
> + if (virtio_vdev_has_feature(svq->vdev, VIRTIO_F_RING_PACKED)) {
> + svq->vring_packed.vring.desc = mmap(NULL,
> vhost_svq_memory_packed(svq),
> + PROT_READ | PROT_WRITE,
> MAP_SHARED | MAP_ANONYMOUS,
> + -1, 0);
> + desc_size = sizeof(struct vring_packed_desc) * svq->vring.num;
> + svq->vring_packed.vring.driver = (void *)((char
> *)svq->vring_packed.vring.desc + desc_size);
> + svq->vring_packed.vring.device = (void *)((char
> *)svq->vring_packed.vring.driver +
> + sizeof(struct
> vring_packed_desc_event));
This is a great start but it will be problematic when you start
mapping the areas to the vdpa device. The driver area should be read
only for the device, but it is placed in the same page as a RW one.
More on this later.
> + } else {
> + 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);
> + }
I think it will be beneficial to avoid "if (packed)" conditionals on
the exposed functions that give information about the memory maps.
These need to be replicated at
hw/virtio/vhost-vdpa.c:vhost_vdpa_svq_map_rings.
However, the current one depends on the driver area to live in the
same page as the descriptor area, so it is not suitable for this.
So what about this action plan:
1) Make the avail ring (or driver area) independent of the descriptor ring.
2) Return the mapping permissions of the descriptor area (not needed
here, but needed for hw/virtio/vhost-vdpa.c:vhost_vdpa_svq_map_rings
> +
> + svq->desc_state = g_new0(SVQDescState, svq->num_free);
> + svq->desc_next = g_new0(uint16_t, svq->num_free);
> + for (unsigned i = 0; i < svq->num_free - 1; i++) {
> svq->desc_next[i] = cpu_to_le16(i + 1);
> }
> }
> @@ -776,8 +801,13 @@ void vhost_svq_stop(VhostShadowVirtqueue *svq)
> svq->vq = NULL;
> g_free(svq->desc_next);
> 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->is_packed) {
> + munmap(svq->vring_packed.vring.desc, vhost_svq_memory_packed(svq));
> + } else {
> + munmap(svq->vring.desc, vhost_svq_driver_area_size(svq));
> + munmap(svq->vring.used, vhost_svq_device_area_size(svq));
> + }
> event_notifier_set_handler(&svq->hdev_call, NULL);
> }
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.h
> b/hw/virtio/vhost-shadow-virtqueue.h
> index ee1a87f523..03b722a186 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.h
> +++ b/hw/virtio/vhost-shadow-virtqueue.h
> @@ -67,6 +67,9 @@ struct vring_packed {
>
> /* Shadow virtqueue to relay notifications */
> typedef struct VhostShadowVirtqueue {
> + /* True if packed virtqueue */
> + bool is_packed;
> +
> /* Virtio queue shadowing */
> VirtQueue *vq;
>
> @@ -150,6 +153,7 @@ 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_memory_packed(const VhostShadowVirtqueue *svq);
>
> void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
> VirtQueue *vq, VhostIOVATree *iova_tree);
> --
> 2.45.2
>
