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); > > } >
Benjamin Herrenschmidt
2015-Mar-11 22:18 UTC
[Qemu-ppc] [PATCH] virtio-pci: fix host notifiers on bi-endian architectures
On Wed, 2015-03-11 at 23:03 +0100, Greg Kurz wrote:> /* 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); > > > +}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.> > > 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:52 UTC
[Qemu-ppc] [PATCH] virtio-pci: fix host notifiers on bi-endian architectures
On Thu, 12 Mar 2015 09:18:38 +1100 Benjamin Herrenschmidt <benh at kernel.crashing.org> wrote:> On Wed, 2015-03-11 at 23:03 +0100, Greg Kurz wrote: > > > /* 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); > > > > +} > > 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 ? >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> Cheers, > Ben. > > > > > 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-12 07:08 UTC
[PATCH] virtio-pci: fix host notifiers on bi-endian architectures
On Wed, Mar 11, 2015 at 11:03:14PM +0100, Greg Kurz wrote:> 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).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.> > > --- > > > > > > 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); > > > } > >
Paolo Bonzini
2015-Mar-12 16:25 UTC
[PATCH] virtio-pci: fix host notifiers on bi-endian architectures
On 12/03/2015 08:08, Michael S. Tsirkin wrote:> 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.I agree it seems to be a QEMU bug. Paolo
Maybe Matching 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
- [PATCH] virtio-pci: fix host notifiers on bi-endian architectures
- [PATCH] virtio-pci: fix host notifiers on bi-endian architectures