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

[Alexander Graf <agraf@suse.de>]

On 02/18/2014 05:17 PM, Cornelia Huck wrote:
> On Tue, 18 Feb 2014 17:02:18 +0100
> Alexander Graf <agraf@suse.de> wrote:
>
>>
>>> Am 18.02.2014 um 16:45 schrieb Cornelia Huck <cornelia.huck@de.ibm.com>:
>>>
>>> On Tue, 18 Feb 2014 16:12:08 +0100
>>> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>>>
>>>> On Tue, 18 Feb 2014 17:03:27 +0200
>>>> "Michael S. Tsirkin" <mst@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 <rusty@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 <gkurz@linux.vnet.ibm.com> ]
>>>>>>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>>>>>>> Signed-off-by: Greg Kurz <gkurz@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.

Yeah, but I would hope we want to share as much code as possible here, 
so that config accessors can be shared between legacy virtio and 1.0 
virtio. And in that case we want to have a generic helper to read/write 
pieces of that config space - which this patch introduces for us :).

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

Yes, they can. No, we don't care :).


Alex