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

[Cédric Le Goater]

On 10/05/2016 01:43 AM, David Gibson wrote:
> On Tue, Oct 04, 2016 at 01:36:09PM +0100, Peter Maydell wrote:
>> On 4 October 2016 at 13:17, Cédric Le Goater <address@hidden> wrote:
>>> Some test scenarios require to access memory regions using a specific
>>> endianness, such as a device region, but the current qtest memory
>>> accessors are done in native endian, which means that the values are
>>> byteswapped in qtest if the endianness of the guest and the host are
>>> different.
>>>
>>> To maintain the endianness of a value, we need a new set of memory
>>> accessor. This can be done in two ways:
>>>
>>> - first, convert the value to the required endianness in libqtest and
>>>   then use the memread/write routines so that qtest accesses the guest
>>>   memory without doing any supplementary byteswapping
>>>
>>> - an alternative method would be to handle the byte swapping on the
>>>   qtest side. For that, we would need to extend the read/write
>>>   protocol with an ending word : "native|le|be" and modify the tswap
>>>   calls accordingly under the qtest_process_command() routine.
>>>
>>> The result is the same and the first method is simpler.
>>
>> 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.

So may be removing the tswap in qtest_process_command() is a better 
solution ? I don't know how much of an earthquake this will be in the 
tests results.

Tests will have to be explicit on which endianness they expect else 
they will be using the qtest default endianness which is the host. 


>> 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.  And it's doubly meaningless since it's a
> property of the guest cpu, which we're essentially replacing with the
> qtest stub anyway.
> 
> 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.
> 
> 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.

and plenty of baroque work arounds.

Thanks,
C.