[PATCH v2] vdpa: Fix endian bugs in shadow virtqueue

[PATCH v2] vdpa: Fix endian bugs in shadow virtqueue

[Konstantin Shkolnyy]

VDPA didn't work on a big-endian machine due to missing/incorrect
CPU<->LE data format conversions.

Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
---
Changes in v2: Change desc_next[] from LE format to "CPU".

 hw/virtio/vhost-shadow-virtqueue.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index 37aca8b431..4af0d7c669 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -165,10 +165,10 @@ static bool vhost_svq_vring_write_descs(VhostShadowVirtqueue *svq, hwaddr *sg,
         descs[i].len = cpu_to_le32(iovec[n].iov_len);
 
         last = i;
-        i = cpu_to_le16(svq->desc_next[i]);
+        i = svq->desc_next[i];
     }
 
-    svq->free_head = le16_to_cpu(svq->desc_next[last]);
+    svq->free_head = svq->desc_next[last];
     return true;
 }
 
@@ -228,10 +228,12 @@ static void vhost_svq_kick(VhostShadowVirtqueue *svq)
     smp_mb();
 
     if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
-        uint16_t avail_event = *(uint16_t *)(&svq->vring.used->ring[svq->vring.num]);
+        uint16_t avail_event = le16_to_cpu(
+                *(uint16_t *)(&svq->vring.used->ring[svq->vring.num]));
         needs_kick = vring_need_event(avail_event, svq->shadow_avail_idx, svq->shadow_avail_idx - 1);
     } else {
-        needs_kick = !(svq->vring.used->flags & VRING_USED_F_NO_NOTIFY);
+        needs_kick =
+                !(svq->vring.used->flags & cpu_to_le16(VRING_USED_F_NO_NOTIFY));
     }
 
     if (!needs_kick) {
@@ -365,7 +367,7 @@ static bool vhost_svq_more_used(VhostShadowVirtqueue *svq)
         return true;
     }
 
-    svq->shadow_used_idx = cpu_to_le16(*(volatile uint16_t *)used_idx);
+    svq->shadow_used_idx = le16_to_cpu(*(volatile uint16_t *)used_idx);
 
     return svq->last_used_idx != svq->shadow_used_idx;
 }
@@ -383,7 +385,7 @@ static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq)
 {
     if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
         uint16_t *used_event = (uint16_t *)&svq->vring.avail->ring[svq->vring.num];
-        *used_event = svq->shadow_used_idx;
+        *used_event = cpu_to_le16(svq->shadow_used_idx);
     } else {
         svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
     }
@@ -408,7 +410,7 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
                                              uint16_t num, uint16_t i)
 {
     for (uint16_t j = 0; j < (num - 1); ++j) {
-        i = le16_to_cpu(svq->desc_next[i]);
+        i = svq->desc_next[i];
     }
 
     return i;
@@ -683,7 +685,7 @@ void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
     svq->desc_state = g_new0(SVQDescState, svq->vring.num);
     svq->desc_next = g_new0(uint16_t, svq->vring.num);
     for (unsigned i = 0; i < svq->vring.num - 1; i++) {
-        svq->desc_next[i] = cpu_to_le16(i + 1);
+        svq->desc_next[i] = i + 1;
     }
 }
 
-- 
2.34.1

Re: [PATCH v2] vdpa: Fix endian bugs in shadow virtqueue

[Philippe Mathieu-Daudé]

On 12/2/25 17:49, Konstantin Shkolnyy wrote:
> VDPA didn't work on a big-endian machine due to missing/incorrect
> CPU<->LE data format conversions.
> 
> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
> ---
> Changes in v2: Change desc_next[] from LE format to "CPU".
> 
>   hw/virtio/vhost-shadow-virtqueue.c | 18 ++++++++++--------
>   1 file changed, 10 insertions(+), 8 deletions(-)


