[PATCH v2 0/3] pci: ACS fixes & quirks

[PATCH v2 0/3] pci: ACS fixes & quirks

[Alex Williamson]

v2:

Revised patch 1/ to match comments from Bjorn.  PCIe event collectors
and PCIe-to-PCI bridges now indicate that they do not support ACS.
I've reached out to try to get clarification on this, but I think it's
reasonable to proceed with a conservative approach until then.  I also
added PCI-to-PCIe bridges for the sake of being complete.  Also added
more comments about the purpose and behavior of pci_acs_enabled().  If
I've overlooked anything else that needs to be addressed, please let
me know.

Patch 2/ had no comments, it's unchanged.

Patch 3/ is added.  This was sent as an RFC nearly a year ago and
Joerg confirmed for us that these devices do not support p2p on AMD
systems with AMD IOMMU.  We can't simply use iommu_present() to test
for an IOMMU because it's setup just after we need this function.  
Instead we test for the ACPI IVRS table that describes the IOMMU.  It
would probably suffice to skip an actual AMD IOMMU check, but I don't
want it to later come bite us if these ASICs get re-used, maybe with
a different IOMMU, and don't make the same guarantees.

Joerg, I was also curious back when we investigated this patch if the
same rules hold true for these other southbridge devices:

1002:43a0 SB700/SB800/SB900 PCI to PCI bridge (PCIE port 0)
1002:43a1 SB700/SB800/SB900 PCI to PCI bridge (PCIE port 1)
1002:43a2 SB900 PCI to PCI bridge (PCIE port 2)
1002:43a3 SB900 PCI to PCI bridge (PCIE port 3)

If you remember or have contacts to poke, I'd be happy to follow-up
with another patch to add them.  Thanks,

Alex

---

Alex Williamson (3):
      pci: Fix flaw in pci_acs_enabled()
      pci: Differentiate ACS controllable from enabled
      pci: ACS quirk for AMD southbridge


 drivers/pci/pci.c    |   93 +++++++++++++++++++++++++++++++++++++++++---------
 drivers/pci/quirks.c |   50 +++++++++++++++++++++++++++
 2 files changed, 127 insertions(+), 16 deletions(-)

[PATCH v2 1/3] pci: Fix flaw in pci_acs_enabled()

[Alex Williamson]

The multifunction ACS rules do not apply to downstream ports.  Those
should be tested regardless of whether they are single function or
multifunciton.  The PCIe spec also fully specifies which PCIe types
are subject to the multifunction rules and excludes event collectors
and PCIe-to-PCI bridges entirely.  Document each rule to the section
of the PCIe spec and provide overall documentation of the function.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/pci/pci.c |   84 ++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 70 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a899d8b..d0c2b35 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2349,6 +2349,19 @@ void pci_enable_acs(struct pci_dev *dev)
 	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
 }
 
