Re: [Qemu-devel] [PATCH 4/8] pci: Use AddressSpace rather than MemoryRegion to represent PCI DMA space

[Paolo Bonzini <pbonzini@redhat.com>]

Il 13/05/2013 12:54, David Gibson ha scritto:
> Currently the PCI iommu_fn hook returns a MemoryRegion * to represent the
> DMA address of this bus's IOMMU, although that MemoryRegion does have to
> be a root MemoryRegion.  Several upcoming users of this need the extra
> features of an AddressSpace object, rather than a MemoryRegion, and while
> they could each construct their own AddressSpace wrapper for the iommu
> MemoryRegion, that leads to unnecessary proliferation of essentially
> identical AddressSpace objects.  This patch avoids that, by instead having
> iommu_fn return an AddressSpace *, assuming the referenced AS's lifetime
> is managed somewhere else (probably the PCI host bridge).

Makes sense too.

Paolo

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/pci/pci.c                |   10 +++++-----
>  hw/ppc/spapr_pci.c          |    6 ++++--
>  include/hw/pci-host/spapr.h |    1 +
>  include/hw/pci/pci.h        |    2 +-
>  4 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 3c947b3..39085d8 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -279,10 +279,10 @@ int pci_find_domain(const PCIBus *bus)
>      return -1;
>  }
>  
> -static MemoryRegion *pci_default_iommu(PCIBus *bus, void *opaque, int devfn)
> +static AddressSpace *pci_default_iommu(PCIBus *bus, void *opaque, int devfn)
>  {
>      /* FIXME: inherit memory region from bus creator */
> -    return get_system_memory();
> +    return &address_space_memory;
>  }
>  
>  static void pci_bus_init(PCIBus *bus, DeviceState *parent,
> @@ -793,7 +793,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>      PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
>      PCIConfigReadFunc *config_read = pc->config_read;
>      PCIConfigWriteFunc *config_write = pc->config_write;
> -    MemoryRegion *dma_mr;
> +    AddressSpace *dma_as;
>  
>      if (devfn < 0) {
>          for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
> @@ -811,9 +811,9 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>      }
>  
>      pci_dev->bus = bus;
> -    dma_mr = bus->iommu_fn(bus, bus->iommu_opaque, devfn);
> +    dma_as = bus->iommu_fn(bus, bus->iommu_opaque, devfn);
>      memory_region_init_alias(&pci_dev->bus_master_enable_region, "bus master",
> -                             dma_mr, 0, memory_region_size(dma_mr));
> +                             dma_as->root, 0, memory_region_size(dma_as->root));
>      memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);
>      address_space_init(&pci_dev->bus_master_as, &pci_dev->bus_master_enable_region,
>                         name);
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index eb1d9e7..762db62 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -506,11 +506,11 @@ static const MemoryRegionOps spapr_msi_ops = {
>  /*
>   * PHB PCI device
>   */
> -static MemoryRegion *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
> +static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>  {
>      sPAPRPHBState *phb = opaque;
>  
> -    return spapr_tce_get_iommu(phb->tcet);
> +    return &phb->iommu_as;
>  }
>  
>  static int spapr_phb_init(SysBusDevice *s)
> @@ -650,6 +650,8 @@ static int spapr_phb_init(SysBusDevice *s)
>          fprintf(stderr, "Unable to create TCE table for %s\n", sphb->dtbusname);
>          return -1;
>      }
> +    address_space_init(&sphb->iommu_as, spapr_tce_get_iommu(sphb->tcet),
> +                       sphb->dtbusname);
>      pci_setup_iommu(bus, spapr_pci_dma_iommu, sphb);
>  
>      QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 653dd40..1e23dbf 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -50,6 +50,7 @@ typedef struct sPAPRPHBState {
>      uint64_t dma_window_start;
>      uint64_t dma_window_size;
>      sPAPRTCETable *tcet;
> +    AddressSpace iommu_as;
>  
>      struct {
>          uint32_t irq;
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 61fe51e..6ef1f97 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -400,7 +400,7 @@ int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp,
>  
>  void pci_device_deassert_intx(PCIDevice *dev);
>  
> -typedef MemoryRegion *(*PCIIOMMUFunc)(PCIBus *, void *, int);
> +typedef AddressSpace *(*PCIIOMMUFunc)(PCIBus *, void *, int);
>  
>  void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque);
>  
>