[PATCH] Calgary: Find nearest matching Calgary while walking up the PCI tree

[PATCH] Calgary: Find nearest matching Calgary while walking up the PCI tree

[Darrick J. Wong]

On a multi-node x3950M2 system, there's a slight oddity in the PCI device tree
for all secondary nodes:

30:1e.0 PCI bridge: Intel Corporation 82801 PCI Bridge (rev e1)
 \-33:00.0 PCI bridge: IBM CalIOC2 PCI-E Root Port (rev 01)
    \-34:00.0 RAID bus controller: LSI Logic / Symbios Logic MegaRAID SAS 1078 (rev 04)

...as compared to the primary node:

00:1e.0 PCI bridge: Intel Corporation 82801 PCI Bridge (rev e1)
 \-01:00.0 VGA compatible controller: ATI Technologies Inc ES1000 (rev 02)
03:00.0 PCI bridge: IBM CalIOC2 PCI-E Root Port (rev 01)
 \-04:00.0 RAID bus controller: LSI Logic / Symbios Logic MegaRAID SAS 1078 (rev 04)

In both nodes, the LSI RAID controller hangs off a CalIOC2 device, but on the
secondary nodes, the BIOS hides the VGA device and substitutes the device tree
ending with the disk controller.

It would seem that Calgary devices don't necessarily appear at the top of the
PCI tree, which means that the current code to find the Calgary IOMMU that goes
with a particular device is buggy.  Rather than walk all the way to the top of
the PCI device tree and try to match bus number with Calgary descriptor, the
code needs to examine each parent of the particular device; if it encounters a
Calgary with a matching bus number, simply use that.  Otherwise, we BUG() when
the bus number of the Calgary doesn't match the bus number of whatever's at the
top of the device tree.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 arch/x86/kernel/pci-calgary_64.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)


diff --git a/arch/x86/kernel/pci-calgary_64.c b/arch/x86/kernel/pci-calgary_64.c
index 971a3be..e6ec8a2 100644
--- a/arch/x86/kernel/pci-calgary_64.c
+++ b/arch/x86/kernel/pci-calgary_64.c
@@ -318,13 +318,15 @@ static inline struct iommu_table *find_iommu_table(struct device *dev)
 
 	pdev = to_pci_dev(dev);
 
+	/* search up the device tree for an iommu */
 	pbus = pdev->bus;
-
-	/* is the device behind a bridge? Look for the root bus */
-	while (pbus->parent)
+	do {
+		tbl = pci_iommu(pbus);
+		if (tbl && tbl->it_busno == pbus->number)
+			break;
+		tbl = NULL;
 		pbus = pbus->parent;
-
-	tbl = pci_iommu(pbus);
+	} while (pbus);
 
 	BUG_ON(tbl && (tbl->it_busno != pbus->number));
 

[PATCH v2] Calgary: Find nearest matching Calgary while walking up the PCI tree

[Darrick J. Wong]

On a multi-node x3950M2 system, there's a slight oddity in the PCI device tree
for all secondary nodes:

30:1e.0 PCI bridge: Intel Corporation 82801 PCI Bridge (rev e1)
 \-33:00.0 PCI bridge: IBM CalIOC2 PCI-E Root Port (rev 01)
    \-34:00.0 RAID bus controller: LSI Logic / Symbios Logic MegaRAID SAS 1078 (rev 04)

...as compared to the primary node:

00:1e.0 PCI bridge: Intel Corporation 82801 PCI Bridge (rev e1)
 \-01:00.0 VGA compatible controller: ATI Technologies Inc ES1000 (rev 02)
03:00.0 PCI bridge: IBM CalIOC2 PCI-E Root Port (rev 01)
 \-04:00.0 RAID bus controller: LSI Logic / Symbios Logic MegaRAID SAS 1078 (rev 04)

In both nodes, the LSI RAID controller hangs off a CalIOC2 device, but on the
secondary nodes, the BIOS hides the VGA device and substitutes the device tree
ending with the disk controller.

It would seem that Calgary devices don't necessarily appear at the top of the
PCI tree, which means that the current code to find the Calgary IOMMU that goes
with a particular device is buggy.  Rather than walk all the way to the top of
the PCI device tree and try to match bus number with Calgary descriptor, the
code needs to examine each parent of the particular device; if it encounters a
Calgary with a matching bus number, simply use that.  Otherwise, we BUG() when
the bus number of the Calgary doesn't match the bus number of whatever's at the
top of the device tree.

Extra note: This patch appears to work correctly for the x3950 that came before
the x3950 M2.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
Acked-by: Muli Ben-Yehuda <muli@il.ibm.com>
---

 arch/x86/kernel/pci-calgary_64.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)


