[Qemu-devel] [PATCH] virtio: move bi-endian target support to a single location

[Qemu-devel] [PATCH] virtio: move bi-endian target support to a single location

[Greg Kurz]

Paolo's recent cpu.h cleanups broke legacy virtio for ppc64 LE guests (and
arm BE guests as well, even if I have not verified that). Especially, commit
"33c11879fd42 qemu-common: push cpu.h inclusion out of qemu-common.h" has
the side-effect of silently hiding the TARGET_IS_BIENDIAN macro from the
virtio memory accessors, and thus fully disabling support of endian changing
targets.

To be sure this cannot happen again, let's gather all the bi-endian bits
where they belong in include/hw/virtio/virtio-access.h.

The changes in hw/virtio/vhost.c are safe because vhost_needs_vring_endian()
is not called on a hot path and non bi-endian targets will return false
anyway.

While here, also rename TARGET_IS_BIENDIAN to be more precise: it is only for
legacy virtio and bi-endian guests.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/virtio/vhost.c                 |    4 ----
 include/hw/virtio/virtio-access.h |    6 +++++-
 target-arm/cpu.h                  |    2 --
 target-ppc/cpu.h                  |    2 --
 4 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 440071815408..81cc5b0ae35c 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -767,15 +767,11 @@ static inline bool vhost_needs_vring_endian(VirtIODevice *vdev)
     if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
         return false;
     }
-#ifdef TARGET_IS_BIENDIAN
 #ifdef HOST_WORDS_BIGENDIAN
     return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
 #else
     return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
 #endif
-#else
-    return false;
-#endif
 }
 
 static int vhost_virtqueue_set_vring_endian_legacy(struct vhost_dev *dev,
diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index 8dc84f520316..4b2803814642 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -17,9 +17,13 @@
 #include "hw/virtio/virtio.h"
 #include "exec/address-spaces.h"
 
+#if defined(TARGET_PPC64) || defined(TARGET_ARM)
+#define LEGACY_VIRTIO_IS_BIENDIAN 1
+#endif
+
 static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
 {
-#if defined(TARGET_IS_BIENDIAN)
+#if defined(LEGACY_VIRTIO_IS_BIENDIAN)
     return virtio_is_big_endian(vdev);
 #elif defined(TARGET_WORDS_BIGENDIAN)
     if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index c741b53ad45f..60971e16f7a4 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -29,8 +29,6 @@
 #  define TARGET_LONG_BITS 32
 #endif
 
-#define TARGET_IS_BIENDIAN 1
-
 #define CPUArchState struct CPUARMState
 
 #include "qemu-common.h"
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index cd33539d1ce9..556d66c39d11 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -28,8 +28,6 @@
 #define TARGET_LONG_BITS 64
 #define TARGET_PAGE_BITS 12
 
-#define TARGET_IS_BIENDIAN 1
-
 /* Note that the official physical address space bits is 62-M where M
    is implementation dependent.  I've not looked up M for the set of
    cpus we emulate at the system level.  */

Re: [Qemu-devel] [PATCH] virtio: move bi-endian target support to a single location

[Cédric Le Goater]

On 05/31/2016 10:09 AM, Greg Kurz wrote:
> Paolo's recent cpu.h cleanups broke legacy virtio for ppc64 LE guests (and
> arm BE guests as well, even if I have not verified that). Especially, commit
> "33c11879fd42 qemu-common: push cpu.h inclusion out of qemu-common.h" has
> the side-effect of silently hiding the TARGET_IS_BIENDIAN macro from the
> virtio memory accessors, and thus fully disabling support of endian changing
> targets.
> 
> To be sure this cannot happen again, let's gather all the bi-endian bits
> where they belong in include/hw/virtio/virtio-access.h.
> 
> The changes in hw/virtio/vhost.c are safe because vhost_needs_vring_endian()
> is not called on a hot path and non bi-endian targets will return false
> anyway.
> 
> While here, also rename TARGET_IS_BIENDIAN to be more precise: it is only for
> legacy virtio and bi-endian guests.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

biendian-ness being only used by virtio devices, I think this is a good place 
where to put it.

Acked-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/virtio/vhost.c                 |    4 ----
>  include/hw/virtio/virtio-access.h |    6 +++++-
>  target-arm/cpu.h                  |    2 --
>  target-ppc/cpu.h                  |    2 --
>  4 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 440071815408..81cc5b0ae35c 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -767,15 +767,11 @@ static inline bool vhost_needs_vring_endian(VirtIODevice *vdev)
>      if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>          return false;
>      }
> -#ifdef TARGET_IS_BIENDIAN
>  #ifdef HOST_WORDS_BIGENDIAN
>      return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
>  #else
>      return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
>  #endif
> -#else
> -    return false;
> -#endif
>  }
> 
>  static int vhost_virtqueue_set_vring_endian_legacy(struct vhost_dev *dev,
> diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
> index 8dc84f520316..4b2803814642 100644
> --- a/include/hw/virtio/virtio-access.h
> +++ b/include/hw/virtio/virtio-access.h
> @@ -17,9 +17,13 @@
>  #include "hw/virtio/virtio.h"
>  #include "exec/address-spaces.h"
> 
> +#if defined(TARGET_PPC64) || defined(TARGET_ARM)
> +#define LEGACY_VIRTIO_IS_BIENDIAN 1
> +#endif
> +
>  static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
>  {
> -#if defined(TARGET_IS_BIENDIAN)
> +#if defined(LEGACY_VIRTIO_IS_BIENDIAN)
>      return virtio_is_big_endian(vdev);
>  #elif defined(TARGET_WORDS_BIGENDIAN)
>      if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index c741b53ad45f..60971e16f7a4 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -29,8 +29,6 @@
>  #  define TARGET_LONG_BITS 32
>  #endif
> 
> -#define TARGET_IS_BIENDIAN 1
> -
>  #define CPUArchState struct CPUARMState
> 
>  #include "qemu-common.h"
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index cd33539d1ce9..556d66c39d11 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -28,8 +28,6 @@
>  #define TARGET_LONG_BITS 64
>  #define TARGET_PAGE_BITS 12
> 
> -#define TARGET_IS_BIENDIAN 1
> -
>  /* Note that the official physical address space bits is 62-M where M
>     is implementation dependent.  I've not looked up M for the set of
>     cpus we emulate at the system level.  */
> 
> 

Re: [Qemu-devel] [PATCH] virtio: move bi-endian target support to a single location

[Paolo Bonzini]

On 31/05/2016 10:09, Greg Kurz wrote:
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 440071815408..81cc5b0ae35c 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -767,15 +767,11 @@ static inline bool vhost_needs_vring_endian(VirtIODevice *vdev)
>      if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>          return false;
>      }
> -#ifdef TARGET_IS_BIENDIAN
>  #ifdef HOST_WORDS_BIGENDIAN
>      return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
>  #else
>      return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
>  #endif
> -#else
> -    return false;
> -#endif

This should be okay.

>  }
>  
>  static int vhost_virtqueue_set_vring_endian_legacy(struct vhost_dev *dev,
> diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
> index 8dc84f520316..4b2803814642 100644
> --- a/include/hw/virtio/virtio-access.h
> +++ b/include/hw/virtio/virtio-access.h
> @@ -17,9 +17,13 @@
>  #include "hw/virtio/virtio.h"
>  #include "exec/address-spaces.h"
>  
> +#if defined(TARGET_PPC64) || defined(TARGET_ARM)
> +#define LEGACY_VIRTIO_IS_BIENDIAN 1
> +#endif

These will only be correct if something else includes cpu.h.  Instead of
defining this, you should add

#include "cpu.h"

at the top of include/hw/virtio-access.h and leave the definitions in
target-*/cpu.h.

Thanks,

Paolo

>  static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
>  {
> -#if defined(TARGET_IS_BIENDIAN)
> +#if defined(LEGACY_VIRTIO_IS_BIENDIAN)
>      return virtio_is_big_endian(vdev);
>  #elif defined(TARGET_WORDS_BIGENDIAN)
>      if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index c741b53ad45f..60971e16f7a4 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -29,8 +29,6 @@
>  #  define TARGET_LONG_BITS 32
>  #endif
>  
> -#define TARGET_IS_BIENDIAN 1
> -
>  #define CPUArchState struct CPUARMState
>  
>  #include "qemu-common.h"
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index cd33539d1ce9..556d66c39d11 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -28,8 +28,6 @@
>  #define TARGET_LONG_BITS 64
>  #define TARGET_PAGE_BITS 12
>  
> -#define TARGET_IS_BIENDIAN 1
> -
>  /* Note that the official physical address space bits is 62-M where M
>     is implementation dependent.  I've not looked up M for the set of
>     cpus we emulate at the system level.  */
> 

Re: [Qemu-devel] [PATCH] virtio: move bi-endian target support to a single location

[Greg Kurz]

On Tue, 31 May 2016 13:54:11 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 31/05/2016 10:09, Greg Kurz wrote:
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 440071815408..81cc5b0ae35c 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -767,15 +767,11 @@ static inline bool vhost_needs_vring_endian(VirtIODevice *vdev)
> >      if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> >          return false;
> >      }
> > -#ifdef TARGET_IS_BIENDIAN
> >  #ifdef HOST_WORDS_BIGENDIAN
> >      return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
> >  #else
> >      return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
> >  #endif
> > -#else
> > -    return false;
> > -#endif  
> 
> This should be okay.
> 
> >  }
> >  
> >  static int vhost_virtqueue_set_vring_endian_legacy(struct vhost_dev *dev,
> > diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
> > index 8dc84f520316..4b2803814642 100644
> > --- a/include/hw/virtio/virtio-access.h
> > +++ b/include/hw/virtio/virtio-access.h
> > @@ -17,9 +17,13 @@
> >  #include "hw/virtio/virtio.h"
> >  #include "exec/address-spaces.h"
> >  
> > +#if defined(TARGET_PPC64) || defined(TARGET_ARM)
> > +#define LEGACY_VIRTIO_IS_BIENDIAN 1
> > +#endif  
> 
> These will only be correct if something else includes cpu.h.  Instead of

Unless I missed something, the TARGET_* macros come from the generated
config-target.h header, which is in turn included by qemu/osdep.h and
thus included by most of the code.

> defining this, you should add
> 
> #include "cpu.h"
> 
> at the top of include/hw/virtio-access.h and leave the definitions in
> target-*/cpu.h.
> 

All this bi-endian stuff is really an old-virtio-only thing... it is
only to be used by virtio_access_is_big_endian(). The fact that it
broke silently with your cleanup series is yet another proof that
this workaround is fragile.

Hence my tentative to put all the details in one place... would it
be ok if I include "config-target.h" to ensure we have the TARGET macros ?


> Thanks,
> 
> Paolo
> 
> >  static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
> >  {
> > -#if defined(TARGET_IS_BIENDIAN)
> > +#if defined(LEGACY_VIRTIO_IS_BIENDIAN)
> >      return virtio_is_big_endian(vdev);
> >  #elif defined(TARGET_WORDS_BIGENDIAN)
> >      if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index c741b53ad45f..60971e16f7a4 100644~/Work/qemu/qemu-gkurz/.mbuild/bin/qemu-system-ppc64 -snapshot -machine pseries,accel=kvm -nodefaults -no-shutdown -nographic -serial mon:stdio -m 8G -device virtio-net,netdev=netdev0,mac=C0:FF:EE:00:00:66,id=net0 -netdev tap,id=netdev0,vhost=off,helper='/usr/libexec/qemu-bridge-helper --br=br0' -drive file=/home/greg/images/fedora23-ppc64-ppc64le.qcow2,id=drive0,if=virtio -fsdev local,security_model=none,id=fsdev1,path=/home/greg -device virtio-9p-pci,id=fs1,fsdev=fsdev1,mount_tag=host
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -29,8 +29,6 @@
> >  #  define TARGET_LONG_BITS 32
> >  #endif
> >  
> > -#define TARGET_IS_BIENDIAN 1
> > -
> >  #define CPUArchState struct CPUARMState
> >  
> >  #include "qemu-common.h"
> > diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> > index cd33539d1ce9..556d66c39d11 100644
> > --- a/target-ppc/cpu.h
> > +++ b/target-ppc/cpu.h
> > @@ -28,8 +28,6 @@
> >  #define TARGET_LONG_BITS 64
> >  #define TARGET_PAGE_BITS 12
> >  
> > -#define TARGET_IS_BIENDIAN 1
> > -
> >  /* Note that the official physical address space bits is 62-M where M
> >     is implementation dependent.  I've not looked up M for the set of
> >     cpus we emulate at the system level.  */
> >   
> 

Re: [Qemu-devel] [PATCH] virtio: move bi-endian target support to a single location

[Peter Maydell]

On 31 May 2016 at 14:10, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> Hence my tentative to put all the details in one place... would it
> be ok if I include "config-target.h" to ensure we have the TARGET macros ?

No, config-target.h is one of the headers that should never be
included by any file except osdep.h (and scripts/clean-includes
will check for this). You're guaranteed that it's always included
for you.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] virtio: move bi-endian target support to a single location

[Paolo Bonzini]

On 31/05/2016 15:10, Greg Kurz wrote:
>>> > > +#if defined(TARGET_PPC64) || defined(TARGET_ARM)
>>> > > +#define LEGACY_VIRTIO_IS_BIENDIAN 1
>>> > > +#endif  
>> > 
>> > These will only be correct if something else includes cpu.h.  Instead of
> Unless I missed something, the TARGET_* macros come from the generated
> config-target.h header, which is in turn included by qemu/osdep.h and
> thus included by most of the code.

You're right.  Problems _could_ happen if virtio-access.h is included in
a file compiled without -DNEED_CPU_H (i.e. with common-obj-y instead of
obj-y) but include/exec/poison.h should take care of that.

>> > defining this, you should add
>> > 
>> > #include "cpu.h"
>> > 
>> > at the top of include/hw/virtio-access.h and leave the definitions in
>> > target-*/cpu.h.
>> > 
> All this bi-endian stuff is really an old-virtio-only thing... it is
> only to be used by virtio_access_is_big_endian(). The fact that it
> broke silently with your cleanup series is yet another proof that
> this workaround is fragile.

It is not fragile actually.  cpu.h doesn't exist in common-obj-y, so the
TARGET_IS_BIENDIAN define can be safely taken from cpu.h.

Anyway because of poison.h your solution isn't fragile either, so

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH] virtio: move bi-endian target support to a single location

[Greg Kurz]

On Tue, 31 May 2016 14:14:15 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 31 May 2016 at 14:10, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> > Hence my tentative to put all the details in one place... would it
> > be ok if I include "config-target.h" to ensure we have the TARGET macros ?  
> 
> No, config-target.h is one of the headers that should never be
> included by any file except osdep.h (and scripts/clean-includes
> will check for this). You're guaranteed that it's always included
> for you.
> 

So we don't even need to include osdep.h in virtio-access.h because we
are sure all .c files that include virtio-access.h also include osdep.h
first. Correct ?

> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH] virtio: move bi-endian target support to a single location

[Greg Kurz]

On Tue, 31 May 2016 15:15:21 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 31/05/2016 15:10, Greg Kurz wrote:
> >>> > > +#if defined(TARGET_PPC64) || defined(TARGET_ARM)
> >>> > > +#define LEGACY_VIRTIO_IS_BIENDIAN 1
> >>> > > +#endif    
> >> > 
> >> > These will only be correct if something else includes cpu.h.  Instead of  
> > Unless I missed something, the TARGET_* macros come from the generated
> > config-target.h header, which is in turn included by qemu/osdep.h and
> > thus included by most of the code.  
> 
> You're right.  Problems _could_ happen if virtio-access.h is included in
> a file compiled without -DNEED_CPU_H (i.e. with common-obj-y instead of
> obj-y) but include/exec/poison.h should take care of that.
> 

