From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59847) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V8nze-0002cg-6G for qemu-devel@nongnu.org; Mon, 12 Aug 2013 05:01:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V8nzX-0003is-JR for qemu-devel@nongnu.org; Mon, 12 Aug 2013 05:00:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49351) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V8nzX-0003ib-6d for qemu-devel@nongnu.org; Mon, 12 Aug 2013 05:00:43 -0400 Date: Mon, 12 Aug 2013 12:02:08 +0300 From: "Michael S. Tsirkin" Message-ID: <20130812090208.GA29884@redhat.com> References: <1376060948-28790-1-git-send-email-aik@ozlabs.ru> <20130811135847.GD15442@redhat.com> <5207A189.9030002@ozlabs.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5207A189.9030002@ozlabs.ru> Subject: Re: [Qemu-devel] [PATCH v3] pci: Introduce helper to retrieve a PCI device's DMA address space List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Anthony Liguori , qemu-devel@nongnu.org, Alex Williamson , Paolo Bonzini , David Gibson On Mon, Aug 12, 2013 at 12:36:57AM +1000, Alexey Kardashevskiy wrote: > On 08/11/2013 11:58 PM, Michael S. Tsirkin wrote: > > On Sat, Aug 10, 2013 at 01:09:08AM +1000, Alexey Kardashevskiy wrote: > >> A PCI device's DMA address space (possibly an IOMMU) is returned by a > >> method on the PCIBus. At the moment that only has one caller, so the > >> method is simply open coded. We'll need another caller for VFIO, so > >> this patch introduces a helper/wrapper function. > >> > >> If IOMMU is not set, the pci_device_iommu_address_space() function > >> returns the parent's IOMMU skipping the "bus master" address space as > >> otherwise proper emulation would require more effort for no benefit. > >> > >> Signed-off-by: David Gibson > >> [aik: added inheritance from parent if iommu is not set for the current bus] > >> Signed-off-by: Alexey Kardashevskiy > >> > >> --- > >> Changes: > >> v3: > >> * added comment about ignoring bus master address space > >> > >> v2: > >> * added inheritance, needed for a pci-bridge on spapr-ppc64 > >> * pci_iommu_as renamed to pci_device_iommu_address_space > >> --- > >> hw/pci/pci.c | 24 ++++++++++++++++++------ > >> include/hw/pci/pci.h | 1 + > >> 2 files changed, 19 insertions(+), 6 deletions(-) > >> > >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c > >> index 4c004f5..dbfa395 100644 > >> --- a/hw/pci/pci.c > >> +++ b/hw/pci/pci.c > >> @@ -812,12 +812,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, > >> } > >> > >> pci_dev->bus = bus; > >> - if (bus->iommu_fn) { > >> - dma_as = bus->iommu_fn(bus, bus->iommu_opaque, devfn); > >> - } else { > >> - /* FIXME: inherit memory region from bus creator */ > >> - dma_as = &address_space_memory; > >> - } > >> + dma_as = pci_device_iommu_address_space(pci_dev); > >> > >> memory_region_init_alias(&pci_dev->bus_master_enable_region, > >> OBJECT(pci_dev), "bus master", > >> @@ -2239,6 +2234,23 @@ static void pci_device_class_init(ObjectClass *klass, void *data) > >> k->props = pci_props; > >> } > >> > >> +AddressSpace *pci_device_iommu_address_space(PCIDevice *dev) > >> +{ > >> + PCIBus *bus = PCI_BUS(dev->bus); > >> + > >> + if (bus->iommu_fn) { > >> + return bus->iommu_fn(bus, bus->iommu_opaque, dev->devfn); > >> + } > >> + > >> + if (bus->parent_dev) { > >> + /** We are ignoring the bus master DMA bit of the bridge > >> + * as it would complicate things such as VFIO for no good reason */ > > > > /* > > * Always > > * like > > * this > > */ > > > > /** Never > > * like this */ > > > Hm. I thought I saw a lot of those but it was the kernel :) > btw may comments start with "/**" (with no text in that line but still) - > what is the difference to "/*"? /** are normally for automated generation of docbook from code. I don't think we do that for QEMU, but in any case, this would be only any good for properly formatted comments at top of a function. > > > The comment should be improved I think. > > I would put it like this: > > /* > > * Note: this does not check bus master enable bit on device or > > * any of the pci to pci bridges above it, it's up to the caller to > > * check that before initiating the transaction. > > * > > * TODO: design a mechanism for callers to do this without > > * doing bus scans on data path. > > */ > > What exactly do you call here "bus scans"? Probably better as 'PCI hierarchy walk'. > > > Would you like me to queue this on the pci tree? If yes I can > > tweak the comment myself, no need to repost. > > Yes, please. Your tree is fine. Thanks! > OK, I'll apply this. Thanks! > >> + return pci_device_iommu_address_space(bus->parent_dev); > >> + } > >> + > >> + return &address_space_memory; > >> +} > >> + > >> void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque) > >> { > >> bus->iommu_fn = fn; > >> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > >> index ccec2ba..2374aa9 100644 > >> --- a/include/hw/pci/pci.h > >> +++ b/include/hw/pci/pci.h > >> @@ -405,6 +405,7 @@ void pci_device_deassert_intx(PCIDevice *dev); > >> > >> typedef AddressSpace *(*PCIIOMMUFunc)(PCIBus *, void *, int); > >> > >> +AddressSpace *pci_device_iommu_address_space(PCIDevice *dev); > >> void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque); > >> > >> static inline void > > > > -- > Alexey