diff --git a/arch/x86/kernel/pci-calgary_64.c b/arch/x86/kernel/pci-calgary_64.c
index 971a3be..e6ec8a2 100644
--- a/arch/x86/kernel/pci-calgary_64.c
+++ b/arch/x86/kernel/pci-calgary_64.c
@@ -318,13 +318,15 @@ static inline struct iommu_table *find_iommu_table(struct device *dev)
 
 	pdev = to_pci_dev(dev);
 
+	/* search up the device tree for an iommu */
 	pbus = pdev->bus;
-
-	/* is the device behind a bridge? Look for the root bus */
-	while (pbus->parent)
+	do {
+		tbl = pci_iommu(pbus);
+		if (tbl && tbl->it_busno == pbus->number)
+			break;
+		tbl = NULL;
 		pbus = pbus->parent;
-
-	tbl = pci_iommu(pbus);
+	} while (pbus);
 
 	BUG_ON(tbl && (tbl->it_busno != pbus->number));
 

Re: [PATCH] Calgary: Find nearest matching Calgary while walking up the PCI tree

[Jon Mason]

On Tue, Dec 1, 2009 at 7:47 PM, Darrick J. Wong <djwong@us.ibm.com> wrote:
> On a multi-node x3950M2 system, there's a slight oddity in the PCI device tree
> for all secondary nodes:
>
> 30:1e.0 PCI bridge: Intel Corporation 82801 PCI Bridge (rev e1)
>  \-33:00.0 PCI bridge: IBM CalIOC2 PCI-E Root Port (rev 01)
>    \-34:00.0 RAID bus controller: LSI Logic / Symbios Logic MegaRAID SAS 1078 (rev 04)
>
> ...as compared to the primary node:
>
> 00:1e.0 PCI bridge: Intel Corporation 82801 PCI Bridge (rev e1)
>  \-01:00.0 VGA compatible controller: ATI Technologies Inc ES1000 (rev 02)
> 03:00.0 PCI bridge: IBM CalIOC2 PCI-E Root Port (rev 01)
>  \-04:00.0 RAID bus controller: LSI Logic / Symbios Logic MegaRAID SAS 1078 (rev 04)
>
> In both nodes, the LSI RAID controller hangs off a CalIOC2 device, but on the
> secondary nodes, the BIOS hides the VGA device and substitutes the device tree
> ending with the disk controller.
>
> It would seem that Calgary devices don't necessarily appear at the top of the
> PCI tree, which means that the current code to find the Calgary IOMMU that goes
> with a particular device is buggy.  Rather than walk all the way to the top of
> the PCI device tree and try to match bus number with Calgary descriptor, the
> code needs to examine each parent of the particular device; if it encounters a
> Calgary with a matching bus number, simply use that.  Otherwise, we BUG() when
> the bus number of the Calgary doesn't match the bus number of whatever's at the
> top of the device tree.
>
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> ---
>
>  arch/x86/kernel/pci-calgary_64.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
>
>
> diff --git a/arch/x86/kernel/pci-calgary_64.c b/arch/x86/kernel/pci-calgary_64.c
> index 971a3be..e6ec8a2 100644
> --- a/arch/x86/kernel/pci-calgary_64.c
> +++ b/arch/x86/kernel/pci-calgary_64.c
> @@ -318,13 +318,15 @@ static inline struct iommu_table *find_iommu_table(struct device *dev)
>
>        pdev = to_pci_dev(dev);
>
> +       /* search up the device tree for an iommu */
>        pbus = pdev->bus;
> -
> -       /* is the device behind a bridge? Look for the root bus */
> -       while (pbus->parent)
> +       do {
> +               tbl = pci_iommu(pbus);
> +               if (tbl && tbl->it_busno == pbus->number)
> +                       break;
> +               tbl = NULL;

I believe the NULL assignment is unnecessary.  If not, then the if
check and BUG_ON are busted.

>                pbus = pbus->parent;
> -
> -       tbl = pci_iommu(pbus);
> +       } while (pbus);
>
>        BUG_ON(tbl && (tbl->it_busno != pbus->number));
>
>

Re: [PATCH] Calgary: Find nearest matching Calgary while walking up the PCI tree

[Darrick J. Wong]

On Thu, Dec 03, 2009 at 08:49:48AM +0200, Muli Ben-Yehuda wrote:
> On Wed, Dec 02, 2009 at 05:51:19PM -0600, Jon Mason wrote:
> 
> > > diff --git a/arch/x86/kernel/pci-calgary_64.c b/arch/x86/kernel/pci-calgary_64.c
> > > index 971a3be..e6ec8a2 100644
> > > --- a/arch/x86/kernel/pci-calgary_64.c
> > > +++ b/arch/x86/kernel/pci-calgary_64.c
> > > @@ -318,13 +318,15 @@ static inline struct iommu_table *find_iommu_table(struct device *dev)
> > >
> > >        pdev = to_pci_dev(dev);
> > >
> > > +       /* search up the device tree for an iommu */
> > >        pbus = pdev->bus;
> > > -
> > > -       /* is the device behind a bridge? Look for the root bus */
> > > -       while (pbus->parent)
> > > +       do {
> > > +               tbl = pci_iommu(pbus);
> > > +               if (tbl && tbl->it_busno == pbus->number)
> > > +                       break;
> > > +               tbl = NULL;
> > 
> > I believe the NULL assignment is unnecessary.  If not, then the if
> > check and BUG_ON are busted.
> 
> I think the NULL assignment is needed for the case where the loop ends
> (pbus is NULL) and we did not find the right tbl. You don't want to
> leave the tbl pointing to the last tbl we saw while walking the bus.

Correct.  I suspect that you'd only hit this situation in a fairly pathological
corner case, but configuring the wrong Calgary for IO could (in theory) be used
to breach the Calgary protections, whereas failing to set up the necessary
permissions will simply crash the system.  In any case it seems cleaner to me to
throw away the tbl pointer once we've decided that we're done with it.

--D

[tip:core/iommu] x86, Calgary IOMMU quirk: Find nearest matching Calgary while walking up the PCI tree

[tip-bot for Darrick J. Wong]

Commit-ID:  4528752f49c1f4025473d12bc5fa9181085c3f22
Gitweb:     http://git.kernel.org/tip/4528752f49c1f4025473d12bc5fa9181085c3f22
Author:     Darrick J. Wong <djwong@us.ibm.com>
AuthorDate: Wed, 2 Dec 2009 15:05:56 -0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 3 Dec 2009 11:44:05 +0100

x86, Calgary IOMMU quirk: Find nearest matching Calgary while walking up the PCI tree

On a multi-node x3950M2 system, there's a slight oddity in the
PCI device tree for all secondary nodes:

 30:1e.0 PCI bridge: Intel Corporation 82801 PCI Bridge (rev e1)
  \-33:00.0 PCI bridge: IBM CalIOC2 PCI-E Root Port (rev 01)
     \-34:00.0 RAID bus controller: LSI Logic / Symbios Logic MegaRAID SAS 1078 (rev 04)

...as compared to the primary node:

 00:1e.0 PCI bridge: Intel Corporation 82801 PCI Bridge (rev e1)
  \-01:00.0 VGA compatible controller: ATI Technologies Inc ES1000 (rev 02)
 03:00.0 PCI bridge: IBM CalIOC2 PCI-E Root Port (rev 01)
  \-04:00.0 RAID bus controller: LSI Logic / Symbios Logic MegaRAID SAS 1078 (rev 04)

In both nodes, the LSI RAID controller hangs off a CalIOC2
device, but on the secondary nodes, the BIOS hides the VGA
device and substitutes the device tree ending with the disk
controller.

It would seem that Calgary devices don't necessarily appear at
the top of the PCI tree, which means that the current code to
find the Calgary IOMMU that goes with a particular device is
buggy.

Rather than walk all the way to the top of the PCI
device tree and try to match bus number with Calgary descriptor,
the code needs to examine each parent of the particular device;
if it encounters a Calgary with a matching bus number, simply
use that.

Otherwise, we BUG() when the bus number of the Calgary doesn't
match the bus number of whatever's at the top of the device tree.

Extra note: This patch appears to work correctly for the x3950
that came before the x3950 M2.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
Acked-by: Muli Ben-Yehuda <muli@il.ibm.com>
Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Joerg Roedel <joerg.roedel@amd.com>
Cc: Yinghai Lu <yhlu.kernel@gmail.com>
Cc: Jon D. Mason <jdmason@kudzu.us>
Cc: Corinna Schultz <coschult@us.ibm.com>
Cc: <stable@kernel.org>
LKML-Reference: <20091202230556.GG10295@tux1.beaverton.ibm.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/pci-calgary_64.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/pci-calgary_64.c b/arch/x86/kernel/pci-calgary_64.c
index 849a099..c563e4c 100644
--- a/arch/x86/kernel/pci-calgary_64.c
+++ b/arch/x86/kernel/pci-calgary_64.c
@@ -316,13 +316,15 @@ static inline struct iommu_table *find_iommu_table(struct device *dev)
 
 	pdev = to_pci_dev(dev);
 
+	/* search up the device tree for an iommu */
 	pbus = pdev->bus;
-
-	/* is the device behind a bridge? Look for the root bus */
-	while (pbus->parent)
+	do {
+		tbl = pci_iommu(pbus);
+		if (tbl && tbl->it_busno == pbus->number)
+			break;
+		tbl = NULL;
 		pbus = pbus->parent;
-
-	tbl = pci_iommu(pbus);
+	} while (pbus);
 
 	BUG_ON(tbl && (tbl->it_busno != pbus->number));