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

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

[Hollis Blanchard]

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

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

[Aurelien Jarno]

On Fri, Mar 07, 2008 at 11:23:51AM -0600, Hollis Blanchard wrote:
> 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.
> 

The problem is actually that the GT64xxx is able to byteswap PCI
accesses depending on a configuration bit, which is enabled by the Linux
kernel. That means that those TARGET_WORDS_BIGENDIAN are indeed wrong
and only a workaround for that case.

To fix the problem, we need to be able to register byteswapped memory
regions. Does someone have an idea how to do that?

[snip]

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

Agreed for those changes, through they would break the current MIPS
Malta emulation.

> @@ -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));
>  }
>  

Are those changes really necessary? The rtl8139 emulation works
correctly on a big endian host (tested with an i386 target). This part
of the patch would probably break it. Unless the Linux driver only does
byte accesses, which are not changed by this patch.

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

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

[Hollis Blanchard]

On Sat, 2008-03-08 at 14:59 +0100, Aurelien Jarno wrote: 
> On Fri, Mar 07, 2008 at 11:23:51AM -0600, Hollis Blanchard wrote:
> > 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.
> > 
> 
> The problem is actually that the GT64xxx is able to byteswap PCI
> accesses depending on a configuration bit, which is enabled by the Linux
> kernel. That means that those TARGET_WORDS_BIGENDIAN are indeed wrong
> and only a workaround for that case.
> 
> To fix the problem, we need to be able to register byteswapped memory
> regions. Does someone have an idea how to do that?

That is an interesting situation.

I don't know qemu internals enough, but from what I've seen this is not
currently possible.

If I were going to add such a thing, I would add an "address space"
argument to cpu_register_physical_memory(), and PCI devices would
specify the address space of the PCI controller. (Note: this would also
help solve qemu's "can't relocate PCI controller BARs" limitation.)

With this system, the PCI controller would register a callback for its
entire physical address space in the "root" IO table. In the gt64xxx
case, that callback would check the config bit and perform whatever
swapping is appropriate, then dispatch to the device callback via a
second IO table.

> > 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;
> 
> Agreed for those changes, through they would break the current MIPS
> Malta emulation.

Yeah, fixing qemu's endianness handling is going to be a little tricky,
since you might need to fix all layers at once, rather than each
component individually.

> > @@ -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));
> >  }
> >  
> 
> Are those changes really necessary? The rtl8139 emulation works
> correctly on a big endian host (tested with an i386 target). This part
> of the patch would probably break it. Unless the Linux driver only does
> byte accesses, which are not changed by this patch.

Hmm, yes, there is a problem with this patch. In the "touching many
1-byte registers as a single 4-byte access" change to rtl8139_io_readl()
above, we made sure that the result was LE. What we should have done is
make it host-endianness like all the other return values from
rtl8139_io_readl(). Then in rtl8139_mmio_readl(), we know we must swap
host->LE.

I don't know why rtl8139 would work today with LE/BE target/host, but if
it does, it must be due to swapping in another layer, because this patch
is the right thing to do here. If qemu currently swaps 5 times and that
comes out the same as swapping once, we still need to fix it... :)

-- 
Hollis Blanchard
IBM Linux Technology Center

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

[Aurelien Jarno]

On Sat, Mar 08, 2008 at 11:08:48AM -0600, Hollis Blanchard wrote:
> On Sat, 2008-03-08 at 14:59 +0100, Aurelien Jarno wrote: 
> > On Fri, Mar 07, 2008 at 11:23:51AM -0600, Hollis Blanchard wrote:
> > > 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

[snip]
 
> > > @@ -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));
> > >  }
> > >  
> > 
> > Are those changes really necessary? The rtl8139 emulation works
> > correctly on a big endian host (tested with an i386 target). This part
> > of the patch would probably break it. Unless the Linux driver only does
> > byte accesses, which are not changed by this patch.

I have tested this part of the patch on a big endian host (PowerPC), and
I confirm it breaks the rtl8139 emulation (tested on i386 and mipsel
targets).

