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

[Greg Kurz]

On Mon, 14 Apr 2014 14:22:36 +0200
Alexander Graf <address@hidden> 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
> 

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                                     address@hidden
                                                 address@hidden
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.