Re: [Qemu-devel] [PATCH RFC v6 06/20] virtio: endianness checks for virtio 1.0 devices

[David Gibson]

On Thu, Dec 11, 2014 at 02:25:08PM +0100, Cornelia Huck wrote:
> Add code that checks for the VERSION_1 feature bit in order to make
> decisions about the device's endianness. This allows us to support
> transitional devices.
> 
> Signed-off-by: Cornelia Huck <address@hidden>
> ---
>  hw/virtio/virtio.c                |    6 +++++-
>  include/hw/virtio/virtio-access.h |    4 ++++
>  include/hw/virtio/virtio.h        |    8 ++++++--
>  3 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 7f74ae5..8f69ffa 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -881,7 +881,11 @@ static bool virtio_device_endian_needed(void *opaque)
>      VirtIODevice *vdev = opaque;
>  
>      assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> -    return vdev->device_endian != virtio_default_endian();
> +    if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> +        return vdev->device_endian != virtio_default_endian();
> +    }
> +    /* Devices conforming to VIRTIO 1.0 or later are always LE. */
> +    return vdev->device_endian != VIRTIO_DEVICE_ENDIAN_LITTLE;

This doesn't seem quite right.  Since virtio 1.0 is always LE, this
should just assert that device_endian == LE and return false,
right?

>  }
>  
>  static const VMStateDescription vmstate_virtio_device_endian = {
> diff --git a/include/hw/virtio/virtio-access.h 
> b/include/hw/virtio/virtio-access.h
> index 46456fd..ee28c21 100644
> --- a/include/hw/virtio/virtio-access.h
> +++ b/include/hw/virtio/virtio-access.h
> @@ -19,6 +19,10 @@
>  
>  static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
>  {
> +    if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> +        /* Devices conforming to VIRTIO 1.0 or later are always LE. */
> +        return false;
> +    }
>  #if defined(TARGET_IS_BIENDIAN)
>      return virtio_is_big_endian(vdev);
>  #elif defined(TARGET_WORDS_BIGENDIAN)
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 08141c7..68c40db 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -297,7 +297,11 @@ static inline bool virtio_has_feature(VirtIODevice 
> *vdev, unsigned int fbit)
>  
>  static inline bool virtio_is_big_endian(VirtIODevice *vdev)
>  {
> -    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> -    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
> +    if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> +        assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> +        return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
> +    }
> +    /* Devices conforming to VIRTIO 1.0 or later are always LE. */
> +    return false;
>  }
>  #endif

AFAICT, the only real difference between virtio_is_big_endian() and
virtio_access_is_big_endian() is that the latter will become
compile-time constant on targets that don't do bi-endian.

With virtio 1.0 support, that's no longer true, so those two macros
should just be merged, I think.

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

pgpfLQJS58J9M.pgp
Description: PGP signature