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

[Aurelien Jarno]

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 <address@hidden>
> > > > > > 
> > > > > > 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
 `. `'   address@hidden         | address@hidden
   `-    people.debian.org/~aurel32 | www.aurel32.net

0001-rtl8139-fix-endianness-on-big-endian-targets.patch
Description: Text Data

0002-e1000-fix-endianness-issues.patch
Description: Text Data