Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access

[Anthony Liguori]

Andreas Färber <address@hidden> writes:

> Am 08.08.2013 15:31, schrieb Anthony Liguori:
>> Rusty Russell <address@hidden> writes:
>> 
>>> Virtio is currently defined to work as "guest endian", but this is a
>>> problem if the guest can change endian.  As most targets can't change
>>> endian, we make it a per-target option to avoid pessimising.
>>>
>>> This is based on a simpler patch by Anthony Liguouri, which only handled
>>> the vring accesses.  We also need some drivers to access these helpers,
>>> eg. for data which contains headers.
>>>
>>> Signed-off-by: Rusty Russell <address@hidden>
>>> ---
>>>  hw/virtio/virtio.c                |  46 +++++++++----
>>>  include/hw/virtio/virtio-access.h | 138 
>>> ++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 170 insertions(+), 14 deletions(-)
>>>  create mode 100644 include/hw/virtio/virtio-access.h
>>>
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 8176c14..2887f17 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -18,6 +18,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.
>>>   * x86 pagesize again. */
>>> @@ -84,6 +85,20 @@ struct VirtQueue
>>>      EventNotifier host_notifier;
>>>  };
>>>  
>>> +#ifdef TARGET_VIRTIO_SWAPENDIAN
>>> +bool virtio_byteswap;
>>> +
>>> +/* Ask target code if we should swap endian for all vring and config 
>>> access. */
>>> +static void mark_endian(void)
>>> +{
>>> +    virtio_byteswap = virtio_swap_endian();
>>> +}
>>> +#else
>>> +static void mark_endian(void)
>>> +{
>>> +}
>>> +#endif
>>> +
>> 
>> It would be very good to avoid a target specific define here.  We would
>> like to move to only building a single copy of the virtio code.
>
> +1
>
>> We have a mechanism to do weak functions via stubs/.  I think it would
>> be better to do cpu_get_byteswap() as a stub function and then overload
>> it in the ppc64 code.
>
> If this as your name indicates is a per-CPU function then it should go
> into CPUClass. Interesting question is, what is virtio supposed to do if
> we have two ppc CPUs, one is Big Endian, the other is Little Endian.

PPC64 is big endian.  AFAIK, there is no such thing as a little endian
PPC64 processor.

This is just a processor mode where loads/stores are byte lane swapped.
Hence the name 'cpu_get_byteswap'.  It's just asking whether the
load/stores are being swapped or not.

At least for PPC64, it's not possible to enable/disable byte lane
swapping for individual CPUs.  It's done through a system-wide hcall.

FWIW, I think most bi-endian architectures are this way too so I think
this is equally applicable to ARM.  Peter, is that right?

Regards,

Anthony Liguori

> We'd need to check current_cpu then, which for Xen is always NULL.
>
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg