Am 16.07.2013 um 08:20 schrieb Paolo Bonzini <pbonz...@redhat.com>:
> Il 16/07/2013 07:19, Alexey Kardashevskiy ha scritto:
>> In the past, IO space could not be mapped into the memory address space
>> so we introduced a workaround for that. Nowadays it does not look
>> necessary so we can remove the workaround and make sPAPR PCI
>> configuration simplier.
>>
>> This also removes all byte swappings as it is not PHB's to take care
>> of endiannes - devices should do convertion if they want to. And almost
>> every PCI device which uses IO ports does that by registering IO ports
>> region with memory_region_init_io() (Intel e1000, RTL8139, virtio).
>>
>> However VGA uses MemoryRegionPortio which does not support endiannes
>> but it still expects the convertion to be done. For this case only,
>> this patch adds LITTLE_ENDIAN flag to portio_ops. Tests on PPC64 show
>> that other devices are not affected by this change. x86 systems should
>> not suffer either as having LITTLE_ENDIAN there has no effect.
>>
>> Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
>> ---
>>
>> This removes bugs at least from SPAPR so any further fixes should be equal
>> for all platforms and hopefully will break all platform altogether but not
>> just PPC64-pSeries :)
>>
>> Did I miss anything here?
>
> No, I don't think so. The patch looks good.
... and will break all Mac targets again, no? Not to speak of non-ppc devices.
Alex
>
> Paolo
>
>>
>> ---
>> hw/ppc/spapr_pci.c | 52 +++-------------------------------------------------
>> ioport.c | 1 +
>> 2 files changed, 4 insertions(+), 49 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index ca588aa..8d76290 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -440,43 +440,6 @@ static void pci_spapr_set_irq(void *opaque, int
>> irq_num, int level)
>> qemu_set_irq(spapr_phb_lsi_qirq(phb, irq_num), level);
>> }
>>
>> -static uint64_t spapr_io_read(void *opaque, hwaddr addr,
>> - unsigned size)
>> -{
>> - switch (size) {
>> - case 1:
>> - return cpu_inb(addr);
>> - case 2:
>> - return cpu_inw(addr);
>> - case 4:
>> - return cpu_inl(addr);
>> - }
>> - g_assert_not_reached();
>> -}
>> -
>> -static void spapr_io_write(void *opaque, hwaddr addr,
>> - uint64_t data, unsigned size)
>> -{
>> - switch (size) {
>> - case 1:
>> - cpu_outb(addr, data);
>> - return;
>> - case 2:
>> - cpu_outw(addr, data);
>> - return;
>> - case 4:
>> - cpu_outl(addr, data);
>> - return;
>> - }
>> - g_assert_not_reached();
>> -}
>> -
>> -static const MemoryRegionOps spapr_io_ops = {
>> - .endianness = DEVICE_LITTLE_ENDIAN,
>> - .read = spapr_io_read,
>> - .write = spapr_io_write
>> -};
>> -
>> /*
>> * MSI/MSIX memory region implementation.
>> * The handler handles both MSI and MSIX.
>> @@ -590,23 +553,14 @@ static int spapr_phb_init(SysBusDevice *s)
>> memory_region_add_subregion(get_system_memory(), sphb->mem_win_addr,
>> &sphb->memwindow);
>>
>> - /* On ppc, we only have MMIO no specific IO space from the CPU
>> - * perspective. In theory we ought to be able to embed the PCI IO
>> - * memory region direction in the system memory space. However,
>> - * if any of the IO BAR subregions use the old_portio mechanism,
>> - * that won't be processed properly unless accessed from the
>> - * system io address space. This hack to bounce things via
>> - * system_io works around the problem until all the users of
>> - * old_portion are updated */
>> + /* Initialize IO regions */
>> sprintf(namebuf, "%s.io", sphb->dtbusname);
>> memory_region_init(&sphb->iospace, OBJECT(sphb),
>> namebuf, SPAPR_PCI_IO_WIN_SIZE);
>> - /* FIXME: fix to support multiple PHBs */
>> - memory_region_add_subregion(get_system_io(), 0, &sphb->iospace);
>>
>> sprintf(namebuf, "%s.io-alias", sphb->dtbusname);
>> - memory_region_init_io(&sphb->iowindow, OBJECT(sphb), &spapr_io_ops,
>> sphb,
>> - namebuf, SPAPR_PCI_IO_WIN_SIZE);
>> + memory_region_init_alias(&sphb->iowindow, OBJECT(sphb),
>> + namebuf, &sphb->iospace, 0,
>> SPAPR_PCI_IO_WIN_SIZE);
>> memory_region_add_subregion(get_system_memory(), sphb->io_win_addr,
>> &sphb->iowindow);
>>
>> diff --git a/ioport.c b/ioport.c
>> index 89b17d6..79b7f1a 100644
>> --- a/ioport.c
>> +++ b/ioport.c
>> @@ -183,6 +183,7 @@ static void portio_write(void *opaque, hwaddr addr,
>> uint64_t data,
>> static const MemoryRegionOps portio_ops = {
>> .read = portio_read,
>> .write = portio_write,
>> + .endianness = DEVICE_LITTLE_ENDIAN,
>> .valid.unaligned = true,
>> .impl.unaligned = true,
>> };
>
