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

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

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