From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38974) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UyLzA-0007Rg-FX for qemu-devel@nongnu.org; Sun, 14 Jul 2013 09:05:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UyLz8-0004r6-Ki for qemu-devel@nongnu.org; Sun, 14 Jul 2013 09:05:08 -0400 Received: from mail-ie0-f173.google.com ([209.85.223.173]:61855) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UyLz8-0004pj-Gf for qemu-devel@nongnu.org; Sun, 14 Jul 2013 09:05:06 -0400 Received: by mail-ie0-f173.google.com with SMTP id k13so23543224iea.4 for ; Sun, 14 Jul 2013 06:05:05 -0700 (PDT) From: Anthony Liguori In-Reply-To: <51E24216.4050406@redhat.com> References: <51C75FA6.6080903@reactos.org> <51C7E21A.9090005@web.de> <8A36D64D-0625-49E1-9E59-391DAEEBD1FC@suse.de> <51DEA91B.40903@suse.de> <51E16683.1040304@redhat.com> <51E24216.4050406@redhat.com> Date: Sun, 14 Jul 2013 08:05:00 -0500 Message-ID: <874nbxqolf.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Liu Ping Fan , qemu-devel , Alexander Graf , =?utf-8?Q?Herv=C3=A9?= Poussineau , Jan Kiszka , Andreas =?utf-8?Q?F=C3=A4rber?= Paolo Bonzini writes: > Il 13/07/2013 17:22, Anthony Liguori ha scritto: >> 1) MMIO request goes to sPAPR PIO area, the vCPU was in BE mode but by >> the time the handler is called, the value is in host byte order. >> >> 2) sPAPR (incorrectly) byte swaps by marking the region as little >> endian (data is now garbage) >> >> 3) The portio layer (incorrectly) byte swaps because it is marked as >> little endian (data is now good) >> >> 4) Dispatch happens to VGA device which (incorrectly) byte swaps >> because it is marked as little endian (data is now bad) >> >> (2), (3), and (4) are all wrong. By removing either (2) or (3) we can >> "fix" the regression but that's just because two wrongs make a right >> in this situation. >> >> We should remove *all* of the LE markings from ISA devices, remove the >> portio mark, and the sPAPR mark. That's the right fix. > > So the bug here is that we have multiple levels of dispatch. Byte > swapping in the dispatch level only works as long as every dispatch is > merged, which is not the case. It's not clear to me if "endianness" makes sense as a concept in the memory API. If a bus wants to byte swap, it would have to redispatch anyway. > However, I do suspect that you have broken PREP again, because PREP has > 1/3/4 but not 2. "Broken" is a relative term... Not all ISA devices do (4)--see the various audio devices. Jan's original patch did code motion and added the LE flag to portio. My patch simply reverted the added logic to the code motion so if it broke PREP, PREP has been broken for quite some time. > Removing (2) IIUC amounts to re-applying commit > a178274efabcbbc5d44805b51def874e47051325, and I think that's a better > fix. It's a bigger change, but I think we should just remove MemoryRegionOps::endianness altogether. > Also, what devices exactly would have a non-native byte order?!? I'm > confused... MMIO/PIO requests don't have a byte order. It's literally 64 or 32 data pins that are numbered D0..D31 whereas D0 is the LSB. It doesn't matter how the pins are arranged. It's possible for busses to "byte lane swap" the data lines such that D0..D7 is rerouted to D23..D31, etc. in order to deal with poorly written drivers but as benh so colorfully put, this introduces more problems than it solves. Unfortunately, the existance of MemoryRegionOps::endianness makes this difficult to untangle because we have a careful combination of "two wrongs making a right". In all fairness, this problem predates the memory API. The memory API just carried forward the brokenness. Device originated DMA is a different story. This has to happen with a specific endianness but that is orthogonal to the memory API. Regards, Anthony Liguori > > Paolo