+static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags)
+{
+	int pos;
+	u16 ctrl;
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ACS);
+	if (!pos)
+		return false;
+
+	pci_read_config_word(pdev, pos + PCI_ACS_CTRL, &ctrl);
+	return (ctrl & acs_flags) == acs_flags;
+}
+
 /**
  * pci_acs_enabled - test ACS against required flags for a given device
  * @pdev: device to test
@@ -2356,36 +2369,79 @@ void pci_enable_acs(struct pci_dev *dev)
  *
  * Return true if the device supports the provided flags.  Automatically
  * filters out flags that are not implemented on multifunction devices.
+ *
+ * Note that this interface checks the effective ACS capabilities of the
+ * device rather than the actual capabilities.  For instance, most single
+ * function endpoints are not required to support ACS because they have no
+ * opportunity for peer-to-peer access.  We therefore return 'true'
+ * regardless of whether the device exposes an ACS capability.  This makes
+ * it much easier for callers of this function to ignore the actual type
+ * or topology of the device when testing ACS support.
  */
 bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags)
 {
-	int pos, ret;
-	u16 ctrl;
+	int ret;
 
 	ret = pci_dev_specific_acs_enabled(pdev, acs_flags);
 	if (ret >= 0)
 		return ret > 0;
 
+	/*
+	 * Conventional PCI and PCI-X devices never support ACS, either
+	 * effectively or actually.  The shared bus topology implies that
+	 * any device on the bus can receive or snoop DMA.
+	 */
 	if (!pci_is_pcie(pdev))
 		return false;
 
-	/* Filter out flags not applicable to multifunction */
-	if (pdev->multifunction)
+	switch (pci_pcie_type(pdev)) {
+	/*
+	 * PCI/X-to-PCIe bridges are not specifically mentioned by the spec,
+	 * but since their primary inteface is PCI/X, we conservatively
+	 * handle them as we would a non-PCIe device.
+	 */
+	case PCI_EXP_TYPE_PCIE_BRIDGE:
+	/*
+	 * PCIe 3.0, 6.12.1 excludes ACS on these devices.  "ACS is never
+	 * applicable... must never implement an ACS Extended Capability...".
+	 * This seems arbitrary, but we take a conservative interpretation
+	 * of this statement.
+	 */
+	case PCI_EXP_TYPE_PCI_BRIDGE:
+	case PCI_EXP_TYPE_RC_EC:
+		return false;
+	/*
+	 * PCIe 3.0, 6.12.1.1 specifies that downstream and root ports should
+	 * implement ACS in order to indicate their peer-to-peer capabilities,
+	 * regardless of whether they are single- or multi-function devices.
+	 */
+	case PCI_EXP_TYPE_DOWNSTREAM:
+	case PCI_EXP_TYPE_ROOT_PORT:
+		return pci_acs_flags_enabled(pdev, acs_flags);
+	/*
+	 * PCIe 3.0, 6.12.1.2 specifies ACS capabilities that should be
+	 * implemented by the remaining PCIe types to indicate peer-to-peer
+	 * capabilities, but only when they are part of a multifunciton
+	 * device.  The footnote for section 6.12 indicates the specific
+	 * PCIe types included here.
+	 */
+	case PCI_EXP_TYPE_ENDPOINT:
+	case PCI_EXP_TYPE_UPSTREAM:
+	case PCI_EXP_TYPE_LEG_END:
+	case PCI_EXP_TYPE_RC_END:
+		if (!pdev->multifunction)
+			break;
+
 		acs_flags &= (PCI_ACS_RR | PCI_ACS_CR |
 			      PCI_ACS_EC | PCI_ACS_DT);
 
-	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM ||
-	    pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT ||
-	    pdev->multifunction) {
-		pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ACS);
-		if (!pos)
-			return false;
-
-		pci_read_config_word(pdev, pos + PCI_ACS_CTRL, &ctrl);
-		if ((ctrl & acs_flags) != acs_flags)
-			return false;
+		return pci_acs_flags_enabled(pdev, acs_flags);
 	}
 
+	/*
+	 * PCIe 3.0, 6.12.1.3 specifies no ACS capabilties are applicable
+	 * to single function devices with the exception of downstream ports.
+	 */
 	return true;
 }
 

[PATCH v2 2/3] pci: Differentiate ACS controllable from enabled

[Alex Williamson]

We currently misinterpret that in order for an ACS feature to be
enabled it must be set in the control field.  In reality, this means
that the feature is not only enabled, but controllable.  Many of the
ACS capability bits are not required if the device behaves by default
in the way specified when both the capability and control bit are set
and does not support or allow the alternate mode.  We therefore need
to check the capabilities and mask out flags that are enabled but not
controllable.  Egress control seems to be the only flag which is
purely optional.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/pci/pci.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d0c2b35..ae372ca 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2352,12 +2352,20 @@ void pci_enable_acs(struct pci_dev *dev)
 static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags)
 {
 	int pos;
-	u16 ctrl;
+	u16 cap, ctrl;
 
 	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ACS);
 	if (!pos)
 		return false;
 
+	/*
+	 * Except for egress control, capabilities are either required
+	 * or only required if controllable.  Features missing from the
+	 * capability field can therefore be assumed as hard-wired enabled.
+	 */
+	pci_read_config_word(pdev, pos + PCI_ACS_CAP, &cap);
+	acs_flags &= (cap | PCI_ACS_EC);
+
 	pci_read_config_word(pdev, pos + PCI_ACS_CTRL, &ctrl);
 	return (ctrl & acs_flags) == acs_flags;
 }
@@ -2432,9 +2440,6 @@ bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags)
 		if (!pdev->multifunction)
 			break;
 
-		acs_flags &= (PCI_ACS_RR | PCI_ACS_CR |
-			      PCI_ACS_EC | PCI_ACS_DT);
-
 		return pci_acs_flags_enabled(pdev, acs_flags);
 	}
 

[PATCH v2 3/3] pci: ACS quirk for AMD southbridge

[Alex Williamson]

