Re: [Qemu-devel] [PATCH] spapr-pci: remove io ports workaround

[Alexander Graf]

Am 16.07.2013 um 08:20 schrieb Paolo Bonzini <address@hidden>:

> 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 <address@hidden>
>> ---
>> 
>> 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,
>> };
>