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

[Alexander Graf]

On 11.07.2013, at 14:48, Alexander Graf wrote:

> 
> On 11.07.2013, at 14:46, Andreas Färber wrote:
> 
>> Am 11.07.2013 14:34, schrieb Alexander Graf:
>>> 
>>> On 11.07.2013, at 14:29, Alexander Graf wrote:
>>> 
>>>> 
>>>> On 24.06.2013, at 08:07, Jan Kiszka wrote:
>>>> 
>>>>> On 2013-06-23 22:50, Hervé Poussineau wrote:
>>>>>> Jan Kiszka a écrit :
>>>>>>> From: Jan Kiszka <address@hidden>
>>>>>>> 
>>>>>>> The current ioport dispatcher is a complex beast, mostly due to the
>>>>>>> need to deal with old portio interface users. But we can overcome it
>>>>>>> without converting all portio users by embedding the required base
>>>>>>> address of a MemoryRegionPortio access into that data structure. That
>>>>>>> removes the need to have the additional MemoryRegionIORange structure
>>>>>>> in the loop on every access.
>>>>>>> 
>>>>>>> To handle old portio memory ops, we simply install dispatching handlers
>>>>>>> for portio memory regions when registering them with the memory core.
>>>>>>> This removes the need for the old_portio field.
>>>>>>> 
>>>>>>> We can drop the additional aliasing of ioport regions and also the
>>>>>>> special address space listener. cpu_in and cpu_out now simply call
>>>>>>> address_space_read/write. And we can concentrate portio handling in a
>>>>>>> single source file.
>>>>>>> 
>>>>>>> Signed-off-by: Jan Kiszka <address@hidden>
>>>>>>> ---
>>>>>> 
>>>>>> ...
>>>>>> 
>>>>>>> +
>>>>>>> +static void portio_write(void *opaque, hwaddr addr, uint64_t data,
>>>>>>> +                         unsigned size)
>>>>>>> +{
>>>>>>> +    MemoryRegionPortioList *mrpio = opaque;
>>>>>>> +    const MemoryRegionPortio *mrp = find_portio(mrpio, addr, size,
>>>>>>> true);
>>>>>>> +
>>>>>>> +    if (mrp) {
>>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
>>>>>>> +    } else if (size == 2) {
>>>>>>> +        mrp = find_portio(mrpio, addr, 1, true);
>>>>>>> +        assert(mrp);
>>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 
>>>>>>> 0xff);
>>>>>>> +        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data
>>>>>>>>> 8);
>>>>>>> +    }
>>>>>>> +}
>>>>>>> +
>>>>>>> +static const MemoryRegionOps portio_ops = {
>>>>>>> +    .read = portio_read,
>>>>>>> +    .write = portio_write,
>>>>>>> +    .valid.unaligned = true,
>>>>>>> +    .impl.unaligned = true,
>>>>>>> +};
>>>>>>> +
>>>>>> 
>>>>>> You need to mark these operations as DEVICE_LITTLE_ENDIAN.
>>>>>> In portio_write above, you clearly assume that data is in LE format.
>>>>> 
>>>>> Anything behind PIO is little endian, of course. Will add this.
>>>> 
>>>> This patch breaks VGA on PPC as it is in master today.
>>> 
>>> If I don't mark portio as little endian it works as expected. There's 
>>> probably someone swapping things twice.
>> 
>> sPAPR has its MemoryRegion marked Little Endian:
> 
> sPAPR works, it's only the Mac machines that break.

So IIUC before cpu_inw and inl were doing portio accesses in host native 
endianness. Now with this change they do little endian accesses. All sensible 
callers of cpu_inX assume that data is passed in native endianness though and 
already do the little endian conversion themselves.

Semantically having your PCI host bridge do the endianness conversion is the 
correct way of handling it, as that's where the conversion happens. If it makes 
life easier to do it in the isa bridging code, that's fine for me too though. 
But then we'll have to get rid of all endianness swaps that already happen in 
PCI bridges.


Alex