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

[Eugenio Perez Martin]

On Fri, Jul 26, 2024 at 11:58 AM Sahil Siddiq <icegambit91@gmail.com> wrote:
>
> Hi,
>
> I have made some progress in this project and thought I would
> send these changes first before continuing. I split patch v1 [1]
> into two commits (#1 and #2) to make it easy to review. There are
> very few changes in the first commit. The second commit has not
> changes.
>
> There are a few things that I am not entirely sure of in commit #3.
>
> 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.

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

> Is this approach alright or is there a better alternative? I would
> like to get your thoughts on this before working on this portion of
> the project.
>
> Thanks,
> Sahil
>

[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