Re: [Qemu-devel] [PATCH for-2.0] hw/pci-host/prep: Don't reverse IO accesses on bigendian hosts

[Richard Henderson <rth@twiddle.net>]

On 04/08/2014 08:51 AM, Peter Maydell wrote:
> The raven_io_read() and raven_io_write() functions pass and
> return values in little-endian format (since the IO op struct
> is marked DEVICE_LITTLE_ENDIAN); however they were storing the
> values in the buffer to pass to address_space_read/write()
> in host-endian order, which meant that on big-endian hosts
> the values were inadvertently reversed. Use the *_le_p()
> accessors instead so that we are consistent regardless of
> host endianness.
> 
> Strictly speaking the byte order of the buffer for
> address_space_rw() is target byte order (which for PPC
> will be BE) but it doesn't actually matter as long as we
> are consistent about the marking on the IO op struct and
> which stl_*_p().
> 
> This bug was probably introduced due to confusion caused by
> the two different versions of ldl_p() and friends:
>  bswap.h defines versions meaning "host endianness access"
>  cpu-all.h defines versions meaning "target endianness access"
> As a target-independent source file prep.c gets the bswap.h
> versions; the very similar looking code in ioport.c is
> compiled per-target and gets the cpu-all.h versions.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> "Why is a raven like a writing desk?
>  Because it is nevar put with the wrong end in front!"
>    -- Lewis Carroll

Ha ha.

> 
> This fixes the endianness test failure on bigendian hosts.
> HOWEVER I have not actually tested it with a guest :-)
> and endianness issues are notoriously hard to reason about
> correctly. Review appreciated.
> 
> RTH suggests that we rename the cpu-all.h ldl_p &c to
> ldl_te_p() &c (for 'target endianness') to reduce confusion;
> I agree but this probably also requires some auditing of
> users to check for other mistaken uses and in any case is
> 2.1 material.
> 
>  hw/pci-host/prep.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~