On 10/05/2016 02:31 PM, 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. > >>> 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. 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.
