From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37052) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WUxGz-000717-Ki for qemu-devel@nongnu.org; Tue, 01 Apr 2014 07:54:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WUxGr-0007YK-5w for qemu-devel@nongnu.org; Tue, 01 Apr 2014 07:54:33 -0400 Received: from e06smtp16.uk.ibm.com ([195.75.94.112]:46450) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WUxGq-0007XA-Q3 for qemu-devel@nongnu.org; Tue, 01 Apr 2014 07:54:25 -0400 Received: from /spool/local by e06smtp16.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 1 Apr 2014 12:54:22 +0100 Received: from b06cxnps4074.portsmouth.uk.ibm.com (d06relay11.portsmouth.uk.ibm.com [9.149.109.196]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id E9F4D17D8062 for ; Tue, 1 Apr 2014 12:55:08 +0100 (BST) Received: from d06av08.portsmouth.uk.ibm.com (d06av08.portsmouth.uk.ibm.com [9.149.37.249]) by b06cxnps4074.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s31Bs8G6852250 for ; Tue, 1 Apr 2014 11:54:08 GMT Received: from d06av08.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av08.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s31BsInv012335 for ; Tue, 1 Apr 2014 05:54:19 -0600 Date: Tue, 1 Apr 2014 13:54:16 +0200 From: Greg Kurz Message-ID: <20140401135416.08d02b9d@bahia.local> In-Reply-To: <533980CF.4020904@suse.de> References: <20140328105709.21018.88000.stgit@bahia.local> <20140328105717.21018.17649.stgit@bahia.local> <533980CF.4020904@suse.de> 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: Alexander Graf Cc: kwolf@redhat.com, peter.maydell@linaro.org, thuth@linux.vnet.ibm.com, mst@redhat.com, marc.zyngier@arm.com, rusty@rustcorp.com.au, qemu-devel@nongnu.org, stefanha@redhat.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com, anthony@codemonkey.ws, afaerber@suse.de On Mon, 31 Mar 2014 16:50:55 +0200 Alexander Graf wrote: > On 03/28/2014 11:57 AM, 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); > > > > diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h > > new file mode 100644 > > index 0000000..70dd1e2 > > --- /dev/null > > +++ b/include/hw/virtio/virtio-access.h > > @@ -0,0 +1,139 @@ > > +/* > > + * Virtio Accessor Support: In case your target can change endian. > > + * > > + * Copyright IBM, Corp. 2013 > > + * > > + * Authors: > > + * Rusty Russell > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation, either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + */ > > +#ifndef _QEMU_VIRTIO_ACCESS_H > > +#define _QEMU_VIRTIO_ACCESS_H > > +#include "hw/virtio/virtio.h" > > + > > +static inline uint16_t virtio_lduw_phys(AddressSpace *as, hwaddr pa, > > + struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + return bswap16(lduw_phys(as, pa)); > > + } > > + return lduw_phys(as, pa); > > +} > > + > > +static inline uint32_t virtio_ldl_phys(AddressSpace *as, hwaddr pa, > > + struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + return bswap32(ldl_phys(as, pa)); > > + } > > + return ldl_phys(as, pa); > > +} > > + > > +static inline uint64_t virtio_ldq_phys(AddressSpace *as, hwaddr pa, > > + struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + return bswap64(ldq_phys(as, pa)); > > + } > > + return ldq_phys(as, pa); > > +} > > + > > +static inline void virtio_stw_phys(AddressSpace *as, hwaddr pa, uint16_t value, > > + struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + stw_phys(as, pa, bswap16(value)); > > + } else { > > + stw_phys(as, pa, value); > > + } > > +} > > + > > +static inline void virtio_stl_phys(AddressSpace *as, hwaddr pa, uint32_t value, > > + struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + stl_phys(as, pa, bswap32(value)); > > + } else { > > + stl_phys(as, pa, value); > > + } > > +} > > + > > +static inline void virtio_stw_p(void *ptr, uint16_t v, > > + struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + stw_p(ptr, bswap16(v)); > > + } else { > > + stw_p(ptr, v); > > + } > > +} > > + > > +static inline void virtio_stl_p(void *ptr, uint32_t v, > > + struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + stl_p(ptr, bswap32(v)); > > + } else { > > + stl_p(ptr, v); > > + } > > +} > > + > > +static inline void virtio_stq_p(void *ptr, uint64_t v, > > + struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + stq_p(ptr, bswap64(v)); > > + } else { > > + stq_p(ptr, v); > > + } > > +} > > + > > +static inline int virtio_lduw_p(const void *ptr, struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + return bswap16(lduw_p(ptr)); > > + } else { > > + return lduw_p(ptr); > > + } > > +} > > + > > +static inline int virtio_ldl_p(const void *ptr, struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + return bswap32(ldl_p(ptr)); > > + } else { > > + return ldl_p(ptr); > > + } > > +} > > + > > +static inline uint64_t virtio_ldq_p(const void *ptr, struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + return bswap64(ldq_p(ptr)); > > + } else { > > + return ldq_p(ptr); > > + } > > +} > > + > > +static inline uint32_t virtio_tswap32(uint32_t s, struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + return bswap32(tswap32(s)); > > + } else { > > + return tswap32(s); > > + } > > +} > > + > > +static inline void virtio_tswap32s(uint32_t *s, struct VirtIODevice *vdev) > > +{ > > + tswap32s(s); > > + if (vdev->needs_byteswap) { > > + *s = bswap32(*s); > > + } > > Couldn't you just reuse virtio_tswap32() here? > Yes. Good catch ! > Also, I think a "needs_byteswap" flag gets confusing. Devices shouldn't > have any access to CPU endian specific RAM accessors. > > How about we introduce a flag like "is_bigendian" that we set depending > on the CPU default endianness for legacy virtio. We can then change that > flag accordingly. For the wrappers here, we could just use accessors > like ldl_be_phys() or ldl_le_phys(). > > That should make things a lot easier to understand - and for virtio 1.0 > we only need to set is_bigendian = false :). > Yeah definitely ! > > Alex > > Thanks. -- 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.