qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Aurelien Jarno
Subject: Re: [Qemu-devel] Re: [PATCH] e1000: fix endianness issues
Date: Wed, 12 Mar 2008 15:38:55 +0100
User-agent: Mutt/1.5.13 (2006-08-11)

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.

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]