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

[Greg Kurz]

On Fri, 28 Mar 2014 15:15:46 +0100
Thomas Huth <address@hidden> wrote:
> 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.
> 

Thomas,

I guess turning needs_byteswap into a uint8_t is ok and less intrusive
for the code.

> 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.)?
> 

Oops, my bad ! :-\

>  Thomas

Thanks for your time.

Cheers.

-- 
Gregory Kurz                                     address@hidden
                                                 address@hidden
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.