Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer

From: Anthony Liguori <anthony@codemonkey.ws>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Liu Ping Fan" <pingfank@linux.vnet.ibm.com>,
	qemu-devel <qemu-devel@nongnu.org>,
	"Alexander Graf" <agraf@suse.de>,
	"Hervé Poussineau" <hpoussin@reactos.org>,
	"Jan Kiszka" <jan.kiszka@web.de>,
	"Andreas Färber" <afaerber@suse.de>
Subject: Re: [Qemu-devel] [PATCH v3 11/14] ioport: Switch dispatching to memory core layer
Date: Sun, 14 Jul 2013 08:05:00 -0500	[thread overview]
Message-ID: <874nbxqolf.fsf@codemonkey.ws> (raw)
In-Reply-To: <51E24216.4050406@redhat.com>

Paolo Bonzini <pbonzini@redhat.com> 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