AMD confirmed that peer-to-peer between these devices is
not possible.  We can therefore claim that they support a
subset of ACS.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/pci/quirks.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 7d68aee..02855dd 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3270,11 +3270,61 @@ struct pci_dev *pci_get_dma_source(struct pci_dev *dev)
 	return pci_dev_get(dev);
 }
 
+/*
+ * AMD has indicated that the devices below do not support peer-to-peer
+ * in any system where they are found in the southbridge with an AMD
+ * IOMMU in the system.  Multifunction devices that do not support
+ * peer-to-peer between functions can claim to support a subset of ACS.
+ * Such devices effectively enable request redirect (RR) and completion
+ * redirect (CR) since all transactions are redirected to the upstream
+ * root complex.
+ * 
+ * http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/94086
+ * http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/94102
+ * http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/99402
+ *
+ * 1002:4385 SBx00 SMBus Controller
+ * 1002:439c SB7x0/SB8x0/SB9x0 IDE Controller
+ * 1002:4383 SBx00 Azalia (Intel HDA)
+ * 1002:439d SB7x0/SB8x0/SB9x0 LPC host controller
+ * 1002:4384 SBx00 PCI to PCI Bridge
+ * 1002:4399 SB7x0/SB8x0/SB9x0 USB OHCI2 Controller
+ */
+static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
+{
+#ifdef CONFIG_ACPI
+	struct acpi_table_header *header = NULL;
+	acpi_status status;
+
+	/* Targeting multifunction devices on the SB (appears on root bus) */
+	if (!dev->multifunction || !pci_is_root_bus(dev->bus))
+		return -ENODEV;
+
+	/* The IVRS table describes the AMD IOMMU */
+	status = acpi_get_table("IVRS", 0, &header);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	/* Filter out flags not applicable to multifunction */
+	acs_flags &= (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC | PCI_ACS_DT);
+
+	return acs_flags & ~(PCI_ACS_RR | PCI_ACS_CR) ? 0 : 1;
+#else
+	return -ENODEV;
+#endif
+}
+
 static const struct pci_dev_acs_enabled {
 	u16 vendor;
 	u16 device;
 	int (*acs_enabled)(struct pci_dev *dev, u16 acs_flags);
 } pci_dev_acs_enabled[] = {
+	{ PCI_VENDOR_ID_ATI, 0x4385, pci_quirk_amd_sb_acs },
+	{ PCI_VENDOR_ID_ATI, 0x439c, pci_quirk_amd_sb_acs },
+	{ PCI_VENDOR_ID_ATI, 0x4383, pci_quirk_amd_sb_acs },
+	{ PCI_VENDOR_ID_ATI, 0x439d, pci_quirk_amd_sb_acs },
+	{ PCI_VENDOR_ID_ATI, 0x4384, pci_quirk_amd_sb_acs },
+	{ PCI_VENDOR_ID_ATI, 0x4399, pci_quirk_amd_sb_acs },
 	{ 0 }
 };
 

Re: [PATCH v2 0/3] pci: ACS fixes & quirks

[Alex Williamson]

Ping.  Comments?

On Thu, 2013-06-27 at 16:39 -0600, Alex Williamson wrote:
> v2:
> 
> Revised patch 1/ to match comments from Bjorn.  PCIe event collectors
> and PCIe-to-PCI bridges now indicate that they do not support ACS.
> I've reached out to try to get clarification on this, but I think it's
> reasonable to proceed with a conservative approach until then.  I also
> added PCI-to-PCIe bridges for the sake of being complete.  Also added
> more comments about the purpose and behavior of pci_acs_enabled().  If
> I've overlooked anything else that needs to be addressed, please let
> me know.
> 
> Patch 2/ had no comments, it's unchanged.
> 
> Patch 3/ is added.  This was sent as an RFC nearly a year ago and
> Joerg confirmed for us that these devices do not support p2p on AMD
> systems with AMD IOMMU.  We can't simply use iommu_present() to test
> for an IOMMU because it's setup just after we need this function.  
> Instead we test for the ACPI IVRS table that describes the IOMMU.  It
> would probably suffice to skip an actual AMD IOMMU check, but I don't
> want it to later come bite us if these ASICs get re-used, maybe with
> a different IOMMU, and don't make the same guarantees.
> 
> Joerg, I was also curious back when we investigated this patch if the
> same rules hold true for these other southbridge devices:
> 
> 1002:43a0 SB700/SB800/SB900 PCI to PCI bridge (PCIE port 0)
> 1002:43a1 SB700/SB800/SB900 PCI to PCI bridge (PCIE port 1)
> 1002:43a2 SB900 PCI to PCI bridge (PCIE port 2)
> 1002:43a3 SB900 PCI to PCI bridge (PCIE port 3)
> 
> If you remember or have contacts to poke, I'd be happy to follow-up
> with another patch to add them.  Thanks,
> 
> Alex
> 
> ---
> 
> Alex Williamson (3):
>       pci: Fix flaw in pci_acs_enabled()
>       pci: Differentiate ACS controllable from enabled
>       pci: ACS quirk for AMD southbridge
> 
> 
>  drivers/pci/pci.c    |   93 +++++++++++++++++++++++++++++++++++++++++---------
>  drivers/pci/quirks.c |   50 +++++++++++++++++++++++++++
>  2 files changed, 127 insertions(+), 16 deletions(-)

