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

[Alexey Kardashevskiy]

On 08/09/2013 08:19 PM, Paolo Bonzini wrote:
> 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 <address@hidden>
>>>>>> [aik: added inheritance from parent if iommu is not set for the current 
>>>>>> bus]
>>>>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>>>>>
>>>>>> ---
>>>>>> 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;

Ok.

Oh. BTW. This "bus master" thing now breaks VFIO's check for all devices
being in the same address space as every single device has its own "bus
master" AddressSpace.


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

btw what would it mean? :)


>>>
>>> Paolo
>>>
>>
>>
> 


-- 
Alexey