> Hmm, yes, there is a problem with this patch. In the "touching many
> 1-byte registers as a single 4-byte access" change to rtl8139_io_readl()
> above, we made sure that the result was LE. What we should have done is
> make it host-endianness like all the other return values from
> rtl8139_io_readl(). Then in rtl8139_mmio_readl(), we know we must swap
> host->LE.
> 
> I don't know why rtl8139 would work today with LE/BE target/host, but if
> it does, it must be due to swapping in another layer, because this patch
> is the right thing to do here. If qemu currently swaps 5 times and that
> comes out the same as swapping once, we still need to fix it... :)

I hope the discussion on IRC last week-end convinced you that you don't
need to swap the value depending on the host endianness. For those who
haven't followed the discussion, qemu does memory accesses with a couple
(address, value). The addresses and values are always stored in host 
endianness, and this does not need to be swapped depending on the host. 
It may have to be swapped depending on the target, if the value is 
swapped on the real hardware (bus connected backward, chipset, etc.)

Thanks to Paul Brook for all the explanations.

Basically that means that one part of my e1000 part is actually correct,
while the other is correct in the case of the MIPS Malta machine, but
may be wrong for other targets/machines. I am therefore planning to 
commit the correct part (see patch below), except if someone still have
some comments on it.


diff --git a/hw/e1000.c b/hw/e1000.c
index 1c77afc..e3dbef9 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -721,7 +721,7 @@ e1000_mmio_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
     unsigned int index = ((addr - s->mmio_base) & 0x1ffff) >> 2;
 
     if (index < NWRITEOPS && macreg_writeops[index])
-        macreg_writeops[index](s, index, le32_to_cpu(val));
+        macreg_writeops[index](s, index, val);
     else if (index < NREADOPS && macreg_readops[index])
         DBGOUT(MMIO, "e1000_mmio_writel RO %x: 0x%04x\n", index<<2, val);
     else
@@ -734,7 +734,7 @@ e1000_mmio_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
     // emulate hw without byte enables: no RMW
     e1000_mmio_writel(opaque, addr & ~3,
-                      cpu_to_le32(le16_to_cpu(val & 0xffff) << (8*(addr & 3))));
+                      (val & 0xffff) << (8*(addr & 3)));
 }
 
 static void
@@ -742,7 +742,7 @@ e1000_mmio_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
     // emulate hw without byte enables: no RMW
     e1000_mmio_writel(opaque, addr & ~3,
-                      cpu_to_le32((val & 0xff)  << (8*(addr & 3))));
+                      (val & 0xff) << (8*(addr & 3)));
 }
 
 static uint32_t
@@ -752,7 +752,7 @@ e1000_mmio_readl(void *opaque, target_phys_addr_t addr)
     unsigned int index = ((addr - s->mmio_base) & 0x1ffff) >> 2;
 
     if (index < NREADOPS && macreg_readops[index])
-        return cpu_to_le32(macreg_readops[index](s, index));
+        return macreg_readops[index](s, index);
     DBGOUT(UNKNOWN, "MMIO unknown read addr=0x%08x\n", index<<2);
     return 0;
 }
@@ -760,15 +760,15 @@ e1000_mmio_readl(void *opaque, target_phys_addr_t addr)
 static uint32_t
 e1000_mmio_readb(void *opaque, target_phys_addr_t addr)
 {
-    return (le32_to_cpu(e1000_mmio_readl(opaque, addr & ~3)) >>
+    return ((e1000_mmio_readl(opaque, addr & ~3)) >>
             (8 * (addr & 3))) & 0xff;
 }
 
 static uint32_t
 e1000_mmio_readw(void *opaque, target_phys_addr_t addr)
 {
-    return cpu_to_le16((le32_to_cpu(e1000_mmio_readl(opaque, addr & ~3)) >>
-                        (8 * (addr & 3))) & 0xffff);
+    return ((e1000_mmio_readl(opaque, addr & ~3)) >>
+            (8 * (addr & 3))) & 0xffff;
 }
 
 int mac_regtosave[] = {

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

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

[Hollis Blanchard]

On Tue, 2008-03-11 at 23:49 +0100, Aurelien Jarno wrote:
> On Sat, Mar 08, 2008 at 11:08:48AM -0600, Hollis Blanchard wrote:
> > On Sat, 2008-03-08 at 14:59 +0100, Aurelien Jarno wrote: 
> > > On Fri, Mar 07, 2008 at 11:23:51AM -0600, Hollis Blanchard wrote:
> > > > 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
> 
> [snip]
> 
> > > > @@ -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));
> > > >  }
> > > >  
> > > 
> > > Are those changes really necessary? The rtl8139 emulation works
> > > correctly on a big endian host (tested with an i386 target). This part
> > > of the patch would probably break it. Unless the Linux driver only does
> > > byte accesses, which are not changed by this patch.
> 
> I have tested this part of the patch on a big endian host (PowerPC), and
> I confirm it breaks the rtl8139 emulation (tested on i386 and mipsel
> targets).

