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

[Sahil Siddiq <icegambit91@gmail.com>]

Hi,

On 11/6/24 8:30 PM, Eugenio Perez Martin wrote:
> On Wed, Nov 6, 2024 at 3:33 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Tue, Nov 05, 2024 at 08:24:17PM +0530, Sahil Siddiq wrote:
>>> 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 for these great details!
>>
>> Okay, so IIUC the only configuration that vp_vdpa supports when
>> VHOST_SET_VRING_BASE is called is idx == 0 and wrap_couter = true for
>> both avail and used.
>>
> 
> Right, it cannot set any other value as there is no standardized way
> in virtio. But vp_vdpa allows the default one, as QEMU always sends
> it.
> 
>> Is this okay with QEMU shadow vq? (More a question for Eugenio).
>>
> 
> Yes, it is a required step to support packed vq.
> 
> I misunderstood the previous thread and I thought the problem was
> somewhere else. It is hard to justify introducing this change in QEMU
> by itself, as the code is not reachable. However, when you post the
> whole series, you can make the two changes requested here (commit id,
> no magic number) and add my acked-by directly :):
> 
> Acked-by: Eugenio Pérez <eperezma@redhat.com>

Sure thing, this makes more sense. I'll do that.

Thanks,
Sahil