[PATCH] vdpa: Support setting vring_base for packed svq

[PATCH] vdpa: Support setting vring_base for packed svq

[Sahil Siddiq]

Linux commit v5.14-rc1~30^2~8 enabled the vp_vdpa driver to set the
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
(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;
+    }
+
     r = vhost_vdpa_set_dev_vring_base(dev, &s);
     if (unlikely(r)) {
         error_setg_errno(errp, -r, "Cannot set vring base");
-- 
2.47.0

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

[Stefano Garzarella]

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")

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

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

>+    }
>+
>     r = vhost_vdpa_set_dev_vring_base(dev, &s);
>     if (unlikely(r)) {
>         error_setg_errno(errp, -r, "Cannot set vring base");
>-- 
>2.47.0
>

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

[Sahil Siddiq]

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

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

[Stefano Garzarella]

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.

Is this okay with QEMU shadow vq? (More a question for Eugenio).


About the magic value, IMHO we should explain it in the code adding a 
comment, and maybe use VRING_PACKED_EVENT_F_WRAP_CTR, something like we 
do in virtqueue_init() in drivers/virtio/virtio_ring.c in Linux:

     uint32_t last_used_idx = 0 | (1 << VRING_PACKED_EVENT_F_WRAP_CTR);
     uint32_t last_avail_idx = 0 | (1 << VRING_PACKED_EVENT_F_WRAP_CTR);

     s.num = (last_used_idx << 16) | last_avail_idx;

WDYT?

Thanks,
Stefano

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

[Eugenio Perez Martin]

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>

>
> About the magic value, IMHO we should explain it in the code adding a
> comment, and maybe use VRING_PACKED_EVENT_F_WRAP_CTR, something like we
> do in virtqueue_init() in drivers/virtio/virtio_ring.c in Linux:
>
>      uint32_t last_used_idx = 0 | (1 << VRING_PACKED_EVENT_F_WRAP_CTR);
>      uint32_t last_avail_idx = 0 | (1 << VRING_PACKED_EVENT_F_WRAP_CTR);
>
>      s.num = (last_used_idx << 16) | last_avail_idx;

I agree, we can avoid the magic constant and the kernel's code is a
good source to follow when developing these changes.

Thanks!

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

[Sahil Siddiq]

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

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

[Michael S. Tsirkin]

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

Looks like a feature, not a bugfix to me, so I guess - next release?

> ---
> 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;
> +    }
> +
>      r = vhost_vdpa_set_dev_vring_base(dev, &s);
>      if (unlikely(r)) {
>          error_setg_errno(errp, -r, "Cannot set vring base");
> -- 
> 2.47.0

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

[Sahil Siddiq]

Hi,

On 11/14/24 12:28 PM, Michael S. Tsirkin 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
>> 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
>> (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>
> 
> Looks like a feature, not a bugfix to me, so I guess - next release?
> 

Right. Also, based on the discussion in the reviews [1], it's been decided
to consider this patch for release when the patches to add support for
packed svqs [2] are also ready.

I'll resend this patch with the other patches.

Thanks,
Sahil

[1] https://lists.nongnu.org/archive/html/qemu-devel/2024-11/msg01432.html
[2] https://wiki.qemu.org/Internships/ProjectIdeas/PackedShadowVirtqueue