Re: [RFC v3 3/3] vhost: Allocate memory for packed vring

[Eugenio Perez Martin <eperezma@redhat.com>]

On Mon, Oct 28, 2024 at 6:38 AM Sahil Siddiq <icegambit91@gmail.com> wrote:
>
> Hi,
>
> It's been a while since I gave my last update. I have one more update
> that I would like to give.
>
> > On Tue, Sep 24, 2024 at 7:31 AM Sahil <icegambit91@gmail.com> wrote:
> > > And I booted L2 by running:
> > >
> > > # ./qemu/build/qemu-system-x86_64 \
> > > -nographic \
> > > -m 4G \
> > > -enable-kvm \
> > > -M q35 \
> > > -drive file=//root/L2.qcow2,media=disk,if=virtio \
> > > -netdev type=vhost-vdpa,vhostdev=/dev/vhost-vdpa-0,id=vhost-vdpa0 \
> > > -device
> > > virtio-net-pci,netdev=vhost-vdpa0,disable-legacy=on,disable-modern=off,ev
> > > ent_idx=off,bus=pcie.0,addr=0x7 \ -smp 4 \
> > > -cpu host \
> > > 2>&1 | tee vm.log
> >
> > With packed=on in the device option, I see that the packed feature bit is
> > set in L2 :)
> >
> > However, I see that vhost shadow virtqueues are still not being used. I am
> > currently trying to find the reason behind this. I have narrowed down the
> > issue to hw/virtio/vhost-vdpa.c [1]. The "vhost_vdpa_svqs_start" function
> > is being called but in the loop, vhost_svq_start is never called. I think it
> > might be because there's an issue with "vhost_vdpa_svq_setup".
> >
> > I'll send an update once I find something.
> >
> > Thanks,
> > Sahil
> >
> > [1] https://github.com/qemu/qemu/blob/master/hw/virtio/vhost-vdpa.c#L1243
>
> I spent some time tinkering with the L0-L1-L2 test environment setup,
> and understanding QEMU's hw/virtio/vhost-vdpa.c [1] as well as Linux's
> drivers/vhost/vdpa.c [2] and /drivers/vhost/vhost.c [3]. I don't think there
> is an issue with the environment itself.
>
> When I boot L2 with the following combinations of "x-svq" and
> "packed", this is what I observe:
>
> 1. x-svq=on and packed=off
>
> The virtio device in L2 has the packed feature bit turned off. Vhost
> shadow virtqueues are used as expected.
>
> 2. x-svq=off and packed=on
>
> The virtio device in L2 has the packed feature bit turned on. Vhost
> shadow virtqueues are not used.
>
> I don't see any issues in either of the above environment
> configurations.
>
> 3. x-svq=on and packed=on
>
> This is the configuration that I need for testing. The virtio device in
> L2 has the packed feature bit turned on. However, vhost shadow
> virtqueues are not being used. This is due to the
> VHOST_SET_VRING_BASE ioctl call returning a EOPNOTSUPP in
> hw/virtio/vhost-vdpa.c:vhost_vdpa_set_dev_vring_base() [4].
>
> I spent some time going through the ioctl's implementation in Linux.
> I used ftrace to trace the functions that were being called in the kernel.
> With x-svq=on (regardless of whether split virtqueues are used or packed
> virtqueues), I got the following trace:
>
> [...]
>  qemu-system-x86-1737    [001] ...1.  3613.371358:
> vhost_vdpa_unlocked_ioctl <-__x64_sys_ioctl
>  qemu-system-x86-1737    [001] ...1.  3613.371358: vhost_vring_ioctl
> <-vhost_vdpa_unlocked_ioctl
>  qemu-system-x86-1737    [001] ...1.  3613.371362:
> vp_vdpa_set_vq_state <-vhost_vdpa_unlocked_ioctl
> [...]
>
> There are 3 virtqueues that the vdpa device offers in L1. There were no
> issues when using split virtqueues and the trace shown above appears
> 3 times. With packed virtqueues, the first call to VHOST_SET_VRING_BASE
> fails because drivers/vdpa/virtio_pci/vp_vdpa.c:vp_vdpa_set_vq_state_packed
> [5] returns EOPNOTSUPP.
>
> The payload that VHOST_SET_VRING_BASE accepts depends on whether
> split virtqueues or packed virtqueues are used [6].  In hw/virtio/vhost-
> vdpa.c:vhost_vdpa_svq_setup() [7], the following payload is used which is
> not suitable for packed virtqueues:
>
> struct vhost_vring_state s = {
>         .index = vq_index,
> };
>
> Based on the implementation in the linux kernel, the payload needs to
> be as shown below for the ioctl to succeed for packed virtqueues:
>
> struct vhost_vring_state s = {
>         .index = vq_index,
>         .num = 0x80008000,
> };
>

Wow, that's a great analysis, very good catch!

> After making these changes, it looks like QEMU is able to set up the
> virtqueues
> and shadow virtqueues are enabled as well.
>
> Unfortunately, before the L2 VM can finish booting the kernel crashes.
> The reason is that even though packed virtqueues are to be used, the
> kernel tries to run
> drivers/virtio/virtio_ring.c:virtqueue_get_buf_ctx_split() [8]
> (instead of virtqueue_get_buf_ctx_packed) and throws an "invalid vring
> head" error. I am still investigating this issue.
>

That's interesting. It's been a while since I haven't tested that
code, maybe you also discovered a regression here :).

> I'll send an update once I resolve this issue. I'll also send a patch that
> crafts the payload correctly based on the format of the virtqueue in
> vhost_vdpa_svq_setup().
>

The QEMU's vhost_vdpa_svq_setup is a valid patch so if you have the
bandwith please send it ASAP and we move it forward :).

Thanks!

> Thanks,
> Sahil
>
> [1] https://gitlab.com/qemu-project/qemu/-/blob/master/hw/virtio/vhost-vdpa.c
> [2] https://github.com/torvalds/linux/blob/master/drivers/vhost/vdpa.c
> [3] https://github.com/torvalds/linux/blob/master/drivers/vhost/vhost.c
> [4] https://gitlab.com/qemu-project/qemu/-/blob/master/hw/virtio/vhost-vdpa.c#L1002
> [5] https://github.com/torvalds/linux/blob/master/drivers/vdpa/virtio_pci/vp_vdpa.c#L278
> [6] https://qemu-project.gitlab.io/qemu/interop/vhost-user.html#front-end-message-types
> [7] https://gitlab.com/qemu-project/qemu/-/blob/master/hw/virtio/vhost-vdpa.c#L1223
> [8] https://github.com/torvalds/linux/blob/master/drivers/virtio/virtio_ring.c#L823
>