From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47780) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UbrgM-0000vJ-94 for qemu-devel@nongnu.org; Mon, 13 May 2013 08:16:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UbrgI-0002ke-VH for qemu-devel@nongnu.org; Mon, 13 May 2013 08:16:46 -0400 Received: from mail-gg0-x232.google.com ([2607:f8b0:4002:c02::232]:38647) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UbrgI-0002kY-Pi for qemu-devel@nongnu.org; Mon, 13 May 2013 08:16:42 -0400 Received: by mail-gg0-f178.google.com with SMTP id k3so481636ggn.37 for ; Mon, 13 May 2013 05:16:42 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <5190D9A5.5080003@redhat.com> Date: Mon, 13 May 2013 14:16:37 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1368442465-14363-1-git-send-email-david@gibson.dropbear.id.au> <1368442465-14363-5-git-send-email-david@gibson.dropbear.id.au> In-Reply-To: <1368442465-14363-5-git-send-email-david@gibson.dropbear.id.au> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/8] pci: Use AddressSpace rather than MemoryRegion to represent PCI DMA space List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: aik@ozlabs.ru, alex.williamson@redhat.com, mst@redhat.com, qemu-devel@nongnu.org, agraf@suse.de 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 > --- > 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); > >