[Qemu-devel] [PATCH] pci: Fix pci_device_iommu_address_space() bus propagation

[Qemu-devel] [PATCH] pci: Fix pci_device_iommu_address_space() bus propagation

[Benjamin Herrenschmidt]

he current code walks up the bus tree for an iommu, however it passes
to the iommu_fn() callback the bus/devfn of the immediate child of
the level where the callback was found, rather than the original
bus/devfn where the search started from.

This prevents iommu's like POWER8 (and in fact also Q35) to properly
provide an address space for a subset of devices that aren't immediate
children of the iommu.

PCIe carries the originator bdfn acccross to the iommu on all DMA
transactions, so we must be able to properly identify devices at all
levels.

This changes the function pci_device_iommu_address_space() to pass
the original pointers to the iommu_fn() callback instead.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

With this, I can implement PHB3's (POWER8) iommu properly, I haven't
submitted the P8 native patch series yet but if you are curious, you
can look there:

https://github.com/ozbenh/qemu

And more specifically:

https://github.com/ozbenh/qemu/commit/67fe0460c75417908a2b54426cb54fe5a1299a13
 
For the PHB3 code.

 hw/pci/pci.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 29f0b0f..8185bbc 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2403,17 +2403,14 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
 AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
 {
     PCIBus *bus = PCI_BUS(dev->bus);
+    PCIBus *iommu_bus = bus;
 
-    if (bus->iommu_fn) {
-        return bus->iommu_fn(bus, bus->iommu_opaque, dev->devfn);
+    while(iommu_bus && !iommu_bus->iommu_fn && iommu_bus->parent_dev) {
+        iommu_bus = PCI_BUS(iommu_bus->parent_dev->bus);
     }
-
-    if (bus->parent_dev) {
-        /** We are ignoring the bus master DMA bit of the bridge
-         *  as it would complicate things such as VFIO for no good reason */
-        return pci_device_iommu_address_space(bus->parent_dev);
+    if (iommu_bus && iommu_bus->iommu_fn) {
+        return iommu_bus->iommu_fn(bus, iommu_bus->iommu_opaque, dev->devfn);
     }
-
     return &address_space_memory;
 }
 

Re: [Qemu-devel] [PATCH] pci: Fix pci_device_iommu_address_space() bus propagation

[Benjamin Herrenschmidt]

On Sun, 2015-07-05 at 09:19 +1000, Benjamin Herrenschmidt wrote:
> he current code walks up the bus tree for an iommu, however it passes
> to the iommu_fn() callback the bus/devfn of the immediate child of
> the level where the callback was found, rather than the original
> bus/devfn where the search started from.

Hi Michael ! Any comment on this ? I'd like to post my series for "bare
metal" power8 support and the iommu implementation relies on this to
work.

> This prevents iommu's like POWER8 (and in fact also Q35) to properly
> provide an address space for a subset of devices that aren't 
> immediate
> children of the iommu.
> 
> PCIe carries the originator bdfn acccross to the iommu on all DMA
> transactions, so we must be able to properly identify devices at all
> levels.
> 
> This changes the function pci_device_iommu_address_space() to pass
> the original pointers to the iommu_fn() callback instead.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> 
> With this, I can implement PHB3's (POWER8) iommu properly, I haven't
> submitted the P8 native patch series yet but if you are curious, you
> can look there:
> 
> https://github.com/ozbenh/qemu
> 
> And more specifically:
> 
> https://github.com/ozbenh/qemu/commit/67fe0460c75417908a2b54426cb54fe
> 5a1299a13
>  
> For the PHB3 code.
> 
>  hw/pci/pci.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 29f0b0f..8185bbc 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2403,17 +2403,14 @@ static void pci_device_class_init(ObjectClass 
> *klass, void *data)
>  AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>  {
>      PCIBus *bus = PCI_BUS(dev->bus);
> +    PCIBus *iommu_bus = bus;
>  
> -    if (bus->iommu_fn) {
> -        return bus->iommu_fn(bus, bus->iommu_opaque, dev->devfn);
> +    while(iommu_bus && !iommu_bus->iommu_fn && iommu_bus
> ->parent_dev) {
> +        iommu_bus = PCI_BUS(iommu_bus->parent_dev->bus);
>      }
> -
> -    if (bus->parent_dev) {
> -        /** We are ignoring the bus master DMA bit of the bridge
> -         *  as it would complicate things such as VFIO for no good 
> reason */
> -        return pci_device_iommu_address_space(bus->parent_dev);
> +    if (iommu_bus && iommu_bus->iommu_fn) {
> +        return iommu_bus->iommu_fn(bus, iommu_bus->iommu_opaque, dev
> ->devfn);
>      }
> -
>      return &address_space_memory;
>  }
>  
> 

Re: [Qemu-devel] [PATCH] pci: Fix pci_device_iommu_address_space() bus propagation

[Michael S. Tsirkin]

On Mon, Aug 17, 2015 at 07:39:14PM +1000, Benjamin Herrenschmidt wrote:
> On Sun, 2015-07-05 at 09:19 +1000, Benjamin Herrenschmidt wrote:
> > he current code walks up the bus tree for an iommu, however it passes
> > to the iommu_fn() callback the bus/devfn of the immediate child of
> > the level where the callback was found, rather than the original
> > bus/devfn where the search started from.
> 
> Hi Michael ! Any comment on this ? I'd like to post my series for "bare
> metal" power8 support and the iommu implementation relies on this to
> work.

I've picked this up for my next pull request.
Thanks, and sorry about the delay.

> > This prevents iommu's like POWER8 (and in fact also Q35) to properly
> > provide an address space for a subset of devices that aren't 
> > immediate
> > children of the iommu.
> > 
> > PCIe carries the originator bdfn acccross to the iommu on all DMA
> > transactions, so we must be able to properly identify devices at all
> > levels.
> > 
> > This changes the function pci_device_iommu_address_space() to pass
> > the original pointers to the iommu_fn() callback instead.
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> > 
> > With this, I can implement PHB3's (POWER8) iommu properly, I haven't
> > submitted the P8 native patch series yet but if you are curious, you
> > can look there:
> > 
> > https://github.com/ozbenh/qemu
> > 
> > And more specifically:
> > 
> > https://github.com/ozbenh/qemu/commit/67fe0460c75417908a2b54426cb54fe
> > 5a1299a13
> >  
> > For the PHB3 code.
> > 
> >  hw/pci/pci.c | 13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 29f0b0f..8185bbc 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -2403,17 +2403,14 @@ static void pci_device_class_init(ObjectClass 
> > *klass, void *data)
> >  AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
> >  {
> >      PCIBus *bus = PCI_BUS(dev->bus);
> > +    PCIBus *iommu_bus = bus;
> >  
> > -    if (bus->iommu_fn) {
> > -        return bus->iommu_fn(bus, bus->iommu_opaque, dev->devfn);
> > +    while(iommu_bus && !iommu_bus->iommu_fn && iommu_bus
> > ->parent_dev) {
> > +        iommu_bus = PCI_BUS(iommu_bus->parent_dev->bus);
> >      }
> > -
> > -    if (bus->parent_dev) {
> > -        /** We are ignoring the bus master DMA bit of the bridge
> > -         *  as it would complicate things such as VFIO for no good 
> > reason */
> > -        return pci_device_iommu_address_space(bus->parent_dev);
> > +    if (iommu_bus && iommu_bus->iommu_fn) {
> > +        return iommu_bus->iommu_fn(bus, iommu_bus->iommu_opaque, dev
> > ->devfn);
> >      }
> > -
> >      return &address_space_memory;
> >  }
> >  
> > 

Re: [Qemu-devel] [PATCH] pci: Fix pci_device_iommu_address_space() bus propagation

[Benjamin Herrenschmidt]

On Mon, 2015-08-17 at 21:50 +0300, Michael S. Tsirkin wrote:
> On Mon, Aug 17, 2015 at 07:39:14PM +1000, Benjamin Herrenschmidt 
> wrote:
> > On Sun, 2015-07-05 at 09:19 +1000, Benjamin Herrenschmidt wrote:
> > > he current code walks up the bus tree for an iommu, however it 
> > > passes
> > > to the iommu_fn() callback the bus/devfn of the immediate child 
> > > of
> > > the level where the callback was found, rather than the original
> > > bus/devfn where the search started from.
> > 
> > Hi Michael ! Any comment on this ? I'd like to post my series for 
> > "bare
> > metal" power8 support and the iommu implementation relies on this 
> > to
> > work.
> 
> I've picked this up for my next pull request.
> Thanks, and sorry about the delay.

No worries, I understand "busy" :-) Thanks for picking it up.

Cheers,
Ben.