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

[Rusty Russell]

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.
> We'd need to check current_cpu then, which for Xen is always NULL.

This is why the check is performed on a random CPU when they first
acknowledge the device.  It's a hacky assumption, but that's why there's
a proposal to nail virtio to LE for the 1.0 OASIS standard.

You can't actually change endian of a virtio device in flight: it
doesn't make sense since there's readable state there already.

Cheers,
Rusty.