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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2716 bytes --]

On Tue, Oct 04, 2016 at 01:36:09PM +0100, Peter Maydell wrote:
> On 4 October 2016 at 13:17, Cédric Le Goater <clg@kaod.org> wrote:
> > Some test scenarios require to access memory regions using a specific
> > endianness, such as a device region, but the current qtest memory
> > accessors are done in native endian, which means that the values are
> > byteswapped in qtest if the endianness of the guest and the host are
> > different.
> >
> > To maintain the endianness of a value, we need a new set of memory
> > accessor. This can be done in two ways:
> >
> > - first, convert the value to the required endianness in libqtest and
> >   then use the memread/write routines so that qtest accesses the guest
> >   memory without doing any supplementary byteswapping
> >
> > - an alternative method would be to handle the byte swapping on the
> >   qtest side. For that, we would need to extend the read/write
> >   protocol with an ending word : "native|le|be" and modify the tswap
> >   calls accordingly under the qtest_process_command() routine.
> >
> > The result is the same and the first method is simpler.
> 
> 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.

> 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.  And it's doubly meaningless since it's a
property of the guest cpu, which we're essentially replacing with the
qtest stub anyway.

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.

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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]