From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36674) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WTYtw-0003go-Pt for qemu-devel@nongnu.org; Fri, 28 Mar 2014 11:41:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WTYto-0005uq-VW for qemu-devel@nongnu.org; Fri, 28 Mar 2014 11:41:00 -0400 Received: from e06smtp10.uk.ibm.com ([195.75.94.106]:52116) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WTYto-0005tR-Lk for qemu-devel@nongnu.org; Fri, 28 Mar 2014 11:40:52 -0400 Received: from /spool/local by e06smtp10.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 28 Mar 2014 15:40:50 -0000 Received: from b06cxnps3075.portsmouth.uk.ibm.com (d06relay10.portsmouth.uk.ibm.com [9.149.109.195]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id 8FF48219005C for ; Fri, 28 Mar 2014 15:40:41 +0000 (GMT) Received: from d06av05.portsmouth.uk.ibm.com (d06av05.portsmouth.uk.ibm.com [9.149.37.229]) by b06cxnps3075.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s2SFeZDA459244 for ; Fri, 28 Mar 2014 15:40:35 GMT Received: from d06av05.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av05.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s2SFekb4001567 for ; Fri, 28 Mar 2014 09:40:46 -0600 Date: Fri, 28 Mar 2014 16:40:42 +0100 From: Greg Kurz Message-ID: <20140328164042.5bf7a987@bahia.local> In-Reply-To: <20140328151546.7b051385@oc7435384737.ibm.com> References: <20140328105709.21018.88000.stgit@bahia.local> <20140328105717.21018.17649.stgit@bahia.local> <20140328151546.7b051385@oc7435384737.ibm.com> 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: Thomas Huth Cc: kwolf@redhat.com, peter.maydell@linaro.org, stefanha@redhat.com, mst@redhat.com, marc.zyngier@arm.com, rusty@rustcorp.com.au, agraf@suse.de, qemu-devel@nongnu.org, anthony@codemonkey.ws, cornelia.huck@de.ibm.com, pbonzini@redhat.com, afaerber@suse.de On Fri, 28 Mar 2014 15:15:46 +0100 Thomas Huth wrote: > 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. > 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 kurzgreg@fr.ibm.com gkurz@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.