Re: [RFC v2 0/3] Add packed virtqueue to shadow virtqueue

[Sahil]

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 <icegambit91@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