> @@ -228,10 +228,12 @@ static void vhost_svq_kick(VhostShadowVirtqueue *svq)
>       smp_mb();
>   
>       if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> -        uint16_t avail_event = *(uint16_t *)(&svq->vring.used->ring[svq->vring.num]);
> +        uint16_t avail_event = le16_to_cpu(
> +                *(uint16_t *)(&svq->vring.used->ring[svq->vring.num]));

Nitpicking, sometimes using the ld/st API is cleaner (here lduw_le_p).

>           needs_kick = vring_need_event(avail_event, svq->shadow_avail_idx, svq->shadow_avail_idx - 1);
>       } else {
> -        needs_kick = !(svq->vring.used->flags & VRING_USED_F_NO_NOTIFY);
> +        needs_kick =
> +                !(svq->vring.used->flags & cpu_to_le16(VRING_USED_F_NO_NOTIFY));
>       }

Re: [PATCH v2] vdpa: Fix endian bugs in shadow virtqueue

[Eugenio Perez Martin]

On Wed, Feb 12, 2025 at 7:11 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 12/2/25 17:49, Konstantin Shkolnyy wrote:
> > VDPA didn't work on a big-endian machine due to missing/incorrect
> > CPU<->LE data format conversions.
> >
> > Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
> > ---
> > Changes in v2: Change desc_next[] from LE format to "CPU".
> >
> >   hw/virtio/vhost-shadow-virtqueue.c | 18 ++++++++++--------
> >   1 file changed, 10 insertions(+), 8 deletions(-)
>
>
> > @@ -228,10 +228,12 @@ static void vhost_svq_kick(VhostShadowVirtqueue *svq)
> >       smp_mb();
> >
> >       if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > -        uint16_t avail_event = *(uint16_t *)(&svq->vring.used->ring[svq->vring.num]);
> > +        uint16_t avail_event = le16_to_cpu(
> > +                *(uint16_t *)(&svq->vring.used->ring[svq->vring.num]));
>
> Nitpicking, sometimes using the ld/st API is cleaner (here lduw_le_p).
>

I'm not sure if it is right in SVQ, as it is not accessing guest
memory but QEMU memory that has been mapped to a device. But if you
think it is still a valid use case for ld* and st* family I'd be
totally ok with that usage.

> >           needs_kick = vring_need_event(avail_event, svq->shadow_avail_idx, svq->shadow_avail_idx - 1);
> >       } else {
> > -        needs_kick = !(svq->vring.used->flags & VRING_USED_F_NO_NOTIFY);
> > +        needs_kick =
> > +                !(svq->vring.used->flags & cpu_to_le16(VRING_USED_F_NO_NOTIFY));
> >       }
>

Re: [PATCH v2] vdpa: Fix endian bugs in shadow virtqueue

[Philippe Mathieu-Daudé]

On 13/2/25 07:43, Eugenio Perez Martin wrote:
> On Wed, Feb 12, 2025 at 7:11 PM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> On 12/2/25 17:49, Konstantin Shkolnyy wrote:
>>> VDPA didn't work on a big-endian machine due to missing/incorrect
>>> CPU<->LE data format conversions.
>>>
>>> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
>>> ---
>>> Changes in v2: Change desc_next[] from LE format to "CPU".
>>>
>>>    hw/virtio/vhost-shadow-virtqueue.c | 18 ++++++++++--------
>>>    1 file changed, 10 insertions(+), 8 deletions(-)
>>
>>
>>> @@ -228,10 +228,12 @@ static void vhost_svq_kick(VhostShadowVirtqueue *svq)
>>>        smp_mb();
>>>
>>>        if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
>>> -        uint16_t avail_event = *(uint16_t *)(&svq->vring.used->ring[svq->vring.num]);
>>> +        uint16_t avail_event = le16_to_cpu(
>>> +                *(uint16_t *)(&svq->vring.used->ring[svq->vring.num]));
>>
>> Nitpicking, sometimes using the ld/st API is cleaner (here lduw_le_p).
>>
> 
> I'm not sure if it is right in SVQ, as it is not accessing guest
> memory but QEMU memory that has been mapped to a device. But if you
> think it is still a valid use case for ld* and st* family I'd be
> totally ok with that usage.

