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

[Eugenio Perez Martin]

On Fri, Jul 26, 2024 at 7:11 PM Sahil <icegambit91@gmail.com> wrote:
>
> 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.
>

Ok, now I get you better. Let me start again from a different angle :).

vhost_vring_addr is already part of the API that QEMU uses between
itself and vhost devices, all vhost-kernel, vhost-user and vhost-vdpa.
To make non-backward compatible changes to it is impossible, as it
involves changes in all of these elements.

QEMU and DPDK, using vhost-user, already send and receive packed
virtqueues addresses using the current structure layout. QEMU's
hw/virtio/vhost.c:vhost_virtqueue_set_addr already sets vq->desc,
vq->avail and vq->user, which has the values of the desc, driver and
device. In that sense, I recommend not to modify it.

On the other hand, DPDK's vhost_virtqueue is not the same struct as
vhost_vring_addr. It is internal to DPDK so it can be modified. We
need to do something similar for the SVQ, yes.

To do that union trick piece by piece in VhostShadowVirtqueue is
possible, but it requires modifying all the usages of the current
vring. I think it is easier for us to follow the kernel's
virtio_ring.c model, as it is a driver too, and create a vring_packed.
We can create an anonymous union and suffix all members with a _packed
so we don't need to modify current split usage.

Let me know what you think.

> > > 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.
>

But you need the if/else anyway, as the members are not vring_desc_t
but vring_packed_desc, and same with vring_avail_t and vring_used_t
with struct vring_packed_desc_event. Or am I missing something?

Thanks!