OK, I will do some regression testing too before I submit a patch for
inclusion.

> > Hmm, yes, there is a problem with this patch. In the "touching many
> > 1-byte registers as a single 4-byte access" change to rtl8139_io_readl()
> > above, we made sure that the result was LE. What we should have done is
> > make it host-endianness like all the other return values from
> > rtl8139_io_readl(). Then in rtl8139_mmio_readl(), we know we must swap
> > host->LE.
> > 
> > I don't know why rtl8139 would work today with LE/BE target/host, but if
> > it does, it must be due to swapping in another layer, because this patch
> > is the right thing to do here. If qemu currently swaps 5 times and that
> > comes out the same as swapping once, we still need to fix it... :)
> 
> I hope the discussion on IRC last week-end convinced you that you don't
> need to swap the value depending on the host endianness.

Absolutely not! At least, not with the current qemu design.

> For those who
> haven't followed the discussion, qemu does memory accesses with a couple
> (address, value). The addresses and values are always stored in host 
> endianness, and this does not need to be swapped depending on the host. 
> It may have to be swapped depending on the target, if the value is 
> swapped on the real hardware (bus connected backward, chipset, etc.)
> 
> Thanks to Paul Brook for all the explanations.

The scheme Paul outlined (but hasn't actually proposed as far as I've
seen) involved an end-to-end overhaul of endianness manipulations in
qemu, removing byte swapping from some device emulation and adding some
to bus layers. I still don't understand his entire idea, and I think
some prototype code will be needed to see it. Until/unless that is
implemented, we will continue to require swapping in the device
emulation.

> Basically that means that one part of my e1000 part is actually correct,
> while the other is correct in the case of the MIPS Malta machine, but
> may be wrong for other targets/machines. I am therefore planning to 
> commit the correct part (see patch below), except if someone still have
> some comments on it.
> 
> 
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 1c77afc..e3dbef9 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -721,7 +721,7 @@ e1000_mmio_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>      unsigned int index = ((addr - s->mmio_base) & 0x1ffff) >> 2;
> 
>      if (index < NWRITEOPS && macreg_writeops[index])
> -        macreg_writeops[index](s, index, le32_to_cpu(val));
> +        macreg_writeops[index](s, index, val);
>      else if (index < NREADOPS && macreg_readops[index])
>          DBGOUT(MMIO, "e1000_mmio_writel RO %x: 0x%04x\n", index<<2, val);
>      else

This cannot work. PowerPC targets have MMIO instructions that can
produce "val" in either BE or LE format. Given that e1000 is a PCI
device, val in this case is LE, and will need swapping on a BE host.

The only reason this code might work for MIPS is due to a *lack* of
endianness handling in the gt64xxx.c bridge emulation, but that is a
bug, not a feature. :)

-- 
Hollis Blanchard
IBM Linux Technology Center

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

[Aurelien Jarno]

