Re: [Qemu-devel] [PATCH] spapr-pci: change endianness for io ports space

From: Alexey Kardashevskiy <aik@ozlabs.ru>
To: Anthony Liguori <anthony@codemonkey.ws>
Cc: Alexander Graf <agraf@suse.de>,
	qemu-ppc@nongnu.org, qemu-devel@nongnu.org,
	David Gibson <david@gibson.dropbear.id.au>
Subject: Re: [Qemu-devel] [PATCH] spapr-pci: change endianness for io ports space
Date: Tue, 16 Jul 2013 10:39:23 +1000	[thread overview]
Message-ID: <51E4963B.3020201@ozlabs.ru> (raw)
In-Reply-To: <871u6ztkjr.fsf@codemonkey.ws>

On 07/16/2013 10:28 AM, Anthony Liguori wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> 
>> On 07/16/2013 01:02 AM, Anthony Liguori wrote:
>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>>
>>>> On 07/13/2013 06:03 PM, David Gibson wrote:
>>>>> On Fri, Jul 12, 2013 at 05:37:19PM +1000, Alexey Kardashevskiy wrote:
>>>>>> sPAPR PHB emulates IO ports on PCI via a special memory region which
>>>>>> routes all reads/writes further via cpu_in*/cpu_out* which are eventually
>>>>>> processed by MemoryRegionOps implemented by devices.
>>>>
>>>>> Hrm.  That double dispatch was a workaround for bugs in the plain
>>>>> memory region dispatching which meant we couldn't directly map regions
>>>>> in memory space to IO areas.
>>>>>
>>>>> It would be worth checking if that workaround is still necessary.
>>>>
>>>> Hm. Good point, thanks! It seems memory_region_init_io is not necessary any
>>>> more. Will make a patch for it.
>>>
>>> You should try the latest qemu.git commit.  There shouldn't be a problem
>>> anymore.
>>
>>
>> Does this mean sPAPR still needs an additional IO memory region? It looks
>> redundand and everything (almost) works without it...
> 
> There's more brokenness...
> 
> Some ISA devices mark themselves as "little endian" whereas others mark
> themselves as "native endian".
> 
> "little endian" really means "do byte lane swapping during dispatch" if
> host endian != target endian.
> 
> So on sPAPR, what you're getting is the redundant IO memory region
> causing a byte lane swap which is then negated by the ISA devices that
> mark themselves as little endian (such as VGA).
> 
> The right solution is to drop the additional IO region on sPAPR and
> remove the ISA devices marking themselves as "little endian".

No, this is not endianness, this is something different caused by a
difference in IO port registration (subpage? section? memoryregion? I am
going to draw a graph and try realize what is what here :) ).

Even very first IO port access does not reach VGA, fails somehow in
address_space_translate_internal() but devices other than VGA (well, at
least network devices) work perfectly.

> But that requires careful testing and fixing the other platforms that
> also are relying on the doube byte lane swapping.

-- 
Alexey