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 <[EMAIL PROTECTED]> > > > > > > > > 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