On Wed, Mar 12, 2008 at 08:52:00AM -0500, Hollis Blanchard wrote:
> On Tue, 2008-03-11 at 23:49 +0100, Aurelien Jarno wrote:
> > On Sat, Mar 08, 2008 at 11:08:48AM -0600, Hollis Blanchard wrote:
> > > On Sat, 2008-03-08 at 14:59 +0100, Aurelien Jarno wrote: 
> > > > On Fri, Mar 07, 2008 at 11:23:51AM -0600, Hollis Blanchard wrote:
> > > > > 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
> > 
> > [snip]
> > 
> > > > > @@ -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));
> > > > >  }
> > > > >  
> > > > 
> > > > Are those changes really necessary? The rtl8139 emulation works
> > > > correctly on a big endian host (tested with an i386 target). This part
> > > > of the patch would probably break it. Unless the Linux driver only does
> > > > byte accesses, which are not changed by this patch.
> > 
> > I have tested this part of the patch on a big endian host (PowerPC), and
> > I confirm it breaks the rtl8139 emulation (tested on i386 and mipsel
> > targets).
> 
> OK, I will do some regression testing too before I submit a patch for
> inclusion.
> 
> > > Hmm, yes, there is a problem with this patch. In the "touching many
> > > 1-byte registers as a single 4-byte access" change to rtl8139_io_readl()
> > > above, we made sure that the result was LE. What we should have done is
> > > make it host-endianness like all the other return values from
> > > rtl8139_io_readl(). Then in rtl8139_mmio_readl(), we know we must swap
> > > host->LE.
> > > 
> > > I don't know why rtl8139 would work today with LE/BE target/host, but if
> > > it does, it must be due to swapping in another layer, because this patch
> > > is the right thing to do here. If qemu currently swaps 5 times and that
> > > comes out the same as swapping once, we still need to fix it... :)
> > 
> > I hope the discussion on IRC last week-end convinced you that you don't
> > need to swap the value depending on the host endianness.
> 
> Absolutely not! At least, not with the current qemu design.
> 
> > For those who
> > haven't followed the discussion, qemu does memory accesses with a couple
> > (address, value). The addresses and values are always stored in host 
> > endianness, and this does not need to be swapped depending on the host. 
> > It may have to be swapped depending on the target, if the value is 
> > swapped on the real hardware (bus connected backward, chipset, etc.)
> > 
> > Thanks to Paul Brook for all the explanations.
> 
> The scheme Paul outlined (but hasn't actually proposed as far as I've
> seen) involved an end-to-end overhaul of endianness manipulations in
> qemu, removing byte swapping from some device emulation and adding some
> to bus layers. I still don't understand his entire idea, and I think
> some prototype code will be needed to see it. Until/unless that is
> implemented, we will continue to require swapping in the device
> emulation.

That was to support the target endianness. The support for hosts of
different endianness is already correctly implemented in the current
design, that is qemu works with host endianness.

> > Basically that means that one part of my e1000 part is actually correct,
> > while the other is correct in the case of the MIPS Malta machine, but
> > may be wrong for other targets/machines. I am therefore planning to 
> > commit the correct part (see patch below), except if someone still have
> > some comments on it.
> > 
> > 
> > diff --git a/hw/e1000.c b/hw/e1000.c
> > index 1c77afc..e3dbef9 100644
> > --- a/hw/e1000.c
> > +++ b/hw/e1000.c
> > @@ -721,7 +721,7 @@ e1000_mmio_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
> >      unsigned int index = ((addr - s->mmio_base) & 0x1ffff) >> 2;
> > 
> >      if (index < NWRITEOPS && macreg_writeops[index])
> > -        macreg_writeops[index](s, index, le32_to_cpu(val));
> > +        macreg_writeops[index](s, index, val);
> >      else if (index < NREADOPS && macreg_readops[index])
> >          DBGOUT(MMIO, "e1000_mmio_writel RO %x: 0x%04x\n", index<<2, val);
> >      else
> 
> This cannot work. PowerPC targets have MMIO instructions that can
> produce "val" in either BE or LE format. Given that e1000 is a PCI
> device, val in this case is LE, and will need swapping on a BE host.

We are speaking about the endianness of the host here, and not the
target one. Access to the PCI bus is done by passing a value. This value
is stored in memory or a CPU register of the host. It is therefore
interpreted with the same endianness as the host.

When it goes through the the busses and/or bridges, it can be mangled
or swapped. But when it comes to the device, it is still a value in the
host endianness. The same way as for the CPU, all the device code work
with values represented in the host endianness, so at the end there is
no conversion to do *depending on the host endianness*.

