On Fri, 28 Mar 2014 15:15:46 +0100
Thomas Huth <th...@linux.vnet.ibm.com> wrote:
> On Fri, 28 Mar 2014 11:57:17 +0100
> Greg Kurz <gk...@linux.vnet.ibm.com> wrote:
>
> > From: Rusty Russell <ru...@rustcorp.com.au>
> >
> > 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 <ru...@rustcorp.com.au>
> > [ 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 <gk...@linux.vnet.ibm.com> ]
> > Signed-off-by: Greg Kurz <gk...@linux.vnet.ibm.com>
> > ---
> > 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 kurzg...@fr.ibm.com
gk...@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
