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

[Fabiano Rosas <farosas@suse.de>]

Thomas Huth <thuth@redhat.com> writes:

> 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...
>
>  v2: Use leXX_to_cpu() / cpu_to_leXX() instead of doing it manually
>
>  tests/qtest/libqos/virtio.c | 44 ++++++++++++++++++++++++-------------
>  1 file changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c
> index 2e7979652fd..5a709d0bc59 100644
> --- a/tests/qtest/libqos/virtio.c
> +++ b/tests/qtest/libqos/virtio.c
> @@ -25,49 +25,63 @@
>   */
>  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));
> +        val = le16_to_cpu(val);
> +    } else {
> +        val = 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);
> +    uint32_t val;
>  
> -    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, &val, sizeof(val));
> +        val = le32_to_cpu(val);
> +    } else {
> +        val = 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);
> +    if (d->features & (1ull << VIRTIO_F_VERSION_1)) {
> +        val = cpu_to_le16(val);
> +        qtest_memwrite(qts, addr, &val, sizeof(val));
> +    } 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);
> +    if (d->features & (1ull << VIRTIO_F_VERSION_1)) {
> +        val = cpu_to_le32(val);
> +        qtest_memwrite(qts, addr, &val, sizeof(val));
> +    } 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);
> +    if (d->features & (1ull << VIRTIO_F_VERSION_1)) {
> +        val = cpu_to_le64(val);
> +        qtest_memwrite(qts, addr, &val, sizeof(val));
> +    } else {
> +        qtest_writeq(qts, addr, val);
>      }
> -    qtest_writeq(qts, addr, val);
>  }
>  
>  uint8_t qvirtio_config_readb(QVirtioDevice *d, uint64_t addr)

Queued to qtest-next, thanks!