Re: [Qemu-devel] [PATCH] qtest: add read/write accessors with a specific endianness

[Cédric Le Goater]

On 10/05/2016 02:31 PM, Peter Maydell wrote:
> On 4 October 2016 at 16:43, David Gibson <address@hidden> wrote:
>> On Tue, Oct 04, 2016 at 01:36:09PM +0100, Peter Maydell wrote:
>>> The difficulty with this patch is that it's hard to tell whether
>>> it's really required, or if this is just adding an extra layer
>>> of byteswapping that should really be done in some other location
>>
>> Actually, it's neither.  It's not essential for anything, but it
>> *removes* an extra layer of byteswapping that really never should have
>> been done in the first place.
> 
> The patch is very clearly adding calls to swapping functions.
> It looks like it's mostly convenience functions for not doing
> those swaps explicitly in the test cases.
> 
>>> in the stack. What's the actual test case here?
>>
>> The current readw, readl, etc. all work in "guest endianness".  But
>> guest endianness is not well defined - there are a number of targets
>> which can support either.
> 
> It's guest bus endianness, and it's pretty well defined I think.
> (ARM for instance is LE bus even if the CPU is doing BE writes.)
> 
>>  And it's doubly meaningless since it's a
>> property of the guest cpu, which we're essentially replacing with the
>> qtest stub anyway.
> 
> The stub sits on the same bus the guest cpu would.
> 
>> Furthermore "guest endianness" isn't useful.  With a tiny handful of
>> exceptions, all peripherals have their own endianness which is known
>> independent of the target.  It makes more sense for test cases to
>> explicitly do their accesses in the correct endianness for the device,
>> without having to compensate for the fact that it'll be swapped into
>> the essentially arbitrary "guest endianness" along the way.
> 
> Here I definitely disagree. I think it makes much more sense
> for writes to be "what the guest CPU would write", because that's
> where we're injecting them. If we had a test framework where the
> test was talking directly to the device, then maybe, but we don't.

This is how it all started with the test I wrote. Quite ignorant
of the middle layers, I used cpu_to_be32() as I would have done 
on the guest and then realized that qtest was byte-swapping also
in some cases and so the test failed on a BE host.

>> Basically, the existing byteswaps, instead of removing the need for
>> them in the testcase code, instead mean that target-conditional
>> byteswaps will be *required* in nearly every testcase.  It's a recipe
>> for endianness-unsafe testcases.
> 
> I prefer the swaps to be explicit in the test cases. If your
> test case running on the real CPU would have had to do
> "swap to LE and then write this word", so does the test
> case in our test framework. If your test case just does
> "write the word", then so does the test.
>
> Most devices IME do not require byteswaps by guest code,
> and I think these functions are confusing -- if you try
> to use them to write tests for ARM devices, for instance,
> the _le versions of these functions will introduce an
> incorrect byteswap on a BE host, even though ARM CPUs and
> devices are typically all LE.

hmmm, yes I agree :/ My scenario is for a ARM SoC running LE 
on a BE host and accessing a BE device. 

So how do we handle the possible bswap due to the host and 
guest endianness being different ?  I am confused. Should we 
try to remove the tswap from qtest ? 

Thanks

C.