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:53 PM, Paolo Bonzini wrote:
> Il 09/08/2013 11:48, Alexey Kardashevskiy ha scritto:
>> 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? :-/
> 
> Doh, I misread the code, I thought it was the "parent" field in
> BusState.  Why do we have parent_dev at all?

The code is too old? Don't know.


>>> 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.
> 
> Yes, but then using the IOMMU address space would be wrong; you would
> have to use the bus-master address space as a base for the child's
> bus-master address space.


Like this? Works too.

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 8bdcedc..a4c70e6 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2247,23 +2247,23 @@ 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);
     }

-    return &address_space_memory;
+    return &dev->bus_master_as;
 }



> Also, we would have to fix the x86 firmware.
> 
> Paolo
> 


-- 
Alexey