Re: [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio

[Thomas Huth]

On Fri, 28 Mar 2014 11:57:17 +0100
Greg Kurz <address@hidden> wrote:

> From: Rusty Russell <address@hidden>
> 
> virtio data structures are defined as "target endian", which assumes
> that's a fixed value.  In fact, that actually means it's platform-specific.
> The OASIS virtio 1.0 spec will fix this, by making all little endian.
> 
> We need to support both implementations and we want to share as much code
> as possible.
> 
> A good way to do it is to introduce a per-device boolean property to tell
> memory accessors whether they should swap bytes or not. This flag should
> be set at device reset time, because:
> - endianness won't change while the device is in use, and if we reboot
>   into a different endianness, a new device reset will occur
> - as suggested by Alexander Graf, we can keep all the logic to set the
>   property in a single place and share all the virtio memory accessors
>   between the two implementations
> 
> For legacy devices, we rely on a per-platform hook to set the flag. The
> virtio 1.0 implementation will just have to add some more logic in
> virtio_reset() instead of calling the hook:
> 
> if (vdev->legacy) {
>    vdev->needs_byteswap = virtio_legacy_get_byteswap();
> } else {
> #ifdef HOST_WORDS_BIGENDIAN
>    vdev->needs_byteswap = true;
> #else
>    vdev->needs_byteswap = false;
> #endif
> }
> 
> The needs_byteswap flag is preserved accross migrations.
> 
> Signed-off-by: Rusty Russell <address@hidden>
> [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
>   ldq_phys() API change,
>   relicensed virtio-access.h to GPLv2+ on Rusty's request,
>   introduce a per-device needs_byteswap flag,
>   add VirtIODevice * arg to virtio helpers,
>   rename virtio_get_byteswap to virtio_legacy_get_byteswap,
>   Greg Kurz <address@hidden> ]
> Signed-off-by: Greg Kurz <address@hidden>
> ---
>  hw/virtio/virtio.c                |    5 +
>  include/hw/virtio/virtio-access.h |  139 
> +++++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio.h        |    3 +
>  stubs/Makefile.objs               |    1 
>  stubs/virtio_get_byteswap.c       |    6 ++
>  5 files changed, 154 insertions(+)
>  create mode 100644 include/hw/virtio/virtio-access.h
>  create mode 100644 stubs/virtio_get_byteswap.c
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index aeabf3a..24b565f 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -19,6 +19,7 @@
>  #include "hw/virtio/virtio.h"
>  #include "qemu/atomic.h"
>  #include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-access.h"
> 
>  /*
>   * The alignment to use between consumer and producer parts of vring.
> @@ -546,6 +547,8 @@ void virtio_reset(void *opaque)
> 
>      virtio_set_status(vdev, 0);
> 
> +    vdev->needs_byteswap = virtio_legacy_get_byteswap();
> +
>      if (k->reset) {
>          k->reset(vdev);
>      }
> @@ -845,6 +848,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> 
>      qemu_put_8s(f, &vdev->status);
>      qemu_put_8s(f, &vdev->isr);
> +    qemu_put_8s(f, (uint8_t *) &vdev->needs_byteswap);
>      qemu_put_be16s(f, &vdev->queue_sel);
>      qemu_put_be32s(f, &vdev->guest_features);
>      qemu_put_be32(f, vdev->config_len);
> @@ -905,6 +909,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> 
>      qemu_get_8s(f, &vdev->status);
>      qemu_get_8s(f, &vdev->isr);
> +    qemu_get_8s(f, (uint8_t *) &vdev->needs_byteswap);
>      qemu_get_be16s(f, &vdev->queue_sel);
>      qemu_get_be32s(f, &features);

Two remarks here:

1) You've declared needs_byteswap as "bool", but in above code you
assume that the "bool" type is implemented as a single byte. That's
most likely true on most system, but AFAIK it's specific to the
compiler. So for portable code, I think you should either change the
type of needs_byteswap or you should use a temporary uint8_t variable
here instead.

2) You're changing the layout of the saved data ... don't you also have
to increase the version numbers in that case, too? (e.g. change the
version id for the register_savevm call in virtio-blk.c, virtio-net.c,
etc.)?

 Thomas