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

[Michael S. Tsirkin]

On Tue, Aug 27, 2013 at 03:01:03PM +1000, Alexey Kardashevskiy wrote:
> 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 <address@hidden>
> >>>> [aik: added inheritance from parent if iommu is not set for the current 
> >>>> bus]
> >>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> >>>>
> >>>> ---
> >>>> 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!
> 

Nope, I did rebases and moved this out to the not-ready part
by mistake.
But it's still on my tree so it will get there in the next pull,
I plan it about next Sunday.


> 
> 
> -- 
> Alexey