[PATCH 0/2] change IVSHMEM endianess to LITTLE_ENDIAN

[PATCH 0/2] change IVSHMEM endianess to LITTLE_ENDIAN

[Daniel Henrique Barboza]

Hi,

This small series fixes an issue reported in Gitlab [1] that
affects PowerPC big-endian and little-endian and probably all
other big-endians in the wild that might use 'ivshmem'.

It's not clear to me who is the maintainer/responsible for this device
(MAINTAINERS doesn't seem to have any 'ivhshmem' entries nor someone
that looks upon all hw/misc/* files) so I didn't add any CC in that
regard. 'qemu-ppc' is being copied for awareness since they are the
folks that are most likely being impacted by the bug.

[1] https://gitlab.com/qemu-project/qemu/-/issues/168

Daniel Henrique Barboza (2):
  ivshmem.c: change endianness to LITTLE_ENDIAN
  ivshmem-test.c: enable test_ivshmem_server for ppc64 arch

 hw/misc/ivshmem.c          | 2 +-
 tests/qtest/ivshmem-test.c | 5 +----
 2 files changed, 2 insertions(+), 5 deletions(-)

-- 
2.31.1

[PATCH 1/2] ivshmem.c: change endianness to LITTLE_ENDIAN

[Daniel Henrique Barboza]

The ivshmem device, as with most PCI devices, uses little endian byte
order. However, the endianess of its mmio_ops is marked as
DEVICE_NATIVE_ENDIAN. This presents not only the usual problems with big
endian hosts but also with PowerPC little endian hosts as well, since
the Power architecture in QEMU uses big endian hardware (XIVE controller,
PCI Host Bridges, etc) even if the host is in little endian byte order.

As it is today, the IVPosition of the device will be byte swapped when
running in Power BE and LE. This can be seen by changing the existing
qtest 'ivshmem-test' to run in ppc64 hosts and printing the IVPOSITION
regs in test_ivshmem_server() right after the VM ids assert. For x86_64
the VM id values read are '0' and '1', for ppc64 (tested in a Power8
RHEL 7.9 BE server) and ppc64le (tested in a Power9 RHEL 8.6 LE server)
the ids will be '0' and '0x1000000'.

Change this device to LITTLE_ENDIAN fixes the issue for Power hosts of
both endianess, and every other big-endian architecture that might use
this device, without impacting x86 users.

Fixes: cb06608e17f8 ("ivshmem: convert to memory API")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/168
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/misc/ivshmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 1ba4a98377..299837e5c1 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -243,7 +243,7 @@ static uint64_t ivshmem_io_read(void *opaque, hwaddr addr,
 static const MemoryRegionOps ivshmem_mmio_ops = {
     .read = ivshmem_io_read,
     .write = ivshmem_io_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .endianness = DEVICE_LITTLE_ENDIAN,
     .impl = {
         .min_access_size = 4,
         .max_access_size = 4,
-- 
2.31.1

[PATCH 2/2] ivshmem-test.c: enable test_ivshmem_server for ppc64 arch

[Daniel Henrique Barboza]

This test, if enabled by hand, was failing when the ivhsmem device was
being declared as DEVICE_NATIVE_ENDIAN with the following error:

/ppc64/ivshmem/pair: OK
/ppc64/ivshmem/server:
**
ERROR:/home/danielhb/qemu/tests/qtest/ivshmem-test.c:367:test_ivshmem_server:
assertion failed (ret != 0): (0 != 0)
Aborted

After the endianess change done in the previous patch, we can verify in
both a a Power 9 little-endian host and in a Power 8 big-endian host
that this test is now passing:

$ QTEST_QEMU_BINARY=./ppc64-softmmu/qemu-system-ppc64 ./tests/qtest/ivshmem-test -m slow
/ppc64/ivshmem/single: OK
/ppc64/ivshmem/hotplug: OK
/ppc64/ivshmem/memdev: OK
/ppc64/ivshmem/pair: OK
/ppc64/ivshmem/server: OK

Let's keep it that way by officialy enabling it for ppc64.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 tests/qtest/ivshmem-test.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tests/qtest/ivshmem-test.c b/tests/qtest/ivshmem-test.c
index dfa69424ed..fe94dd3b96 100644
--- a/tests/qtest/ivshmem-test.c
+++ b/tests/qtest/ivshmem-test.c
@@ -463,7 +463,6 @@ static gchar *mktempshm(int size, int *fd)
 int main(int argc, char **argv)
 {
     int ret, fd;
-    const char *arch = qtest_get_arch();
     gchar dir[] = "/tmp/ivshmem-test.XXXXXX";
 
     g_test_init(&argc, &argv, NULL);
@@ -488,9 +487,7 @@ int main(int argc, char **argv)
     qtest_add_func("/ivshmem/memdev", test_ivshmem_memdev);
     if (g_test_slow()) {
         qtest_add_func("/ivshmem/pair", test_ivshmem_pair);
-        if (strcmp(arch, "ppc64") != 0) {
-            qtest_add_func("/ivshmem/server", test_ivshmem_server);
-        }
+        qtest_add_func("/ivshmem/server", test_ivshmem_server);
     }
 
 out:
-- 
2.31.1

Re: [PATCH 2/2] ivshmem-test.c: enable test_ivshmem_server for ppc64 arch

[Thomas Huth]

On 23/11/2021 22.19, Daniel Henrique Barboza wrote:
> This test, if enabled by hand, was failing when the ivhsmem device was
> being declared as DEVICE_NATIVE_ENDIAN with the following error:
> 
> /ppc64/ivshmem/pair: OK
> /ppc64/ivshmem/server:
> **
> ERROR:/home/danielhb/qemu/tests/qtest/ivshmem-test.c:367:test_ivshmem_server:
> assertion failed (ret != 0): (0 != 0)
> Aborted
> 
> After the endianess change done in the previous patch, we can verify in

s/endianess/endianness/

> both a a Power 9 little-endian host and in a Power 8 big-endian host
> that this test is now passing:
> 
> $ QTEST_QEMU_BINARY=./ppc64-softmmu/qemu-system-ppc64 ./tests/qtest/ivshmem-test -m slow
> /ppc64/ivshmem/single: OK
> /ppc64/ivshmem/hotplug: OK
> /ppc64/ivshmem/memdev: OK
> /ppc64/ivshmem/pair: OK
> /ppc64/ivshmem/server: OK
> 
> Let's keep it that way by officialy enabling it for ppc64.

s/officialy/officially/

> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   tests/qtest/ivshmem-test.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/tests/qtest/ivshmem-test.c b/tests/qtest/ivshmem-test.c
> index dfa69424ed..fe94dd3b96 100644
> --- a/tests/qtest/ivshmem-test.c
> +++ b/tests/qtest/ivshmem-test.c
> @@ -463,7 +463,6 @@ static gchar *mktempshm(int size, int *fd)
>   int main(int argc, char **argv)
>   {
>       int ret, fd;
> -    const char *arch = qtest_get_arch();
>       gchar dir[] = "/tmp/ivshmem-test.XXXXXX";
>   
>       g_test_init(&argc, &argv, NULL);
> @@ -488,9 +487,7 @@ int main(int argc, char **argv)
>       qtest_add_func("/ivshmem/memdev", test_ivshmem_memdev);
>       if (g_test_slow()) {
>           qtest_add_func("/ivshmem/pair", test_ivshmem_pair);
> -        if (strcmp(arch, "ppc64") != 0) {
> -            qtest_add_func("/ivshmem/server", test_ivshmem_server);
> -        }
> +        qtest_add_func("/ivshmem/server", test_ivshmem_server);
>       }

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 0/2] change IVSHMEM endianess to LITTLE_ENDIAN

[Cédric Le Goater]

On 11/23/21 22:19, Daniel Henrique Barboza wrote:
> Hi,
> 
> This small series fixes an issue reported in Gitlab [1] that
> affects PowerPC big-endian and little-endian and probably all
> other big-endians in the wild that might use 'ivshmem'.
> 
> It's not clear to me who is the maintainer/responsible for this device
> (MAINTAINERS doesn't seem to have any 'ivhshmem' entries nor someone
> that looks upon all hw/misc/* files) so I didn't add any CC in that
> regard. 'qemu-ppc' is being copied for awareness since they are the
> folks that are most likely being impacted by the bug.
> 
> [1] https://gitlab.com/qemu-project/qemu/-/issues/168

Do we want these fixes for 6.2 ?

Thanks,

C.

> 
> Daniel Henrique Barboza (2):
>    ivshmem.c: change endianness to LITTLE_ENDIAN
>    ivshmem-test.c: enable test_ivshmem_server for ppc64 arch
> 
>   hw/misc/ivshmem.c          | 2 +-
>   tests/qtest/ivshmem-test.c | 5 +----
>   2 files changed, 2 insertions(+), 5 deletions(-)
> 

Re: [PATCH 0/2] change IVSHMEM endianess to LITTLE_ENDIAN

[Daniel Henrique Barboza]

On 11/23/21 20:22, Cédric Le Goater wrote:
> On 11/23/21 22:19, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> This small series fixes an issue reported in Gitlab [1] that
>> affects PowerPC big-endian and little-endian and probably all
>> other big-endians in the wild that might use 'ivshmem'.
>>
>> It's not clear to me who is the maintainer/responsible for this device
>> (MAINTAINERS doesn't seem to have any 'ivhshmem' entries nor someone
>> that looks upon all hw/misc/* files) so I didn't add any CC in that
>> regard. 'qemu-ppc' is being copied for awareness since they are the
>> folks that are most likely being impacted by the bug.
>>
>> [1] https://gitlab.com/qemu-project/qemu/-/issues/168
> 
> Do we want these fixes for 6.2 ?


No, I don't think it's necessary. Changing endianess is something that I'd rather
do in the start of the 7.0 cycle. This bug has been around for years at this
point. It can wait a couple of months.

I'll re-send these with Thomas' Ack and corrections and with a "for-7.0" subject
for extra clarity.


Thanks,


Daniel



> 
> Thanks,
> 
> C.
> 
>>
>> Daniel Henrique Barboza (2):
>>    ivshmem.c: change endianness to LITTLE_ENDIAN
>>    ivshmem-test.c: enable test_ivshmem_server for ppc64 arch
>>
>>   hw/misc/ivshmem.c          | 2 +-
>>   tests/qtest/ivshmem-test.c | 5 +----
>>   2 files changed, 2 insertions(+), 5 deletions(-)
>>
>