On Tue, 18 Feb 2014 17:02:18 +0100 Alexander Graf <ag...@suse.de> wrote:
> > > > Am 18.02.2014 um 16:45 schrieb Cornelia Huck <cornelia.h...@de.ibm.com>: > > > > On Tue, 18 Feb 2014 16:12:08 +0100 > > Cornelia Huck <cornelia.h...@de.ibm.com> wrote: > > > >> On Tue, 18 Feb 2014 17:03:27 +0200 > >> "Michael S. Tsirkin" <m...@redhat.com> 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 <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. 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 <gk...@linux.vnet.ibm.com> ] > >>>>> Signed-off-by: Rusty Russell <ru...@rustcorp.com.au> > >>>>> Signed-off-by: Greg Kurz <gk...@linux.vnet.ibm.com> > >>>>> --- > >>>>> 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.)