On Mon, May 13, 2024 at 3:49 PM Sahil <icegambi...@gmail.com> wrote:
>
> Hi,
>
> On Wednesday, May 8, 2024 8:53:12 AM GMT+5:30 Sahil wrote:
> > Hi,
> >
> > On Tuesday, May 7, 2024 12:44:33 PM IST Eugenio Perez Martin wrote:
> > > [...]
> > >
> > > > Shall I start by implementing a mechanism to check if the feature bit
> > > > "VIRTIO_F_RING_PACKED" is set (using "virtio_vdev_has_feature")? And
> > > > if it's supported, "vhost_svq_add" should call "vhost_svq_add_packed".
> > > > Following this, I can then start implementing "vhost_svq_add_packed"
> > > > and progress from there.
> > > >
> > > > What are your thoughts on this?
> > >
> > > Yes, that's totally right.
> > >
> > > I recommend you to also disable _F_EVENT_IDX to start, so the first
> > > version is easier.
> > >
> > > Also, you can send as many incomplete RFCs as you want. For example,
> > > you can send a first version that only implements reading of the guest
> > > avail ring, so we know we're aligned on that. Then, we can send
> > > subsequents RFCs adding features on top.
> >
>
> I have started working on implementing packed virtqueue support in
> vhost-shadow-virtqueue.c. The changes I have made so far are very
> minimal. I have one confusion as well.
>
> In "vhost_svq_add()" [1], a structure of type "VhostShadowVirtqueue"
> is being used. My initial idea was to create a whole new structure (eg:
> VhostShadowVirtqueuePacked). But I realized that "VhostShadowVirtqueue"
> is being used in a lot of other places such as in "struct vhost_vdpa" [2]
> (in "vhost-vdpa.h"). So maybe this isn't a good idea.
>
> The problem is that "VhostShadowVirtqueue" has a member of type "struct
> vring" [3] which represents a split virtqueue [4]. My idea now is to instead
> wrap this member in a union so that the struct would look something like
> this.
>
> struct VhostShadowVirtqueue {
> union {
> struct vring vring;
> struct packed_vring vring;
> }
> ...
> }
>
> I am not entirely sure if this is a good idea. It is similar to what's been
> done
> in linux's "drivers/virtio/virtio_ring.c" ("struct vring_virtqueue" [5]).
>
> I thought I would ask this first before continuing further.
>
That's right, this second option makes perfect sense.
VhostShadowVirtqueue should abstract both split and packed. You'll see
that some members are reused, while others are only used in one
version so they are placed after a union. They should follow the same
pattern, although it is not a problem if we need to divert a little
bit from the kernel's code.
Thanks!
> Thanks,
> Sahil
>
> [1]
> https://gitlab.com/qemu-project/qemu/-/blob/master/hw/virtio/vhost-shadow-virtqueue.c#L249
> [2]
> https://gitlab.com/qemu-project/qemu/-/blob/master/include/hw/virtio/vhost-vdpa.h#L69
> [3]
> https://gitlab.com/qemu-project/qemu/-/blob/master/hw/virtio/vhost-shadow-virtqueue.h#L52
> [4]
> https://gitlab.com/qemu-project/qemu/-/blob/master/include/standard-headers/linux/virtio_ring.h#L156
> [5]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/virtio/virtio_ring.c#n199
>
>
