[PATCH] vdpa: Fix endian bugs in shadow virtqueue

[PATCH] 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>
---
 hw/virtio/vhost-shadow-virtqueue.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index 37aca8b431..b3c83f0dfa 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -157,7 +157,7 @@ static bool vhost_svq_vring_write_descs(VhostShadowVirtqueue *svq, hwaddr *sg,
     for (n = 0; n < num; n++) {
         if (more_descs || (n + 1 < num)) {
             descs[i].flags = flags | cpu_to_le16(VRING_DESC_F_NEXT);
-            descs[i].next = cpu_to_le16(svq->desc_next[i]);
+            descs[i].next = svq->desc_next[i];
         } else {
             descs[i].flags = flags;
         }
@@ -165,7 +165,7 @@ 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 = le16_to_cpu(svq->desc_next[i]);
     }
 
     svq->free_head = le16_to_cpu(svq->desc_next[last]);
@@ -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);
     }
@@ -449,7 +451,7 @@ static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
     num = svq->desc_state[used_elem.id].ndescs;
     svq->desc_state[used_elem.id].ndescs = 0;
     last_used_chain = vhost_svq_last_desc_of_chain(svq, num, used_elem.id);
-    svq->desc_next[last_used_chain] = svq->free_head;
+    svq->desc_next[last_used_chain] = cpu_to_le16(svq->free_head);
     svq->free_head = used_elem.id;
     svq->num_free += num;
 
-- 
2.34.1

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

[Eugenio Perez Martin]

On Tue, Feb 11, 2025 at 5:20 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.
>

Thank you very much for this!

> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
> ---
>  hw/virtio/vhost-shadow-virtqueue.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> index 37aca8b431..b3c83f0dfa 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -157,7 +157,7 @@ static bool vhost_svq_vring_write_descs(VhostShadowVirtqueue *svq, hwaddr *sg,
>      for (n = 0; n < num; n++) {
>          if (more_descs || (n + 1 < num)) {
>              descs[i].flags = flags | cpu_to_le16(VRING_DESC_F_NEXT);
> -            descs[i].next = cpu_to_le16(svq->desc_next[i]);
> +            descs[i].next = svq->desc_next[i];
>          } else {
>              descs[i].flags = flags;
>          }
> @@ -165,7 +165,7 @@ 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 = le16_to_cpu(svq->desc_next[i]);

Both svq->desc_next and "i" are in QEMU. We can skip the conversion
and assign directly.

>      }
>
>      svq->free_head = le16_to_cpu(svq->desc_next[last]);
> @@ -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);
>      }
> @@ -449,7 +451,7 @@ static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
>      num = svq->desc_state[used_elem.id].ndescs;
>      svq->desc_state[used_elem.id].ndescs = 0;
>      last_used_chain = vhost_svq_last_desc_of_chain(svq, num, used_elem.id);
> -    svq->desc_next[last_used_chain] = svq->free_head;
> +    svq->desc_next[last_used_chain] = cpu_to_le16(svq->free_head);

And skip this one too.

With that,

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

Thanks!

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

[Konstantin Shkolnyy]

On 2/12/2025 09:06, Eugenio Perez Martin wrote:
>> -        i = cpu_to_le16(svq->desc_next[i]);
>> +        i = le16_to_cpu(svq->desc_next[i]);
> 
> Both svq->desc_next and "i" are in QEMU. We can skip the conversion
> and assign directly.

Are you saying that desc_next[] should be in "CPU" and not LE format?

The original code contained statements (below) that led me to think that 
desc_next[] was designed to be LE...

vhost_svq_last_desc_of_chain()
         i = le16_to_cpu(svq->desc_next[i]);

vhost_svq_start()
         svq->desc_next[i] = cpu_to_le16(i + 1);

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

[Eugenio Perez Martin]

On Wed, Feb 12, 2025 at 4:37 PM Konstantin Shkolnyy <kshk@linux.ibm.com> wrote:
>
> On 2/12/2025 09:06, Eugenio Perez Martin wrote:
> >> -        i = cpu_to_le16(svq->desc_next[i]);
> >> +        i = le16_to_cpu(svq->desc_next[i]);
> >
> > Both svq->desc_next and "i" are in QEMU. We can skip the conversion
> > and assign directly.
>
> Are you saying that desc_next[] should be in "CPU" and not LE format?
>
> The original code contained statements (below) that led me to think that
> desc_next[] was designed to be LE...
>
> vhost_svq_last_desc_of_chain()
>          i = le16_to_cpu(svq->desc_next[i]);
>
> vhost_svq_start()
>          svq->desc_next[i] = cpu_to_le16(i + 1);
>

Yes, I did a mess with the endianness back then :(. But we can remove
both conversions, and add the conversion at
vhost_svq_vring_write_descs instead.