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

[Andreas Färber]

Am 09.08.2013 09:00, schrieb Rusty Russell:
> Andreas Färber <address@hidden> writes:
>> Am 08.08.2013 15:31, schrieb Anthony Liguori:
>>> Rusty Russell <address@hidden> writes:
>>> 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.
> 
> Below is the minimal solution, which is sufficient for virtio.
> 
> If Anton wants per-cpu endianness for gdb, he'll need something more
> sophisticated.
> 
> Feedback welcome!
> Rusty.
> 
> Subject: cpu_get_byteswap: function for endian-ambivalent targets.
> 
> Signed-off-by: Rusty Russell <address@hidden>
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index a5bb515..ed84267 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -357,4 +357,13 @@ void cpu_reset_interrupt(CPUState *cpu, int mask);
>   */
>  void cpu_resume(CPUState *cpu);
>  
> +/**
> + * cpu_get_byteswap:
> + *
> + * Is (any) CPU running in byteswapped mode: normally false.  This
> + * doesn't take a cpu argument, because we don't support heterogeneous
> + * endianness.
> + */
> +bool cpu_get_byteswap(void);
> +
>  #endif
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 9b701b4..d4af94a 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -25,3 +25,4 @@ stub-obj-y += vm-stop.o
>  stub-obj-y += vmstate.o
>  stub-obj-$(CONFIG_WIN32) += fd-register.o
>  stub-obj-y += cpus.o
> +stub-obj-y += cpu_byteswap.o
> diff --git a/stubs/cpu_byteswap.c b/stubs/cpu_byteswap.c
> new file mode 100644
> index 0000000..b3b669f
> --- /dev/null
> +++ b/stubs/cpu_byteswap.c
> @@ -0,0 +1,6 @@
> +#include "qom/cpu.h"
> +
> +bool cpu_get_byteswap(void)
> +{
> +    return false;
> +}

That is exactly what I asked not to do. first_cpu_get_byteswap() or
virtio_get_byteswap() etc. would be names OK for me. But see below.

> 
> Subject: target-ppc: ppc64 targets can be either endian.
> 
> In this case, we just query the first cpu.
> 
> Signed-off-by: Rusty Russell <address@hidden>
> 
> diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
> index 616aab6..0a508eb 100644
> --- a/target-ppc/misc_helper.c
> +++ b/target-ppc/misc_helper.c
> @@ -116,3 +116,8 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value)
>  {
>      hreg_store_msr(env, value, 0);
>  }
> +
> +bool cpu_get_byteswap(void)
> +{
> +    return first_cpu->hflags & (1 << MSR_LE);
> +}

This assumes that first_cpu != NULL, which I pointed out is not the case
for Xen. I'm not aware of a ppc Xen implementation, but it shows that
you should define which endianness (probably little) you want to adopt
in such a case.

You should also urgently update your QEMU since that code will not
build. first_cpu is CPUState since a number of weeks, you would need to
cast to PowerPCCPU *cpu = POWERPC_CPU(first_cpu); in the non-NULL case.

Andreas

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