Exactly !

> >> > defining this, you should add
> >> > 
> >> > #include "cpu.h"
> >> > 
> >> > at the top of include/hw/virtio-access.h and leave the definitions in
> >> > target-*/cpu.h.
> >> >   
> > All this bi-endian stuff is really an old-virtio-only thing... it is
> > only to be used by virtio_access_is_big_endian(). The fact that it
> > broke silently with your cleanup series is yet another proof that
> > this workaround is fragile.  
> 
> It is not fragile actually.  cpu.h doesn't exist in common-obj-y, so the
> TARGET_IS_BIENDIAN define can be safely taken from cpu.h.
> 

I was referring to the current situation where virtio_access_is_big_endian()
silently lost its ability to support endian changing guests with:

--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -22,11 +22,6 @@
 
 #include "qemu/option.h"
 
-/* FIXME: Remove NEED_CPU_H.  */
-#ifdef NEED_CPU_H
-#include "cpu.h"
-#endif /* !defined(NEED_CPU_H) */
-

which then leads to the usual ENOMEM errors in the guest:

[    8.959600] blk-mq: failed to allocate request map
[    8.960171] virtio_blk: probe of virtio0 failed with error -12

> Anyway because of poison.h your solution isn't fragile either, so
> 

At least, if someone tries to include virtio-access.h in common-obj-y, she
gets a build break.

> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Paolo
> 

Thanks !

--
Greg

Re: [Qemu-devel] [PATCH] virtio: move bi-endian target support to a single location

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]

On Tue, May 31, 2016 at 03:15:21PM +0200, Paolo Bonzini wrote:
> 
> 
> On 31/05/2016 15:10, Greg Kurz wrote:
> >>> > > +#if defined(TARGET_PPC64) || defined(TARGET_ARM)
> >>> > > +#define LEGACY_VIRTIO_IS_BIENDIAN 1
> >>> > > +#endif  
> >> > 
> >> > These will only be correct if something else includes cpu.h.  Instead of
> > Unless I missed something, the TARGET_* macros come from the generated
> > config-target.h header, which is in turn included by qemu/osdep.h and
> > thus included by most of the code.
> 
> You're right.  Problems _could_ happen if virtio-access.h is included in
> a file compiled without -DNEED_CPU_H (i.e. with common-obj-y instead of
> obj-y) but include/exec/poison.h should take care of that.
> 
> >> > defining this, you should add
> >> > 
> >> > #include "cpu.h"
> >> > 
> >> > at the top of include/hw/virtio-access.h and leave the definitions in
> >> > target-*/cpu.h.
> >> > 
> > All this bi-endian stuff is really an old-virtio-only thing... it is
> > only to be used by virtio_access_is_big_endian(). The fact that it
> > broke silently with your cleanup series is yet another proof that
> > this workaround is fragile.
> 
> It is not fragile actually.  cpu.h doesn't exist in common-obj-y, so the
> TARGET_IS_BIENDIAN define can be safely taken from cpu.h.
> 
> Anyway because of poison.h your solution isn't fragile either, so
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Should I take this through my tree?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] virtio: move bi-endian target support to a single location

[Paolo Bonzini]

On 01/06/2016 04:33, David Gibson wrote:
> On Tue, May 31, 2016 at 03:15:21PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 31/05/2016 15:10, Greg Kurz wrote:
>>>>>>> +#if defined(TARGET_PPC64) || defined(TARGET_ARM)
>>>>>>> +#define LEGACY_VIRTIO_IS_BIENDIAN 1
>>>>>>> +#endif  
>>>>>
>>>>> These will only be correct if something else includes cpu.h.  Instead of
>>> Unless I missed something, the TARGET_* macros come from the generated
>>> config-target.h header, which is in turn included by qemu/osdep.h and
>>> thus included by most of the code.
>>
>> You're right.  Problems _could_ happen if virtio-access.h is included in
>> a file compiled without -DNEED_CPU_H (i.e. with common-obj-y instead of
>> obj-y) but include/exec/poison.h should take care of that.
>>
>>>>> defining this, you should add
>>>>>
>>>>> #include "cpu.h"
>>>>>
>>>>> at the top of include/hw/virtio-access.h and leave the definitions in
>>>>> target-*/cpu.h.
>>>>>
>>> All this bi-endian stuff is really an old-virtio-only thing... it is
>>> only to be used by virtio_access_is_big_endian(). The fact that it
>>> broke silently with your cleanup series is yet another proof that
>>> this workaround is fragile.
>>
>> It is not fragile actually.  cpu.h doesn't exist in common-obj-y, so the
>> TARGET_IS_BIENDIAN define can be safely taken from cpu.h.
>>
>> Anyway because of poison.h your solution isn't fragile either, so
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Should I take this through my tree?

If you don't hear from mst, go ahead.

Paolo

Re: [Qemu-devel] [PATCH] virtio: move bi-endian target support to a single location

[Greg Kurz]

