Re: [PATCH] vdpa: Support setting vring_base for packed svq

[Sahil Siddiq <icegambit91@gmail.com>]

Hi,

Thank you for the review.

On 11/5/24 3:06 PM, Stefano Garzarella wrote:
> On Mon, Nov 04, 2024 at 09:51:24PM +0530, Sahil Siddiq wrote:
>> Linux commit v5.14-rc1~30^2~8 enabled the vp_vdpa driver to set the
> 
> To refer to a commit, please use the SHA-1 id or even better the form
> suggested in
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
> 
> So in this case I'd use:
> Linux commit 1225c216d954 ("vp_vdpa: allow set vq state to initial state
> after reset")

Understood, I'll change this in v2.

>> vq state to the device's initial state. This works differently for
>> split and packed vqs.
>>
>> With shadow virtqueues enabled, vhost-vdpa sets the vring base using
>> the VHOST_SET_VRING_BASE ioctl. The payload (vhost_vring_state)
>> differs for split and packed vqs. The implementation in QEMU currently
>> uses the payload required for split vqs (i.e., the num field of
>> vhost_vring_state is set to 0). The kernel throws EOPNOTSUPP when this
>> payload is used with packed vqs.
>>
>> This patch sets the num field in the payload appropriately so vhost-vdpa
> 
> I'm not very familiar with shadow virtqueue, so can you elaborate what
> "appropriately" means here?

My understanding is that the ioctl and the payload themselves are not
directly related to shadow virtqueues [1]. They concern virtqueues in general.

In QEMU's implementation, hw/virtio/vhost-vdpa.c:vhost_vdpa_svq_setup [2]
is called from hw/virtio/vhost-vdpa.c:vhost_vdpa_svqs_start [3] only when
shadow virtqueues are enabled.

QEMU's vhost-user doc [1] states that the payload for the VHOST_SET_VRING_BASE
ioctl is different for split and packed vqs. The struct is the same:

struct vhost_vring_state {
	unsigned int index;
	unsigned int num;
};

The num field takes a different value depending on the virtqueue's format
(split vs packed). The explanation below throws more light on this.

>> (with the vp_vdpa driver) can use packed svqs.
>>
>> Link: https://lists.nongnu.org/archive/html/qemu-devel/2024-10/msg05106.html
>> Link: https://lore.kernel.org/r/20210602021536.39525-4-jasowang@redhat.com
>> Signed-off-by: Sahil Siddiq <sahilcdq@proton.me>
>> ---
>> QEMU currently does not support packed vhost shadow virtqueues. I am
>> working on adding support for packed svqs [1]. The test environment
>> that I am using [2] requires vhost-vdpa to use the relevant payload
>> when setting vring base.
>>
>> [1] https://wiki.qemu.org/Internships/ProjectIdeas/PackedShadowVirtqueue
>> [2] https://www.redhat.com/en/blog/hands-vdpa-what-do-you-do-when-you-aint-got-hardware-part-2
>>
>> hw/virtio/vhost-vdpa.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> index 3cdaa12ed5..5f81945109 100644
>> --- a/hw/virtio/vhost-vdpa.c
>> +++ b/hw/virtio/vhost-vdpa.c
>> @@ -1230,6 +1230,10 @@ static bool vhost_vdpa_svq_setup(struct vhost_dev *dev,
>>     };
>>     int r;
>>
>> +    if (virtio_vdev_has_feature(dev->vdev, VIRTIO_F_RING_PACKED)) {
>> +        s.num = 0x80008000;
> 
> Why this magic value?
> 
> Looking at the kernel code it looks like we are assgining 0x8000 for
> both last_avail_idx and last_used_idx, but why 0x8000?
> 
> Thanks,
> Stefano
> 

When I boot a VM with packed=on and x-svq=true, QEMU sets the vring base
using VHOST_SET_VRING_BASE. I used ftrace to trace the functions in the
linux kernel and 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
[...]

In the kernel, drivers/vhost/vhost.c:vhost_vring_ioctl [4] uses
the vhost_vring_state payload to set the last_avail_idx. For
packed vqs, it also sets last_used_idx.

     vq->last_avail_idx = s.num & 0xffff;
     vq->last_used_idx = (s.num >> 16) & 0xffff;

These values are used to populate a new struct vdpa_vq_state in
drivers/vhost/vdpa.c:vhost_vdpa_vring_ioctl [5].

     vq_state.packed.last_avail_idx = vq->last_avail_idx & 0x7fff;
     vq_state.packed.last_avail_counter = !!(vq->last_avail_idx & 0x8000);
     vq_state.packed.last_used_idx = vq->last_used_idx & 0x7fff;
     vq_state.packed.last_used_counter = !!(vq->last_used_idx & 0x8000);

The following check is then made in drivers/vdpa/virtio_pci/vp_vdpa.c:
vp_vdpa_set_vq_state_packed [6]:

     if (packed->last_avail_counter == 1 &&
         packed->last_avail_idx == 0 &&
         packed->last_used_counter == 1 &&
         packed->last_used_idx == 0)
         return 0;
     return -EOPNOTSUPP;

The most significant bit in 0x8000 is used to set the wrap counters.
All the other bits are 0 and so the avail and used idx are also set
to 0.

Thanks,
Sahil

[1] https://qemu-project.gitlab.io/qemu/interop/vhost-user.html#front-end-message-types
[2] https://gitlab.com/qemu-project/qemu/-/blob/master/hw/virtio/vhost-vdpa.c#L1223
[3] https://gitlab.com/qemu-project/qemu/-/blob/master/hw/virtio/vhost-vdpa.c#L1253
[4] https://github.com/torvalds/linux/blob/master/drivers/vhost/vhost.c#L1992
[5] https://github.com/torvalds/linux/blob/master/drivers/vhost/vdpa.c#L740
[6] https://github.com/torvalds/linux/blob/master/drivers/vdpa/virtio_pci/vp_vdpa.c#L283