The fact that PowerPC can do either BE or LE access has to be handled 
within the CPU emulation, not with chipset and/or devices.

Let's try another argument to convince you: Why don't you also swap the
addresses as PCI is LE? This is not done either in your patch or in the
current (wrong) e1000 emulation.

> The only reason this code might work for MIPS is due to a *lack* of
> endianness handling in the gt64xxx.c bridge emulation, but that is a
> bug, not a feature. :)
> 

Yes, there is clearly a lack of endianness handling in the gt64xxx.c
bridge emulation, but that only concerns support for the endianness of
the target. The support for the host endianness already works correctly,
for most devices (except e1000 emulation), not only for the MIPS target, 
but also the PPC, i386 ones.

BTW, if you really want to play with endianness and using Linux as the
target, I really suggests you to try on the e1000 emulation instead of
the rtl8139 one. The rtl8139 driver in Linux seems to work (except
concerning the traffic) even if the endianness is not correct. The e1000
driver will fail very early while trying to read the MAC address, so
even if interrupt routing for example is not implemented, you will be
able to detect if the endianness is correctly implemented or not.

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

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

[Aurelien Jarno]

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

On Wed, Mar 12, 2008 at 03:38:55PM +0100, Aurelien Jarno wrote:
> On Wed, Mar 12, 2008 at 08:52:00AM -0500, Hollis Blanchard wrote:
> > On Tue, 2008-03-11 at 23:49 +0100, Aurelien Jarno wrote:
> > > On Sat, Mar 08, 2008 at 11:08:48AM -0600, Hollis Blanchard wrote:
> > > > On Sat, 2008-03-08 at 14:59 +0100, Aurelien Jarno wrote: 
> > > > > On Fri, Mar 07, 2008 at 11:23:51AM -0600, Hollis Blanchard wrote:
> > > > > > 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
> > > 
> > > [snip]
> > > 
> > > > > > @@ -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));
> > > > > >  }
> > > > > >  
> > > > > 
> > > > > Are those changes really necessary? The rtl8139 emulation works
> > > > > correctly on a big endian host (tested with an i386 target). This part
> > > > > of the patch would probably break it. Unless the Linux driver only does
> > > > > byte accesses, which are not changed by this patch.
> > > 
> > > I have tested this part of the patch on a big endian host (PowerPC), and
> > > I confirm it breaks the rtl8139 emulation (tested on i386 and mipsel
> > > targets).
> > 
> > OK, I will do some regression testing too before I submit a patch for
> > inclusion.
> > 
> > > > Hmm, yes, there is a problem with this patch. In the "touching many
> > > > 1-byte registers as a single 4-byte access" change to rtl8139_io_readl()
> > > > above, we made sure that the result was LE. What we should have done is
> > > > make it host-endianness like all the other return values from
> > > > rtl8139_io_readl(). Then in rtl8139_mmio_readl(), we know we must swap
> > > > host->LE.
> > > > 
> > > > I don't know why rtl8139 would work today with LE/BE target/host, but if
> > > > it does, it must be due to swapping in another layer, because this patch
> > > > is the right thing to do here. If qemu currently swaps 5 times and that
> > > > comes out the same as swapping once, we still need to fix it... :)
> > > 
> > > I hope the discussion on IRC last week-end convinced you that you don't
> > > need to swap the value depending on the host endianness.
> > 
> > Absolutely not! At least, not with the current qemu design.
> > 
> > > For those who
> > > haven't followed the discussion, qemu does memory accesses with a couple
> > > (address, value). The addresses and values are always stored in host 
> > > endianness, and this does not need to be swapped depending on the host. 
> > > It may have to be swapped depending on the target, if the value is 
> > > swapped on the real hardware (bus connected backward, chipset, etc.)
> > > 
> > > Thanks to Paul Brook for all the explanations.
> > 
> > The scheme Paul outlined (but hasn't actually proposed as far as I've
> > seen) involved an end-to-end overhaul of endianness manipulations in
> > qemu, removing byte swapping from some device emulation and adding some
> > to bus layers. I still don't understand his entire idea, and I think
> > some prototype code will be needed to see it. Until/unless that is
> > implemented, we will continue to require swapping in the device
> > emulation.
> 
> That was to support the target endianness. The support for hosts of
> different endianness is already correctly implemented in the current
> design, that is qemu works with host endianness.
> 
> > > Basically that means that one part of my e1000 part is actually correct,
> > > while the other is correct in the case of the MIPS Malta machine, but
> > > may be wrong for other targets/machines. I am therefore planning to 
> > > commit the correct part (see patch below), except if someone still have
> > > some comments on it.
> > > 
> > > 
> > > diff --git a/hw/e1000.c b/hw/e1000.c
> > > index 1c77afc..e3dbef9 100644
> > > --- a/hw/e1000.c
> > > +++ b/hw/e1000.c
> > > @@ -721,7 +721,7 @@ e1000_mmio_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
> > >      unsigned int index = ((addr - s->mmio_base) & 0x1ffff) >> 2;
> > > 
> > >      if (index < NWRITEOPS && macreg_writeops[index])
> > > -        macreg_writeops[index](s, index, le32_to_cpu(val));
> > > +        macreg_writeops[index](s, index, val);
> > >      else if (index < NREADOPS && macreg_readops[index])
> > >          DBGOUT(MMIO, "e1000_mmio_writel RO %x: 0x%04x\n", index<<2, val);
> > >      else
> > 
> > This cannot work. PowerPC targets have MMIO instructions that can
> > produce "val" in either BE or LE format. Given that e1000 is a PCI
> > device, val in this case is LE, and will need swapping on a BE host.
> 
> We are speaking about the endianness of the host here, and not the
> target one. Access to the PCI bus is done by passing a value. This value
> is stored in memory or a CPU register of the host. It is therefore
> interpreted with the same endianness as the host.
> 
> When it goes through the the busses and/or bridges, it can be mangled
> or swapped. But when it comes to the device, it is still a value in the
> host endianness. The same way as for the CPU, all the device code work
> with values represented in the host endianness, so at the end there is
> no conversion to do *depending on the host endianness*.
> 
> The fact that PowerPC can do either BE or LE access has to be handled 
> within the CPU emulation, not with chipset and/or devices.
> 
> Let's try another argument to convince you: Why don't you also swap the
> addresses as PCI is LE? This is not done either in your patch or in the
> current (wrong) e1000 emulation.
> 
> > The only reason this code might work for MIPS is due to a *lack* of
> > endianness handling in the gt64xxx.c bridge emulation, but that is a
> > bug, not a feature. :)
> > 
> 
> Yes, there is clearly a lack of endianness handling in the gt64xxx.c
> bridge emulation, but that only concerns support for the endianness of
> the target. The support for the host endianness already works correctly,
> for most devices (except e1000 emulation), not only for the MIPS target, 
> but also the PPC, i386 ones.
> 
> BTW, if you really want to play with endianness and using Linux as the
> target, I really suggests you to try on the e1000 emulation instead of
> the rtl8139 one. The rtl8139 driver in Linux seems to work (except
> concerning the traffic) even if the endianness is not correct. The e1000
> driver will fail very early while trying to read the MAC address, so
> even if interrupt routing for example is not implemented, you will be
> able to detect if the endianness is correctly implemented or not.
> 

