From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50013) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WUdYI-0003GE-Ca for qemu-devel@nongnu.org; Mon, 31 Mar 2014 10:51:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WUdY8-00067X-PB for qemu-devel@nongnu.org; Mon, 31 Mar 2014 10:51:06 -0400 Received: from cantor2.suse.de ([195.135.220.15]:41981 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WUdY8-00067J-FX for qemu-devel@nongnu.org; Mon, 31 Mar 2014 10:50:56 -0400 Message-ID: <533980CF.4020904@suse.de> Date: Mon, 31 Mar 2014 16:50:55 +0200 From: Alexander Graf MIME-Version: 1.0 References: <20140328105709.21018.88000.stgit@bahia.local> <20140328105717.21018.17649.stgit@bahia.local> In-Reply-To: <20140328105717.21018.17649.stgit@bahia.local> Content-Type: text/plain; charset=UTF-8; format=flowed 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, 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 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? 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 :). Alex