On Wed, 1 Jun 2016 12:33:28 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, May 31, 2016 at 03:15:21PM +0200, Paolo Bonzini wrote:
> > 
> > 
> > On 31/05/2016 15:10, Greg Kurz wrote:  
> > >>> > > +#if defined(TARGET_PPC64) || defined(TARGET_ARM)
> > >>> > > +#define LEGACY_VIRTIO_IS_BIENDIAN 1
> > >>> > > +#endif    
> > >> > 
> > >> > These will only be correct if something else includes cpu.h.  Instead of  
> > > Unless I missed something, the TARGET_* macros come from the generated
> > > config-target.h header, which is in turn included by qemu/osdep.h and
> > > thus included by most of the code.  
> > 
> > You're right.  Problems _could_ happen if virtio-access.h is included in
> > a file compiled without -DNEED_CPU_H (i.e. with common-obj-y instead of
> > obj-y) but include/exec/poison.h should take care of that.
> >   
> > >> > defining this, you should add
> > >> > 
> > >> > #include "cpu.h"
> > >> > 
> > >> > at the top of include/hw/virtio-access.h and leave the definitions in
> > >> > target-*/cpu.h.
> > >> >   
> > > All this bi-endian stuff is really an old-virtio-only thing... it is
> > > only to be used by virtio_access_is_big_endian(). The fact that it
> > > broke silently with your cleanup series is yet another proof that
> > > this workaround is fragile.  
> > 
> > It is not fragile actually.  cpu.h doesn't exist in common-obj-y, so the
> > TARGET_IS_BIENDIAN define can be safely taken from cpu.h.
> > 
> > Anyway because of poison.h your solution isn't fragile either, so
> > 
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>  
> 
> Should I take this through my tree?
> 

That would be great !

--
Greg

Re: [Qemu-devel] [PATCH] virtio: move bi-endian target support to a single location

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2092 bytes --]

On Thu, Jun 02, 2016 at 06:04:37PM +0200, Greg Kurz wrote:
> On Wed, 1 Jun 2016 12:33:28 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, May 31, 2016 at 03:15:21PM +0200, Paolo Bonzini wrote:
> > > 
> > > 
> > > On 31/05/2016 15:10, Greg Kurz wrote:  
> > > >>> > > +#if defined(TARGET_PPC64) || defined(TARGET_ARM)
> > > >>> > > +#define LEGACY_VIRTIO_IS_BIENDIAN 1
> > > >>> > > +#endif    
> > > >> > 
> > > >> > These will only be correct if something else includes cpu.h.  Instead of  
> > > > Unless I missed something, the TARGET_* macros come from the generated
> > > > config-target.h header, which is in turn included by qemu/osdep.h and
> > > > thus included by most of the code.  
> > > 
> > > You're right.  Problems _could_ happen if virtio-access.h is included in
> > > a file compiled without -DNEED_CPU_H (i.e. with common-obj-y instead of
> > > obj-y) but include/exec/poison.h should take care of that.
> > >   
> > > >> > defining this, you should add
> > > >> > 
> > > >> > #include "cpu.h"
> > > >> > 
> > > >> > at the top of include/hw/virtio-access.h and leave the definitions in
> > > >> > target-*/cpu.h.
> > > >> >   
> > > > All this bi-endian stuff is really an old-virtio-only thing... it is
> > > > only to be used by virtio_access_is_big_endian(). The fact that it
> > > > broke silently with your cleanup series is yet another proof that
> > > > this workaround is fragile.  
> > > 
> > > It is not fragile actually.  cpu.h doesn't exist in common-obj-y, so the
> > > TARGET_IS_BIENDIAN define can be safely taken from cpu.h.
> > > 
> > > Anyway because of poison.h your solution isn't fragile either, so
> > > 
> > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>  
> > 
> > Should I take this through my tree?
> > 
> 
> That would be great !

Actually, that was a question for Paolo..


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] virtio: move bi-endian target support to a single location

[Greg Kurz]