We finally found the problem with rtl8139 emulation. The byteswapping
depending on the target was not done for all registers. The 8139too
driver was able to cope with that, but not the 8139cp. In his patch
Hollis addressed the issue by byteswapping accesses to all registers,
but depending the host endianness (cpu_to_ and _to_cpu functions) 
instead of the target endianness. This was working correctly in his 
case, as he tested it on both big endian target and hosts.

Byte swapping does not depend on host endianness nor target endianness
but on the path from the CPU to the device, which is currently and 
*wrongly* implemented in Qemu as a byteswap on big endian targets. This
has to be fixed by providing a layer modeling the endianness. It has to
maintain a bus network/tree, so it can invert the endianness of all the
devices behind a PCI host controller or a PCI bridge. It also has to be
dynamic, as some chipsets (e.g. gt64xxx) have a configurable bit that 
can be changed at runtime to control the endianness.

Before it is implemented correctly, please find attached two patches to
fix the rtl8139 and e1000 emulations on big endian hosts and big endian
targets, using the current and *wrong* Qemu implementation with regard
to endianness. Hopefully that works as all the systems we support happen
to do the same thing.

Any comments?

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

[-- Attachment #2: 0001-rtl8139-fix-endianness-on-big-endian-targets.patch --]
[-- Type: text/x-diff, Size: 4643 bytes --]

>From b434843958dcad6c7edbcaebc5766e916db46ac4 Mon Sep 17 00:00:00 2001
From: Aurelien Jarno <aurelien@aurel32.net>
Date: Wed, 12 Mar 2008 21:38:50 +0100
Subject: [PATCH] rtl8139: fix endianness on big endian targets

On big endian targets with mmio accesses, the values are not always
swapped, depending on the accessed register. The Linux 8139too module
was able to cope with that, but not the 8139cp one.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 hw/rtl8139.c |   42 ++++++++++++++++--------------------------
 1 files changed, 16 insertions(+), 26 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 2c6a698..a7ab704 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -2735,13 +2735,8 @@ static void rtl8139_io_writew(void *opaque, uint8_t addr, uint32_t val)
         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 *opaque, uint8_t addr, uint32_t val)
 
         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 *opaque, uint8_t addr)
         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 *opaque, uint8_t addr)
         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,11 +3067,17 @@ static void rtl8139_mmio_writeb(void *opaque, target_phys_addr_t addr, uint32_t
 
 static void rtl8139_mmio_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
+#ifdef TARGET_WORDS_BIGENDIAN
+    val = bswap16(val);
+#endif
     rtl8139_io_writew(opaque, addr & 0xFF, val);
 }
 
 static void rtl8139_mmio_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
