hw/virtio/vhost-shadow-virtqueue.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
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.112.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
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.
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
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.
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
>
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
> >
>
>
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?
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. >
On 2/13/2025 20:24, Lei Yang wrote: > vdpa's regression tests
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));
> }
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));
> > }
>
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.
© 2016 - 2025 Red Hat, Inc.