[PATCH 0/2] Fix ACS path checking

[PATCH 0/2] Fix ACS path checking

[Alex Williamson]

David Ahern reported an oops caused by passing a NULL start device
to pci_acs_path_enabled().  This can happen when a bus is created
for sr-iov devices without an associated bridge.  To handle this,
skip over to the parent bus and look for a bridge there.  Continue
until we find something or reach the root bus.

This works on my system, but still needs to be tested by David.  I'd
like to get this in for 3.6-rc2.

Joerg, I notice that ACS isn't getting enabled on my AMD system on
3.6-rc1.  I'll investigate more, but mention it in case you get to
it first.  Thanks,

Alex

---

Alex Williamson (2):
      amd-iommu: Fix ACS path checking
      intel-iommu: Fix ACS path checking


 drivers/iommu/amd_iommu.c   |   25 ++++++++++++++++++++++---
 drivers/iommu/intel-iommu.c |   25 ++++++++++++++++++++++---
 2 files changed, 44 insertions(+), 6 deletions(-)

[PATCH 1/2] intel-iommu: Fix ACS path checking

[Alex Williamson]

SR-IOV can create buses without a bridge.  There may be other cases
where this happens as well.  In these cases skip to the parent bus
and continue testing devices there.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 drivers/iommu/intel-iommu.c |   25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 7469b53..bae1357 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4124,8 +4124,13 @@ static int intel_iommu_add_device(struct device *dev)
 	} else
 		dma_pdev = pci_dev_get(pdev);
 
+	/* Account for quirked devices */
 	swap_pci_ref(&dma_pdev, pci_get_dma_source(dma_pdev));
 
+	/*
+	 * If it's a multifunction device that does not support our
+	 * required ACS flags, add to the same group as function 0.
+	 */
 	if (dma_pdev->multifunction &&
 	    !pci_acs_enabled(dma_pdev, REQ_ACS_FLAGS))
 		swap_pci_ref(&dma_pdev,
@@ -4133,14 +4138,28 @@ static int intel_iommu_add_device(struct device *dev)
 					  PCI_DEVFN(PCI_SLOT(dma_pdev->devfn),
 					  0)));
 
+	/*
+	 * Devices on the root bus go through the iommu.  If that's not us,
+	 * find the next upstream device and test ACS up to the root bus.
+	 * Finding the next device may require skipping virtual buses.
+	 */
 	while (!pci_is_root_bus(dma_pdev->bus)) {
-		if (pci_acs_path_enabled(dma_pdev->bus->self,
-					 NULL, REQ_ACS_FLAGS))
+		struct pci_bus *bus = dma_pdev->bus;
+
+		while (!bus->self) {
+			if (!pci_is_root_bus(bus))
+				bus = bus->parent;
+			else
+				goto root_bus;
+		}
+
+		if (pci_acs_path_enabled(bus->self, NULL, REQ_ACS_FLAGS))
 			break;
 
-		swap_pci_ref(&dma_pdev, pci_dev_get(dma_pdev->bus->self));
+		swap_pci_ref(&dma_pdev, pci_dev_get(bus->self));
 	}
 
+root_bus:
 	group = iommu_group_get(&dma_pdev->dev);
 	pci_dev_put(dma_pdev);
 	if (!group) {

Re: [PATCH 1/2] intel-iommu: Fix ACS path checking

[David Ahern]

On 8/4/12 12:08 PM, Alex Williamson wrote:
> SR-IOV can create buses without a bridge.  There may be other cases
> where this happens as well.  In these cases skip to the parent bus
> and continue testing devices there.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Tested-by: David Ahern <dsahern@gmail.com>

[PATCH 2/2] amd-iommu: Fix ACS path checking

[Alex Williamson]

SR-IOV can create buses without a bridge.  There may be other cases
where this happens as well.  In these cases skip to the parent bus
and continue testing devices there.
    
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 drivers/iommu/amd_iommu.c |   25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 6d1cbdf..b64502d 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -296,8 +296,13 @@ static int iommu_init_device(struct device *dev)
 	} else
 		dma_pdev = pci_dev_get(pdev);
 
+	/* Account for quirked devices */
 	swap_pci_ref(&dma_pdev, pci_get_dma_source(dma_pdev));
 
+	/*
+	 * If it's a multifunction device that does not support our
+	 * required ACS flags, add to the same group as function 0.
+	 */
 	if (dma_pdev->multifunction &&
 	    !pci_acs_enabled(dma_pdev, REQ_ACS_FLAGS))
 		swap_pci_ref(&dma_pdev,
@@ -305,14 +310,28 @@ static int iommu_init_device(struct device *dev)
 					  PCI_DEVFN(PCI_SLOT(dma_pdev->devfn),
 					  0)));
 
+	/*
+	 * Devices on the root bus go through the iommu.  If that's not us,
+	 * find the next upstream device and test ACS up to the root bus.
+	 * Finding the next device may require skipping virtual buses.
+	 */
 	while (!pci_is_root_bus(dma_pdev->bus)) {
-		if (pci_acs_path_enabled(dma_pdev->bus->self,
-					 NULL, REQ_ACS_FLAGS))
+		struct pci_bus *bus = dma_pdev->bus;
+
+		while (!bus->self) {
+			if (!pci_is_root_bus(bus))
+				bus = bus->parent;
+			else
+				goto root_bus;
+		}
+
+		if (pci_acs_path_enabled(bus->self, NULL, REQ_ACS_FLAGS))
 			break;
 
-		swap_pci_ref(&dma_pdev, pci_dev_get(dma_pdev->bus->self));
+		swap_pci_ref(&dma_pdev, pci_dev_get(bus->self));
 	}
 
