| Message ID | 1396972271-22660-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 8 April 2014 16:51, Peter Maydell <peter.maydell@linaro.org> wrote: > This fixes the endianness test failure on bigendian hosts. > HOWEVER I have not actually tested it with a guest :-) Alex found me a prep kernel. It seems to have issues with its userspace, but; without this patch, when we probe the IDE devices we get: hdc: EQUMD DVR-MO, ATAPI cdrom or floppy?, assuming FLOPPY drive (spot the byte-reversed halfwords!) and with the patch we get: hdc: QEMU DVD-ROM, ATAPI CD/DVD-ROM drive so I'm pretty confident this patch is not just fixing a test case but also fixing problems with real guests. thanks -- PMM
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~
On 8 April 2014 18:24, Richard Henderson <rth@twiddle.net> wrote: > 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. > Reviewed-by: Richard Henderson <rth@twiddle.net> Thanks; applied to master. -- PMM
diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c index d3e746c..4014540 100644 --- a/hw/pci-host/prep.c +++ b/hw/pci-host/prep.c @@ -145,9 +145,9 @@ static uint64_t raven_io_read(void *opaque, hwaddr addr, if (size == 1) { return buf[0]; } else if (size == 2) { - return lduw_p(buf); + return lduw_le_p(buf); } else if (size == 4) { - return ldl_p(buf); + return ldl_le_p(buf); } else { g_assert_not_reached(); } @@ -164,9 +164,9 @@ static void raven_io_write(void *opaque, hwaddr addr, if (size == 1) { buf[0] = val; } else if (size == 2) { - stw_p(buf, val); + stw_le_p(buf, val); } else if (size == 4) { - stl_p(buf, val); + stl_le_p(buf, val); } else { g_assert_not_reached(); }
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 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(-)