virtio-pci: fix host notifiers on bi-endian architectures
PATCH 1/1 virtio-pci: fix host notifiers on bi-endian architectures
vhost is seriously broken with ppc64le guests, even in the supposedly supported case where the host is ppc64le and we don't need cross-endian support.
The TX virtqueue fails to be handled by vhost and falls back to QEMU. Despite this unexpected scenario where RX is vhost and TX is QEMU, the guest runs well with reduced upload performances... until you reboot, migrate, managed save or in fact any operation that causes vhost_net to be re-started. Network connectivity is then permanantly lost for the guest.
TX falling back to QEMU is the result of a failed MMIO store emulation in KVM. Debugging shows that:
kvmppc_emulate_mmio()
|
+-> kvmppc_handle_store()
|
+-> kvm_io_bus_write()
|
+-> __kvm_io_bus_write() returns -EOPNOTSUPPThis happens because no matching device was found:
__kvm_io_bus_write()
|
+->kvm_iodevice_write()
|
+->ioeventfd_write()
|
+->ioeventfd_in_range() returns false for all registered vringsExtra debugging shows that the TX vring number (16-bit) is supposed to be 0x0100 but QEMU passes 0x0001 to KVM... This happens *again* because QEMU still assumes powerpc is big endian (TARGET_WORDS_BIGENDIAN) by default.
This patch adds an extra swap in virtio_pci_set_host_notifier_internal() to negate the one that is done in adjust_endianness(). Since this is not a hot path and we want to keep virtio-pci.o in common-obj, we don't care whether the guest is bi-endian or not.
---
I guess it is also a fix for virtio-1 but I didn't check.
hw/virtio/virtio-pci.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
hw/virtio/virtio-pci.c +9 -2
@@ -133,6 +133,11 @@ static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f)return 0 ; } +static uint16_t cpu_to_host_notifier16 (VirtIODevice *vdev,uint16_t val) +{ +return virtio_is_big_endian (vdev) ? val :bswap16 (val); +} +static int virtio_pci_set_host_notifier_internal (VirtIOPCIProxy *proxy,int n,bool assign,bool set_handler) {
@@ -150,10 +155,12 @@ static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy, }virtio_queue_set_host_notifier_fd_handler (vq, true, set_handler);memory_region_add_eventfd (&proxy->bar,VIRTIO_PCI_QUEUE_NOTIFY ,2 , - true, n, notifier); + true,cpu_to_host_notifier16 (vdev, n), + notifier); }else {memory_region_del_eventfd (&proxy->bar,VIRTIO_PCI_QUEUE_NOTIFY ,2 , - true, n, notifier); + true,cpu_to_host_notifier16 (vdev, n), + notifier);virtio_queue_set_host_notifier_fd_handler (vq, false, false);event_notifier_cleanup (notifier); }
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Michael S. Tsirkin
maintainerQuoted 37 lines · Greg Kurz
vhost is seriously broken with ppc64le guests, even in the supposedly supported case where the host is ppc64le and we don't need cross-endian support.
The TX virtqueue fails to be handled by vhost and falls back to QEMU. Despite this unexpected scenario where RX is vhost and TX is QEMU, the guest runs well with reduced upload performances... until you reboot, migrate, managed save or in fact any operation that causes
vhost_netto be re-started. Network connectivity is then permanantly lost for the guest.TX falling back to QEMU is the result of a failed MMIO store emulation in KVM. Debugging shows that:
kvmppc_emulate_mmio()
|+-> kvmppc_handle_store() | +-> kvm_io_bus_write() | +-> __kvm_io_bus_write() returns -EOPNOTSUPPThis happens because no matching device was found:
__kvm_io_bus_write()
|+->kvm_iodevice_write() | +->ioeventfd_write() | +->ioeventfd_in_range() returns false for all registered vringsExtra debugging shows that the TX vring number (16-bit) is supposed to be
0x0100but QEMU passes0x0001to KVM... This happens *again* because QEMU still assumes powerpc is big endian (TARGET_WORDS_BIGENDIAN) by default.
Quoted 8 lines
This patch adds an extra swap in
virtio_pci_set_host_notifier_internal()to negate the one that is done inadjust_endianness(). Since this is not a hot path and we want to keep virtio-pci.o in common-obj, we don't care whether the guest is bi-endian or not.Reported-by Cédric Le Goater <[email protected]>Suggested-by Michael Roth <[email protected]>Signed-off-by Greg Kurz <[email protected]>
I am confused.
The value that notifications use is always LE.
Can't we avoid multiple swaps?
They make my head spin.
Quoted 38 lines
---
I guess it is also a fix for virtio-1 but I didn't check.
hw/virtio/virtio-pci.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)hw/virtio/virtio-pci.c +9
-2@@ -133,6 +133,11 @@ static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f)return 0 ; } +static uint16_t cpu_to_host_notifier16 (VirtIODevice *vdev,uint16_t val) +{ +return virtio_is_big_endian (vdev) ? val :bswap16 (val); +} +static int virtio_pci_set_host_notifier_internal (VirtIOPCIProxy *proxy,int n,bool assign,bool set_handler) {@@ -150,10 +155,12 @@ static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy, }virtio_queue_set_host_notifier_fd_handler (vq, true, set_handler);memory_region_add_eventfd (&proxy->bar,VIRTIO_PCI_QUEUE_NOTIFY ,2 , - true, n, notifier); + true,cpu_to_host_notifier16 (vdev, n), + notifier); }else {memory_region_del_eventfd (&proxy->bar,VIRTIO_PCI_QUEUE_NOTIFY ,2 , - true, n, notifier); + true,cpu_to_host_notifier16 (vdev, n), + notifier);virtio_queue_set_host_notifier_fd_handler (vq, false, false);event_notifier_cleanup (notifier); }
Quoted 43 lines · On Wed, 11 Mar 2015 21:06:05 +0100 "Michael S. Tsirkin" <[email protected]> wrote:
Quoted 42 lines · Greg Kurz
vhost is seriously broken with ppc64le guests, even in the supposedly supported case where the host is ppc64le and we don't need cross-endian support.
The TX virtqueue fails to be handled by vhost and falls back to QEMU. Despite this unexpected scenario where RX is vhost and TX is QEMU, the guest runs well with reduced upload performances... until you reboot, migrate, managed save or in fact any operation that causes
vhost_netto be re-started. Network connectivity is then permanantly lost for the guest.TX falling back to QEMU is the result of a failed MMIO store emulation in KVM. Debugging shows that:
kvmppc_emulate_mmio()
|+-> kvmppc_handle_store() | +-> kvm_io_bus_write() | +-> __kvm_io_bus_write() returns -EOPNOTSUPPThis happens because no matching device was found:
__kvm_io_bus_write()
|+->kvm_iodevice_write() | +->ioeventfd_write() | +->ioeventfd_in_range() returns false for all registered vringsExtra debugging shows that the TX vring number (16-bit) is supposed to be
0x0100but QEMU passes0x0001to KVM... This happens *again* because QEMU still assumes powerpc is big endian (TARGET_WORDS_BIGENDIAN) by default.This patch adds an extra swap in
virtio_pci_set_host_notifier_internal()to negate the one that is done inadjust_endianness(). Since this is not a hot path and we want to keep virtio-pci.o in common-obj, we don't care whether the guest is bi-endian or not.
Quoted 6 lines
Quoted 3 lines
Reported-by Cédric Le Goater <[email protected]>Suggested-by Michael Roth <[email protected]>Signed-off-by Greg Kurz <[email protected]>I am confused.
The value that notifications use is always LE.
True but adjust_endianness() does swap unconditionally for ppc64 because of TARGET_WORDS_BIGENDIAN.
Quoted 1 line
Can't we avoid multiple swaps?
That would mean adding an extra endianness argument down to memory_region_wrong_endianness()... not sure we want to do that.
Quoted 2 lines
They make my head spin.
I understand that the current fixed target endianness paradigm is best suited for most architectures. Extra swaps in specific non-critical locations allows to support odd beasts like ppc64le and arm64be without trashing more common paths. Maybe I can add a comment for better clarity (see below).
Quoted 7 lines
Quoted 7 lines
---
I guess it is also a fix for virtio-1 but I didn't check.
hw/virtio/virtio-pci.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
Quoted 8 lines
hw/virtio/virtio-pci.c static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f) · line 135
Quoted 8 lines
hw/virtio/virtio-pci.c +0
-0@@ -133,6 +133,11 @@ static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f)return 0 ; }
/* The host notifier will be swapped in adjust_endianness() according to the
* target default endianness. We need to negate this swap if the device uses
* an endianness that is not the default (ppc64le for example). */
Quoted 15 lines
Quoted 15 lines
hw/virtio/virtio-pci.c +7
-1+static uint16_t cpu_to_host_notifier16 (VirtIODevice *vdev,uint16_t val) +{ +return virtio_is_big_endian (vdev) ? val :bswap16 (val); +} +static int virtio_pci_set_host_notifier_internal (VirtIOPCIProxy *proxy,int n,bool assign,bool set_handler) {@@ -150,10 +155,12 @@ static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy, }virtio_queue_set_host_notifier_fd_handler (vq, true, set_handler);memory_region_add_eventfd (&proxy->bar,VIRTIO_PCI_QUEUE_NOTIFY ,2 , - true, n, notifier); + true,cpu_to_host_notifier16 (vdev, n), + notifier);
Quoted 9 lines
hw/virtio/virtio-pci.c static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy, · line 166
Quoted 8 lines
hw/virtio/virtio-pci.c +2
-1}else {memory_region_del_eventfd (&proxy->bar,VIRTIO_PCI_QUEUE_NOTIFY ,2 , - true, n, notifier); + true,cpu_to_host_notifier16 (vdev, n), + notifier);virtio_queue_set_host_notifier_fd_handler (vq, false, false);event_notifier_cleanup (notifier); }
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Quoted 9 lines · Greg Kurz
/* The host notifier will be swapped in
adjust_endianness()according to the
* target default endianness. We need to negate this swap if the device uses
* an endianness that is not the default (ppc64le for example). */Quoted 4 lines
hw/virtio/virtio-pci.c static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f) · line 139
Quoted 4 lines
hw/virtio/virtio-pci.c +4
-0+static uint16_t cpu_to_host_notifier16 (VirtIODevice *vdev,uint16_t val) +{ +return virtio_is_big_endian (vdev) ? val :bswap16 (val); +}
But but but ... The above ... won't it do things like break x86 ? Ie, shouldn't we swap only if TARGET_BIG_ENDIAN and !virtio_is_big_endian ? Or better, "fixed target endian" ^ "virtio endian" to cover all cases ?
Cheers,
Ben.
Quoted 20 lines
Quoted 19 lines
Quoted 18 lines
static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy,
int n, bool assign, boolset_handler)
{hw/virtio/virtio-pci.c +4
-2@@ -150,10 +155,12 @@ static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy, }virtio_queue_set_host_notifier_fd_handler (vq, true, set_handler);memory_region_add_eventfd (&proxy->bar,VIRTIO_PCI_QUEUE_NOTIFY ,2 , - true, n, notifier); + true,cpu_to_host_notifier16 (vdev, n), + notifier); }else {memory_region_del_eventfd (&proxy->bar,VIRTIO_PCI_QUEUE_NOTIFY ,2 , - true, n, notifier); + true,cpu_to_host_notifier16 (vdev, n), + notifier);virtio_queue_set_host_notifier_fd_handler (vq, false, false);event_notifier_cleanup (notifier); }
Quoted 9 lines · Greg Kurz
/* The host notifier will be swapped in
adjust_endianness()according to the
* target default endianness. We need to negate this swap if the device uses
* an endianness that is not the default (ppc64le for example). */Quoted 4 lines
hw/virtio/virtio-pci.c static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f) · line 139
Quoted 4 lines
hw/virtio/virtio-pci.c +4
-0+static uint16_t cpu_to_host_notifier16 (VirtIODevice *vdev,uint16_t val) +{ +return virtio_is_big_endian (vdev) ? val :bswap16 (val); +}
But but but ... The above ... won't it do things like break x86 ? Ie, shouldn't we swap only if TARGET_BIG_ENDIAN and !virtio_is_big_endian ? Or better, "fixed target endian" ^ "virtio endian" to cover all cases ?
Cheers,
Ben.
Quoted 20 lines
Quoted 19 lines
Quoted 18 lines
static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy,
int n, bool assign, boolset_handler)
{hw/virtio/virtio-pci.c +4
-2@@ -150,10 +155,12 @@ static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy, }virtio_queue_set_host_notifier_fd_handler (vq, true, set_handler);memory_region_add_eventfd (&proxy->bar,VIRTIO_PCI_QUEUE_NOTIFY ,2 , - true, n, notifier); + true,cpu_to_host_notifier16 (vdev, n), + notifier); }else {memory_region_del_eventfd (&proxy->bar,VIRTIO_PCI_QUEUE_NOTIFY ,2 , - true, n, notifier); + true,cpu_to_host_notifier16 (vdev, n), + notifier);virtio_queue_set_host_notifier_fd_handler (vq, false, false);event_notifier_cleanup (notifier); }
Quoted 7 lines · On Thu, 12 Mar 2015 09:18:38 +1100 Benjamin Herrenschmidt…
Quoted 5 lines · Greg Kurz
/* The host notifier will be swapped in
adjust_endianness()according to the
* target default endianness. We need to negate this swap if the device uses
* an endianness that is not the default (ppc64le for example). */
Quoted 9 lines
Quoted 4 lines
Quoted 4 lines
hw/virtio/virtio-pci.c static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f) · line 139
Quoted 4 lines
hw/virtio/virtio-pci.c +4
-0+static uint16_t cpu_to_host_notifier16 (VirtIODevice *vdev,uint16_t val) +{ +return virtio_is_big_endian (vdev) ? val :bswap16 (val); +}But but but ... The above ... won't it do things like break x86 ? Ie, shouldn't we swap only if
TARGET_BIG_ENDIANand !virtio_is_big_endian ? Or better, "fixed target endian" ^ "virtio endian" to cover all cases ?
Yeah you're right, it's a mess :)
To avoid virtio-pci.o being built per target, we can use virtio_default_endian() instead (to be exported from virtio.c):
return vdev->device_endian() != virtio_default_endian() ? val : bswap16(val);
I shall test on x86 and post a v2.
Thanks.
--
G
Quoted 25 lines
Cheers,
Ben.Quoted 20 lines
Quoted 19 lines
Quoted 18 lines
static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy,
int n, bool assign, boolset_handler)
{hw/virtio/virtio-pci.c +4
-2@@ -150,10 +155,12 @@ static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy, }virtio_queue_set_host_notifier_fd_handler (vq, true, set_handler);memory_region_add_eventfd (&proxy->bar,VIRTIO_PCI_QUEUE_NOTIFY ,2 , - true, n, notifier); + true,cpu_to_host_notifier16 (vdev, n), + notifier); }else {memory_region_del_eventfd (&proxy->bar,VIRTIO_PCI_QUEUE_NOTIFY ,2 , - true, n, notifier); + true,cpu_to_host_notifier16 (vdev, n), + notifier);virtio_queue_set_host_notifier_fd_handler (vq, false, false);event_notifier_cleanup (notifier); }
Michael S. Tsirkin
maintainerQuoted 25 lines · Greg Kurz
Quoted 7 lines · On Thu, 12 Mar 2015 09:18:38 +1100 Benjamin Herrenschmidt…
Quoted 5 lines · Greg Kurz
/* The host notifier will be swapped in
adjust_endianness()according to the
* target default endianness. We need to negate this swap if the device uses
* an endianness that is not the default (ppc64le for example). */Quoted 9 lines
Quoted 4 lines
Quoted 4 lines
hw/virtio/virtio-pci.c static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f) · line 139
Quoted 4 lines
hw/virtio/virtio-pci.c +4
-0+static uint16_t cpu_to_host_notifier16 (VirtIODevice *vdev,uint16_t val) +{ +return virtio_is_big_endian (vdev) ? val :bswap16 (val); +}But but but ... The above ... won't it do things like break x86 ? Ie, shouldn't we swap only if
TARGET_BIG_ENDIANand !virtio_is_big_endian ? Or better, "fixed target endian" ^ "virtio endian" to cover all cases ?Yeah you're right, it's a mess :)
To avoid virtio-pci.o being built per target, we can use
virtio_default_endian()instead (to be exported fromvirtio.c):
Quoted 8 lines
return
vdev->device_endian()!=virtio_default_endian()? val : bswap16(val);I shall test on x86 and post a v2.
Thanks.
--
G
It's a mess because the issue is not in virtio I think. virtio common region is just a simple device, using fixed LE format.
It's some bug in ioeventfd support. Need to understand whether it's in kernel (and maybe do a work-around) or in qemu memory core.
Quoted 25 lines
Quoted 25 lines
Cheers,
Ben.Quoted 20 lines
Quoted 19 lines
Quoted 18 lines
static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy,
int n, bool assign, boolset_handler)
{hw/virtio/virtio-pci.c +4
-2@@ -150,10 +155,12 @@ static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy, }virtio_queue_set_host_notifier_fd_handler (vq, true, set_handler);memory_region_add_eventfd (&proxy->bar,VIRTIO_PCI_QUEUE_NOTIFY ,2 , - true, n, notifier); + true,cpu_to_host_notifier16 (vdev, n), + notifier); }else {memory_region_del_eventfd (&proxy->bar,VIRTIO_PCI_QUEUE_NOTIFY ,2 , - true, n, notifier); + true,cpu_to_host_notifier16 (vdev, n), + notifier);virtio_queue_set_host_notifier_fd_handler (vq, false, false);event_notifier_cleanup (notifier); }
Michael S. Tsirkin
maintainerQuoted 61 lines · Greg Kurz
Quoted 43 lines · On Wed, 11 Mar 2015 21:06:05 +0100 "Michael S. Tsirkin" <[email protected]> wrote:
Quoted 42 lines · Greg Kurz
vhost is seriously broken with ppc64le guests, even in the supposedly supported case where the host is ppc64le and we don't need cross-endian support.
The TX virtqueue fails to be handled by vhost and falls back to QEMU. Despite this unexpected scenario where RX is vhost and TX is QEMU, the guest runs well with reduced upload performances... until you reboot, migrate, managed save or in fact any operation that causes
vhost_netto be re-started. Network connectivity is then permanantly lost for the guest.TX falling back to QEMU is the result of a failed MMIO store emulation in KVM. Debugging shows that:
kvmppc_emulate_mmio()
|+-> kvmppc_handle_store() | +-> kvm_io_bus_write() | +-> __kvm_io_bus_write() returns -EOPNOTSUPPThis happens because no matching device was found:
__kvm_io_bus_write()
|+->kvm_iodevice_write() | +->ioeventfd_write() | +->ioeventfd_in_range() returns false for all registered vringsExtra debugging shows that the TX vring number (16-bit) is supposed to be
0x0100but QEMU passes0x0001to KVM... This happens *again* because QEMU still assumes powerpc is big endian (TARGET_WORDS_BIGENDIAN) by default.This patch adds an extra swap in
virtio_pci_set_host_notifier_internal()to negate the one that is done inadjust_endianness(). Since this is not a hot path and we want to keep virtio-pci.o in common-obj, we don't care whether the guest is bi-endian or not.Quoted 6 lines
Quoted 3 lines
Reported-by Cédric Le Goater <[email protected]>Suggested-by Michael Roth <[email protected]>Signed-off-by Greg Kurz <[email protected]>I am confused.
The value that notifications use is always LE.True but
adjust_endianness()does swap unconditionally for ppc64 because ofTARGET_WORDS_BIGENDIAN.Quoted 1 line
Can't we avoid multiple swaps?
That would mean adding an extra endianness argument down to
memory_region_wrong_endianness()... not sure we want to do that.
Quoted 8 lines
Quoted 2 lines
They make my head spin.
I understand that the current fixed target endianness paradigm is best suited for most architectures. Extra swaps in specific non-critical locations allows to support odd beasts like ppc64le and arm64be without trashing more common paths. Maybe I can add a comment for better clarity (see below).
But common header format is simple, it's always LE. It does not depend on target.
To me this looks like a bug in memory_region_add_eventfd, it should do the right thing depending on device endian-ness.
Quoted 45 lines
Quoted 7 lines
Quoted 7 lines
---
I guess it is also a fix for virtio-1 but I didn't check.
hw/virtio/virtio-pci.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)Quoted 8 lines
hw/virtio/virtio-pci.c static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f) · line 135
Quoted 8 lines
hw/virtio/virtio-pci.c +0
-0@@ -133,6 +133,11 @@ static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f)return 0 ; }/* The host notifier will be swapped in
adjust_endianness()according to the
* target default endianness. We need to negate this swap if the device uses
* an endianness that is not the default (ppc64le for example). */Quoted 24 lines
Quoted 23 lines
hw/virtio/virtio-pci.c +9
-2+static uint16_t cpu_to_host_notifier16 (VirtIODevice *vdev,uint16_t val) +{ +return virtio_is_big_endian (vdev) ? val :bswap16 (val); +} +static int virtio_pci_set_host_notifier_internal (VirtIOPCIProxy *proxy,int n,bool assign,bool set_handler) {@@ -150,10 +155,12 @@ static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy, }virtio_queue_set_host_notifier_fd_handler (vq, true, set_handler);memory_region_add_eventfd (&proxy->bar,VIRTIO_PCI_QUEUE_NOTIFY ,2 , - true, n, notifier); + true,cpu_to_host_notifier16 (vdev, n), + notifier); }else {memory_region_del_eventfd (&proxy->bar,VIRTIO_PCI_QUEUE_NOTIFY ,2 , - true, n, notifier); + true,cpu_to_host_notifier16 (vdev, n), + notifier);virtio_queue_set_host_notifier_fd_handler (vq, false, false);event_notifier_cleanup (notifier); }
Quoted 5 lines · Michael S. Tsirkin
But common header format is simple, it's always LE. It does not depend on target.
To me this looks like a bug inmemory_region_add_eventfd, it should do the right thing depending on device endian-ness.
I agree it seems to be a QEMU bug.
Paolo
Quoted 9 lines · On Thu, 12 Mar 2015 17:25:15 +0100 Paolo Bonzini <[email protected]> wrote:
Quoted 5 lines · Michael S. Tsirkin
But common header format is simple, it's always LE. It does not depend on target.
To me this looks like a bug inmemory_region_add_eventfd, it should do the right thing depending on device endian-ness.
Quoted 4 lines
I agree it seems to be a QEMU bug.
Paolo
Yes you're right ! QEMU swaps the virtqueue id (adjust_endianness) according to TARGET_WORDS, like it was coming from the guest but in fact it comes from the host. The id should be fixed according to HOST_WORDS instead. Of course this went unnoticed until TARGET_WORDS_BIGENDIAN != HOST_WORDS_BIGENDIAN, which we have now with ppc64le hosts.
Patches to follow.
Thanks.
--
Greg
PATCH 1/2 memory: make adjust_endianness() generic
This patch just moves the decision to swap up to the callers. It changes no functionnality and will allow a subsequent patch to perform host endianness based swapping.
--- memory.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
memory.c +6 -6
@@ -347,9 +347,9 @@ static bool memory_region_wrong_endianness(MemoryRegion *mr)#endif } -static void adjust_endianness (MemoryRegion *mr,uint64_t *data,unsigned size) +static void adjust_endianness (uint64_t *data,unsigned size,bool need_swap) { -if (memory_region_wrong_endianness (mr)) { +if (need_swap) {switch (size) {case 1 :break ;
@@ -1083,7 +1083,7 @@ static bool memory_region_dispatch_read(MemoryRegion *mr, } *pval =memory_region_dispatch_read1 (mr, addr, size); -adjust_endianness (mr, pval, size); +adjust_endianness (pval, size,memory_region_wrong_endianness (mr));return false; }
@@ -1097,7 +1097,7 @@ static bool memory_region_dispatch_write(MemoryRegion *mr,return true; } -adjust_endianness (mr, &data, size); +adjust_endianness (&data, size,memory_region_wrong_endianness (mr));if (mr->ops->write) { access_with_adjusted_size(addr, &data, size,
@@ -1565,7 +1565,7 @@ void memory_region_add_eventfd(MemoryRegion *mr, };unsigned i; -adjust_endianness (mr, &mrfd.data, size); +adjust_endianness (&mrfd.data, size,memory_region_wrong_endianness (mr));memory_region_transaction_begin ();for (i =0 ; i < mr->ioeventfd_nb; ++i) {if (memory_region_ioeventfd_before (mrfd, mr->ioeventfds[i])) {
@@ -1598,7 +1598,7 @@ void memory_region_del_eventfd(MemoryRegion *mr, };unsigned i; -adjust_endianness (mr, &mrfd.data, size); +adjust_endianness (&mrfd.data, size,memory_region_wrong_endianness (mr));memory_region_transaction_begin ();for (i =0 ; i < mr->ioeventfd_nb; ++i) {if (memory_region_ioeventfd_equal (mrfd, mr->ioeventfds[i])) {
PATCH 2/2 memory: fix the eventfd data endianness according to the host
In reply to Greg KurzThe data argument is a host entity. It is not related to the target endianness. Let's introduce a HOST_WORDS_BIGENDIAN based helper for that.
This patch fixes ioeventfd and vhost for a ppc64le host running a ppc64le guest (only virtqueue 0 was handled, all others being byteswapped because of TARGET_WORDS_BIGENDIAN). It doesn't change functionnality for fixed endian architectures (i.e. doesn't break x86).
--- memory.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
memory.c +11 -2
@@ -1549,6 +1549,15 @@ void memory_region_clear_flush_coalesced(MemoryRegion *mr) } } +static bool eventfd_wrong_endianness (MemoryRegion *mr) +{ +#ifdef HOST_WORDS_BIGENDIAN +return mr->ops->endianness ==DEVICE_LITTLE_ENDIAN ; +#else +return mr->ops->endianness ==DEVICE_BIG_ENDIAN ; +#endif +} + void memory_region_add_eventfd(MemoryRegion *mr, hwaddr addr, unsigned size,
@@ -1565,7 +1574,7 @@ void memory_region_add_eventfd(MemoryRegion *mr, };unsigned i; -adjust_endianness (&mrfd.data, size,memory_region_wrong_endianness (mr)); +adjust_endianness (&mrfd.data, size,eventfd_wrong_endianness (mr));memory_region_transaction_begin ();for (i =0 ; i < mr->ioeventfd_nb; ++i) {if (memory_region_ioeventfd_before (mrfd, mr->ioeventfds[i])) {
@@ -1598,7 +1607,7 @@ void memory_region_del_eventfd(MemoryRegion *mr, };unsigned i; -adjust_endianness (&mrfd.data, size,memory_region_wrong_endianness (mr)); +adjust_endianness (&mrfd.data, size,eventfd_wrong_endianness (mr));memory_region_transaction_begin ();for (i =0 ; i < mr->ioeventfd_nb; ++i) {if (memory_region_ioeventfd_equal (mrfd, mr->ioeventfds[i])) {
Re: [Qemu-devel] [PATCH 2/2] memory: fix the eventfd data endianness according to the host
Quoted 36 lines · Greg Kurz
The data argument is a host entity. It is not related to the target endianness. Let's introduce a
HOST_WORDS_BIGENDIANbased helper for that.This patch fixes ioeventfd and vhost for a ppc64le host running a ppc64le guest (only virtqueue 0 was handled, all others being byteswapped because of
TARGET_WORDS_BIGENDIAN). It doesn't change functionnality for fixed endian architectures (i.e. doesn't break x86).Reported-by Cédric Le Goater <[email protected]>Signed-off-by Greg Kurz <[email protected]>--- memory.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)memory.c +9
-0@@ -1549,6 +1549,15 @@ void memory_region_clear_flush_coalesced(MemoryRegion *mr) } } +static bool eventfd_wrong_endianness (MemoryRegion *mr) +{ +#ifdef HOST_WORDS_BIGENDIAN +return mr->ops->endianness ==DEVICE_LITTLE_ENDIAN ; +#else +return mr->ops->endianness ==DEVICE_BIG_ENDIAN ; +#endif +} + void memory_region_add_eventfd(MemoryRegion *mr, hwaddr addr, unsigned size,
memory.c void memory_region_add_eventfd(MemoryRegion *mr, · line 1577
Quoted 5 lines
memory.c +1
-1@@ -1565,7 +1574,7 @@ void memory_region_add_eventfd(MemoryRegion *mr, };unsigned i; -adjust_endianness (&mrfd.data, size,memory_region_wrong_endianness (mr)); +adjust_endianness (&mrfd.data, size,eventfd_wrong_endianness (mr));
Strictly speaking, the place to do this would be kvm_set_ioeventfd_mmio.
A hypothetical userspace ioeventfd emulation would not need the swap.
I can accept the patch, but it's better to add a comment.
Paolo
Quoted 13 lines
memory.c +1
-1memory_region_transaction_begin ();for (i =0 ; i < mr->ioeventfd_nb; ++i) {if (memory_region_ioeventfd_before (mrfd, mr->ioeventfds[i])) {@@ -1598,7 +1607,7 @@ void memory_region_del_eventfd(MemoryRegion *mr, };unsigned i; -adjust_endianness (&mrfd.data, size,memory_region_wrong_endianness (mr)); +adjust_endianness (&mrfd.data, size,eventfd_wrong_endianness (mr));memory_region_transaction_begin ();for (i =0 ; i < mr->ioeventfd_nb; ++i) {if (memory_region_ioeventfd_equal (mrfd, mr->ioeventfds[i])) {
Re: [Qemu-devel] [PATCH 2/2] memory: fix the eventfd data endianness according to the host
Quoted 42 lines · On Fri, 13 Mar 2015 12:06:06 +0100 Paolo Bonzini <[email protected]> wrote:
Quoted 39 lines · Greg Kurz
The data argument is a host entity. It is not related to the target endianness. Let's introduce a
HOST_WORDS_BIGENDIANbased helper for that.This patch fixes ioeventfd and vhost for a ppc64le host running a ppc64le guest (only virtqueue 0 was handled, all others being byteswapped because of
TARGET_WORDS_BIGENDIAN). It doesn't change functionnality for fixed endian architectures (i.e. doesn't break x86).Reported-by Cédric Le Goater <[email protected]>Signed-off-by Greg Kurz <[email protected]>--- memory.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)memory.c +9
-0@@ -1549,6 +1549,15 @@ void memory_region_clear_flush_coalesced(MemoryRegion *mr) } } +static bool eventfd_wrong_endianness (MemoryRegion *mr) +{ +#ifdef HOST_WORDS_BIGENDIAN +return mr->ops->endianness ==DEVICE_LITTLE_ENDIAN ; +#else +return mr->ops->endianness ==DEVICE_BIG_ENDIAN ; +#endif +} + void memory_region_add_eventfd(MemoryRegion *mr, hwaddr addr, unsigned size,@@ -1565,7 +1574,7 @@ void memory_region_add_eventfd(MemoryRegion *mr, };unsigned i;
Quoted 4 lines
memory.c void memory_region_add_eventfd(MemoryRegion *mr, · line 1577
Quoted 2 lines
memory.c +1
-1-adjust_endianness (&mrfd.data, size,memory_region_wrong_endianness (mr)); +adjust_endianness (&mrfd.data, size,eventfd_wrong_endianness (mr));Strictly speaking, the place to do this would be
kvm_set_ioeventfd_mmio.
FWIW the swap is being done in memory.c since commit:
commit 28f362be6e7f45ea9b7a57a08555c4c784f36198
Author: Alexander Graf <[email protected]> Date: Mon Oct 15 20:30:28 2012 +0200
memory: Make eventfd adhere to device endianness
Are you asking to revert this commit and to pass the device endianness to kvm_set_ioeventfd_mmio() so it can fix the ordering ?
Quoted 2 lines
A hypothetical userspace ioeventfd emulation would not need the swap.
I don't understand why "would not need the swap"...
Quoted 2 lines
I can accept the patch, but it's better to add a comment.
... and so I don't know what to write. :)
Please enlight !
--
Greg
Quoted 16 lines
Paolo
Quoted 13 lines
memory.c +1
-1memory_region_transaction_begin ();for (i =0 ; i < mr->ioeventfd_nb; ++i) {if (memory_region_ioeventfd_before (mrfd, mr->ioeventfds[i])) {@@ -1598,7 +1607,7 @@ void memory_region_del_eventfd(MemoryRegion *mr, };unsigned i; -adjust_endianness (&mrfd.data, size,memory_region_wrong_endianness (mr)); +adjust_endianness (&mrfd.data, size,eventfd_wrong_endianness (mr));memory_region_transaction_begin ();for (i =0 ; i < mr->ioeventfd_nb; ++i) {if (memory_region_ioeventfd_equal (mrfd, mr->ioeventfds[i])) {
Re: [Qemu-devel] [PATCH 2/2] memory: fix the eventfd data endianness according to the host
Quoted 51 lines · Greg Kurz
Quoted 42 lines · On Fri, 13 Mar 2015 12:06:06 +0100 Paolo Bonzini <[email protected]> wrote:
Quoted 39 lines · Greg Kurz
The data argument is a host entity. It is not related to the target endianness. Let's introduce a
HOST_WORDS_BIGENDIANbased helper for that.This patch fixes ioeventfd and vhost for a ppc64le host running a ppc64le guest (only virtqueue 0 was handled, all others being byteswapped because of
TARGET_WORDS_BIGENDIAN). It doesn't change functionnality for fixed endian architectures (i.e. doesn't break x86).Reported-by Cédric Le Goater <[email protected]>Signed-off-by Greg Kurz <[email protected]>--- memory.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)memory.c +9
-0@@ -1549,6 +1549,15 @@ void memory_region_clear_flush_coalesced(MemoryRegion *mr) } } +static bool eventfd_wrong_endianness (MemoryRegion *mr) +{ +#ifdef HOST_WORDS_BIGENDIAN +return mr->ops->endianness ==DEVICE_LITTLE_ENDIAN ; +#else +return mr->ops->endianness ==DEVICE_BIG_ENDIAN ; +#endif +} + void memory_region_add_eventfd(MemoryRegion *mr, hwaddr addr, unsigned size,@@ -1565,7 +1574,7 @@ void memory_region_add_eventfd(MemoryRegion *mr, };unsigned i;Quoted 4 lines
memory.c void memory_region_add_eventfd(MemoryRegion *mr, · line 1577
Quoted 2 lines
memory.c +1
-1-adjust_endianness (&mrfd.data, size,memory_region_wrong_endianness (mr)); +adjust_endianness (&mrfd.data, size,eventfd_wrong_endianness (mr));Strictly speaking, the place to do this would be
kvm_set_ioeventfd_mmio.FWIW the swap is being done in
memory.csince commit:
Quoted 8 lines
commit
28f362be6e7f45ea9b7a57a08555c4c784f36198Author: Alexander Graf <[email protected]> Date: Mon Oct 15 20:30:28 2012 +0200memory: Make eventfd adhere to device endianness
Are you asking to revert this commit and to pass the device endianness to
kvm_set_ioeventfd_mmio()so it can fix the ordering ?
Yes.
Quoted 3 lines
Quoted 1 line
A hypothetical userspace ioeventfd emulation would not need the swap.
I don't understand why "would not need the swap"...
Because the userspace ioeventfd emulation would look at the value as it comes from the target CPU, in target CPU endianness. So it would have a swap done at ioeventfd time, and a swap done at access time. Host endianness doesn't matter.
Here, QEMU believes that the target's natural endianness is big-endian, but the kernel believes that the target's natural endianness is little-endian (based on MSR_KERNEL). So there is a different number of swaps in memory_region_add_eventfd and in the kernel's kvmppc_handle_store.
Does something like this work?
#if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN)
/* The kernel expects ioeventfd values in HOST_WORDS_BIGENDIAN
* endianness, but the memory core hands them in target endianness.
* For example, PPC is always treated as big-endian even if running
* on KVM and on PPC64LE. Correct here.
*/
switch(size) {
case 2:
val = bswap16(val);
break;
case 4:
val = bswap32(val);
break;
}#endif
in kvm_set_ioeventfd_mmio and kvm_set_ioeventfd_pio?
Paolo
Quoted 28 lines
Quoted 3 lines
I can accept the patch, but it's better to add a comment.
... and so I don't know what to write. :)
Please enlight !
--
GregQuoted 16 lines
Paolo
Quoted 13 lines
memory.c +1
-1memory_region_transaction_begin ();for (i =0 ; i < mr->ioeventfd_nb; ++i) {if (memory_region_ioeventfd_before (mrfd, mr->ioeventfds[i])) {@@ -1598,7 +1607,7 @@ void memory_region_del_eventfd(MemoryRegion *mr, };unsigned i; -adjust_endianness (&mrfd.data, size,memory_region_wrong_endianness (mr)); +adjust_endianness (&mrfd.data, size,eventfd_wrong_endianness (mr));memory_region_transaction_begin ();for (i =0 ; i < mr->ioeventfd_nb; ++i) {if (memory_region_ioeventfd_equal (mrfd, mr->ioeventfds[i])) {
Michael S. Tsirkin
maintainerRe: [Qemu-devel] [PATCH 2/2] memory: fix the eventfd data endianness according to the host
Quoted 95 lines · Paolo Bonzini
Quoted 51 lines · Greg Kurz
Quoted 42 lines · On Fri, 13 Mar 2015 12:06:06 +0100 Paolo Bonzini <[email protected]> wrote:
Quoted 39 lines · Greg Kurz
The data argument is a host entity. It is not related to the target endianness. Let's introduce a
HOST_WORDS_BIGENDIANbased helper for that.This patch fixes ioeventfd and vhost for a ppc64le host running a ppc64le guest (only virtqueue 0 was handled, all others being byteswapped because of
TARGET_WORDS_BIGENDIAN). It doesn't change functionnality for fixed endian architectures (i.e. doesn't break x86).Reported-by Cédric Le Goater <[email protected]>Signed-off-by Greg Kurz <[email protected]>--- memory.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)memory.c +9
-0@@ -1549,6 +1549,15 @@ void memory_region_clear_flush_coalesced(MemoryRegion *mr) } } +static bool eventfd_wrong_endianness (MemoryRegion *mr) +{ +#ifdef HOST_WORDS_BIGENDIAN +return mr->ops->endianness ==DEVICE_LITTLE_ENDIAN ; +#else +return mr->ops->endianness ==DEVICE_BIG_ENDIAN ; +#endif +} + void memory_region_add_eventfd(MemoryRegion *mr, hwaddr addr, unsigned size,@@ -1565,7 +1574,7 @@ void memory_region_add_eventfd(MemoryRegion *mr, };unsigned i;Quoted 4 lines
memory.c void memory_region_add_eventfd(MemoryRegion *mr, · line 1577
Quoted 2 lines
memory.c +1
-1-adjust_endianness (&mrfd.data, size,memory_region_wrong_endianness (mr)); +adjust_endianness (&mrfd.data, size,eventfd_wrong_endianness (mr));Strictly speaking, the place to do this would be
kvm_set_ioeventfd_mmio.FWIW the swap is being done in
memory.csince commit:Quoted 8 lines
commit
28f362be6e7f45ea9b7a57a08555c4c784f36198Author: Alexander Graf <[email protected]> Date: Mon Oct 15 20:30:28 2012 +0200memory: Make eventfd adhere to device endianness
Are you asking to revert this commit and to pass the device endianness to
kvm_set_ioeventfd_mmio()so it can fix the ordering ?Yes.
Quoted 3 lines
Quoted 1 line
A hypothetical userspace ioeventfd emulation would not need the swap.
I don't understand why "would not need the swap"...
Because the userspace ioeventfd emulation would look at the value as it comes from the target CPU, in target CPU endianness. So it would have a swap done at ioeventfd time, and a swap done at access time. Host endianness doesn't matter.
Here, QEMU believes that the target's natural endianness is big-endian, but the kernel believes that the target's natural endianness is little-endian (based on
MSR_KERNEL). So there is a different number of swaps inmemory_region_add_eventfdand in the kernel'skvmppc_handle_store.Does something like this work?
#if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN) /* The kernel expects ioeventfd values in HOST_WORDS_BIGENDIAN * endianness, but the memory core hands them in target endianness. * For example, PPC is always treated as big-endian even if running * on KVM and on PPC64LE. Correct here. */ switch(size) { case 2: val = bswap16(val); break; case 4: val = bswap32(val); break; }
Quoted 5 lines
#endif
in
kvm_set_ioeventfd_mmioandkvm_set_ioeventfd_pio?Paolo
Maybe you can post a patch? Would be easier ...
Quoted 28 lines
Quoted 28 lines
Quoted 3 lines
I can accept the patch, but it's better to add a comment.
... and so I don't know what to write. :)
Please enlight !
--
GregQuoted 16 lines
Paolo
Quoted 13 lines
memory.c +1
-1memory_region_transaction_begin ();for (i =0 ; i < mr->ioeventfd_nb; ++i) {if (memory_region_ioeventfd_before (mrfd, mr->ioeventfds[i])) {@@ -1598,7 +1607,7 @@ void memory_region_del_eventfd(MemoryRegion *mr, };unsigned i; -adjust_endianness (&mrfd.data, size,memory_region_wrong_endianness (mr)); +adjust_endianness (&mrfd.data, size,eventfd_wrong_endianness (mr));memory_region_transaction_begin ();for (i =0 ; i < mr->ioeventfd_nb; ++i) {if (memory_region_ioeventfd_equal (mrfd, mr->ioeventfds[i])) {
Michael S. Tsirkin
maintainerRe: [Qemu-devel] [PATCH 2/2] memory: fix the eventfd data endianness according to the host
In reply to Paolo BonziniQuoted 48 lines · Paolo Bonzini
Quoted 36 lines · Greg Kurz
The data argument is a host entity. It is not related to the target endianness. Let's introduce a
HOST_WORDS_BIGENDIANbased helper for that.This patch fixes ioeventfd and vhost for a ppc64le host running a ppc64le guest (only virtqueue 0 was handled, all others being byteswapped because of
TARGET_WORDS_BIGENDIAN). It doesn't change functionnality for fixed endian architectures (i.e. doesn't break x86).Reported-by Cédric Le Goater <[email protected]>Signed-off-by Greg Kurz <[email protected]>--- memory.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)memory.c +9
-0@@ -1549,6 +1549,15 @@ void memory_region_clear_flush_coalesced(MemoryRegion *mr) } } +static bool eventfd_wrong_endianness (MemoryRegion *mr) +{ +#ifdef HOST_WORDS_BIGENDIAN +return mr->ops->endianness ==DEVICE_LITTLE_ENDIAN ; +#else +return mr->ops->endianness ==DEVICE_BIG_ENDIAN ; +#endif +} + void memory_region_add_eventfd(MemoryRegion *mr, hwaddr addr, unsigned size,memory.c void memory_region_add_eventfd(MemoryRegion *mr, · line 1577
Quoted 5 lines
memory.c +1
-1@@ -1565,7 +1574,7 @@ void memory_region_add_eventfd(MemoryRegion *mr, };unsigned i; -adjust_endianness (&mrfd.data, size,memory_region_wrong_endianness (mr)); +adjust_endianness (&mrfd.data, size,eventfd_wrong_endianness (mr));Strictly speaking, the place to do this would be
kvm_set_ioeventfd_mmio.
A hypothetical userspace ioeventfd emulation would not need the swap.
Quoted 3 lines
I can accept the patch, but it's better to add a comment.
Paolo
Better to fix really, making ioeventfd work with tcg seems something quite reasonable.
Quoted 12 lines
Quoted 12 lines
memory.c +1
-1for (i =0 ; i < mr->ioeventfd_nb; ++i) {if (memory_region_ioeventfd_before (mrfd, mr->ioeventfds[i])) {@@ -1598,7 +1607,7 @@ void memory_region_del_eventfd(MemoryRegion *mr, };unsigned i; -adjust_endianness (&mrfd.data, size,memory_region_wrong_endianness (mr)); +adjust_endianness (&mrfd.data, size,eventfd_wrong_endianness (mr));memory_region_transaction_begin ();for (i =0 ; i < mr->ioeventfd_nb; ++i) {if (memory_region_ioeventfd_equal (mrfd, mr->ioeventfds[i])) {