[v2] pci: Introduce helper to retrieve a PCI device's DMA address space

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(-)

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.  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?)

Paolo

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

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.

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?

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

Also, we would have to fix the x86 firmware.

Paolo

On Fri, 2013-08-09 at 11:40 +0200, Paolo Bonzini wrote:
> (BTW, do you need to enable bus-master DMA on PCI bridges, to do DMA
> for devices sitting on the secondary bus?)

In theory yes though I have seen bridges who ignore it...

Cheers,
Ben.

Il 09/08/2013 12:20, Benjamin Herrenschmidt ha scritto:
> On Fri, 2013-08-09 at 11:40 +0200, Paolo Bonzini wrote:
>> (BTW, do you need to enable bus-master DMA on PCI bridges, to do DMA
>> for devices sitting on the secondary bus?)
> 
> In theory yes though I have seen bridges who ignore it...

And we would need a compatibility property anyway.  So let's choose to
be among the ones who ignore it (i.e. the same as this patch).  But it
needs a comment.

Paolo

On Fri, Aug 09, 2013 at 07:48:16PM +1000, Alexey Kardashevskiy wrote:
> 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? :-/

It will be a PCI device, though the confusion is understandable.
There is a parent_dev in both PCIBus, which is always a PCI device if
non-NULL, as well as a parent_dev in BusState, which can be any sort
of device.  If both are non-NULL they will be equal apart from type.

So bus->parent_dev is safe here, bus->qbus.parent_dev would not be.