No need to change, better use a consistent API over the file.

Regards,

Phil.

Re: [PATCH v2] vdpa: Fix endian bugs in shadow virtqueue

[Eugenio Perez Martin]

On Wed, Feb 12, 2025 at 5:49 PM Konstantin Shkolnyy <kshk@linux.ibm.com> wrote:
>
> VDPA didn't work on a big-endian machine due to missing/incorrect
> CPU<->LE data format conversions.
>

Fixes: 10857ec0ad ("vhost: Add VhostShadowVirtqueue")

> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>

Acked-by: Eugenio Pérez <eperezma@redhat.com>

Thanks!

> ---
> Changes in v2: Change desc_next[] from LE format to "CPU".
>
>  hw/virtio/vhost-shadow-virtqueue.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> index 37aca8b431..4af0d7c669 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -165,10 +165,10 @@ static bool vhost_svq_vring_write_descs(VhostShadowVirtqueue *svq, hwaddr *sg,
>          descs[i].len = cpu_to_le32(iovec[n].iov_len);
>
>          last = i;
> -        i = cpu_to_le16(svq->desc_next[i]);
> +        i = svq->desc_next[i];
>      }
>
> -    svq->free_head = le16_to_cpu(svq->desc_next[last]);
> +    svq->free_head = svq->desc_next[last];
>      return true;
>  }
>
> @@ -228,10 +228,12 @@ static void vhost_svq_kick(VhostShadowVirtqueue *svq)
>      smp_mb();
>
>      if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> -        uint16_t avail_event = *(uint16_t *)(&svq->vring.used->ring[svq->vring.num]);
> +        uint16_t avail_event = le16_to_cpu(
> +                *(uint16_t *)(&svq->vring.used->ring[svq->vring.num]));
>          needs_kick = vring_need_event(avail_event, svq->shadow_avail_idx, svq->shadow_avail_idx - 1);
>      } else {
> -        needs_kick = !(svq->vring.used->flags & VRING_USED_F_NO_NOTIFY);
> +        needs_kick =
> +                !(svq->vring.used->flags & cpu_to_le16(VRING_USED_F_NO_NOTIFY));
>      }
>
>      if (!needs_kick) {
> @@ -365,7 +367,7 @@ static bool vhost_svq_more_used(VhostShadowVirtqueue *svq)
>          return true;
>      }
>
> -    svq->shadow_used_idx = cpu_to_le16(*(volatile uint16_t *)used_idx);
> +    svq->shadow_used_idx = le16_to_cpu(*(volatile uint16_t *)used_idx);
>
>      return svq->last_used_idx != svq->shadow_used_idx;
>  }
> @@ -383,7 +385,7 @@ static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq)
>  {
>      if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
>          uint16_t *used_event = (uint16_t *)&svq->vring.avail->ring[svq->vring.num];
> -        *used_event = svq->shadow_used_idx;
> +        *used_event = cpu_to_le16(svq->shadow_used_idx);
>      } else {
>          svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
>      }
> @@ -408,7 +410,7 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
>                                               uint16_t num, uint16_t i)
>  {
>      for (uint16_t j = 0; j < (num - 1); ++j) {
> -        i = le16_to_cpu(svq->desc_next[i]);
> +        i = svq->desc_next[i];
>      }
>
>      return i;
> @@ -683,7 +685,7 @@ void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
>      svq->desc_state = g_new0(SVQDescState, svq->vring.num);
>      svq->desc_next = g_new0(uint16_t, svq->vring.num);
>      for (unsigned i = 0; i < svq->vring.num - 1; i++) {
> -        svq->desc_next[i] = cpu_to_le16(i + 1);
> +        svq->desc_next[i] = i + 1;
>      }
>  }
>
> --
> 2.34.1
>