+#ifdef TARGET_WORDS_BIGENDIAN
+    val = bswap32(val);
+#endif
     rtl8139_io_writel(opaque, addr & 0xFF, val);
 }
 
@@ -3106,12 +3088,20 @@ static uint32_t rtl8139_mmio_readb(void *opaque, target_phys_addr_t addr)
 
 static uint32_t rtl8139_mmio_readw(void *opaque, target_phys_addr_t addr)
 {
-    return rtl8139_io_readw(opaque, addr & 0xFF);
+    uint32_t val = rtl8139_io_readw(opaque, addr & 0xFF);
+#ifdef TARGET_WORDS_BIGENDIAN
+    val = bswap16(val);
+#endif
+    return val;
 }
 
 static uint32_t rtl8139_mmio_readl(void *opaque, target_phys_addr_t addr)
 {
-    return rtl8139_io_readl(opaque, addr & 0xFF);
+    uint32_t val = rtl8139_io_readl(opaque, addr & 0xFF);
+#ifdef TARGET_WORDS_BIGENDIAN
+    val = bswap32(val);
+#endif
+    return val;
 }
 
 /* */
-- 
1.5.4.3


[-- Attachment #3: 0002-e1000-fix-endianness-issues.patch --]
[-- Type: text/x-diff, Size: 3571 bytes --]

>From f0f37cac74b87a55cf78cda2dd25de87915a186f Mon Sep 17 00:00:00 2001
From: Aurelien Jarno <aurelien@aurel32.net>
Date: Wed, 12 Mar 2008 22:25:32 +0100
Subject: [PATCH] e1000: fix endianness issues

This patch fixes endianness issues in the e1000 nic emulation, which
currently only works on little endian hosts with little endian targets.

Byte swapping does not depend on host endianness, so this patch remove
the use of cpu_to_le32 and le32_to_cpu functions. It depends on the path
from the CPU to the device, which is currently and *wrongly* implemented
in Qemu as a byteswap on big endian targets. This patch does the same
as in other devices emulation as all the currently implemented targets
work with this implementation.

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 1c77afc..6e22ef9 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
     if (index < NWRITEOPS && macreg_writeops[index])
-        macreg_writeops[index](s, index, le32_to_cpu(val));
+        macreg_writeops[index](s, index, val);
     else if (index < NREADOPS && macreg_readops[index])
         DBGOUT(MMIO, "e1000_mmio_writel RO %x: 0x%04x\n", index<<2, val);
     else
@@ -734,7 +737,7 @@ e1000_mmio_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
     // emulate hw without byte enables: no RMW
     e1000_mmio_writel(opaque, addr & ~3,
-                      cpu_to_le32(le16_to_cpu(val & 0xffff) << (8*(addr & 3))));
+                      (val & 0xffff) << (8*(addr & 3)));
 }
 
 static void
