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

[Paolo Bonzini <pbonzini@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 <aik@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.

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,
>  };
>