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:
> > 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 <[EMAIL PROTECTED]> ---
> > > 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?
That is an interesting situation.
I don't know qemu internals enough, but from what I've seen this is not
currently possible.
If I were going to add such a thing, I would add an "address space"
argument to cpu_register_physical_memory(), and PCI devices would
specify the address space of the PCI controller. (Note: this would also
help solve qemu's "can't relocate PCI controller BARs" limitation.)
With this system, the PCI controller would register a callback for its
entire physical address space in the "root" IO table. In the gt64xxx
case, that callback would check the config bit and perform whatever
swapping is appropriate, then dispatch to the device callback via a
second IO table.
> > 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 <[EMAIL PROTECTED]>
> >
> > 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.
Yeah, fixing qemu's endianness handling is going to be a little tricky,
since you might need to fix all layers at once, rather than each
component individually.
> > @@ -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.
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... :)
--
Hollis Blanchard
IBM Linux Technology Center
