On Thu, Oct 11, 2012 at 1:27 PM, Avi Kivity <a...@redhat.com> wrote: > Instead of requesting a DMAContext from the bus implementation, use a > MemoryRegion. This can be initialized using memory_region_init_iommu() > (or memory_region_init_alias() for simple, static translations). > > Add a destructor, since setups that have per-device translations will > need to return a different iommu region for every device. > > Signed-off-by: Avi Kivity <a...@redhat.com> > --- > hw/pci.c | 59 > +++++++++++++++++++++++++++++++++--------------------- > hw/pci.h | 7 +++++-- > hw/pci_internals.h | 5 +++-- > hw/spapr.h | 2 ++ > hw/spapr_iommu.c | 35 ++++++++++++++------------------ > hw/spapr_pci.c | 26 ++++++++++++++++++++---- > hw/spapr_pci.h | 1 + > 7 files changed, 84 insertions(+), 51 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index 7adf61b..02e1989 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -274,6 +274,21 @@ int pci_find_domain(const PCIBus *bus) > return -1; > } > > +static MemoryRegion *pci_default_iommu(PCIBus *bus, void *opaque, int devfn) > +{ > + MemoryRegion *mr = g_new(MemoryRegion, 1); > + > + /* FIXME: inherit memory region from bus creator */ > + memory_region_init_alias(mr, "iommu-nop", get_system_memory(), 0, > INT64_MAX); > + return mr; > +} > + > +static void pci_default_iommu_dtor(MemoryRegion *mr) > +{ > + memory_region_destroy(mr); > + g_free(mr); > +} > + > void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent, > const char *name, > MemoryRegion *address_space_mem, > @@ -285,6 +300,7 @@ void pci_bus_new_inplace(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, pci_default_iommu_dtor, NULL); > > /* host bridge */ > QLIST_INIT(&bus->child); > @@ -775,21 +791,16 @@ static PCIDevice *do_pci_register_device(PCIDevice > *pci_dev, PCIBus *bus, > PCI_SLOT(devfn), PCI_FUNC(devfn), name, > bus->devices[devfn]->name); > return NULL; > } > + > pci_dev->bus = bus; > - if (bus->dma_context_fn) { > - pci_dev->dma = bus->dma_context_fn(bus, bus->dma_context_opaque, > devfn); > - } else { > - /* FIXME: Make dma_context_fn use MemoryRegions instead, so this > path is > - * taken unconditionally */ > - /* FIXME: inherit memory region from bus creator */ > - memory_region_init_alias(&pci_dev->bus_master_enable_region, "bus > master", > - get_system_memory(), 0, > - memory_region_size(get_system_memory())); > - 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); > - pci_dev->dma = g_new(DMAContext, 1); > - dma_context_init(pci_dev->dma, &pci_dev->bus_master_as, NULL, NULL, > NULL); > - } > + pci_dev->iommu = 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)); > + 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); > + pci_dev->dma = g_new(DMAContext, 1); > + dma_context_init(pci_dev->dma, &pci_dev->bus_master_as, NULL, NULL, > NULL); > + > pci_dev->devfn = devfn; > pstrcpy(pci_dev->name, sizeof(pci_dev->name), name); > pci_dev->irq_state = 0; > @@ -843,12 +854,12 @@ static void do_pci_unregister_device(PCIDevice *pci_dev) > pci_dev->bus->devices[pci_dev->devfn] = NULL; > pci_config_free(pci_dev); > > - if (!pci_dev->bus->dma_context_fn) { > - address_space_destroy(&pci_dev->bus_master_as); > - memory_region_destroy(&pci_dev->bus_master_enable_region); > - g_free(pci_dev->dma); > - pci_dev->dma = NULL; > - } > + address_space_destroy(&pci_dev->bus_master_as); > + memory_region_del_subregion(&pci_dev->bus_master_enable_region, > pci_dev->iommu); > + pci_dev->bus->iommu_dtor_fn(pci_dev->iommu); > + memory_region_destroy(&pci_dev->bus_master_enable_region); > + g_free(pci_dev->dma); > + pci_dev->dma = NULL; > } > > static void pci_unregister_io_regions(PCIDevice *pci_dev) > @@ -2092,10 +2103,12 @@ static void pci_device_class_init(ObjectClass *klass, > void *data) > k->props = pci_props; > } > > -void pci_setup_iommu(PCIBus *bus, PCIDMAContextFunc fn, void *opaque) > +void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, PCIIOMMUDestructorFunc > dtor, > + void *opaque) > { > - bus->dma_context_fn = fn; > - bus->dma_context_opaque = opaque; > + bus->iommu_fn = fn; > + bus->iommu_dtor_fn = dtor; > + bus->iommu_opaque = opaque; > } > > static TypeInfo pci_device_type_info = { > diff --git a/hw/pci.h b/hw/pci.h > index a65e490..370354a 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -213,6 +213,7 @@ struct PCIDevice { > PCIIORegion io_regions[PCI_NUM_REGIONS]; > AddressSpace bus_master_as; > MemoryRegion bus_master_enable_region; > + MemoryRegion *iommu; > DMAContext *dma; > > /* do not access the following fields */ > @@ -351,9 +352,11 @@ int pci_read_devaddr(Monitor *mon, const char *addr, int > *domp, int *busp, > > void pci_device_deassert_intx(PCIDevice *dev); > > -typedef DMAContext *(*PCIDMAContextFunc)(PCIBus *, void *, int); > +typedef MemoryRegion *(*PCIIOMMUFunc)(PCIBus *, void *, int); > +typedef void (*PCIIOMMUDestructorFunc)(MemoryRegion *mr); > > -void pci_setup_iommu(PCIBus *bus, PCIDMAContextFunc fn, void *opaque); > +void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, PCIIOMMUDestructorFunc > dtor, > + void *opaque); > > static inline void > pci_set_byte(uint8_t *config, uint8_t val) > diff --git a/hw/pci_internals.h b/hw/pci_internals.h > index c931b64..040508f 100644 > --- a/hw/pci_internals.h > +++ b/hw/pci_internals.h > @@ -17,8 +17,9 @@ > > struct PCIBus { > BusState qbus; > - PCIDMAContextFunc dma_context_fn; > - void *dma_context_opaque; > + PCIIOMMUFunc iommu_fn; > + PCIIOMMUDestructorFunc iommu_dtor_fn; > + void *iommu_opaque;
Maybe the opaque could be avoided (in later patches) by clever use of container_of() by the functions to get the parent structure? > uint8_t devfn_min; > pci_set_irq_fn set_irq; > pci_map_irq_fn map_irq; > diff --git a/hw/spapr.h b/hw/spapr.h > index ac34a17..452efba1 100644 > --- a/hw/spapr.h > +++ b/hw/spapr.h > @@ -334,6 +334,8 @@ typedef struct sPAPRTCE { > #define SPAPR_PCI_BASE_LIOBN 0x80000000 > > void spapr_iommu_init(void); > +IOMMUTLBEntry spapr_tce_translate(DMAContext *dma, target_phys_addr_t addr, > + bool is_write); > DMAContext *spapr_tce_new_dma_context(uint32_t liobn, size_t window_size); > void spapr_tce_free(DMAContext *dma); > int spapr_dma_dt(void *fdt, int node_off, const char *propname, > diff --git a/hw/spapr_iommu.c b/hw/spapr_iommu.c > index 54798a3..79e35d1 100644 > --- a/hw/spapr_iommu.c > +++ b/hw/spapr_iommu.c > @@ -63,15 +63,11 @@ static sPAPRTCETable *spapr_tce_find_by_liobn(uint32_t > liobn) > return NULL; > } > > -static int spapr_tce_translate(DMAContext *dma, > - dma_addr_t addr, > - target_phys_addr_t *paddr, > - target_phys_addr_t *len, > - DMADirection dir) > +IOMMUTLBEntry spapr_tce_translate(DMAContext *dma, target_phys_addr_t addr, > + bool is_write) > { > sPAPRTCETable *tcet = DO_UPCAST(sPAPRTCETable, dma, dma); > - enum sPAPRTCEAccess access = (dir == DMA_DIRECTION_FROM_DEVICE) > - ? SPAPR_TCE_WO : SPAPR_TCE_RO; > + enum sPAPRTCEAccess access = is_write ? SPAPR_TCE_WO : SPAPR_TCE_RO; > uint64_t tce; > > #ifdef DEBUG_TCE > @@ -84,29 +80,27 @@ static int spapr_tce_translate(DMAContext *dma, > #ifdef DEBUG_TCE > fprintf(stderr, "spapr_tce_translate out of bounds\n"); > #endif > - return -EFAULT; > + return (IOMMUTLBEntry) { .valid = false }; > } > > tce = tcet->table[addr >> SPAPR_TCE_PAGE_SHIFT].tce; > > /* Check TCE */ > if (!(tce & access)) { > - return -EPERM; > + return (IOMMUTLBEntry) { .valid = false }; > } > > - /* How much til end of page ? */ > - *len = ((~addr) & SPAPR_TCE_PAGE_MASK) + 1; > - > - /* Translate */ > - *paddr = (tce & ~SPAPR_TCE_PAGE_MASK) | > - (addr & SPAPR_TCE_PAGE_MASK); > - > #ifdef DEBUG_TCE > fprintf(stderr, " -> *paddr=0x" TARGET_FMT_plx ", *len=0x" > - TARGET_FMT_plx "\n", *paddr, *len); > + TARGET_FMT_plx "\n", (tce & ~SPAPR_TCE_PAGE_MASK), > SPAPR_TCE_PAGE_MASK + 1); > #endif > > - return 0; > + return (IOMMUTLBEntry) { > + .device_addr = addr & SPAPR_TCE_PAGE_MASK, > + .translated_addr = (tce & ~SPAPR_TCE_PAGE_MASK), > + .addr_mask = SPAPR_TCE_PAGE_MASK, > + .valid = true, > + }; > } > > DMAContext *spapr_tce_new_dma_context(uint32_t liobn, size_t window_size) > @@ -118,7 +112,7 @@ DMAContext *spapr_tce_new_dma_context(uint32_t liobn, > size_t window_size) > } > > tcet = g_malloc0(sizeof(*tcet)); > - dma_context_init(&tcet->dma, &address_space_memory, spapr_tce_translate, > NULL, NULL); > + dma_context_init(&tcet->dma, &address_space_memory, NULL, NULL, NULL); > > tcet->liobn = liobn; > tcet->window_size = window_size; > @@ -253,7 +247,8 @@ int spapr_tcet_dma_dt(void *fdt, int node_off, const char > *propname, > return 0; > } > > - if (iommu->translate == spapr_tce_translate) { > + /* FIXME: WHAT?? */ > + if (true) { > sPAPRTCETable *tcet = DO_UPCAST(sPAPRTCETable, dma, iommu); > return spapr_dma_dt(fdt, node_off, propname, > tcet->liobn, 0, tcet->window_size); > diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c > index 661c05b..24f5b46 100644 > --- a/hw/spapr_pci.c > +++ b/hw/spapr_pci.c > @@ -506,14 +506,30 @@ static void spapr_msi_write(void *opaque, > target_phys_addr_t addr, > /* > * PHB PCI device > */ > -static DMAContext *spapr_pci_dma_context_fn(PCIBus *bus, void *opaque, > - int devfn) > +static MemoryRegion *spapr_pci_dma_iommu_new(PCIBus *bus, void *opaque, int > devfn) > { > sPAPRPHBState *phb = opaque; > > - return phb->dma; > + return &phb->iommu; > } > > +static void spapr_pci_dma_iommu_destroy(MemoryRegion *iommu) > +{ > + /* iommu is shared among devices, do nothing */ > +} > + > +static IOMMUTLBEntry spapr_phb_translate(MemoryRegion *iommu, > target_phys_addr_t addr, > + bool is_write) > +{ > + sPAPRPHBState *phb = container_of(iommu, sPAPRPHBState, iommu); > + > + return spapr_tce_translate(phb->dma, addr, is_write); > +} > + > +static MemoryRegionIOMMUOps spapr_iommu_ops = { > + .translate = spapr_phb_translate, > +}; > + > static int spapr_phb_init(SysBusDevice *s) > { > sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s); > @@ -576,7 +592,9 @@ static int spapr_phb_init(SysBusDevice *s) > sphb->dma_window_start = 0; > sphb->dma_window_size = 0x40000000; > sphb->dma = spapr_tce_new_dma_context(sphb->dma_liobn, > sphb->dma_window_size); > - pci_setup_iommu(bus, spapr_pci_dma_context_fn, sphb); > + memory_region_init_iommu(&sphb->iommu, &spapr_iommu_ops, > get_system_memory(), > + "iommu-spapr", INT64_MAX); > + pci_setup_iommu(bus, spapr_pci_dma_iommu_new, > spapr_pci_dma_iommu_destroy, sphb); > > QLIST_INSERT_HEAD(&spapr->phbs, sphb, list); > > diff --git a/hw/spapr_pci.h b/hw/spapr_pci.h > index 670dc62..171db45 100644 > --- a/hw/spapr_pci.h > +++ b/hw/spapr_pci.h > @@ -45,6 +45,7 @@ typedef struct sPAPRPHBState { > target_phys_addr_t mem_win_addr, mem_win_size, io_win_addr, io_win_size; > target_phys_addr_t msi_win_addr; > MemoryRegion memwindow, iowindow, msiwindow; > + MemoryRegion iommu; > > uint32_t dma_liobn; > uint64_t dma_window_start; > -- > 1.7.12 >