Re: [PATCH v2] vdpa: Fix endian bugs in shadow virtqueue

[Lei Yang]

I tested this patch with vdpa's regression tests, everything works fine.

Tested-by: Lei Yang <leiyang@redhat.com>

On Thu, Feb 13, 2025 at 2:51 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Wed, Feb 12, 2025 at 5:49 PM Konstantin Shkolnyy <kshk@linux.ibm.com> wrote:
> >
> > VDPA didn't work on a big-endian machine due to missing/incorrect
> > CPU<->LE data format conversions.
> >
>
> Fixes: 10857ec0ad ("vhost: Add VhostShadowVirtqueue")
>
> > Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
>
> Acked-by: Eugenio Pérez <eperezma@redhat.com>
>
> Thanks!
>
> > ---
> > Changes in v2: Change desc_next[] from LE format to "CPU".
> >
> >  hw/virtio/vhost-shadow-virtqueue.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > index 37aca8b431..4af0d7c669 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -165,10 +165,10 @@ static bool vhost_svq_vring_write_descs(VhostShadowVirtqueue *svq, hwaddr *sg,
> >          descs[i].len = cpu_to_le32(iovec[n].iov_len);
> >
> >          last = i;
> > -        i = cpu_to_le16(svq->desc_next[i]);
> > +        i = svq->desc_next[i];
> >      }
> >
> > -    svq->free_head = le16_to_cpu(svq->desc_next[last]);
> > +    svq->free_head = svq->desc_next[last];
> >      return true;
> >  }
> >
> > @@ -228,10 +228,12 @@ static void vhost_svq_kick(VhostShadowVirtqueue *svq)
> >      smp_mb();
> >
> >      if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > -        uint16_t avail_event = *(uint16_t *)(&svq->vring.used->ring[svq->vring.num]);
> > +        uint16_t avail_event = le16_to_cpu(
> > +                *(uint16_t *)(&svq->vring.used->ring[svq->vring.num]));
> >          needs_kick = vring_need_event(avail_event, svq->shadow_avail_idx, svq->shadow_avail_idx - 1);
> >      } else {
> > -        needs_kick = !(svq->vring.used->flags & VRING_USED_F_NO_NOTIFY);
> > +        needs_kick =
> > +                !(svq->vring.used->flags & cpu_to_le16(VRING_USED_F_NO_NOTIFY));
> >      }
> >
> >      if (!needs_kick) {
> > @@ -365,7 +367,7 @@ static bool vhost_svq_more_used(VhostShadowVirtqueue *svq)
> >          return true;
> >      }
> >
> > -    svq->shadow_used_idx = cpu_to_le16(*(volatile uint16_t *)used_idx);
> > +    svq->shadow_used_idx = le16_to_cpu(*(volatile uint16_t *)used_idx);
> >
> >      return svq->last_used_idx != svq->shadow_used_idx;
> >  }
> > @@ -383,7 +385,7 @@ static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq)
> >  {
> >      if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> >          uint16_t *used_event = (uint16_t *)&svq->vring.avail->ring[svq->vring.num];
> > -        *used_event = svq->shadow_used_idx;
> > +        *used_event = cpu_to_le16(svq->shadow_used_idx);
> >      } else {
> >          svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> >      }
> > @@ -408,7 +410,7 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> >                                               uint16_t num, uint16_t i)
> >  {
> >      for (uint16_t j = 0; j < (num - 1); ++j) {
> > -        i = le16_to_cpu(svq->desc_next[i]);
> > +        i = svq->desc_next[i];
> >      }
> >
> >      return i;
> > @@ -683,7 +685,7 @@ void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
> >      svq->desc_state = g_new0(SVQDescState, svq->vring.num);
> >      svq->desc_next = g_new0(uint16_t, svq->vring.num);
> >      for (unsigned i = 0; i < svq->vring.num - 1; i++) {
> > -        svq->desc_next[i] = cpu_to_le16(i + 1);
> > +        svq->desc_next[i] = i + 1;
> >      }
> >  }
> >
> > --
> > 2.34.1
> >
>
>

