From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34261) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VEBP5-0002BP-8s for qemu-devel@nongnu.org; Tue, 27 Aug 2013 01:01:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VEBP0-0005w1-9d for qemu-devel@nongnu.org; Tue, 27 Aug 2013 01:01:19 -0400 Received: from mail-pd0-f176.google.com ([209.85.192.176]:43979) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VEBOz-0005vl-VR for qemu-devel@nongnu.org; Tue, 27 Aug 2013 01:01:14 -0400 Received: by mail-pd0-f176.google.com with SMTP id q10so4315475pdj.7 for ; Mon, 26 Aug 2013 22:01:12 -0700 (PDT) Message-ID: <521C328F.4010001@ozlabs.ru> Date: Tue, 27 Aug 2013 15:01:03 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1376060948-28790-1-git-send-email-aik@ozlabs.ru> <20130811135847.GD15442@redhat.com> <5207A189.9030002@ozlabs.ru> <20130812090208.GA29884@redhat.com> In-Reply-To: <20130812090208.GA29884@redhat.com> Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 7bit 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: "Michael S. Tsirkin" Cc: Anthony Liguori , qemu-devel@nongnu.org, Alex Williamson , Paolo Bonzini , David Gibson On 08/12/2013 07:02 PM, Michael S. Tsirkin wrote: > 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! I could not see this patch in the pull request you send today. Something happened to the patch what I should fix? Thanks! -- Alexey