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

[Alexey Kardashevskiy]

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 "/*"?


> 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"?


> 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!


>> +        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