[Qemu-devel] Re: [PATCH] e1000: fix endianness issues

[Hollis Blanchard <hollisb@us.ibm.com>]

On Tue, 04 Mar 2008 01:30:15 +0100, Aurelien Jarno wrote:

> This patches fixes endianness issues in the e1000 nic emulation, which
> currently only works on little endian hosts with little endian targets.
> 
> Byte swapping is only needed on big endian targets, as PCI is always
> little endian. cpu_to_le32 and le32_to_cpu functions do not work in that
> case as they refer to the host endianness and not the target one.
> 
> I have tested it on both little and big endian targets (mipsel and mips)
> on both little and big endian hosts (amd64 and powerpc using a
> backported version of e1000.c on top of 0.9.1).
> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> ---
>  hw/e1000.c |   23 ++++++++++++++++------- 1 files changed, 16
>  insertions(+), 7 deletions(-)
> 
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 943f25f..d8419ce 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -720,8 +720,11 @@ e1000_mmio_writel(void *opaque, target_phys_addr_t
> addr, uint32_t val)
>      E1000State *s = opaque;
>      unsigned int index = ((addr - s->mmio_base) & 0x1ffff) >> 2;
>  
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    val = bswap32(val);
> +#endif
...

I cannot explain your successful testing results without further
investigation. (More than likely, there is another qemu emulation layer
that is also incorrectly swapping, canceling this out.) However, almost
all code using TARGET_WORDS_BIGENDIAN is wrong. Yes, I know there's a
lot of it, and it's all wrong.

Unfortunately endianness is one of those really simple ideas that ends
up causing lots of confusion, but I think you can see from this simple
example that *something* isn't quite right:

	target: PowerPC
	host: x86 (i.e. qemu userland runs LE)
	TARGET_WORDS_BIGENDIAN: 1
	swap: yes

	target: PowerPC
	host: PowerPC (i.e. qemu userland runs BE)
	TARGET_WORDS_BIGENDIAN: 1
	swap: yes

Clearly these situations can't *both* be correct, but by using
TARGET_WORDS_BIGENDIAN, the only decision factor above is the target.

(In fact, trying to define "an architecture's endianness" doesn't make
sense at all. For example, some PowerPC have an endianness mode bit, and
can run in either mode (but only one at a time). Some have per-page MMU
endianness attributes, so a load from one page could be BE but a load
from the next is LE. All PowerPC have byte-reversed load/store
instructions that reverse endianness for only that particular memory
access. This situation cannot possibly be represented by an ifdef! I
haven't done a complete search, but I'm willing to be
TARGET_WORDS_BIGENDIAN should be removed completely to avoid future
confusion.)

Before you object because PCI is LE, think about it this way: device
drivers on real hardware must have already arranged for the data written
to a PCI device to be LE. So the 'val' that qemu device callbacks get
for PCI devices is already LE, and "the target's endianness" would be
totally irrelevant even if you could define it.

Below is a patch I'm using to fix rtl8139.c for PowerPC/PowerPC
target/host combination. It works enough for the target to send a DHCP
request and get a response from slirp, but other unrelated bugs prevent
me from testing it more thoroughly. (I'm only sending it now at
Aurelien's request.) Other code that I know is broken (because I've
tried to use it) include isa_mmio.c and pci_host.h, but I don't have
patches for these at this time.




Fix endianness conversion in rtl8139.c.

PCI data is always in LE format, so convert from LE at the interface to
"qemu endianness" internally, then convert again on the way back out.

Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>

diff --git a/qemu/hw/rtl8139.c b/qemu/hw/rtl8139.c
--- a/qemu/hw/rtl8139.c
+++ b/qemu/hw/rtl8139.c
@@ -2735,13 +2735,8 @@ static void rtl8139_io_writew(void *opaq
         default:
             DEBUG_PRINT(("RTL8139: ioport write(w) addr=0x%x val=0x%04x via write(b)\n", addr, val));
 
-#ifdef TARGET_WORDS_BIGENDIAN
-            rtl8139_io_writeb(opaque, addr, (val >> 8) & 0xff);
-            rtl8139_io_writeb(opaque, addr + 1, val & 0xff);
-#else
             rtl8139_io_writeb(opaque, addr, val & 0xff);
             rtl8139_io_writeb(opaque, addr + 1, (val >> 8) & 0xff);
-#endif
             break;
     }
 }