Re: [PATCH v2] vdpa: Fix endian bugs in shadow virtqueue

[Konstantin Shkolnyy]

On 2/13/2025 20:24, Lei Yang wrote:
> vdpa's regression tests

Re: [PATCH v2] vdpa: Fix endian bugs in shadow virtqueue

[Konstantin Shkolnyy]

On 2/13/2025 20:24, Lei Yang wrote:
> I tested this patch with vdpa's regression tests, everything works fine.
> 
> Tested-by: Lei Yang <leiyang@redhat.com>

Could you point me to those tests?

Re: [PATCH v2] vdpa: Fix endian bugs in shadow virtqueue

[Lei Yang]

On Fri, Feb 14, 2025 at 9:02 PM Konstantin Shkolnyy <kshk@linux.ibm.com> wrote:
>
> On 2/13/2025 20:24, Lei Yang wrote:
> > I tested this patch with vdpa's regression tests, everything works fine.
> >
> > Tested-by: Lei Yang <leiyang@redhat.com>
>
> Could you point me to those tests?

Sure, the test scenarios include ping, mq tests under netperf stress,
hotplug/unplug, reboot,shutdown, pxe_boot etc.
>

Re: [PATCH v2] vdpa: Fix endian bugs in shadow virtqueue

[Michael Tokarev]

12.02.2025 19:49, Konstantin Shkolnyy wrote:
> VDPA didn't work on a big-endian machine due to missing/incorrect
> CPU<->LE data format conversions.
> 
> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>

This looks like a qemu-stable material.
Please let me know if it is not.

Thanks,

/mjt

Re: [PATCH v2] vdpa: Fix endian bugs in shadow virtqueue

[Konstantin Shkolnyy]

On 2/25/2025 03:30, Michael Tokarev wrote:
> 12.02.2025 19:49, Konstantin Shkolnyy wrote:
>> VDPA didn't work on a big-endian machine due to missing/incorrect
>> CPU<->LE data format conversions.
>>
>> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
> 
> This looks like a qemu-stable material.
> Please let me know if it is not.

It won't help without my other "[PATCH v2] vdpa: Allow vDPA to work on 
big-endian machine". With both patches, VDPA works on a big-endian machine.

Re: [PATCH v2] vdpa: Fix endian bugs in shadow virtqueue

[Michael Tokarev]

25.02.2025 15:39, Konstantin Shkolnyy wrote:
> On 2/25/2025 03:30, Michael Tokarev wrote:

>> This looks like a qemu-stable material.
>> Please let me know if it is not.
> 
> It won't help without my other "[PATCH v2] vdpa: Allow vDPA to work on big-endian machine". With both patches, VDPA works on a big-endian machine.

Aha. And it is not in master yet.  Thank you for letting me know!

How do you think, is it worth the effort to pick these up for
older stable releases (7.2, 8.2) too?

Thanks,

/mjt

Re: [PATCH v2] vdpa: Fix endian bugs in shadow virtqueue

[Konstantin Shkolnyy]

On 2/27/2025 00:33, Michael Tokarev wrote:
> 25.02.2025 15:39, Konstantin Shkolnyy wrote:
>> On 2/25/2025 03:30, Michael Tokarev wrote:
> 
>>> This looks like a qemu-stable material.
>>> Please let me know if it is not.
>>
>> It won't help without my other "[PATCH v2] vdpa: Allow vDPA to work on 
>> big-endian machine". With both patches, VDPA works on a big-endian 
>> machine.
> 
> Aha. And it is not in master yet.  Thank you for letting me know!
> 
> How do you think, is it worth the effort to pick these up for
> older stable releases (7.2, 8.2) too?

Yes. It's legitimate bugfixes. I suspect, more and more people will use 
VDPA as time goes by, so someone might try it on s390 with a stable QEMU.