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

[Greg Kurz]

On Tue, 31 May 2016 13:54:11 +0200
Paolo Bonzini <address@hidden> 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.  */
> >   
>