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 17:40, schrieb Anthony Liguori:
>> Andreas Färber <address@hidden> writes:
>>> Am 08.08.2013 15:31, schrieb Anthony Liguori:
>>>> 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.
>
> Exactly, just read it as "is in ... Endian mode". On the CPUs I am more
> familiar with (e.g., 970), this used to be controlled via an MSR bit,
> which as CPUPPCState::msr exists per CPUState. I did not check on real
> hardware, but from the QEMU code this would allow for the mixed-endian
> scenario described above.
>
>> 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.
>
> What is offending me is only the following: If we name it
> cpu_get_byteswap() as proposed by you, then its first argument should be
> a CPUState *cpu. Its value would be read from the derived type's state,
> such as the MSR bit in the code path that you wanted duplicated. The
> function implementing that register-reading would be a hook in CPUClass,
> with a default implementation in qom/cpu.c rather than a fallback in
> stubs/. To access CPUClass, CPUState cannot be NULL, as brought up by
> Stefano for cpu_do_unassigned_access(); not following that pattern
> prevents mixing CPU architectures, which my large refactorings have
> partially been about. Cf. my guest-memory-dump refactoring.
>
> If it is just some random global value, then please don't call it
> cpu_*(). Since sPAPR is not a target of its own, I don't see how/where
> you want to implement that hcall query as per-target function either,
> that might rather call for a QEMUMachine hook?
>
> I don't care or argue about byte lanes here, I am just trying to keep
> API design consistent and not regressing on the way to heterogeneous
> emulation.

That's a lot of replumbing and indirect function calls for a fairly
obscure case.  We certainly don't have a nice CPUState lying around in
virtio at the moment, for example.

I can try to plumb this in if there's consensus, but I suspect it's
making the job 10x harder.

(The next logical step would be for st* and ld* to take the cpu to query
its endianness, Anthony's weird ideas notwithstanding).

Cheers,
Rusty.