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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 08/09/2013 07:40 PM, Paolo Bonzini wrote:
> Il 09/08/2013 10:49, Alexey Kardashevskiy ha scritto:
>> 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.
>>
>> 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:
>> 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         | 22 ++++++++++++++++------
>>  include/hw/pci/pci.h |  1 +
>>  2 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 4c004f5..0072b54 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,21 @@ 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) {
>> +        return pci_device_iommu_address_space(bus->parent_dev);
>> +    }
> 
> No, this would fail if bus->parent_dev is not NULL but not a PCI device
> either.

parent_dev is of the PCIDevice* type, how can it be not a PCI device? :-/


> You can use object_dynamic_cast to convert the parent_dev to
> PCIDevice, and if the cast succeeds you call the new function.
> 
> Perhaps you could make the new function take a PCIBus instead.
> Accessing the PCIDevice's IOMMU address space (as opposed to the
> bus-master address space) doesn't make much sense, VFIO is really a
> special case.  Putting the new API on the bus side instead looks better.
> 
> (BTW, do you need to enable bus-master DMA on PCI bridges, to do DMA for
> devices sitting on the secondary bus?)


It happens naturally I guess when linux enables devices.


-- 
Alexey