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

[Peter Maydell]

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.

> 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.

thanks
-- PMM