Re: [Qemu-devel] [PATCH 1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

[Cornelia Huck]

On Tue, 18 Feb 2014 16:12:08 +0100
Cornelia Huck <address@hidden> wrote:

> On Tue, 18 Feb 2014 17:03:27 +0200
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote:
> > > On 02/18/2014 01:38 PM, 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.  Meanwhile, create a hook for
> > > >little endian ppc (and potentially ARM).  This is called at device
> > > >reset time (which is done before any driver is loaded) since it
> > > >may involve a system call to get the status when running under kvm.
> > > >
> > > >[ fixed checkpatch.pl error with the virtio_byteswap initialisation,
> > > >   ldq_phys() API change, Greg Kurz <address@hidden> ]
> > > >Signed-off-by: Rusty Russell <address@hidden>
> > > >Signed-off-by: Greg Kurz <address@hidden>
> > > >---
> > > >  hw/virtio/virtio.c                |    6 ++
> > > >  include/hw/virtio/virtio-access.h |  132 
> > > > +++++++++++++++++++++++++++++++++++++
> > > >  include/hw/virtio/virtio.h        |    2 +
> > > >  stubs/Makefile.objs               |    1
> > > >  stubs/virtio_get_byteswap.c       |    6 ++
> > > >  5 files changed, 147 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..4fd6ac2 100644
> > > >--- a/hw/virtio/virtio.c
> > > >+++ b/hw/virtio/virtio.c
> > > >@@ -19,6 +19,9 @@
> > > >  #include "hw/virtio/virtio.h"
> > > >  #include "qemu/atomic.h"
> > > >  #include "hw/virtio/virtio-bus.h"
> > > >+#include "hw/virtio/virtio-access.h"
> > > >+
> > > >+bool virtio_byteswap;
> > > 
> > > Could this be a virtio object property rather than a global? Imagine
> > > an AMP guest system with a BE and an LE system running in parallel
> > > accessing two separate virtio devices. With a single global that
> > > would break.
> > > 
> > > 
> > > Alex
> > 
> > Well, how does a device know which CPU uses it?
> > I suspect we are better off waiting for 1.0 with this one.
> > 
> 
> 1.0 makes this a bit more complex, no?
> 
> virtio-endian accessors are defined by the endianness of host and guest
> (doing a bswap depends on the host/guest combination). This needs to be
> per qemu instance. (ioctl under kvm? machine option?)
> 
> For 1.0, we'll have everything le, so a be host will always do a bswap
> (as will a be guest). But whether a device is 1.0 or legacy is not
> something that can be decided globally, or we can't have transitional
> devices with qemu.
> 

So here are two stupid tables on who needs to do byteswaps, one for
legacy devices, one for 1.0 devices:

legacy devices:

            host
       be        le

g be  host no    host yes
u     guest no   guest no
e
s le  host yes   host no
t     guest no   guest no



virtio 1.0 devices:

            host
       be        le

g be  host yes   host no
u     guest yes  guest yes
e
s le  host yes   host no
t     guest no   guest no


This means byteswaps in qemu always depend on guest-endianness for
legacy and on host-endianness for 1.0. If we want to support
transitional devices with a mixture of legacy/1.0, we'll need both a
per-machine and per-device swap flag:

virtio_whatever(device, parameters...)
{
    if (device->legacy) {
        if (guest_needs_byteswap) {
            whatever_byteswap(parameters...);
        } else {
            whatever(parameters...);
        }
    } else { /* 1.0 */
        whatever_le(parameters...);
    }
}

Comments?