| Message ID | 20250211162010.1478402-1-kshk@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vdpa: Fix endian bugs in shadow virtqueue | expand |
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!
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);
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.
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;
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(-)