Re: [Qemu-devel] [PATCH 3/8] pci: Rework PCI iommu lifetime assumptions

[Paolo Bonzini]

Il 13/05/2013 12:54, David Gibson ha scritto:
> The current bus callbacks to assign and destroy an iommu memory region for
> a PCI device tacitly assume that the lifetime of that address space is
> tied to the lifetime of the PCI device.  Although that's certainly
> possible, it's much more likely that the region will be (at least
> potentially) shared between several devices and have a lifetime instead
> tied to the PCI host bridge, or the IOMMU itself.  This is demonstrated in
> the fact that there are no existing users of the destructor hook.
> 
> This patch simplifies the code by moving to the more likely use model.
> This means we can eliminate the destructor hook entirely, and the iommu_fn
> hook is now assumed to inform us of the device's pre-existing DMA adddress
> space, rather than creating or assigning it.  That in turn means we have
> no need to keep a reference to the region around in the PCIDevice
> structure, which saves a little space.

Good idea, applying this too.

Paolo

> Signed-off-by: David Gibson <address@hidden>
> ---
>  hw/pci/pci.c             |   16 +++++-----------
>  hw/ppc/spapr_pci.c       |    2 +-
>  include/hw/pci/pci.h     |    5 +----
>  include/hw/pci/pci_bus.h |    1 -
>  4 files changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 58d3f69..3c947b3 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -285,10 +285,6 @@ static MemoryRegion *pci_default_iommu(PCIBus *bus, void 
> *opaque, int devfn)
>      return get_system_memory();
>  }
>  
> -static void pci_default_iommu_dtor(MemoryRegion *mr)
> -{
> -}
> -
>  static void pci_bus_init(PCIBus *bus, DeviceState *parent,
>                           const char *name,
>                           MemoryRegion *address_space_mem,
> @@ -299,7 +295,7 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent,
>      bus->devfn_min = devfn_min;
>      bus->address_space_mem = address_space_mem;
>      bus->address_space_io = address_space_io;
> -    pci_setup_iommu(bus, pci_default_iommu, NULL, NULL);
> +    pci_setup_iommu(bus, pci_default_iommu, NULL);
>  
>      /* host bridge */
>      QLIST_INIT(&bus->child);
> @@ -797,6 +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;
>  
>      if (devfn < 0) {
>          for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
> @@ -814,9 +811,9 @@ static PCIDevice *do_pci_register_device(PCIDevice 
> *pci_dev, PCIBus *bus,
>      }
>  
>      pci_dev->bus = bus;
> -    pci_dev->iommu = bus->iommu_fn(bus, bus->iommu_opaque, devfn);
> +    dma_mr = bus->iommu_fn(bus, bus->iommu_opaque, devfn);
>      memory_region_init_alias(&pci_dev->bus_master_enable_region, "bus 
> master",
> -                             pci_dev->iommu, 0, 
> memory_region_size(pci_dev->iommu));
> +                             dma_mr, 0, memory_region_size(dma_mr));
>      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);
> @@ -875,7 +872,6 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
>      pci_config_free(pci_dev);
>  
>      address_space_destroy(&pci_dev->bus_master_as);
> -    pci_dev->bus->iommu_dtor_fn(pci_dev->iommu);
>      memory_region_destroy(&pci_dev->bus_master_enable_region);
>  }
>  
> @@ -2237,11 +2233,9 @@ static void pci_device_class_init(ObjectClass *klass, 
> void *data)
>      k->props = pci_props;
>  }
>  
> -void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, PCIIOMMUDestructorFunc 
> dtor,
> -                     void *opaque)
> +void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque)
>  {
>      bus->iommu_fn = fn;
> -    bus->iommu_dtor_fn = dtor ? dtor : pci_default_iommu_dtor;
>      bus->iommu_opaque = opaque;
>  }
>  
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 7014b61..eb1d9e7 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -650,7 +650,7 @@ static int spapr_phb_init(SysBusDevice *s)
>          fprintf(stderr, "Unable to create TCE table for %s\n", 
> sphb->dtbusname);
>          return -1;
>      }
> -    pci_setup_iommu(bus, spapr_pci_dma_iommu, NULL, sphb);
> +    pci_setup_iommu(bus, spapr_pci_dma_iommu, sphb);
>  
>      QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
>  
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 400e772..61fe51e 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -242,7 +242,6 @@ struct PCIDevice {
>      PCIIORegion io_regions[PCI_NUM_REGIONS];
>      AddressSpace bus_master_as;
>      MemoryRegion bus_master_enable_region;
> -    MemoryRegion *iommu;
>  
>      /* do not access the following fields */
>      PCIConfigReadFunc *config_read;
> @@ -402,10 +401,8 @@ 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 void (*PCIIOMMUDestructorFunc)(MemoryRegion *mr);
>  
> -void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, PCIIOMMUDestructorFunc 
> dtor,
> -                     void *opaque);
> +void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque);
>  
>  static inline void
>  pci_set_byte(uint8_t *config, uint8_t val)
> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> index fada8f5..66762f6 100644
> --- a/include/hw/pci/pci_bus.h
> +++ b/include/hw/pci/pci_bus.h
> @@ -11,7 +11,6 @@
>  struct PCIBus {
>      BusState qbus;
>      PCIIOMMUFunc iommu_fn;
> -    PCIIOMMUDestructorFunc iommu_dtor_fn;
>      void *iommu_opaque;
>      uint8_t devfn_min;
>      pci_set_irq_fn set_irq;
>