| Message ID | 146468219419.25446.8053365248306938869.stgit@bahia.huguette.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. */ > >
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. */ >
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. */ > > >
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
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
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 >
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?
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
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
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..
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... :)
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
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. */
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(-)