From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60910) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WXZkz-00034J-J3 for qemu-devel@nongnu.org; Tue, 08 Apr 2014 13:24:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WXZku-0007Lj-OZ for qemu-devel@nongnu.org; Tue, 08 Apr 2014 13:24:21 -0400 Sender: Richard Henderson Message-ID: <534430BA.70801@twiddle.net> Date: Tue, 08 Apr 2014 10:24:10 -0700 From: Richard Henderson MIME-Version: 1.0 References: <1396972271-22660-1-git-send-email-peter.maydell@linaro.org> In-Reply-To: <1396972271-22660-1-git-send-email-peter.maydell@linaro.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH for-2.0] hw/pci-host/prep: Don't reverse IO accesses on bigendian hosts List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell , qemu-devel@nongnu.org Cc: qemu-ppc@nongnu.org, Paolo Bonzini , =?ISO-8859-1?Q?Herv=E9_Poussineau?= , Alexander Graf , patches@linaro.org 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 > --- > "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 r~