Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio

[Michael S. Tsirkin]

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 <address@hidden>
> >
> >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 <address@hidden>
> >[ 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 <address@hidden> ]
> >Cc: Cédric Le Goater <address@hidden>
> >Signed-off-by: Greg Kurz <address@hidden>
> >---
> >
> >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   <address@hidden>
> >+ *
> >+ * 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.

Or maybe get rid of the header completely ...

-- 
MST