Re: [PATCH v2 0/3] pci: ACS fixes & quirks

[Don Dutile]

On 07/08/2013 12:17 PM, Alex Williamson wrote:
> Ping.  Comments?
>
As I read the PCIe & SRIOV specs wrt ACS, this patch set appears
to fix the nuances we have learned about whether a device (MFD or bridge)
implements (the equiv. of) ACS, or not, as required (for secure, device assignment).
Like the layering of which flag bits to check based on ACS control/enablement,
so it de-complicated the per-PCIe-type checking in pci_acs_enabled().

You can add my ack to the patch set...

Acked-by: Donald Dutile <ddutile@redhat.com>

ps -- I'll patch a kernel & test it out on a few knarly configs I have back in 
the lab
         when I back from vacation and report any issues I find, if any.

> On Thu, 2013-06-27 at 16:39 -0600, Alex Williamson wrote:
>> v2:
>>
>> Revised patch 1/ to match comments from Bjorn.  PCIe event collectors
>> and PCIe-to-PCI bridges now indicate that they do not support ACS.
>> I've reached out to try to get clarification on this, but I think it's
>> reasonable to proceed with a conservative approach until then.  I also
>> added PCI-to-PCIe bridges for the sake of being complete.  Also added
>> more comments about the purpose and behavior of pci_acs_enabled().  If
>> I've overlooked anything else that needs to be addressed, please let
>> me know.
>>
>> Patch 2/ had no comments, it's unchanged.
>>
>> Patch 3/ is added.  This was sent as an RFC nearly a year ago and
>> Joerg confirmed for us that these devices do not support p2p on AMD
>> systems with AMD IOMMU.  We can't simply use iommu_present() to test
>> for an IOMMU because it's setup just after we need this function.
>> Instead we test for the ACPI IVRS table that describes the IOMMU.  It
>> would probably suffice to skip an actual AMD IOMMU check, but I don't
>> want it to later come bite us if these ASICs get re-used, maybe with
>> a different IOMMU, and don't make the same guarantees.
>>
>> Joerg, I was also curious back when we investigated this patch if the
>> same rules hold true for these other southbridge devices:
>>
>> 1002:43a0 SB700/SB800/SB900 PCI to PCI bridge (PCIE port 0)
>> 1002:43a1 SB700/SB800/SB900 PCI to PCI bridge (PCIE port 1)
>> 1002:43a2 SB900 PCI to PCI bridge (PCIE port 2)
>> 1002:43a3 SB900 PCI to PCI bridge (PCIE port 3)
>>
>> If you remember or have contacts to poke, I'd be happy to follow-up
>> with another patch to add them.  Thanks,
>>
>> Alex
>>
>> ---
>>
>> Alex Williamson (3):
>>        pci: Fix flaw in pci_acs_enabled()
>>        pci: Differentiate ACS controllable from enabled
>>        pci: ACS quirk for AMD southbridge
>>
>>
>>   drivers/pci/pci.c    |   93 +++++++++++++++++++++++++++++++++++++++++---------
>>   drivers/pci/quirks.c |   50 +++++++++++++++++++++++++++
>>   2 files changed, 127 insertions(+), 16 deletions(-)
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] pci: ACS fixes & quirks

[Alex Williamson]

This series was ack'd by Don.  I haven't heard reports of any problems.
What more needs to be done here?  Bjorn?  Thanks,

Alex


