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

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

[Alexey Kardashevskiy]

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);
+    }
+
+    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
-- 
1.8.3.2

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

[Paolo Bonzini]

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
> 

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

[Alexey Kardashevskiy]

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

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

[Paolo Bonzini]

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

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

[Alexey Kardashevskiy]

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

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

[Paolo Bonzini]

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

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

[Benjamin Herrenschmidt]

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.
 

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

[Paolo Bonzini]

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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2989 bytes --]

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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

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

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

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

[Paolo Bonzini]

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

Yes, fixing that check is another good use of the new API you introduced.

But after Ben's answer, I guess the above change is not really needed.
It would add complication for VFIO, too.  Proper emulation would require
QEMU to trap writes to the device's bus-master bit.  QEMU would have to
take of the value that the guest writes, AND it with the bus-master
bit(s) of all bridges between the host bridge and the VFIO device, and
write the result to the passed-through device.  This is because the
bridges are emulated, and do not exist in real hardware.

>>>> Also, we would have to fix the x86 firmware.
> 
> btw what would it mean? :)

Firmware would have to look for bridges and enable bus master DMA on
them.  Otherwise, for example a SCSI controller below a bridge (not
virtio-scsi) would fail to boot.

Paolo

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

[Alexey Kardashevskiy]

On 08/09/2013 09:07 PM, Paolo Bonzini wrote:
> Il 09/08/2013 12:58, Alexey Kardashevskiy ha scritto:
>> 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 <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;
>>
>> 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.
> 
> Yes, fixing that check is another good use of the new API you introduced.
> 
> But after Ben's answer, I guess the above change is not really needed.
> It would add complication for VFIO, too.  Proper emulation would require
> QEMU to trap writes to the device's bus-master bit.  QEMU would have to
> take of the value that the guest writes, AND it with the bus-master
> bit(s) of all bridges between the host bridge and the VFIO device, and
> write the result to the passed-through device.  This is because the
> bridges are emulated, and do not exist in real hardware.


So does this mean that we go with the original patch and ignore bus master
address space here?


I guess I could overcome that VFIO check by comparing not just AddressSpace
but AddressSpace->root if AddressSpace is different but it does not make a
lot of sense.

> 
>>>>> Also, we would have to fix the x86 firmware.
>>
>> btw what would it mean? :)
> 
> Firmware would have to look for bridges and enable bus master DMA on
> them.  Otherwise, for example a SCSI controller below a bridge (not
> virtio-scsi) would fail to boot.
> 
> Paolo
> 


-- 
Alexey

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

[Paolo Bonzini]

Il 09/08/2013 13:21, Alexey Kardashevskiy ha scritto:
> On 08/09/2013 09:07 PM, Paolo Bonzini wrote:
>> Il 09/08/2013 12:58, Alexey Kardashevskiy ha scritto:
>>> 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 <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;
>>>
>>> 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.
>>
>> Yes, fixing that check is another good use of the new API you introduced.
>>
>> But after Ben's answer, I guess the above change is not really needed.
>> It would add complication for VFIO, too.  Proper emulation would require
>> QEMU to trap writes to the device's bus-master bit.  QEMU would have to
>> take of the value that the guest writes, AND it with the bus-master
>> bit(s) of all bridges between the host bridge and the VFIO device, and
>> write the result to the passed-through device.  This is because the
>> bridges are emulated, and do not exist in real hardware.
> 
> So does this mean that we go with the original patch and ignore bus master
> address space here?

Yes.  Just add a comment that we are ignoring the bus master DMA bit of
the bridge.

> I guess I could overcome that VFIO check by comparing not just AddressSpace
> but AddressSpace->root if AddressSpace is different but it does not make a
> lot of sense.

Paolo