From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42410) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WTXZj-00044x-EN for qemu-devel@nongnu.org; Fri, 28 Mar 2014 10:16:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WTXZc-0002kP-AN for qemu-devel@nongnu.org; Fri, 28 Mar 2014 10:16:03 -0400 Received: from e06smtp14.uk.ibm.com ([195.75.94.110]:59590) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WTXZc-0002kH-0z for qemu-devel@nongnu.org; Fri, 28 Mar 2014 10:15:56 -0400 Received: from /spool/local by e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 28 Mar 2014 14:15:53 -0000 Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id 9B3A82190068 for ; Fri, 28 Mar 2014 14:15:45 +0000 (GMT) Received: from d06av02.portsmouth.uk.ibm.com (d06av02.portsmouth.uk.ibm.com [9.149.37.228]) by b06cxnps4075.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s2SEFdi563176838 for ; Fri, 28 Mar 2014 14:15:39 GMT Received: from d06av02.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av02.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s2SEFm3m002169 for ; Fri, 28 Mar 2014 08:15:50 -0600 Date: Fri, 28 Mar 2014 15:15:46 +0100 From: Thomas Huth Message-ID: <20140328151546.7b051385@oc7435384737.ibm.com> In-Reply-To: <20140328105717.21018.17649.stgit@bahia.local> References: <20140328105709.21018.88000.stgit@bahia.local> <20140328105717.21018.17649.stgit@bahia.local> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: kwolf@redhat.com, peter.maydell@linaro.org, anthony@codemonkey.ws, mst@redhat.com, marc.zyngier@arm.com, rusty@rustcorp.com.au, agraf@suse.de, qemu-devel@nongnu.org, stefanha@redhat.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com, afaerber@suse.de On Fri, 28 Mar 2014 11:57:17 +0100 Greg Kurz wrote: > From: Rusty Russell > > 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 > [ 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 ] > Signed-off-by: Greg Kurz > --- > 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