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 <da...@gibson.dropbear.id.au> > --- > 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; >