On Fri, 3 Jun 2016 11:16:04 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 02, 2016 at 06:04:37PM +0200, Greg Kurz wrote:
> > On Wed, 1 Jun 2016 12:33:28 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Tue, May 31, 2016 at 03:15:21PM +0200, Paolo Bonzini wrote:  
> > > > 
> > > > 
> > > > On 31/05/2016 15:10, Greg Kurz wrote:    
> > > > >>> > > +#if defined(TARGET_PPC64) || defined(TARGET_ARM)
> > > > >>> > > +#define LEGACY_VIRTIO_IS_BIENDIAN 1
> > > > >>> > > +#endif      
> > > > >> > 
> > > > >> > These will only be correct if something else includes cpu.h.  Instead of    
> > > > > Unless I missed something, the TARGET_* macros come from the generated
> > > > > config-target.h header, which is in turn included by qemu/osdep.h and
> > > > > thus included by most of the code.    
> > > > 
> > > > You're right.  Problems _could_ happen if virtio-access.h is included in
> > > > a file compiled without -DNEED_CPU_H (i.e. with common-obj-y instead of
> > > > obj-y) but include/exec/poison.h should take care of that.
> > > >     
> > > > >> > defining this, you should add
> > > > >> > 
> > > > >> > #include "cpu.h"
> > > > >> > 
> > > > >> > at the top of include/hw/virtio-access.h and leave the definitions in
> > > > >> > target-*/cpu.h.
> > > > >> >     
> > > > > All this bi-endian stuff is really an old-virtio-only thing... it is
> > > > > only to be used by virtio_access_is_big_endian(). The fact that it
> > > > > broke silently with your cleanup series is yet another proof that
> > > > > this workaround is fragile.    
> > > > 
> > > > It is not fragile actually.  cpu.h doesn't exist in common-obj-y, so the
> > > > TARGET_IS_BIENDIAN define can be safely taken from cpu.h.
> > > > 
> > > > Anyway because of poison.h your solution isn't fragile either, so
> > > > 
> > > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>    
> > > 
> > > Should I take this through my tree?
> > >   
> > 
> > That would be great !  
> 
> Actually, that was a question for Paolo..
> 
> 

Sure, I was just expressing my interest for this possibility... :)

Re: [Qemu-devel] [PATCH] virtio: move bi-endian target support to a single location

[Paolo Bonzini]

On 03/06/2016 03:16, David Gibson wrote:
> On Thu, Jun 02, 2016 at 06:04:37PM +0200, Greg Kurz wrote:
>> On Wed, 1 Jun 2016 12:33:28 +1000
>> David Gibson <david@gibson.dropbear.id.au> wrote:
>>
>>> On Tue, May 31, 2016 at 03:15:21PM +0200, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 31/05/2016 15:10, Greg Kurz wrote:  
>>>>>>>>> +#if defined(TARGET_PPC64) || defined(TARGET_ARM)
>>>>>>>>> +#define LEGACY_VIRTIO_IS_BIENDIAN 1
>>>>>>>>> +#endif    
>>>>>>>
>>>>>>> These will only be correct if something else includes cpu.h.  Instead of  
>>>>> Unless I missed something, the TARGET_* macros come from the generated
>>>>> config-target.h header, which is in turn included by qemu/osdep.h and
>>>>> thus included by most of the code.  
>>>>
>>>> You're right.  Problems _could_ happen if virtio-access.h is included in
>>>> a file compiled without -DNEED_CPU_H (i.e. with common-obj-y instead of
>>>> obj-y) but include/exec/poison.h should take care of that.
>>>>   
>>>>>>> defining this, you should add
>>>>>>>
>>>>>>> #include "cpu.h"
>>>>>>>
>>>>>>> at the top of include/hw/virtio-access.h and leave the definitions in
>>>>>>> target-*/cpu.h.
>>>>>>>   
>>>>> All this bi-endian stuff is really an old-virtio-only thing... it is
>>>>> only to be used by virtio_access_is_big_endian(). The fact that it
>>>>> broke silently with your cleanup series is yet another proof that
>>>>> this workaround is fragile.  
>>>>
>>>> It is not fragile actually.  cpu.h doesn't exist in common-obj-y, so the
>>>> TARGET_IS_BIENDIAN define can be safely taken from cpu.h.
>>>>
>>>> Anyway because of poison.h your solution isn't fragile either, so
>>>>
>>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>  
>>>
>>> Should I take this through my tree?
>>>
>>
>> That would be great !
> 
> Actually, that was a question for Paolo..

It would be more of a question for mst; I do not maintain virtio (that's
why I wrote R-b and not Acked-by).

Thanks,

Paolo