[Qemu-devel] [PATCH 03/11] pci: Rework PCI iommu lifetime assumptions

[David Gibson]

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.

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;
-- 
1.7.10.4