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

[Aurelien Jarno <aurelien@aurel32.net>]

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