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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 09/08/2013 12:13, Alexey Kardashevskiy ha scritto:
> 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;
>  }

I was thinking more like this:

     if (bus->parent_dev) {
-        return pci_device_iommu_address_space(bus->parent_dev);
+        /* Take parent device's bus-master enable bit into account.  */
+        return pci_get_address_space(bus->parent_dev);
     }

+    /* Not a secondary bus and no IOMMU.  Use system memory.  */
     return &address_space_memory;

Paolo

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