@@ -2802,17 +2797,10 @@ static void rtl8139_io_writel(void *opaq
 
         default:
             DEBUG_PRINT(("RTL8139: ioport write(l) addr=0x%x val=0x%08x via write(b)\n", addr, val));
-#ifdef TARGET_WORDS_BIGENDIAN
-            rtl8139_io_writeb(opaque, addr, (val >> 24) & 0xff);
-            rtl8139_io_writeb(opaque, addr + 1, (val >> 16) & 0xff);
-            rtl8139_io_writeb(opaque, addr + 2, (val >> 8) & 0xff);
-            rtl8139_io_writeb(opaque, addr + 3, val & 0xff);
-#else
             rtl8139_io_writeb(opaque, addr, val & 0xff);
             rtl8139_io_writeb(opaque, addr + 1, (val >> 8) & 0xff);
             rtl8139_io_writeb(opaque, addr + 2, (val >> 16) & 0xff);
             rtl8139_io_writeb(opaque, addr + 3, (val >> 24) & 0xff);
-#endif
             break;
     }
 }
@@ -2958,13 +2946,8 @@ static uint32_t rtl8139_io_readw(void *o
         default:
             DEBUG_PRINT(("RTL8139: ioport read(w) addr=0x%x via read(b)\n", addr));
 
-#ifdef TARGET_WORDS_BIGENDIAN
-            ret  = rtl8139_io_readb(opaque, addr) << 8;
-            ret |= rtl8139_io_readb(opaque, addr + 1);
-#else
             ret  = rtl8139_io_readb(opaque, addr);
             ret |= rtl8139_io_readb(opaque, addr + 1) << 8;
-#endif
 
             DEBUG_PRINT(("RTL8139: ioport read(w) addr=0x%x val=0x%04x\n", addr, ret));
             break;
@@ -3031,17 +3014,10 @@ static uint32_t rtl8139_io_readl(void *o
         default:
             DEBUG_PRINT(("RTL8139: ioport read(l) addr=0x%x via read(b)\n", addr));
 
-#ifdef TARGET_WORDS_BIGENDIAN
-            ret  = rtl8139_io_readb(opaque, addr) << 24;
-            ret |= rtl8139_io_readb(opaque, addr + 1) << 16;
-            ret |= rtl8139_io_readb(opaque, addr + 2) << 8;
-            ret |= rtl8139_io_readb(opaque, addr + 3);
-#else
             ret  = rtl8139_io_readb(opaque, addr);
             ret |= rtl8139_io_readb(opaque, addr + 1) << 8;
             ret |= rtl8139_io_readb(opaque, addr + 2) << 16;
             ret |= rtl8139_io_readb(opaque, addr + 3) << 24;
-#endif
 
             DEBUG_PRINT(("RTL8139: read(l) addr=0x%x val=%08x\n", addr, ret));
             break;
@@ -3091,12 +3067,12 @@ static void rtl8139_mmio_writeb(void *op
 
 static void rtl8139_mmio_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
-    rtl8139_io_writew(opaque, addr & 0xFF, val);
+    rtl8139_io_writew(opaque, addr & 0xFF, le16_to_cpu(val));
 }
 
 static void rtl8139_mmio_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
-    rtl8139_io_writel(opaque, addr & 0xFF, val);
+    rtl8139_io_writel(opaque, addr & 0xFF, le32_to_cpu(val));
 }
 
 static uint32_t rtl8139_mmio_readb(void *opaque, target_phys_addr_t addr)
@@ -3106,12 +3082,12 @@ static uint32_t rtl8139_mmio_readb(void 
 
 static uint32_t rtl8139_mmio_readw(void *opaque, target_phys_addr_t addr)
 {
-    return rtl8139_io_readw(opaque, addr & 0xFF);
+    return cpu_to_le16(rtl8139_io_readw(opaque, addr & 0xFF));
 }
 
 static uint32_t rtl8139_mmio_readl(void *opaque, target_phys_addr_t addr)
 {
-    return rtl8139_io_readl(opaque, addr & 0xFF);
+    return cpu_to_le32(rtl8139_io_readl(opaque, addr & 0xFF));
 }
 
 /* */

-- 
Hollis Blanchard
IBM Linux Technology Center