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

["Andreas Färber" <afaerber@suse.de>]

Am 09.08.2013 09:35, schrieb Rusty Russell:
> Andreas Färber <afaerber@suse.de> writes:
>> [...] 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.

It's how QOM methods generally work. And yes, little endian ppc64 is in
fact a pretty obscure case. But IBM was just recently reported to adopt
the IP licensing model like ARM, so chances are we will see the same
mixed-core scenarios as with ARM + MicroBlaze/SuperH these days.

http://news.techeye.net/chips/ibms-launches-intel-server-challenge

Generally the problem is that we can't have multiple same-named global
functions when combining multiple targets, so we need a way to dispatch
- either from the individual CPU or from the machine. I would assume in
practice mixed cores will have the same endianness.

Or by making endianness a user-tweakable property of the virtio devices
rather than trying to auto-detect it.

>  We certainly don't have a nice CPUState lying around in
> virtio at the moment, for example.

Compare
http://git.qemu.org/?p=qemu.git;a=commit;h=c658b94f6e8c206c59d02aa6fbac285b86b53d2c

cpu_single_env has since been renamed to the mentioned current_cpu and
been changed to CPUState type.

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

I doubt it's that complicated, estimated less than ten minutes for me,
and not doing it is making the other job significantly harder.
cpu_get_dump_info() is already a hard nut to crack.

Regards,
Andreas

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