Re: [Qemu-devel] [PATCH v3] pci: Introduce helper to retrieve a PCI device's DMA address space

["Michael S. Tsirkin" <mst@redhat.com>]

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 <david@gibson.dropbear.id.au>
> >> [aik: added inheritance from parent if iommu is not set for the current bus]
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>
> >> ---
> >> 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