Greg Kurz
2015-Mar-11 18:04 UTC
[PATCH] 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 -EOPNOTSUPP This happens because no matching device was found: __kvm_io_bus_write() | +->kvm_iodevice_write() | +->ioeventfd_write() | +->ioeventfd_in_range() returns false for all registered vrings Extra 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. Reported-by: C?dric Le Goater <clg at fr.ibm.com> Suggested-by: Michael Roth <mdroth at linux.vnet.ibm.com> Signed-off-by: Greg Kurz <gkurz at linux.vnet.ibm.com> --- 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(-) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index e7baf7b..62b04c9 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -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); }
Michael S. Tsirkin
2015-Mar-11 20:06 UTC
[PATCH] virtio-pci: fix host notifiers on bi-endian architectures
On Wed, Mar 11, 2015 at 07:04:38PM +0100, Greg Kurz wrote:> 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 -EOPNOTSUPP > > This happens because no matching device was found: > > __kvm_io_bus_write() > | > +->kvm_iodevice_write() > | > +->ioeventfd_write() > | > +->ioeventfd_in_range() returns false for all registered vrings > > Extra 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. > > Reported-by: C?dric Le Goater <clg at fr.ibm.com> > Suggested-by: Michael Roth <mdroth at linux.vnet.ibm.com> > Signed-off-by: Greg Kurz <gkurz at linux.vnet.ibm.com>I am confused. The value that notifications use is always LE. Can't we avoid multiple swaps? They make my head spin.> --- > > 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(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index e7baf7b..62b04c9 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -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); > }
Greg Kurz
2015-Mar-11 22:03 UTC
[PATCH] virtio-pci: fix host notifiers on bi-endian architectures
On Wed, 11 Mar 2015 21:06:05 +0100 "Michael S. Tsirkin" <mst at redhat.com> wrote:> On Wed, Mar 11, 2015 at 07:04:38PM +0100, Greg Kurz wrote: > > 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 -EOPNOTSUPP > > > > This happens because no matching device was found: > > > > __kvm_io_bus_write() > > | > > +->kvm_iodevice_write() > > | > > +->ioeventfd_write() > > | > > +->ioeventfd_in_range() returns false for all registered vrings > > > > Extra 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. > > > > Reported-by: C?dric Le Goater <clg at fr.ibm.com> > > Suggested-by: Michael Roth <mdroth at linux.vnet.ibm.com> > > Signed-off-by: Greg Kurz <gkurz at linux.vnet.ibm.com> > > 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.> 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.> 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).> > --- > > > > 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(-) > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index e7baf7b..62b04c9 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -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). */> > +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); > > } >
Possibly Parallel Threads
- [PATCH] virtio-pci: fix host notifiers on bi-endian architectures
- [PATCH] virtio-pci: fix host notifiers on bi-endian architectures
- [PATCH] virtio-pci: fix host notifiers on bi-endian architectures
- [Qemu-ppc] [PATCH] virtio-pci: fix host notifiers on bi-endian architectures
- [Qemu-ppc] [PATCH] virtio-pci: fix host notifiers on bi-endian architectures