From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49353) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bs1tG-0000fW-L7 for qemu-devel@nongnu.org; Thu, 06 Oct 2016 02:10:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bs1tE-0004AB-1m for qemu-devel@nongnu.org; Thu, 06 Oct 2016 02:10:45 -0400 Received: from ozlabs.org ([2401:3900:2:1::2]:42393) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bs1tC-000454-FN for qemu-devel@nongnu.org; Thu, 06 Oct 2016 02:10:43 -0400 Date: Thu, 6 Oct 2016 17:10:32 +1100 From: David Gibson Message-ID: <20161006061032.GK18733@umbus.fritz.box> References: <1475583448-21013-1-git-send-email-clg@kaod.org> <20161004234352.GE18648@umbus.fritz.box> <20161006033836.GC18733@umbus.fritz.box> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="W/+CTqSGWdiRg+8j" Content-Disposition: inline In-Reply-To: <20161006033836.GC18733@umbus.fritz.box> Subject: Re: [Qemu-devel] [PATCH] qtest: add read/write accessors with a specific endianness List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: =?iso-8859-1?Q?C=E9dric?= Le Goater , QEMU Developers , Greg Kurz , Laurent Vivier --W/+CTqSGWdiRg+8j Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 = 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. > >=20 > > 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. >=20 > 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). >=20 > 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. >=20 > > >> 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. > >=20 > > 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.) >=20 > 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. >=20 > > > And it's doubly meaningless since it's a > > > property of the guest cpu, which we're essentially replacing with the > > > qtest stub anyway. > >=20 > > The stub sits on the same bus the guest cpu would. > >=20 > > > 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. > >=20 > > 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. >=20 > 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 >=20 > [There might even be variants with both a global mode flag and > per-page flags in the TLB, I'm not sure] >=20 > 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. >=20 > 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 >=20 > > > 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. > >=20 > > I prefer the swaps to be explicit in the test cases. If your >=20 > 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=20 >=20 > > 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. > >=20 > > Most devices IME do not require byteswaps by guest code, >=20 > 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()). --=20 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 --W/+CTqSGWdiRg+8j Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX9erYAAoJEGw4ysog2bOSG7gQAONXohCdhqPmSLNpVQnl+BMF kLlJG5V0AQ6aZ/H98noXFC/Yf5PvZLIt1NlfGeFmzXtkMrdo8gITuamVsHEpgy96 9LnKHP6xwmeZKIXZKJ5iMqLof7qgw+khUKqF4t4XCI7KeJfBZ8Mrd5+hTr9eTjbE 5/EmQDxKAO7+5I/Ja60iChnFBCAM76qfXjOZnE1ebpVgUWGywhXF+DbSVVAuRC+J 9PZUQsvYwKoWBlTqHC0e4IFqzLDaZToSBOOnttdDcDhOaMRNqH9vo0T3KTVEP9Pk BzgQpRwHpkHeDu3KLXr7TnSJyE0bQX2dhB8jbpkKCILi2WCeSDqcyB7tcZv9Ak8U UIhB3I7/RwlTXsecN2njjTkA23fs4txaT8yJUDWozqmpMX917LB3IWNDDNJTTtmq o2qqdutAI9NPf/m8zxI4G1eJff9Adtc07S1NxPO3ergN4YUVL7stSwCwFJ/9626V r5EDeiRHFvW5Az7QYQi9judcRAn34ONEEEKkXumHJw9VkFKoto7wVWLIYEIhfHvk 5Aki2lJib0mnkP9SJiPSnZLqFqbWtzGi9AOZROXTiq+wAqXG6nuFOpQm8kZerdxj L3V5372NgyGCE9xcUfUlu5PJLc3fq5dHXvqYQ60wtD/DoL2TldzstVii/oetHVSJ KzQElDqhY2KP/VBEGgFl =sjc3 -----END PGP SIGNATURE----- --W/+CTqSGWdiRg+8j--