On Mon, 14 Apr 2014 14:22:36 +0200 Alexander Graf <ag...@suse.de> wrote:
> > On 14.04.14 13:58, Greg Kurz 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 it all little endian. > > > > We introduce memory accessors to be used accross the virtio code where > > needed. These accessors should support both legacy and 1.0 devices. > > A good way to do it is to introduce a per-device property to store the > > endianness. We choose to set this flag at device reset time because it > > is reasonnable to assume the endianness won't change unless we reboot or > > kexec another kernel. And it is also reasonnable to assume the new kernel > > will reset the devices before using them (otherwise it will break). > > > > We reuse the virtio_is_big_endian() helper since it provides the right > > value for legacy devices with most of the targets, that have fixed > > endianness. It can then be overriden to support endian-ambivalent targets. > > > > To support migration, we need to set the flag in virtio_load() as well. > > > > (a) One solution would be to add it to the stream, but it have some > > drawbacks: > > - since this only affects a few targets, the field should be put into a > > subsection > > - virtio migration code should be ported to vmstate to be able to introduce > > such a subsection > > > > (b) If we assume the following to be true: > > - target endianness falls under some cpu state > > - cpu state is always restored before virtio devices state because they > > get initialized in this order in main(). > > Then an alternative is to rely on virtio_is_big_endian() again at > > load time. No need to mess around with the migration stream in this case. > > > > This patch implements (b). > > > > Note that the tswap helpers are implemented in virtio.c so that > > virtio-access.h stays platform independant. Most of the virtio code > > will be buildable under common-obj instead of obj then, and spare > > some cycles when building for multiple targets. > > > > Signed-off-by: Rusty Russell <ru...@rustcorp.com.au> > > [ ldq_phys() API change, > > relicensed virtio-access.h to GPLv2+ on Rusty's request, > > introduce a per-device is_big_endian flag (supersedes needs_byteswap > > global) > > add VirtIODevice * arg to virtio helpers, > > use the existing virtio_is_big_endian() helper, > > virtio-pci: use the device is_big_endian flag, > > introduce virtio tswap16 and tswap64 helpers, > > move calls to tswap* out of virtio-access.h to make it platform > > independant, > > migration support, > > Greg Kurz <gk...@linux.vnet.ibm.com> ] > > Cc: Cédric Le Goater <c...@fr.ibm.com> > > Signed-off-by: Greg Kurz <gk...@linux.vnet.ibm.com> > > --- > > > > Changes since v6: > > - merge the virtio_needs_byteswap() helper from v6 and existing > > virtio_is_big_endian() > > - virtio-pci: now supports endianness changes > > - virtio-access.h fixes (target independant) > > > > exec.c | 2 - > > hw/virtio/Makefile.objs | 2 - > > hw/virtio/virtio-pci.c | 11 +-- > > hw/virtio/virtio.c | 35 +++++++++ > > include/hw/virtio/virtio-access.h | 138 > > +++++++++++++++++++++++++++++++++++++ > > include/hw/virtio/virtio.h | 2 + > > vl.c | 4 + > > 7 files changed, 185 insertions(+), 9 deletions(-) > > create mode 100644 include/hw/virtio/virtio-access.h > > > > diff --git a/exec.c b/exec.c > > index 91513c6..e6777d0 100644 > > --- a/exec.c > > +++ b/exec.c > > @@ -42,6 +42,7 @@ > > #else /* !CONFIG_USER_ONLY */ > > #include "sysemu/xen-mapcache.h" > > #include "trace.h" > > +#include "hw/virtio/virtio.h" > > #endif > > #include "exec/cpu-all.h" > > > > @@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong > > addr, > > * A helper function for the _utterly broken_ virtio device model to find > > out if > > * it's running on a big endian machine. Don't do this at home kids! > > */ > > -bool virtio_is_big_endian(void); > > bool virtio_is_big_endian(void) > > { > > #if defined(TARGET_WORDS_BIGENDIAN) > > diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs > > index 1ba53d9..68c3064 100644 > > --- a/hw/virtio/Makefile.objs > > +++ b/hw/virtio/Makefile.objs > > @@ -4,5 +4,5 @@ common-obj-y += virtio-bus.o > > common-obj-y += virtio-mmio.o > > common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += dataplane/ > > > > -obj-y += virtio.o virtio-balloon.o > > +obj-y += virtio.o virtio-balloon.o > > obj-$(CONFIG_LINUX) += vhost.o > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index ce97514..82a1689 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -89,9 +89,6 @@ > > /* Flags track per-device state like workarounds for quirks in older > > guests. */ > > #define VIRTIO_PCI_FLAG_BUS_MASTER_BUG (1 << 0) > > > > -/* HACK for virtio to determine if it's running a big endian guest */ > > -bool virtio_is_big_endian(void); > > - > > static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size, > > VirtIOPCIProxy *dev); > > > > @@ -409,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque, > > hwaddr addr, > > break; > > case 2: > > val = virtio_config_readw(vdev, addr); > > - if (virtio_is_big_endian()) { > > + if (vdev->is_big_endian) { > > val = bswap16(val); > > } > > break; > > case 4: > > val = virtio_config_readl(vdev, addr); > > - if (virtio_is_big_endian()) { > > + if (vdev->is_big_endian) { > > val = bswap32(val); > > } > > break; > > @@ -443,13 +440,13 @@ static void virtio_pci_config_write(void *opaque, > > hwaddr addr, > > virtio_config_writeb(vdev, addr, val); > > break; > > case 2: > > - if (virtio_is_big_endian()) { > > + if (vdev->is_big_endian) { > > val = bswap16(val); > > } > > virtio_config_writew(vdev, addr, val); > > break; > > case 4: > > - if (virtio_is_big_endian()) { > > + if (vdev->is_big_endian) { > > val = bswap32(val); > > } > > virtio_config_writel(vdev, addr, val); > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > index aeabf3a..bb646f0 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->is_big_endian = virtio_is_big_endian(); > > + > > if (k->reset) { > > k->reset(vdev); > > } > > @@ -897,6 +900,11 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f) > > BusState *qbus = qdev_get_parent_bus(DEVICE(vdev)); > > VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); > > > > + /* NOTE: we assume that endianness is a cpu state AND > > + * cpu state is restored before virtio devices. > > + */ > > + vdev->is_big_endian = virtio_is_big_endian(); > > + > > if (k->load_config) { > > ret = k->load_config(qbus->parent, f); > > if (ret) > > @@ -1153,6 +1161,33 @@ void virtio_device_set_child_bus_name(VirtIODevice > > *vdev, char *bus_name) > > } > > } > > > > +uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s) > > +{ > > + if (vdev->is_big_endian) { > > + return tswap16(s); > > + } else { > > + return bswap16(tswap16(s)); > > + } > > +} > > + > > +uint32_t virtio_tswap32(VirtIODevice *vdev, uint32_t s) > > +{ > > + if (vdev->is_big_endian) { > > + return tswap32(s); > > + } else { > > + return bswap32(tswap32(s)); > > + } > > +} > > + > > +uint64_t virtio_tswap64(VirtIODevice *vdev, uint64_t s) > > +{ > > + if (vdev->is_big_endian) { > > + return tswap64(s); > > + } else { > > + return bswap64(tswap64(s)); > > + } > > +} > > + > > static void virtio_device_realize(DeviceState *dev, Error **errp) > > { > > VirtIODevice *vdev = VIRTIO_DEVICE(dev); > > diff --git a/include/hw/virtio/virtio-access.h > > b/include/hw/virtio/virtio-access.h > > new file mode 100644 > > index 0000000..5c9013e > > --- /dev/null > > +++ b/include/hw/virtio/virtio-access.h > > @@ -0,0 +1,138 @@ > > +/* > > + * Virtio Accessor Support: In case your target can change endian. > > + * > > + * Copyright IBM, Corp. 2013 > > + * > > + * Authors: > > + * Rusty Russell <ru...@au.ibm.com> > > + * > > + * 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" > > +#include "exec/address-spaces.h" > > + > > +static inline uint16_t virtio_lduw_phys(VirtIODevice *vdev, hwaddr pa) > > +{ > > + if (vdev->is_big_endian) { > > + return lduw_be_phys(&address_space_memory, pa); > > + } > > + return lduw_le_phys(&address_space_memory, pa); > > +} > > + > > +static inline uint32_t virtio_ldl_phys(VirtIODevice *vdev, hwaddr pa) > > +{ > > + if (vdev->is_big_endian) { > > + return ldl_be_phys(&address_space_memory, pa); > > + } > > + return ldl_le_phys(&address_space_memory, pa); > > +} > > + > > +static inline uint64_t virtio_ldq_phys(VirtIODevice *vdev, hwaddr pa) > > +{ > > + if (vdev->is_big_endian) { > > + return ldq_be_phys(&address_space_memory, pa); > > + } > > + return ldq_le_phys(&address_space_memory, pa); > > +} > > + > > +static inline void virtio_stw_phys(VirtIODevice *vdev, hwaddr pa, > > + uint16_t value) > > +{ > > + if (vdev->is_big_endian) { > > + stw_be_phys(&address_space_memory, pa, value); > > + } else { > > + stw_le_phys(&address_space_memory, pa, value); > > + } > > +} > > + > > +static inline void virtio_stl_phys(VirtIODevice *vdev, hwaddr pa, > > + uint32_t value) > > +{ > > + if (vdev->is_big_endian) { > > + stl_be_phys(&address_space_memory, pa, value); > > + } else { > > + stl_le_phys(&address_space_memory, pa, value); > > + } > > +} > > + > > +static inline void virtio_stw_p(VirtIODevice *vdev, void *ptr, uint16_t v) > > +{ > > + if (vdev->is_big_endian) { > > + stw_be_p(ptr, v); > > + } else { > > + stw_le_p(ptr, v); > > + } > > +} > > + > > +static inline void virtio_stl_p(VirtIODevice *vdev, void *ptr, uint32_t v) > > +{ > > + if (vdev->is_big_endian) { > > + stl_be_p(ptr, v); > > + } else { > > + stl_le_p(ptr, v); > > + } > > +} > > + > > +static inline void virtio_stq_p(VirtIODevice *vdev, void *ptr, uint64_t v) > > +{ > > + if (vdev->is_big_endian) { > > + stq_be_p(ptr, v); > > + } else { > > + stq_le_p(ptr, v); > > + } > > +} > > + > > +static inline int virtio_lduw_p(VirtIODevice *vdev, const void *ptr) > > +{ > > + if (vdev->is_big_endian) { > > + return lduw_be_p(ptr); > > + } else { > > + return lduw_le_p(ptr); > > + } > > +} > > + > > +static inline int virtio_ldl_p(VirtIODevice *vdev, const void *ptr) > > +{ > > + if (vdev->is_big_endian) { > > + return ldl_be_p(ptr); > > + } else { > > + return ldl_le_p(ptr); > > + } > > +} > > + > > +static inline uint64_t virtio_ldq_p(VirtIODevice *vdev, const void *ptr) > > +{ > > + if (vdev->is_big_endian) { > > + return ldq_be_p(ptr); > > + } else { > > + return ldq_le_p(ptr); > > + } > > +} > > + > > +uint16_t virtio_tswap16(VirtIODevice *vdev, uint16_t s); > > + > > +static inline void virtio_tswap16s(VirtIODevice *vdev, uint16_t *s) > > +{ > > + *s = virtio_tswap16(vdev, *s); > > +} > > + > > +uint32_t virtio_tswap32(VirtIODevice *vdev, uint32_t s); > > + > > +static inline void virtio_tswap32s(VirtIODevice *vdev, uint32_t *s) > > +{ > > + *s = virtio_tswap32(vdev, *s); > > +} > > + > > +uint64_t virtio_tswap64(VirtIODevice *vdev, uint64_t s); > > + > > +static inline void virtio_tswap64s(VirtIODevice *vdev, uint64_t *s) > > +{ > > + *s = virtio_tswap64(vdev, *s); > > +} > > Could we try to poison any non-virtio, non-endian-specific memory > accessors when this file is included? That way we don't fall into > pitfalls where we end up running stl_p in a tiny corner case somewhere > still. > > > Alex > Hmmm... there are still some stl_p users in virtio.c that I could not port to the new virtio memory accessors... These are the virtio_config_* helpers that gets called from virtio-pci and virtio-mmio. -- 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.