@@ -742,7 +745,7 @@ e1000_mmio_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
     // emulate hw without byte enables: no RMW
     e1000_mmio_writel(opaque, addr & ~3,
-                      cpu_to_le32((val & 0xff)  << (8*(addr & 3))));
+                      (val & 0xff) << (8*(addr & 3)));
 }
 
 static uint32_t
@@ -752,7 +755,13 @@ e1000_mmio_readl(void *opaque, target_phys_addr_t addr)
     unsigned int index = ((addr - s->mmio_base) & 0x1ffff) >> 2;
 
     if (index < NREADOPS && macreg_readops[index])
-        return cpu_to_le32(macreg_readops[index](s, index));
+    {
+        uint32_t val = macreg_readops[index](s, index);
+#ifdef TARGET_WORDS_BIGENDIAN
+        val = bswap32(val);
+#endif
+        return val;
+    }
     DBGOUT(UNKNOWN, "MMIO unknown read addr=0x%08x\n", index<<2);
     return 0;
 }
@@ -760,15 +769,15 @@ e1000_mmio_readl(void *opaque, target_phys_addr_t addr)
 static uint32_t
 e1000_mmio_readb(void *opaque, target_phys_addr_t addr)
 {
-    return (le32_to_cpu(e1000_mmio_readl(opaque, addr & ~3)) >>
+    return ((e1000_mmio_readl(opaque, addr & ~3)) >>
             (8 * (addr & 3))) & 0xff;
 }
 
 static uint32_t
 e1000_mmio_readw(void *opaque, target_phys_addr_t addr)
 {
-    return cpu_to_le16((le32_to_cpu(e1000_mmio_readl(opaque, addr & ~3)) >>
-                        (8 * (addr & 3))) & 0xffff);
+    return ((e1000_mmio_readl(opaque, addr & ~3)) >>
+            (8 * (addr & 3))) & 0xffff;
 }
 
 int mac_regtosave[] = {
-- 
1.5.4.3

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

[Hollis Blanchard]

On Wed, 2008-03-12 at 23:07 +0100, Aurelien Jarno wrote:
> 
> We finally found the problem with rtl8139 emulation. The byteswapping
> depending on the target was not done for all registers. The 8139too
> driver was able to cope with that, but not the 8139cp. In his patch
> Hollis addressed the issue by byteswapping accesses to all registers,
> but depending the host endianness (cpu_to_ and _to_cpu functions) 
> instead of the target endianness. This was working correctly in his 
> case, as he tested it on both big endian target and hosts.
> 
> Byte swapping does not depend on host endianness nor target endianness
> but on the path from the CPU to the device, which is currently and 
> *wrongly* implemented in Qemu as a byteswap on big endian targets.
> This
> has to be fixed by providing a layer modeling the endianness. It has
> to
> maintain a bus network/tree, so it can invert the endianness of all
> the
> devices behind a PCI host controller or a PCI bridge. It also has to
> be
> dynamic, as some chipsets (e.g. gt64xxx) have a configurable bit that 
> can be changed at runtime to control the endianness.
> 
> Before it is implemented correctly, please find attached two patches
> to
> fix the rtl8139 and e1000 emulations on big endian hosts and big
> endian
> targets, using the current and *wrong* Qemu implementation with regard
> to endianness. Hopefully that works as all the systems we support
> happen
> to do the same thing.

Thanks for pursuing this Aurelien.

Although patches that replace cpu_to_* with TARGET_WORDS_BIGENDIAN usage
make me cringe, these patches do indeed seem to fix BE/BE host/target
behavior right now.

-- 
Hollis Blanchard
IBM Linux Technology Center