On Mon, 2013-07-08 at 10:17 -0600, Alex Williamson wrote:
> Ping.  Comments?
> 
> On Thu, 2013-06-27 at 16:39 -0600, Alex Williamson wrote:
> > v2:
> > 
> > Revised patch 1/ to match comments from Bjorn.  PCIe event collectors
> > and PCIe-to-PCI bridges now indicate that they do not support ACS.
> > I've reached out to try to get clarification on this, but I think it's
> > reasonable to proceed with a conservative approach until then.  I also
> > added PCI-to-PCIe bridges for the sake of being complete.  Also added
> > more comments about the purpose and behavior of pci_acs_enabled().  If
> > I've overlooked anything else that needs to be addressed, please let
> > me know.
> > 
> > Patch 2/ had no comments, it's unchanged.
> > 
> > Patch 3/ is added.  This was sent as an RFC nearly a year ago and
> > Joerg confirmed for us that these devices do not support p2p on AMD
> > systems with AMD IOMMU.  We can't simply use iommu_present() to test
> > for an IOMMU because it's setup just after we need this function.  
> > Instead we test for the ACPI IVRS table that describes the IOMMU.  It
> > would probably suffice to skip an actual AMD IOMMU check, but I don't
> > want it to later come bite us if these ASICs get re-used, maybe with
> > a different IOMMU, and don't make the same guarantees.
> > 
> > Joerg, I was also curious back when we investigated this patch if the
> > same rules hold true for these other southbridge devices:
> > 
> > 1002:43a0 SB700/SB800/SB900 PCI to PCI bridge (PCIE port 0)
> > 1002:43a1 SB700/SB800/SB900 PCI to PCI bridge (PCIE port 1)
> > 1002:43a2 SB900 PCI to PCI bridge (PCIE port 2)
> > 1002:43a3 SB900 PCI to PCI bridge (PCIE port 3)
> > 
> > If you remember or have contacts to poke, I'd be happy to follow-up
> > with another patch to add them.  Thanks,
> > 
> > Alex
> > 
> > ---
> > 
> > Alex Williamson (3):
> >       pci: Fix flaw in pci_acs_enabled()
> >       pci: Differentiate ACS controllable from enabled
> >       pci: ACS quirk for AMD southbridge
> > 
> > 
> >  drivers/pci/pci.c    |   93 +++++++++++++++++++++++++++++++++++++++++---------
> >  drivers/pci/quirks.c |   50 +++++++++++++++++++++++++++
> >  2 files changed, 127 insertions(+), 16 deletions(-)
> 
> 

Re: [PATCH v2 0/3] pci: ACS fixes & quirks

[Bjorn Helgaas]

On Thu, Jun 27, 2013 at 04:39:41PM -0600, Alex Williamson wrote:
> v2:
> 
> Revised patch 1/ to match comments from Bjorn.  PCIe event collectors
> and PCIe-to-PCI bridges now indicate that they do not support ACS.
> I've reached out to try to get clarification on this, but I think it's
> reasonable to proceed with a conservative approach until then.  I also
> added PCI-to-PCIe bridges for the sake of being complete.  Also added
> more comments about the purpose and behavior of pci_acs_enabled().  If
> I've overlooked anything else that needs to be addressed, please let
> me know.
> 
> Patch 2/ had no comments, it's unchanged.
> 
> Patch 3/ is added.  This was sent as an RFC nearly a year ago and
> Joerg confirmed for us that these devices do not support p2p on AMD
> systems with AMD IOMMU.  We can't simply use iommu_present() to test
> for an IOMMU because it's setup just after we need this function.  
> Instead we test for the ACPI IVRS table that describes the IOMMU.  It
> would probably suffice to skip an actual AMD IOMMU check, but I don't
> want it to later come bite us if these ASICs get re-used, maybe with
> a different IOMMU, and don't make the same guarantees.
> 
> Joerg, I was also curious back when we investigated this patch if the
> same rules hold true for these other southbridge devices:
> 
> 1002:43a0 SB700/SB800/SB900 PCI to PCI bridge (PCIE port 0)
> 1002:43a1 SB700/SB800/SB900 PCI to PCI bridge (PCIE port 1)
> 1002:43a2 SB900 PCI to PCI bridge (PCIE port 2)
> 1002:43a3 SB900 PCI to PCI bridge (PCIE port 3)
> 
> If you remember or have contacts to poke, I'd be happy to follow-up
> with another patch to add them.  Thanks,
> 
> Alex
> 
> ---
> 
> Alex Williamson (3):
>       pci: Fix flaw in pci_acs_enabled()
>       pci: Differentiate ACS controllable from enabled
>       pci: ACS quirk for AMD southbridge

Applied to pci/aw-acs-fixes-v2 for v3.12, thanks!