+root_bus:
 	group = iommu_group_get(&dma_pdev->dev);
 	pci_dev_put(dma_pdev);
 	if (!group) {

Re: [PATCH 0/2] Fix ACS path checking

[Joerg Roedel]

On Sat, Aug 04, 2012 at 12:08:46PM -0600, Alex Williamson wrote:
> Joerg, I notice that ACS isn't getting enabled on my AMD system on
> 3.6-rc1.  I'll investigate more, but mention it in case you get to
> it first.

Hmm, tried it here, At least pci_request_acs() still gets called. How do
you detect if ACS is really enabled?

Regards,

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 0/2] Fix ACS path checking

[Joerg Roedel]

On Mon, Aug 06, 2012 at 12:50:10PM +0200, Joerg Roedel wrote:
> On Sat, Aug 04, 2012 at 12:08:46PM -0600, Alex Williamson wrote:
> Hmm, tried it here, At least pci_request_acs() still gets called. How do
> you detect if ACS is really enabled?

Okay, I found a problem. pci_request_acs needs to be called before PCI
probing. Does the attached patch help?

>From 87a4363be30d5d015a984a60769f29b0607fc5fb Mon Sep 17 00:00:00 2001
From: Joerg Roedel <joerg.roedel@amd.com>
Date: Mon, 6 Aug 2012 14:18:42 +0200
Subject: [PATCH] iommu/amd: Fix pci_request_acs() call-place

The pci_request_acs() function needs to be called before PCI
probing to be effective. So move it to another call-place to
ensure that.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 drivers/iommu/amd_iommu_init.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 500e7f1..0a2ea31 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1131,9 +1131,6 @@ static int __init amd_iommu_init_pci(void)
 			break;
 	}
 
-	/* Make sure ACS will be enabled */
-	pci_request_acs();
-
 	ret = amd_iommu_init_devices();
 
 	print_iommu_info();
@@ -1652,6 +1649,9 @@ static bool detect_ivrs(void)
 
 	early_acpi_os_unmap_memory((char __iomem *)ivrs_base, ivrs_size);
 
+	/* Make sure ACS will be enabled during PCI probe */
+	pci_request_acs();
+
 	return true;
 }
 
-- 
1.7.9.5

Re: [PATCH 0/2] Fix ACS path checking

[Alex Williamson]

On Mon, 2012-08-06 at 14:20 +0200, Joerg Roedel wrote:
> On Mon, Aug 06, 2012 at 12:50:10PM +0200, Joerg Roedel wrote:
> > On Sat, Aug 04, 2012 at 12:08:46PM -0600, Alex Williamson wrote:
> > Hmm, tried it here, At least pci_request_acs() still gets called. How do
> > you detect if ACS is really enabled?
> 
> Okay, I found a problem. pci_request_acs needs to be called before PCI
> probing. Does the attached patch help?
> 
> From 87a4363be30d5d015a984a60769f29b0607fc5fb Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <joerg.roedel@amd.com>
> Date: Mon, 6 Aug 2012 14:18:42 +0200
> Subject: [PATCH] iommu/amd: Fix pci_request_acs() call-place
> 
> The pci_request_acs() function needs to be called before PCI
> probing to be effective. So move it to another call-place to
> ensure that.
> 
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  drivers/iommu/amd_iommu_init.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)


Yes, that's it.  Before and after:

00:04.0 PCI bridge: Advanced Micro Devices [AMD] nee ATI RD890 PCI to PCI bridge (PCI express gpp port D) (prog-if 00 [Normal decode])
	Capabilities: [190] Access Control Services
		ACSCap:	SrcValid+ TransBlk+ ReqRedir+ CmpltRedir+ UpstreamFwd+ EgressCtrl- DirectTrans+
		ACSCtl:	SrcValid- TransBlk- ReqRedir- CmpltRedir- UpstreamFwd- EgressCtrl- DirectTrans-

	Capabilities: [190] Access Control Services
		ACSCap:	SrcValid+ TransBlk+ ReqRedir+ CmpltRedir+ UpstreamFwd+ EgressCtrl- DirectTrans+
		ACSCtl:	SrcValid+ TransBlk- ReqRedir+ CmpltRedir+ UpstreamFwd+ EgressCtrl- DirectTrans-

The former makes iommu grouping put all my VFs in a single group below
this device.  Correct given the ACS state, but not very useful.  Thanks,

Alex

Tested-by: Alex Williamson <alex.williamson@redhat.com>

> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index 500e7f1..0a2ea31 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -1131,9 +1131,6 @@ static int __init amd_iommu_init_pci(void)
>  			break;
>  	}
>  
> -	/* Make sure ACS will be enabled */
> -	pci_request_acs();
> -
>  	ret = amd_iommu_init_devices();
>  
>  	print_iommu_info();
> @@ -1652,6 +1649,9 @@ static bool detect_ivrs(void)
>  
>  	early_acpi_os_unmap_memory((char __iomem *)ivrs_base, ivrs_size);
>  
> +	/* Make sure ACS will be enabled during PCI probe */
> +	pci_request_acs();
> +
>  	return true;
>  }
>  

Re: [PATCH 0/2] Fix ACS path checking

[Joerg Roedel]

On Sat, Aug 04, 2012 at 12:08:46PM -0600, Alex Williamson wrote:
> Alex Williamson (2):
>       amd-iommu: Fix ACS path checking
>       intel-iommu: Fix ACS path checking

Applied these two patches together with my ACS fix to iommu/fixes,
thanks.

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632