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

[Aurelien Jarno <aurelien@aurel32.net>]

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