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

[David Gibson]

On Wed, Oct 05, 2016 at 05:31:07AM -0700, 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.

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.

The only devices I can think of which don't have a fixed endianness
regardless of platform are:
   * legacy virtio: this was an insane design choice, which is why it
     went away in virtio-1.0
   * graphics card framebuffers, which can usually be configured
     either way (or have both BE and LE views of the framebuffer)
   * a handful of simple devices, usually from a while back, where
     some idiot thought a BE version of a previously LE device (or vice
     versa) was a good idea

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

Huh?  How so?

If you're accessing an LE device (like PCI) you say "write_le" and
there are no byteswaps at all on an LE host, and a single byteswap on
a BE host, which is just what you want.  For a PCI device "write_le"
is the correct option regardless of both host and guest platform.

That makes testcases straightforward to write.  As a bonus, it forces
the test author to think momentarily about what the right endianness
is for each access, which is a good habit to be in (not thinking about
this is why we've so often had endianness-broken drivers).

Switching to an arbitrary "guest endianness" and back again makes the
test more complicated, and accomplishes no extra coverage (because all
the swaps under consideration are in the test code anyway).  It gains
nothing but confusion.

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