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

[Andreas Färber]

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.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg