On Mon, Apr 14, 2014 at 02:31:06PM +0200, Alexander Graf wrote:
>
> On 14.04.14 14:28, Michael S. Tsirkin wrote:
> >On Mon, Apr 14, 2014 at 02:22:36PM +0200, Alexander Graf 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
> >Not sure about all users but it looks like the only file including this
> >is virtio.c right?
> >I'm guessing that's safe there.
>
> any virtio device implementations, since they need to communicate
> with the guest :).
In that case how can we poison regular memory accesses?
Devices normally do more than virtio related things.
> >Or maybe get rid of the header completely ...
>
> and use what instead? Function calls to an external helper .c file?
>
>
> Alex
