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

[David Gibson]

On Wed, Oct 05, 2016 at 03:49:18PM +0200, Cédric Le Goater wrote:
> 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. 

This case is handled perfectly well by this proposal.

The ARM devices are LE, so testcases testing them should use the
write_le operations.  That will do the right thing regardless of host
endianness.  It will also do the right thing regardless of guest
endianness, if for whatever reason you try to attach an ARM SoC device
to a (notionally) BE target.

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

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature