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

[Cornelia Huck]

On Tue, 18 Feb 2014 17:02:18 +0100
Alexander Graf <address@hidden> wrote:

> 
> 
> > Am 18.02.2014 um 16:45 schrieb Cornelia Huck <address@hidden>:
> > 
> > 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?
> 
> Yes. My point was that we could move all of the ifery above into the device 
> reset function and from then on only use the swap bool property.
> 
> Alex
> 
Hm. So whatever_le for 1.0 devices, and virtio_whatever (checking the
byteswap value) for legacy devices? The device implementation will be
aware of the virtio version anyway.

(Btw., can some of those architectures supporting both le/be run with
mixed le/be cpus? That would be a mess for legacy devices.)