From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1JZRP6-00043f-DX for qemu-devel@nongnu.org; Wed, 12 Mar 2008 09:54:00 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1JZRP2-00040y-W9 for qemu-devel@nongnu.org; Wed, 12 Mar 2008 09:53:59 -0400 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1JZRP2-00040t-Qf for qemu-devel@nongnu.org; Wed, 12 Mar 2008 09:53:56 -0400 Received: from e1.ny.us.ibm.com ([32.97.182.141]) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1JZRP2-0005li-EZ for qemu-devel@nongnu.org; Wed, 12 Mar 2008 09:53:56 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by e1.ny.us.ibm.com (8.13.8/8.13.8) with ESMTP id m2CDq2Tv029832 for ; Wed, 12 Mar 2008 09:52:02 -0400 Received: from d01av01.pok.ibm.com (d01av01.pok.ibm.com [9.56.224.215]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v8.7) with ESMTP id m2CDq3Qv264722 for ; Wed, 12 Mar 2008 09:52:03 -0400 Received: from d01av01.pok.ibm.com (loopback [127.0.0.1]) by d01av01.pok.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id m2CDq2Qi006520 for ; Wed, 12 Mar 2008 09:52:02 -0400 Subject: Re: [Qemu-devel] Re: [PATCH] e1000: fix endianness issues From: Hollis Blanchard In-Reply-To: <20080311224925.GA8753@volta.aurel32.net> References: <1204910632.2876.12.camel@basalt> <20080308135946.GA7387@hall.aurel32.net> <1204996128.5742.21.camel@basalt> <20080311224925.GA8753@volta.aurel32.net> Content-Type: text/plain Date: Wed, 12 Mar 2008 08:52:00 -0500 Message-Id: <1205329920.16914.20.camel@diesel> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aurelien Jarno Cc: kvm-ppc-devel , qemu-devel@nongnu.org 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 > > > > > > > > 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