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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6074 bytes --]

On Thu, Oct 06, 2016 at 02:38:36PM +1100, David Gibson wrote:
> On Wed, Oct 05, 2016 at 05:31:07AM -0700, Peter Maydell wrote:
> > On 4 October 2016 at 16:43, David Gibson <david@gibson.dropbear.id.au> 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.
> 
> It's adding 1 swap on top of the memread/memwrite path - that's the
> path which had no existing swaps (intended primarily for bag-o'-bytes
> block access AFAICT).
> 
> It's less swaps compared to the alternative approach of using the
> readw/writew/readl/writel set of functions.  Those already include a
> tswap, and will in nearly all cases require an additional
> (conditional) swap in the caller.
> 
> > >> 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.)
> 
> I don't see that guest bus endianness is any better defined, or any
> more useful than "guest endianness".  It might have a vague meaning
> for ARM (or embedded Power) in the sense that the on-SoC devices will
> use that endianness.  But since the SoC devices are generally unique
> to the architecture anyway, you still know their endianness
> independent of any notion of guest endianness.
> 
> > >  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.
> 
> When I say that guest endianness is not well defined, what I mean is
> precisely that "what the guest CPU would write" is not well defined.
> For example on Power, the endianness of a given access will depend on:
>     - For server chips, whether the CPU is in LE or BE mode
>     - For embedded chips, the endianness flag in the TLB
>     - For all chips, whether the plain or byte-reversed load/store
>       instructions are used
> 
> [There might even be variants with both a global mode flag and
> per-page flags in the TLB, I'm not sure]
> 
> All of those components are not present in the qtest model.  So
> attempting to do "what the cpu would do" - by making a bunch of
> essentially arbitrary assumptions - gives us zero extra coverage.
> 
> The model proposed here is that our testcases, on every access specify
> a specific final-result endianness.  The alternative is that every
> write from qtest has to do:
>     1) Start with a value (host endian)
>     2) Conditional swap for host endian vs. device endian difference
>     3) Conditional swap for host endian vs. "guest endian" difference
>        simply to compensate for..
>     4) [Existing tswap() within writew/writel]
>        Conditional swap for host endian vs. "guest endian difference
> 
> > > 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
> 
> The are explicit in the testcases, in the sense that on every read or
> write you say this is an LE access or a BE access.  Potentially we
> could have a "guest native" access option as well, but I think that's
> useful 
> 
> > 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,
> 
> What!?  So speaks the person working on a historically LE platform.
> In the kernel, if a driver isn't littered with cpu_to_leXX() it's
> almost certainly broken on a BE platform.

Clarifying here.  In the kernel, byteswaps are not typically required
on writew(), writel() operations.  That's because the kernel writew()
and writel() are defined to have always-LE operation, and most
hardware is LE.

In other words, the kernel writew() and writel() have the same
semantics as the proposed writew_le() and writel_le() *NOT* the
existing qtest writew() and writel() (they're more like the rarely
used kernel __raw_writew() and __raw_writel()).

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]