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 <da...@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
signature.asc
Description: PGP signature
