[Qemu-devel] [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[Qemu-devel] [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[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_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@fr.ibm.com>
Suggested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Greg Kurz <gkurz@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);
     }

[PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[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_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@fr.ibm.com>
Suggested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Greg Kurz <gkurz@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);
     }

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Qemu-devel] [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[Michael S. Tsirkin]

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@fr.ibm.com>
> Suggested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Greg Kurz <gkurz@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);
>      }

Re: [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[Michael S. Tsirkin]

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@fr.ibm.com>
> Suggested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Greg Kurz <gkurz@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);
>      }

Re: [Qemu-devel] [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[Greg Kurz]

On Wed, 11 Mar 2015 21:06:05 +0100
"Michael S. Tsirkin" <mst@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@fr.ibm.com>
> > Suggested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > Signed-off-by: Greg Kurz <gkurz@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);
> >      }
> 

Re: [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[Greg Kurz]

On Wed, 11 Mar 2015 21:06:05 +0100
"Michael S. Tsirkin" <mst@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@fr.ibm.com>
> > Suggested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > Signed-off-by: Greg Kurz <gkurz@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);
> >      }
> 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Qemu-devel] [Qemu-ppc] [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[Benjamin Herrenschmidt]

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);
> > >      }
> > 
> 

Re: [Qemu-ppc] [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[Benjamin Herrenschmidt]

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);
> > >      }
> > 
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[Greg Kurz]

On Thu, 12 Mar 2015 09:18:38 +1100
Benjamin Herrenschmidt <benh@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);
> > > >      }
> > > 
> > 
> 
> 

Re: [Qemu-ppc] [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[Greg Kurz]

On Thu, 12 Mar 2015 09:18:38 +1100
Benjamin Herrenschmidt <benh@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);
> > > >      }
> > > 
> > 
> 
> 

Re: [Qemu-devel] [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[Michael S. Tsirkin]

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@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@fr.ibm.com>
> > > Suggested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > Signed-off-by: Greg Kurz <gkurz@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);
> > >      }
> > 

Re: [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[Michael S. Tsirkin]

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@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@fr.ibm.com>
> > > Suggested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > Signed-off-by: Greg Kurz <gkurz@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);
> > >      }
> > 

Re: [Qemu-devel] [Qemu-ppc] [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[Michael S. Tsirkin]

On Wed, Mar 11, 2015 at 11:52:13PM +0100, Greg Kurz wrote:
> On Thu, 12 Mar 2015 09:18:38 +1100
> Benjamin Herrenschmidt <benh@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

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.


> > 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);
> > > > >      }
> > > > 
> > > 
> > 
> > 

Re: [Qemu-ppc] [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[Michael S. Tsirkin]

On Wed, Mar 11, 2015 at 11:52:13PM +0100, Greg Kurz wrote:
> On Thu, 12 Mar 2015 09:18:38 +1100
> Benjamin Herrenschmidt <benh@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

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.


> > 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);
> > > > >      }
> > > > 
> > > 
> > 
> > 

Re: [Qemu-devel] [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[Paolo Bonzini]

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

Re: [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[Greg Kurz]

On Thu, 12 Mar 2015 17:25:15 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> 
> 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
> 

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

Re: [PATCH] virtio-pci: fix host notifiers on bi-endian architectures

[Greg Kurz]

On Thu, 12 Mar 2015 17:25:15 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> 
> 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
> 

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

[Qemu-devel] [PATCH 1/2] memory: make adjust_endianness() generic

[Greg Kurz]

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.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 memory.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/memory.c b/memory.c
index 20f6d9e..6291cc0 100644
--- a/memory.c
+++ b/memory.c
@@ -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])) {

[Qemu-devel] [PATCH 2/2] memory: fix the eventfd data endianness according to the host

[Greg Kurz]

The 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).

Reported-by: Cédric Le Goater <clg@fr.ibm.com>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 memory.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/memory.c b/memory.c
index 6291cc0..1e29d40 100644
--- a/memory.c
+++ b/memory.c
@@ -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

[Paolo Bonzini]

On 13/03/2015 09:11, Greg Kurz wrote:
> The 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).
> 
> Reported-by: Cédric Le Goater <clg@fr.ibm.com>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  memory.c |   13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index 6291cc0..1e29d40 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -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));

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

>      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

[Greg Kurz]

On Fri, 13 Mar 2015 12:06:06 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> 
> On 13/03/2015 09:11, Greg Kurz wrote:
> > The 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).
> > 
> > Reported-by: Cédric Le Goater <clg@fr.ibm.com>
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  memory.c |   13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/memory.c b/memory.c
> > index 6291cc0..1e29d40 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -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));
> 
> 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 <agraf@suse.de>
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 ?

>  A hypothetical userspace ioeventfd emulation would not need the swap.
> 

I don't understand why "would not need the swap"...


> 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

> Paolo
> 
> >      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

[Michael S. Tsirkin]

On Fri, Mar 13, 2015 at 12:06:06PM +0100, Paolo Bonzini wrote:
> 
> 
> On 13/03/2015 09:11, Greg Kurz wrote:
> > The 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).
> > 
> > Reported-by: Cédric Le Goater <clg@fr.ibm.com>
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  memory.c |   13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/memory.c b/memory.c
> > index 6291cc0..1e29d40 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -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));
> 
> 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

Better to fix really, making ioeventfd work with tcg seems
something quite reasonable.

> >      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

[Paolo Bonzini]

On 13/03/2015 12:32, Greg Kurz wrote:
> On Fri, 13 Mar 2015 12:06:06 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 13/03/2015 09:11, Greg Kurz wrote:
>>> The 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).
>>>
>>> Reported-by: Cédric Le Goater <clg@fr.ibm.com>
>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>>> ---
>>>  memory.c |   13 +++++++++++--
>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/memory.c b/memory.c
>>> index 6291cc0..1e29d40 100644
>>> --- a/memory.c
>>> +++ b/memory.c
>>> @@ -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));
>>
>> 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 <agraf@suse.de>
> 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 ?

Yes.

>>  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

> 
>> 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
> 
>> Paolo
>>
>>>      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

[Michael S. Tsirkin]

On Fri, Mar 13, 2015 at 03:52:50PM +0100, Paolo Bonzini wrote:
> 
> 
> On 13/03/2015 12:32, Greg Kurz wrote:
> > On Fri, 13 Mar 2015 12:06:06 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>
> >>
> >> On 13/03/2015 09:11, Greg Kurz wrote:
> >>> The 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).
> >>>
> >>> Reported-by: Cédric Le Goater <clg@fr.ibm.com>
> >>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >>> ---
> >>>  memory.c |   13 +++++++++++--
> >>>  1 file changed, 11 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/memory.c b/memory.c
> >>> index 6291cc0..1e29d40 100644
> >>> --- a/memory.c
> >>> +++ b/memory.c
> >>> @@ -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));
> >>
> >> 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 <agraf@suse.de>
> > 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 ?
> 
> Yes.
> 
> >>  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

Maybe you can post a patch? Would be easier ...

> > 
> >> 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
> > 
> >> Paolo
> >>
> >>>      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])) {
> >>>
> >>
> >