Hi, On Friday, July 26, 2024 7:10:24 PM GMT+5:30 Eugenio Perez Martin wrote: > On Fri, Jul 26, 2024 at 11:58 AM Sahil Siddiq <icegambi...@gmail.com> wrote: > > [...] > > Q1. > > In virtio_ring.h [2], new aliases with memory alignment enforcement > > such as "vring_desc_t" have been created. I am not sure if this > > is required for the packed vq descriptor ring (vring_packed_desc) > > as well. I don't see a type alias that enforces memory alignment > > for "vring_packed_desc" in the linux kernel. I haven't used any > > alias either. > > The alignment is required to be 16 for the descriptor ring and 4 for > the device and driver ares by the standard [1]. In QEMU, this is > solved by calling mmap, which always returns page-aligned addresses.
Ok, I understand this now. > > Q2. > > I see that parts of the "vhost-vdpa" implementation is based on > > the assumption that SVQ uses the split vq format. For example, > > "vhost_vdpa_svq_map_rings" [3], calls "vhost_svq_device_area_size" > > which is specific to split vqs. The "vhost_vring_addr" [4] struct > > is also specific to split vqs. > > > > My idea is to have a generic "vhost_vring_addr" structure that > > wraps around split and packed vq specific structures, rather > > than using them directly in if-else conditions wherever the > > vhost-vdpa functions require their usage. However, this will > > involve checking their impact in several other places where this > > struct is currently being used (eg.: "vhost-user", "vhost-backend", > > "libvhost-user"). > > Ok I've just found this is under-documented actually :). > > As you mention, vhost-user is already using this same struct for > packed vqs [2], just translating the driver area from the avail vring > and the device area from the used vring. So the best option is to > stick with that, unless I'm missing something. > > > [1] https://docs.oasis-open.org/virtio/virtio/v1.3/virtio-v1.3.html > [2] > https://github.com/DPDK/dpdk/blob/82c47f005b9a0a1e3a649664b7713443d18abe43/ > lib/vhost/vhost_user.c#L841C1-L841C25 Sorry, I am a little confused here. I was referring to QEMU's vhost-user implementation here. Based on what I have understood, "vhost_vring_addr" is only being used for split vqs in QEMU's vhost-user and in other places too. The implementation does not take into account packed vqs. I was going through DPDK's source. In DPDK's implementation of vhost-user [1], the same struct (vhost_virtqueue) is being used for split vqs and packed vqs. This is possible since "vhost_virtqueue" [2] uses a union to wrap around the split and packed versions of the vq. > > My idea is to have a generic "vhost_vring_addr" structure that > > wraps around split and packed vq specific structures, rather > > than using them directly in if-else conditions wherever the > > vhost-vdpa functions require their usage. However, this will > > involve checking their impact in several other places where this > > struct is currently being used (eg.: "vhost-user", "vhost-backend", > > "libvhost-user"). I was referring to something similar by this. The current "vhost_vring_addr" can be renamed to "vhost_vring_addr_split" for example, and a new struct "vhost_vring_addr" can wrap around this and "vhost_vring_addr_packed" using a union. I liked the idea of using three unions for each member in the virtqueue format instead of having one union for the whole format. I didn't think of this. I think by having three unions the "vhost_svq_get_vring_addr" [3] function won't have to be split into two new functions to handle split and packed formats separately. I am not sure if this is what you were referring to. Thanks, Sahil [1] https://github.com/DPDK/dpdk/blob/82c47f005b9a0a1e3a649664b7713443d18abe43/lib/vhost/vhost_user.c#L861 [2] https://github.com/DPDK/dpdk/blob/82c47f005b9a0a1e3a649664b7713443d18abe43/lib/vhost/vhost.h#L275 [3] https://gitlab.com/qemu-project/qemu/-/blob/master/hw/virtio/vhost-shadow-virtqueue.c#L595
