Message ID | 20240726095822.104017-4-sahilcdq@proton.me (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Add packed virtqueue to shadow virtqueue | expand |
On Fri, Jul 26, 2024 at 11:59 AM Sahil Siddiq <icegambit91@gmail.com> wrote: > > Allocate memory for the packed vq format and support > packed vq in the SVQ "start" operation. > > Signed-off-by: Sahil Siddiq <sahilcdq@proton.me> > --- > Changes v1 -> v2: > * vhost-shadow-virtqueue.h > (struct VhostShadowVirtqueue): New member "is_packed" > (vhost_svq_get_vring_addr): Renamed function. > (vhost_svq_get_vring_addr_packed): New function. > (vhost_svq_memory_packed): Likewise. > * vhost-shadow-virtqueue.c: > (vhost_svq_add): Use "is_packed" to check vq format. > (vhost_svq_get_vring_addr): Rename function. > (vhost_svq_get_vring_addr_packed): New function but is yet to be implemented. > (vhost_svq_memory_packed): New function. > (vhost_svq_start): Support packed vq format. > * vhost-vdpa.c > (vhost_svq_get_vring_addr): Rename function. > > > hw/virtio/vhost-shadow-virtqueue.c | 70 ++++++++++++++++++++++-------- > hw/virtio/vhost-shadow-virtqueue.h | 10 ++++- > hw/virtio/vhost-vdpa.c | 4 +- > 3 files changed, 63 insertions(+), 21 deletions(-) > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > index c7b7e0c477..045c07304c 100644 > --- a/hw/virtio/vhost-shadow-virtqueue.c > +++ b/hw/virtio/vhost-shadow-virtqueue.c > @@ -343,7 +343,7 @@ int vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg, > return -ENOSPC; > } > > - if (virtio_vdev_has_feature(svq->vdev, VIRTIO_F_RING_PACKED)) { > + if (svq->is_packed) { > ok = vhost_svq_add_packed(svq, out_sg, out_num, > in_sg, in_num, &qemu_head); > } else { > @@ -679,18 +679,29 @@ void vhost_svq_set_svq_call_fd(VhostShadowVirtqueue *svq, int call_fd) > } > > /** > - * Get the shadow vq vring address. > + * Get the split shadow vq vring address. > * @svq: Shadow virtqueue > * @addr: Destination to store address > */ > -void vhost_svq_get_vring_addr(const VhostShadowVirtqueue *svq, > - struct vhost_vring_addr *addr) > +void vhost_svq_get_vring_addr_split(const VhostShadowVirtqueue *svq, > + struct vhost_vring_addr *addr) > { > addr->desc_user_addr = (uint64_t)(uintptr_t)svq->vring.desc; > addr->avail_user_addr = (uint64_t)(uintptr_t)svq->vring.avail; > addr->used_user_addr = (uint64_t)(uintptr_t)svq->vring.used; > } > > +/** > + * Get the packed shadow vq vring address. > + * @svq: Shadow virtqueue > + * @addr: Destination to store address > + */ > +void vhost_svq_get_vring_addr_packed(const VhostShadowVirtqueue *svq, > + struct vhost_vring_addr *addr) > +{ > + /* TODO */ > +} > + > size_t vhost_svq_driver_area_size(const VhostShadowVirtqueue *svq) > { > size_t desc_size = sizeof(vring_desc_t) * svq->vring.num; > @@ -707,6 +718,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 > * > @@ -759,19 +780,34 @@ void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev, > svq->vq = vq; > svq->iova_tree = iova_tree; > > - 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++) { > + if (virtio_vdev_has_feature(svq->vdev, VIRTIO_F_RING_PACKED)) { > + svq->is_packed = true; > + svq->vring_packed.vring.num = virtio_queue_get_num(vdev, virtio_get_queue_index(vq)); > + svq->num_free = svq->vring_packed.vring.num; > + 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); (Expanding on the cover letter comment) Here the driver area is aligned properly too as each descriptor is 16 bytes length, and required alignment for the driver area is 4 bytes by VirtIO standard. > + svq->vring_packed.vring.device = (void *)((char *)svq->vring_packed.vring.driver + > + sizeof(struct vring_packed_desc_event)); > + } else { > + svq->is_packed = false; > + svq->vring.num = virtio_queue_get_num(vdev, virtio_get_queue_index(vq)); > + svq->num_free = svq->vring.num; Nitpicks, The variables is_packed and num_free can be merged out of the if/else to reduce code duplication. Also it is clearer to me to assign svq->is_packed = virtio_vdev_has_feature(...) and then check the variable in the if. But other parts of QEMU do as you do here so I don't have a strong opinion. > + 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->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); > } > } > diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h > index ee1a87f523..b396daf57d 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; > > @@ -146,10 +149,13 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num); > > void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd); > void vhost_svq_set_svq_call_fd(VhostShadowVirtqueue *svq, int call_fd); > -void vhost_svq_get_vring_addr(const VhostShadowVirtqueue *svq, > - struct vhost_vring_addr *addr); > +void vhost_svq_get_vring_addr_split(const VhostShadowVirtqueue *svq, > + struct vhost_vring_addr *addr); > +void vhost_svq_get_vring_addr_packed(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); Ok now I get the question on the cover letter better, It is ok to reuse the already present functions, no need to create new ones to export in this header. Thanks! > > void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev, > VirtQueue *vq, VhostIOVATree *iova_tree); > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index 3cdaa12ed5..688de4a662 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -1130,7 +1130,7 @@ static void vhost_vdpa_svq_unmap_rings(struct vhost_dev *dev, > struct vhost_vdpa *v = dev->opaque; > struct vhost_vring_addr svq_addr; > > - vhost_svq_get_vring_addr(svq, &svq_addr); > + vhost_svq_get_vring_addr_split(svq, &svq_addr); > > vhost_vdpa_svq_unmap_ring(v, svq_addr.desc_user_addr); > > @@ -1189,7 +1189,7 @@ static bool vhost_vdpa_svq_map_rings(struct vhost_dev *dev, > size_t avail_offset; > bool ok; > > - vhost_svq_get_vring_addr(svq, &svq_addr); > + vhost_svq_get_vring_addr_split(svq, &svq_addr); > > driver_region = (DMAMap) { > .translated_addr = svq_addr.desc_user_addr, > -- > 2.45.2 >
Hi, On Friday, July 26, 2024 7:58:49 PM GMT+5:30 Eugenio Perez Martin wrote: > On Fri, Jul 26, 2024 at 11:59 AM Sahil Siddiq <icegambit91@gmail.com> wrote: > > [...] > > @@ -759,19 +780,34 @@ void vhost_svq_start(VhostShadowVirtqueue *svq, > > VirtIODevice *vdev,> > > svq->vq = vq; > > svq->iova_tree = iova_tree; > > > > - 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++) { > > + if (virtio_vdev_has_feature(svq->vdev, VIRTIO_F_RING_PACKED)) { > > + svq->is_packed = true; > > + svq->vring_packed.vring.num = virtio_queue_get_num(vdev, > > virtio_get_queue_index(vq)); + svq->num_free = > > svq->vring_packed.vring.num; > > + 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); > (Expanding on the cover letter comment) Here the driver area is > aligned properly too as each descriptor is 16 bytes length, and > required alignment for the driver area is 4 bytes by VirtIO standard. Ok, this makes sense now. > > + svq->vring_packed.vring.device = (void *)((char *)svq- >vring_packed.vring.driver + > > + sizeof(struct vring_packed_desc_event)); + } else { > > + svq->is_packed = false; > > + svq->vring.num = virtio_queue_get_num(vdev, virtio_get_queue_index(vq)); > > + svq->num_free = svq->vring.num; > > Nitpicks, > > The variables is_packed and num_free can be merged out of the if/else > to reduce code duplication. > > Also it is clearer to me to assign svq->is_packed = > virtio_vdev_has_feature(...) and then check the variable in the if. > But other parts of QEMU do as you do here so I don't have a strong > opinion. I think your suggestion will reduce code duplication. I'll change this. > > [...] > > @@ -146,10 +149,13 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num); > > > > void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd); > > void vhost_svq_set_svq_call_fd(VhostShadowVirtqueue *svq, int call_fd); > > -void vhost_svq_get_vring_addr(const VhostShadowVirtqueue *svq, > > - struct vhost_vring_addr *addr); > > +void vhost_svq_get_vring_addr_split(const VhostShadowVirtqueue *svq, > > + struct vhost_vring_addr *addr); > > +void vhost_svq_get_vring_addr_packed(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); > > Ok now I get the question on the cover letter better, > > It is ok to reuse the already present functions, no need to create new > ones to export in this header. I think "vhost_svq_memory_packed" will be required while the others can be removed. Thanks, Sahil
diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c index c7b7e0c477..045c07304c 100644 --- a/hw/virtio/vhost-shadow-virtqueue.c +++ b/hw/virtio/vhost-shadow-virtqueue.c @@ -343,7 +343,7 @@ int vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg, return -ENOSPC; } - if (virtio_vdev_has_feature(svq->vdev, VIRTIO_F_RING_PACKED)) { + if (svq->is_packed) { ok = vhost_svq_add_packed(svq, out_sg, out_num, in_sg, in_num, &qemu_head); } else { @@ -679,18 +679,29 @@ void vhost_svq_set_svq_call_fd(VhostShadowVirtqueue *svq, int call_fd) } /** - * Get the shadow vq vring address. + * Get the split shadow vq vring address. * @svq: Shadow virtqueue * @addr: Destination to store address */ -void vhost_svq_get_vring_addr(const VhostShadowVirtqueue *svq, - struct vhost_vring_addr *addr) +void vhost_svq_get_vring_addr_split(const VhostShadowVirtqueue *svq, + struct vhost_vring_addr *addr) { addr->desc_user_addr = (uint64_t)(uintptr_t)svq->vring.desc; addr->avail_user_addr = (uint64_t)(uintptr_t)svq->vring.avail; addr->used_user_addr = (uint64_t)(uintptr_t)svq->vring.used; } +/** + * Get the packed shadow vq vring address. + * @svq: Shadow virtqueue + * @addr: Destination to store address + */ +void vhost_svq_get_vring_addr_packed(const VhostShadowVirtqueue *svq, + struct vhost_vring_addr *addr) +{ + /* TODO */ +} + size_t vhost_svq_driver_area_size(const VhostShadowVirtqueue *svq) { size_t desc_size = sizeof(vring_desc_t) * svq->vring.num; @@ -707,6 +718,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 * @@ -759,19 +780,34 @@ void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev, svq->vq = vq; svq->iova_tree = iova_tree; - 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++) { + if (virtio_vdev_has_feature(svq->vdev, VIRTIO_F_RING_PACKED)) { + svq->is_packed = true; + svq->vring_packed.vring.num = virtio_queue_get_num(vdev, virtio_get_queue_index(vq)); + svq->num_free = svq->vring_packed.vring.num; + 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)); + } else { + svq->is_packed = false; + 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->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); } } diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h index ee1a87f523..b396daf57d 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; @@ -146,10 +149,13 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq, size_t num); void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd); void vhost_svq_set_svq_call_fd(VhostShadowVirtqueue *svq, int call_fd); -void vhost_svq_get_vring_addr(const VhostShadowVirtqueue *svq, - struct vhost_vring_addr *addr); +void vhost_svq_get_vring_addr_split(const VhostShadowVirtqueue *svq, + struct vhost_vring_addr *addr); +void vhost_svq_get_vring_addr_packed(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); diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 3cdaa12ed5..688de4a662 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -1130,7 +1130,7 @@ static void vhost_vdpa_svq_unmap_rings(struct vhost_dev *dev, struct vhost_vdpa *v = dev->opaque; struct vhost_vring_addr svq_addr; - vhost_svq_get_vring_addr(svq, &svq_addr); + vhost_svq_get_vring_addr_split(svq, &svq_addr); vhost_vdpa_svq_unmap_ring(v, svq_addr.desc_user_addr); @@ -1189,7 +1189,7 @@ static bool vhost_vdpa_svq_map_rings(struct vhost_dev *dev, size_t avail_offset; bool ok; - vhost_svq_get_vring_addr(svq, &svq_addr); + vhost_svq_get_vring_addr_split(svq, &svq_addr); driver_region = (DMAMap) { .translated_addr = svq_addr.desc_user_addr,
Allocate memory for the packed vq format and support packed vq in the SVQ "start" operation. Signed-off-by: Sahil Siddiq <sahilcdq@proton.me> --- Changes v1 -> v2: * vhost-shadow-virtqueue.h (struct VhostShadowVirtqueue): New member "is_packed" (vhost_svq_get_vring_addr): Renamed function. (vhost_svq_get_vring_addr_packed): New function. (vhost_svq_memory_packed): Likewise. * vhost-shadow-virtqueue.c: (vhost_svq_add): Use "is_packed" to check vq format. (vhost_svq_get_vring_addr): Rename function. (vhost_svq_get_vring_addr_packed): New function but is yet to be implemented. (vhost_svq_memory_packed): New function. (vhost_svq_start): Support packed vq format. * vhost-vdpa.c (vhost_svq_get_vring_addr): Rename function. hw/virtio/vhost-shadow-virtqueue.c | 70 ++++++++++++++++++++++-------- hw/virtio/vhost-shadow-virtqueue.h | 10 ++++- hw/virtio/vhost-vdpa.c | 4 +- 3 files changed, 63 insertions(+), 21 deletions(-)