1. Patchew
  2. QEMU
  3. View series

[PATCH] tests/qtest/libqos: Avoid double swapping when using modern virtio

Thomas Huth posted 1 patch 6 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20250430073343.526867-1-thuth@redhat.com
Maintainers: Fabiano Rosas <farosas@suse.de>, Laurent Vivier <lvivier@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
There is a newer version of this series
tests/qtest/libqos/virtio.c | 61 ++++++++++++++++++++++++++-----------
1 file changed, 44 insertions(+), 17 deletions(-)
[PATCH] tests/qtest/libqos: Avoid double swapping when using modern virtio
Posted by Thomas Huth 6 months, 2 weeks ago 
From: Thomas Huth <thuth@redhat.com>

The logic in the qvirtio_read/write function is rather a headache,
involving byte-swapping when the target is big endian, just to
maybe involve another byte-swapping  in the qtest_read/write
function immediately afterwards (on the QEMU side). Let's do it in
a more obvious way here: For virtio 1.0, we know that the values have
to be little endian, so let's read/write the bytes in that well known
order here.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 This also decreases our usage of qtest_big_endian() which might (or
 might not) get helpful for the universal binary one day...

 tests/qtest/libqos/virtio.c | 61 ++++++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 17 deletions(-)

diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c
index 2e7979652fd..078adf3c8dc 100644
--- a/tests/qtest/libqos/virtio.c
+++ b/tests/qtest/libqos/virtio.c
@@ -25,49 +25,76 @@
  */
 static uint16_t qvirtio_readw(QVirtioDevice *d, QTestState *qts, uint64_t addr)
 {
-    uint16_t val = qtest_readw(qts, addr);
+    uint8_t buf[2];
 
-    if (d->features & (1ull << VIRTIO_F_VERSION_1) && qtest_big_endian(qts)) {
-        val = bswap16(val);
+    if (d->features & (1ull << VIRTIO_F_VERSION_1)) {
+        qtest_memread(qts, addr, buf, sizeof(buf));
+        return (buf[1] << 8) | buf[0];
+    } else {
+        return qtest_readw(qts, addr);
     }
-    return val;
 }
 
 static uint32_t qvirtio_readl(QVirtioDevice *d, QTestState *qts, uint64_t addr)
 {
-    uint32_t val = qtest_readl(qts, addr);
+    uint8_t buf[4];
 
-    if (d->features & (1ull << VIRTIO_F_VERSION_1) && qtest_big_endian(qts)) {
-        val = bswap32(val);
+    if (d->features & (1ull << VIRTIO_F_VERSION_1)) {
+        qtest_memread(qts, addr, buf, sizeof(buf));
+        return (buf[3] << 24) | (buf[2] << 16) | (buf[1] << 8) | buf[0];
+    } else {
+        return qtest_readl(qts, addr);
     }
-    return val;
 }
 
 static void qvirtio_writew(QVirtioDevice *d, QTestState *qts,
                            uint64_t addr, uint16_t val)
 {
-    if (d->features & (1ull << VIRTIO_F_VERSION_1) && qtest_big_endian(qts)) {
-        val = bswap16(val);
+    uint8_t buf[2];
+
+    if (d->features & (1ull << VIRTIO_F_VERSION_1)) {
+        buf[0] = val;
+        buf[1] = val >> 8;
+        qtest_memwrite(qts, addr, buf, sizeof(buf));
+    } else {
+        qtest_writew(qts, addr, val);
     }
-    qtest_writew(qts, addr, val);
 }
 
 static void qvirtio_writel(QVirtioDevice *d, QTestState *qts,
                            uint64_t addr, uint32_t val)
 {
-    if (d->features & (1ull << VIRTIO_F_VERSION_1) && qtest_big_endian(qts)) {
-        val = bswap32(val);
+    uint8_t buf[4];
+
+    if (d->features & (1ull << VIRTIO_F_VERSION_1)) {
+        buf[0] = val;
+        buf[1] = val >> 8;
+        buf[2] = val >> 16;
+        buf[3] = val >> 24;
+        qtest_memwrite(qts, addr, buf, sizeof(buf));
+    } else {
+        qtest_writel(qts, addr, val);
     }
-    qtest_writel(qts, addr, val);
 }
 
 static void qvirtio_writeq(QVirtioDevice *d, QTestState *qts,
                            uint64_t addr, uint64_t val)
 {
-    if (d->features & (1ull << VIRTIO_F_VERSION_1) && qtest_big_endian(qts)) {
-        val = bswap64(val);
+    uint8_t buf[8];
+
+    if (d->features & (1ull << VIRTIO_F_VERSION_1)) {
+        buf[0] = val;
+        buf[1] = val >> 8;
+        buf[2] = val >> 16;
+        buf[3] = val >> 24;
+        buf[4] = val >> 32;
+        buf[5] = val >> 40;
+        buf[6] = val >> 48;
+        buf[7] = val >> 56;
+        qtest_memwrite(qts, addr, buf, sizeof(buf));
+    } else {
+        qtest_writeq(qts, addr, val);
     }
-    qtest_writeq(qts, addr, val);
 }
 
 uint8_t qvirtio_config_readb(QVirtioDevice *d, uint64_t addr)
-- 
2.49.0
Re: [PATCH] tests/qtest/libqos: Avoid double swapping when using modern virtio
Posted by Philippe Mathieu-Daudé 6 months, 2 weeks ago 
On 30/4/25 09:33, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> The logic in the qvirtio_read/write function is rather a headache,
> involving byte-swapping when the target is big endian, just to
> maybe involve another byte-swapping  in the qtest_read/write
> function immediately afterwards (on the QEMU side). Let's do it in
> a more obvious way here: For virtio 1.0, we know that the values have
> to be little endian, so let's read/write the bytes in that well known
> order here.

Thanks for looking at this!

> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   This also decreases our usage of qtest_big_endian() which might (or
>   might not) get helpful for the universal binary one day...
> 
>   tests/qtest/libqos/virtio.c | 61 ++++++++++++++++++++++++++-----------
>   1 file changed, 44 insertions(+), 17 deletions(-)
> 
> diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c
> index 2e7979652fd..078adf3c8dc 100644
> --- a/tests/qtest/libqos/virtio.c
> +++ b/tests/qtest/libqos/virtio.c
> @@ -25,49 +25,76 @@
>    */
>   static uint16_t qvirtio_readw(QVirtioDevice *d, QTestState *qts, uint64_t addr)
>   {
> -    uint16_t val = qtest_readw(qts, addr);
> +    uint8_t buf[2];
>   
> -    if (d->features & (1ull << VIRTIO_F_VERSION_1) && qtest_big_endian(qts)) {
> -        val = bswap16(val);
> +    if (d->features & (1ull << VIRTIO_F_VERSION_1)) {
> +        qtest_memread(qts, addr, buf, sizeof(buf));
> +        return (buf[1] << 8) | buf[0];
> +    } else {
> +        return qtest_readw(qts, addr);
>       }
> -    return val;
>   }

What about using cpu_to_le() API?

-- >8 --
  static uint16_t qvirtio_readw(QVirtioDevice *d, QTestState *qts, 
uint64_t addr)
  {
-    uint16_t val = qtest_readw(qts, addr);
+    uint16_t val;

-    if (d->features & (1ull << VIRTIO_F_VERSION_1) && 
qtest_big_endian(qts)) {
-        val = bswap16(val);
+    if (d->features & (1ull << VIRTIO_F_VERSION_1)) {
+        qtest_memread(qts, addr, &val, sizeof(val));
+        cpu_to_le16s(&val);
+    } else {
+        val = qtest_readw(qts, addr);
      }
+
      return val;
  }
---
Re: [PATCH] tests/qtest/libqos: Avoid double swapping when using modern virtio
Posted by Thomas Huth 6 months, 2 weeks ago 
On 30/04/2025 10.40, Philippe Mathieu-Daudé wrote:
> On 30/4/25 09:33, Thomas Huth wrote:
>> From: Thomas Huth <thuth@redhat.com>
>>
>> The logic in the qvirtio_read/write function is rather a headache,
>> involving byte-swapping when the target is big endian, just to
>> maybe involve another byte-swapping  in the qtest_read/write
>> function immediately afterwards (on the QEMU side). Let's do it in
>> a more obvious way here: For virtio 1.0, we know that the values have
>> to be little endian, so let's read/write the bytes in that well known
>> order here.
> 
> Thanks for looking at this!
> 
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   This also decreases our usage of qtest_big_endian() which might (or
>>   might not) get helpful for the universal binary one day...
>>
>>   tests/qtest/libqos/virtio.c | 61 ++++++++++++++++++++++++++-----------
>>   1 file changed, 44 insertions(+), 17 deletions(-)
>>
>> diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c
>> index 2e7979652fd..078adf3c8dc 100644
>> --- a/tests/qtest/libqos/virtio.c
>> +++ b/tests/qtest/libqos/virtio.c
>> @@ -25,49 +25,76 @@
>>    */
>>   static uint16_t qvirtio_readw(QVirtioDevice *d, QTestState *qts, 
>> uint64_t addr)
>>   {
>> -    uint16_t val = qtest_readw(qts, addr);
>> +    uint8_t buf[2];
>> -    if (d->features & (1ull << VIRTIO_F_VERSION_1) && 
>> qtest_big_endian(qts)) {
>> -        val = bswap16(val);
>> +    if (d->features & (1ull << VIRTIO_F_VERSION_1)) {
>> +        qtest_memread(qts, addr, buf, sizeof(buf));
>> +        return (buf[1] << 8) | buf[0];
>> +    } else {
>> +        return qtest_readw(qts, addr);
>>       }
>> -    return val;
>>   }
> 
> What about using cpu_to_le() API?
> 
> -- >8 --
>   static uint16_t qvirtio_readw(QVirtioDevice *d, QTestState *qts, uint64_t 
> addr)
>   {
> -    uint16_t val = qtest_readw(qts, addr);
> +    uint16_t val;
> 
> -    if (d->features & (1ull << VIRTIO_F_VERSION_1) && qtest_big_endian(qts)) {
> -        val = bswap16(val);
> +    if (d->features & (1ull << VIRTIO_F_VERSION_1)) {
> +        qtest_memread(qts, addr, &val, sizeof(val));
> +        cpu_to_le16s(&val);
> +    } else {
> +        val = qtest_readw(qts, addr);
>       }
> +
>       return val;
>   }

D'oh, not sure how I could have forgotten about these functions, that's much 
nicer indeed! I'll respin my patch with that, thanks for the reminder!

  Thomas

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.