On Thu, 6 Oct 2016 12:03:34 +0100 Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 6 October 2016 at 04:38, David Gibson <da...@gibson.dropbear.id.au> 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). > > Yeah, I hadn't noticed it was using the memread/memwrite path. > I disagree with using that code path for what ought to be > register read/writes (among other things, it's not clear to me > that it guarantees that a 4-byte access by the test code is > always a 4-byte access on the device, etc). > FWIW, Cedric had another proposal which apparently went unnoticed: <fc24ad74-da26-a713-9312-a2c2d07fb...@kaod.org> The idea is to add an optional endianness argument to the read*/write* commands in the qtest protocol: - libqtest then provides explicit _le and _be APIs - no extra byteswap is performed on the test program side: qtest actually handles that and does exactly 1 or 0 byteswap. - it does not use memread/memwrite - the current 'guest native' API where qtest tswaps is preserved > >> >> 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. > > It means "the endianness that you would see if you could snoop > the data bus at the output of the CPU". It is definitely well > defined for every QEMU target because that's what TARGET_WORDS_BIGENDIAN > tells you. It's not the same as whatever the device thinks it > might interpret values as (though obviously people don't > often design systems where they differ). > > >> > 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] > > AFAIK, these things all take effect inside the guest CPU, and affect > the value the guest eventually writes to the CPU bus. > > > 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. > > Well, platforms where the devices and the CPU agree about > endianness. > > > 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? > > This was the result of my mis-reading of the code: I thought > they were doing a byteswap and then calling the readl/readw/etc > functions, not the readmem etc functions. > > > 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. > > I think I need to think about this a bit... > > thanks > -- PMM >
