| Message ID | 20211124092948.335389-2-danielhb413@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | change ivshmem endianness to LITTLE_ENDIAN | expand |
On 11/24/21 10:29, Daniel Henrique Barboza wrote: > The ivshmem device, as with most PCI devices, uses little endian byte > order. However, the endianness 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. Maybe mention commit f7a199b2b44 ("ivshmem: use little-endian int64_t for the protocol")? > 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 endianness, 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, > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On 11/24/21 08:28, Philippe Mathieu-Daudé wrote: > On 11/24/21 10:29, Daniel Henrique Barboza wrote: >> The ivshmem device, as with most PCI devices, uses little endian byte >> order. However, the endianness 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. > > Maybe mention commit f7a199b2b44 ("ivshmem: use little-endian > int64_t for the protocol")? Good point. I'll send a v3. > >> 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 endianness, 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, >> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Thanks! Daniel >
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 11/24/21 10:29, Daniel Henrique Barboza wrote: >> The ivshmem device, as with most PCI devices, uses little endian byte >> order. However, the endianness 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. > > Maybe mention commit f7a199b2b44 ("ivshmem: use little-endian > int64_t for the protocol")? "The protocol" is the interface between ivshmem-doorbell device (client) and ivshmem server. This commit is about the interface between ivshmem-* device and the guest. The two interfaces are about as related as SSH and DNS: software exists that uses both. Mentioning f7a199b2b44 feels superfluous to me. [...]
On 11/24/21 09:55, Markus Armbruster wrote: > Philippe Mathieu-Daudé <philmd@redhat.com> writes: > >> On 11/24/21 10:29, Daniel Henrique Barboza wrote: >>> The ivshmem device, as with most PCI devices, uses little endian byte >>> order. However, the endianness 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. >> >> Maybe mention commit f7a199b2b44 ("ivshmem: use little-endian >> int64_t for the protocol")? > > "The protocol" is the interface between ivshmem-doorbell device (client) > and ivshmem server. This commit is about the interface between > ivshmem-* device and the guest. The two interfaces are about as related > as SSH and DNS: software exists that uses both. > Yeah, I was about to reply asking what's the relevance of how client-server communicates and the problem I'm trying to fix. It really seems a problem with the commit I mentioned in the "Fixes" tag that introduced the memory API with native endian instead of little endian. Let's keep the commit msg as is. Thanks, Daniel > Mentioning f7a199b2b44 feels superfluous to me. > > [...] >
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,
The ivshmem device, as with most PCI devices, uses little endian byte order. However, the endianness 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 endianness, 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(-)