iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 1/4] PCI: Move pci_enable_acs() and its dependencies up in pci.c
@ 2020-07-07 22:46 Rajat Jain via iommu
  2020-07-07 22:46 ` [PATCH v4 2/4] PCI: Keep the ACS capability offset in device Rajat Jain via iommu
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Rajat Jain via iommu @ 2020-07-07 22:46 UTC (permalink / raw)
  To: David Woodhouse, Lu Baolu, Joerg Roedel, Bjorn Helgaas,
	Rafael J. Wysocki, Len Brown, iommu, linux-kernel, linux-pci,
	linux-acpi, Raj Ashok, lalithambika.krishnakumar,
	Mika Westerberg, Jean-Philippe Brucker, Prashant Malani,
	Benson Leung, Todd Broch, Alex Levin, Mattias Nissler,
	Rajat Jain, Bernie Keany, Aaron Durbin, Diego Rivas,
	Duncan Laurie, Furquan Shaikh, Jesse Barnes, Christian Kellner,
	Alex Williamson, Greg Kroah-Hartman, oohall, Saravana Kannan,
	Suzuki K Poulose, Arnd Bergmann, Heikki Krogerus
  Cc: Rajat Jain

Move pci_enable_acs() and the functions it depends on, further up in the
source code to avoid having to forward declare it when we make it static
in near future (next patch).

No functional changes intended.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
v4: Same as v3
v3: Initial version of the patch, created per Bjorn's suggestion

 drivers/pci/pci.c | 254 +++++++++++++++++++++++-----------------------
 1 file changed, 127 insertions(+), 127 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ce096272f52b1..eec625f0e594e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -777,6 +777,133 @@ int pci_wait_for_pending(struct pci_dev *dev, int pos, u16 mask)
 	return 0;
 }
 
+static int pci_acs_enable;
+
+/**
+ * pci_request_acs - ask for ACS to be enabled if supported
+ */
+void pci_request_acs(void)
+{
+	pci_acs_enable = 1;
+}
+
+static const char *disable_acs_redir_param;
+
+/**
+ * pci_disable_acs_redir - disable ACS redirect capabilities
+ * @dev: the PCI device
+ *
+ * For only devices specified in the disable_acs_redir parameter.
+ */
+static void pci_disable_acs_redir(struct pci_dev *dev)
+{
+	int ret = 0;
+	const char *p;
+	int pos;
+	u16 ctrl;
+
+	if (!disable_acs_redir_param)
+		return;
+
+	p = disable_acs_redir_param;
+	while (*p) {
+		ret = pci_dev_str_match(dev, p, &p);
+		if (ret < 0) {
+			pr_info_once("PCI: Can't parse disable_acs_redir parameter: %s\n",
+				     disable_acs_redir_param);
+
+			break;
+		} else if (ret == 1) {
+			/* Found a match */
+			break;
+		}
+
+		if (*p != ';' && *p != ',') {
+			/* End of param or invalid format */
+			break;
+		}
+		p++;
+	}
+
+	if (ret != 1)
+		return;
+
+	if (!pci_dev_specific_disable_acs_redir(dev))
+		return;
+
+	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+	if (!pos) {
+		pci_warn(dev, "cannot disable ACS redirect for this hardware as it does not have ACS capabilities\n");
+		return;
+	}
+
+	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
+
+	/* P2P Request & Completion Redirect */
+	ctrl &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC);
+
+	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+
+	pci_info(dev, "disabled ACS redirect\n");
+}
+
+/**
+ * pci_std_enable_acs - enable ACS on devices using standard ACS capabilities
+ * @dev: the PCI device
+ */
+static void pci_std_enable_acs(struct pci_dev *dev)
+{
+	int pos;
+	u16 cap;
+	u16 ctrl;
+
+	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+	if (!pos)
+		return;
+
+	pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
+	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
+
+	/* Source Validation */
+	ctrl |= (cap & PCI_ACS_SV);
+
+	/* P2P Request Redirect */
+	ctrl |= (cap & PCI_ACS_RR);
+
+	/* P2P Completion Redirect */
+	ctrl |= (cap & PCI_ACS_CR);
+
+	/* Upstream Forwarding */
+	ctrl |= (cap & PCI_ACS_UF);
+
+	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+}
+
+/**
+ * pci_enable_acs - enable ACS if hardware support it
+ * @dev: the PCI device
+ */
+void pci_enable_acs(struct pci_dev *dev)
+{
+	if (!pci_acs_enable)
+		goto disable_acs_redir;
+
+	if (!pci_dev_specific_enable_acs(dev))
+		goto disable_acs_redir;
+
+	pci_std_enable_acs(dev);
+
+disable_acs_redir:
+	/*
+	 * Note: pci_disable_acs_redir() must be called even if ACS was not
+	 * enabled by the kernel because it may have been enabled by
+	 * platform firmware.  So if we are told to disable it, we should
+	 * always disable it after setting the kernel's default
+	 * preferences.
+	 */
+	pci_disable_acs_redir(dev);
+}
+
 /**
  * pci_restore_bars - restore a device's BAR values (e.g. after wake-up)
  * @dev: PCI device to have its BARs restored
@@ -3230,133 +3357,6 @@ void pci_configure_ari(struct pci_dev *dev)
 	}
 }
 
-static int pci_acs_enable;
-
-/**
- * pci_request_acs - ask for ACS to be enabled if supported
- */
-void pci_request_acs(void)
-{
-	pci_acs_enable = 1;
-}
-
-static const char *disable_acs_redir_param;
-
-/**
- * pci_disable_acs_redir - disable ACS redirect capabilities
- * @dev: the PCI device
- *
- * For only devices specified in the disable_acs_redir parameter.
- */
-static void pci_disable_acs_redir(struct pci_dev *dev)
-{
-	int ret = 0;
-	const char *p;
-	int pos;
-	u16 ctrl;
-
-	if (!disable_acs_redir_param)
-		return;
-
-	p = disable_acs_redir_param;
-	while (*p) {
-		ret = pci_dev_str_match(dev, p, &p);
-		if (ret < 0) {
-			pr_info_once("PCI: Can't parse disable_acs_redir parameter: %s\n",
-				     disable_acs_redir_param);
-
-			break;
-		} else if (ret == 1) {
-			/* Found a match */
-			break;
-		}
-
-		if (*p != ';' && *p != ',') {
-			/* End of param or invalid format */
-			break;
-		}
-		p++;
-	}
-
-	if (ret != 1)
-		return;
-
-	if (!pci_dev_specific_disable_acs_redir(dev))
-		return;
-
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
-	if (!pos) {
-		pci_warn(dev, "cannot disable ACS redirect for this hardware as it does not have ACS capabilities\n");
-		return;
-	}
-
-	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
-
-	/* P2P Request & Completion Redirect */
-	ctrl &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC);
-
-	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
-
-	pci_info(dev, "disabled ACS redirect\n");
-}
-
-/**
- * pci_std_enable_acs - enable ACS on devices using standard ACS capabilities
- * @dev: the PCI device
- */
-static void pci_std_enable_acs(struct pci_dev *dev)
-{
-	int pos;
-	u16 cap;
-	u16 ctrl;
-
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
-	if (!pos)
-		return;
-
-	pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
-	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
-
-	/* Source Validation */
-	ctrl |= (cap & PCI_ACS_SV);
-
-	/* P2P Request Redirect */
-	ctrl |= (cap & PCI_ACS_RR);
-
-	/* P2P Completion Redirect */
-	ctrl |= (cap & PCI_ACS_CR);
-
-	/* Upstream Forwarding */
-	ctrl |= (cap & PCI_ACS_UF);
-
-	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
-}
-
-/**
- * pci_enable_acs - enable ACS if hardware support it
- * @dev: the PCI device
- */
-void pci_enable_acs(struct pci_dev *dev)
-{
-	if (!pci_acs_enable)
-		goto disable_acs_redir;
-
-	if (!pci_dev_specific_enable_acs(dev))
-		goto disable_acs_redir;
-
-	pci_std_enable_acs(dev);
-
-disable_acs_redir:
-	/*
-	 * Note: pci_disable_acs_redir() must be called even if ACS was not
-	 * enabled by the kernel because it may have been enabled by
-	 * platform firmware.  So if we are told to disable it, we should
-	 * always disable it after setting the kernel's default
-	 * preferences.
-	 */
-	pci_disable_acs_redir(dev);
-}
-
 static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags)
 {
 	int pos;
-- 
2.27.0.212.ge8ba1cc988-goog

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v4 2/4] PCI: Keep the ACS capability offset in device
  2020-07-07 22:46 [PATCH v4 1/4] PCI: Move pci_enable_acs() and its dependencies up in pci.c Rajat Jain via iommu
@ 2020-07-07 22:46 ` Rajat Jain via iommu
  2020-07-07 22:46 ` [PATCH v4 3/4] PCI: Treat "external-facing" devices as internal Rajat Jain via iommu
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 16+ messages in thread
From: Rajat Jain via iommu @ 2020-07-07 22:46 UTC (permalink / raw)
  To: David Woodhouse, Lu Baolu, Joerg Roedel, Bjorn Helgaas,
	Rafael J. Wysocki, Len Brown, iommu, linux-kernel, linux-pci,
	linux-acpi, Raj Ashok, lalithambika.krishnakumar,
	Mika Westerberg, Jean-Philippe Brucker, Prashant Malani,
	Benson Leung, Todd Broch, Alex Levin, Mattias Nissler,
	Rajat Jain, Bernie Keany, Aaron Durbin, Diego Rivas,
	Duncan Laurie, Furquan Shaikh, Jesse Barnes, Christian Kellner,
	Alex Williamson, Greg Kroah-Hartman, oohall, Saravana Kannan,
	Suzuki K Poulose, Arnd Bergmann, Heikki Krogerus
  Cc: Rajat Jain

Currently ACS capabiity is being looked up at a number of places. Read and
store it once at bootup so that it can be used by all later.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
v4: No change
v3: fix commit log, remove forward declation of static function
v2: Commit log cosmetic changes

 drivers/pci/p2pdma.c |  2 +-
 drivers/pci/pci.c    | 20 ++++++++++++++++----
 drivers/pci/pci.h    |  2 +-
 drivers/pci/probe.c  |  2 +-
 drivers/pci/quirks.c |  8 ++++----
 include/linux/pci.h  |  1 +
 6 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index e8e444eeb1cd2..f29a48f8fa594 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -253,7 +253,7 @@ static int pci_bridge_has_acs_redir(struct pci_dev *pdev)
 	int pos;
 	u16 ctrl;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ACS);
+	pos = pdev->acs_cap;
 	if (!pos)
 		return 0;
 
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index eec625f0e594e..73a8627822140 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -831,7 +831,7 @@ static void pci_disable_acs_redir(struct pci_dev *dev)
 	if (!pci_dev_specific_disable_acs_redir(dev))
 		return;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+	pos = dev->acs_cap;
 	if (!pos) {
 		pci_warn(dev, "cannot disable ACS redirect for this hardware as it does not have ACS capabilities\n");
 		return;
@@ -857,7 +857,7 @@ static void pci_std_enable_acs(struct pci_dev *dev)
 	u16 cap;
 	u16 ctrl;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+	pos = dev->acs_cap;
 	if (!pos)
 		return;
 
@@ -883,7 +883,7 @@ static void pci_std_enable_acs(struct pci_dev *dev)
  * pci_enable_acs - enable ACS if hardware support it
  * @dev: the PCI device
  */
-void pci_enable_acs(struct pci_dev *dev)
+static void pci_enable_acs(struct pci_dev *dev)
 {
 	if (!pci_acs_enable)
 		goto disable_acs_redir;
@@ -3362,7 +3362,7 @@ static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags)
 	int pos;
 	u16 cap, ctrl;
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ACS);
+	pos = pdev->acs_cap;
 	if (!pos)
 		return false;
 
@@ -3487,6 +3487,18 @@ bool pci_acs_path_enabled(struct pci_dev *start,
 	return true;
 }
 
+/**
+ * pci_acs_init - Initialize ACS if hardware supports it
+ * @dev: the PCI device
+ */
+void pci_acs_init(struct pci_dev *dev)
+{
+	dev->acs_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+
+	if (dev->acs_cap)
+		pci_enable_acs(dev);
+}
+
 /**
  * pci_rebar_find_pos - find position of resize ctrl reg for BAR
  * @pdev: PCI device
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6d3f758671064..12fb79fbe29d3 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -532,7 +532,7 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev,
 	return resource_alignment(res);
 }
 
-void pci_enable_acs(struct pci_dev *dev);
+void pci_acs_init(struct pci_dev *dev);
 #ifdef CONFIG_PCI_QUIRKS
 int pci_dev_specific_acs_enabled(struct pci_dev *dev, u16 acs_flags);
 int pci_dev_specific_enable_acs(struct pci_dev *dev);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 2f66988cea257..6d87066a5ecc5 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2390,7 +2390,7 @@ static void pci_init_capabilities(struct pci_dev *dev)
 	pci_ats_init(dev);		/* Address Translation Services */
 	pci_pri_init(dev);		/* Page Request Interface */
 	pci_pasid_init(dev);		/* Process Address Space ID */
-	pci_enable_acs(dev);		/* Enable ACS P2P upstream forwarding */
+	pci_acs_init(dev);		/* Access Control Services */
 	pci_ptm_init(dev);		/* Precision Time Measurement */
 	pci_aer_init(dev);		/* Advanced Error Reporting */
 	pci_dpc_init(dev);		/* Downstream Port Containment */
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 812bfc32ecb82..b341628e47527 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4653,7 +4653,7 @@ static int pci_quirk_intel_spt_pch_acs(struct pci_dev *dev, u16 acs_flags)
 	if (!pci_quirk_intel_spt_pch_acs_match(dev))
 		return -ENOTTY;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+	pos = dev->acs_cap;
 	if (!pos)
 		return -ENOTTY;
 
@@ -4961,7 +4961,7 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev)
 	if (!pci_quirk_intel_spt_pch_acs_match(dev))
 		return -ENOTTY;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+	pos = dev->acs_cap;
 	if (!pos)
 		return -ENOTTY;
 
@@ -4988,7 +4988,7 @@ static int pci_quirk_disable_intel_spt_pch_acs_redir(struct pci_dev *dev)
 	if (!pci_quirk_intel_spt_pch_acs_match(dev))
 		return -ENOTTY;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
+	pos = dev->acs_cap;
 	if (!pos)
 		return -ENOTTY;
 
@@ -5355,7 +5355,7 @@ int pci_idt_bus_quirk(struct pci_bus *bus, int devfn, u32 *l, int timeout)
 	bool found;
 	struct pci_dev *bridge = bus->self;
 
-	pos = pci_find_ext_capability(bridge, PCI_EXT_CAP_ID_ACS);
+	pos = bridge->acs_cap;
 
 	/* Disable ACS SV before initial config reads */
 	if (pos) {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 34c1c4f45288f..0ca39042507ce 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -486,6 +486,7 @@ struct pci_dev {
 #ifdef CONFIG_PCI_P2PDMA
 	struct pci_p2pdma *p2pdma;
 #endif
+	u16		acs_cap;	/* ACS Capability offset */
 	phys_addr_t	rom;		/* Physical address if not from BAR */
 	size_t		romlen;		/* Length if not from BAR */
 	char		*driver_override; /* Driver name to force a match */
-- 
2.27.0.212.ge8ba1cc988-goog

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v4 3/4] PCI: Treat "external-facing" devices as internal
  2020-07-07 22:46 [PATCH v4 1/4] PCI: Move pci_enable_acs() and its dependencies up in pci.c Rajat Jain via iommu
  2020-07-07 22:46 ` [PATCH v4 2/4] PCI: Keep the ACS capability offset in device Rajat Jain via iommu
@ 2020-07-07 22:46 ` Rajat Jain via iommu
  2020-07-07 22:46 ` [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices Rajat Jain via iommu
  2020-07-10 20:39 ` [PATCH v4 1/4] PCI: Move pci_enable_acs() and its dependencies up in pci.c Bjorn Helgaas
  3 siblings, 0 replies; 16+ messages in thread
From: Rajat Jain via iommu @ 2020-07-07 22:46 UTC (permalink / raw)
  To: David Woodhouse, Lu Baolu, Joerg Roedel, Bjorn Helgaas,
	Rafael J. Wysocki, Len Brown, iommu, linux-kernel, linux-pci,
	linux-acpi, Raj Ashok, lalithambika.krishnakumar,
	Mika Westerberg, Jean-Philippe Brucker, Prashant Malani,
	Benson Leung, Todd Broch, Alex Levin, Mattias Nissler,
	Rajat Jain, Bernie Keany, Aaron Durbin, Diego Rivas,
	Duncan Laurie, Furquan Shaikh, Jesse Barnes, Christian Kellner,
	Alex Williamson, Greg Kroah-Hartman, oohall, Saravana Kannan,
	Suzuki K Poulose, Arnd Bergmann, Heikki Krogerus
  Cc: Rajat Jain

The "ExternalFacingPort" devices (root ports) are internal devices that
sit on the internal system fabric. Ref:
https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports

Currently they were treated (marked as untrusted) at par with other
external devices downstream those external facing rootports.

Use the platform flag to identify the external facing devices and then
treat them at par with internal devices (don't mark them untrusted).
Any devices downstream continue to be marked as "untrusted". This was
discussed here:
https://lore.kernel.org/linux-pci/20200610230906.GA1528594@bjorn-Precision-5520/

Signed-off-by: Rajat Jain <rajatja@google.com>
---
v4: No change
v3: * fix commit log and minor code comment
    * Don't check for "ExternalFacingPort" on PCI_EXP_TYPE_DOWNSTREAM
    * Check only for pdev->external_facing in iommu.c
v2: cosmetic changes in commit log

 drivers/iommu/intel/iommu.c |  6 +++---
 drivers/pci/of.c            |  2 +-
 drivers/pci/pci-acpi.c      | 10 +++++-----
 drivers/pci/probe.c         |  2 +-
 include/linux/pci.h         |  8 ++++++++
 5 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index d759e7234e982..4f0f6ee2d4aaa 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4738,12 +4738,12 @@ const struct attribute_group *intel_iommu_groups[] = {
 	NULL,
 };
 
-static inline bool has_untrusted_dev(void)
+static inline bool has_external_pci(void)
 {
 	struct pci_dev *pdev = NULL;
 
 	for_each_pci_dev(pdev)
-		if (pdev->untrusted)
+		if (pdev->external_facing)
 			return true;
 
 	return false;
@@ -4751,7 +4751,7 @@ static inline bool has_untrusted_dev(void)
 
 static int __init platform_optin_force_iommu(void)
 {
-	if (!dmar_platform_optin() || no_platform_optin || !has_untrusted_dev())
+	if (!dmar_platform_optin() || no_platform_optin || !has_external_pci())
 		return 0;
 
 	if (no_iommu || dmar_disabled)
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 27839cd2459f6..22727fc9558df 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -42,7 +42,7 @@ void pci_set_bus_of_node(struct pci_bus *bus)
 	} else {
 		node = of_node_get(bus->self->dev.of_node);
 		if (node && of_property_read_bool(node, "external-facing"))
-			bus->self->untrusted = true;
+			bus->self->external_facing = true;
 	}
 
 	bus->dev.of_node = node;
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 7224b1e5f2a83..43a5158b2b662 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1213,7 +1213,7 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
 	ACPI_FREE(obj);
 }
 
-static void pci_acpi_set_untrusted(struct pci_dev *dev)
+static void pci_acpi_set_external_facing(struct pci_dev *dev)
 {
 	u8 val;
 
@@ -1223,12 +1223,12 @@ static void pci_acpi_set_untrusted(struct pci_dev *dev)
 		return;
 
 	/*
-	 * These root ports expose PCIe (including DMA) outside of the
-	 * system so make sure we treat them and everything behind as
+	 * These root/down ports expose PCIe (including DMA) outside of the
+	 * system so make sure we treat everything behind them as
 	 * untrusted.
 	 */
 	if (val)
-		dev->untrusted = 1;
+		dev->external_facing = 1;
 }
 
 static void pci_acpi_setup(struct device *dev)
@@ -1240,7 +1240,7 @@ static void pci_acpi_setup(struct device *dev)
 		return;
 
 	pci_acpi_optimize_delay(pci_dev, adev->handle);
-	pci_acpi_set_untrusted(pci_dev);
+	pci_acpi_set_external_facing(pci_dev);
 	pci_acpi_add_edr_notifier(pci_dev);
 
 	pci_acpi_add_pm_notifier(adev, pci_dev);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6d87066a5ecc5..8c40c00413e74 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1552,7 +1552,7 @@ static void set_pcie_untrusted(struct pci_dev *dev)
 	 * untrusted as well.
 	 */
 	parent = pci_upstream_bridge(dev);
-	if (parent && parent->untrusted)
+	if (parent && (parent->untrusted || parent->external_facing))
 		dev->untrusted = true;
 }
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0ca39042507ce..281be857d2430 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -432,6 +432,14 @@ struct pci_dev {
 	 * mappings to make sure they cannot access arbitrary memory.
 	 */
 	unsigned int	untrusted:1;
+	/*
+	 * Devices are marked as external-facing using info from platform
+	 * (ACPI / devicetree). An external-facing device is still an internal
+	 * trusted device, but it faces external untrusted devices. Thus any
+	 * device enumerated downstream an external-facing device, is marked
+	 * as untrusted.
+	 */
+	unsigned int	external_facing:1;
 	unsigned int	broken_intx_masking:1;	/* INTx masking can't be used */
 	unsigned int	io_window_1k:1;		/* Intel bridge 1K I/O windows */
 	unsigned int	irq_managed:1;
-- 
2.27.0.212.ge8ba1cc988-goog

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
  2020-07-07 22:46 [PATCH v4 1/4] PCI: Move pci_enable_acs() and its dependencies up in pci.c Rajat Jain via iommu
  2020-07-07 22:46 ` [PATCH v4 2/4] PCI: Keep the ACS capability offset in device Rajat Jain via iommu
  2020-07-07 22:46 ` [PATCH v4 3/4] PCI: Treat "external-facing" devices as internal Rajat Jain via iommu
@ 2020-07-07 22:46 ` Rajat Jain via iommu
  2020-07-10 20:29   ` Bjorn Helgaas
  2020-09-16 21:46   ` Bjorn Helgaas
  2020-07-10 20:39 ` [PATCH v4 1/4] PCI: Move pci_enable_acs() and its dependencies up in pci.c Bjorn Helgaas
  3 siblings, 2 replies; 16+ messages in thread
From: Rajat Jain via iommu @ 2020-07-07 22:46 UTC (permalink / raw)
  To: David Woodhouse, Lu Baolu, Joerg Roedel, Bjorn Helgaas,
	Rafael J. Wysocki, Len Brown, iommu, linux-kernel, linux-pci,
	linux-acpi, Raj Ashok, lalithambika.krishnakumar,
	Mika Westerberg, Jean-Philippe Brucker, Prashant Malani,
	Benson Leung, Todd Broch, Alex Levin, Mattias Nissler,
	Rajat Jain, Bernie Keany, Aaron Durbin, Diego Rivas,
	Duncan Laurie, Furquan Shaikh, Jesse Barnes, Christian Kellner,
	Alex Williamson, Greg Kroah-Hartman, oohall, Saravana Kannan,
	Suzuki K Poulose, Arnd Bergmann, Heikki Krogerus
  Cc: Rajat Jain

When enabling ACS, enable translation blocking for external facing ports
and untrusted devices.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
v4: Add braces to avoid warning from kernel robot
    print warning for only external-facing devices.
v3: print warning if ACS_TB not supported on external-facing/untrusted ports.
    Minor code comments fixes.
v2: Commit log change

 drivers/pci/pci.c    |  8 ++++++++
 drivers/pci/quirks.c | 15 +++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 73a8627822140..a5a6bea7af7ce 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev)
 	/* Upstream Forwarding */
 	ctrl |= (cap & PCI_ACS_UF);
 
+	/* Enable Translation Blocking for external devices */
+	if (dev->external_facing || dev->untrusted) {
+		if (cap & PCI_ACS_TB)
+			ctrl |= PCI_ACS_TB;
+		else if (dev->external_facing)
+			pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n");
+	}
+
 	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
 }
 
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index b341628e47527..bb22b46c1d719 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev)
 	}
 }
 
+/*
+ * Currently this quirk does the equivalent of
+ * PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF
+ *
+ * TODO: This quirk also needs to do equivalent of PCI_ACS_TB,
+ * if dev->external_facing || dev->untrusted
+ */
 static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev)
 {
 	if (!pci_quirk_intel_pch_acs_match(dev))
@@ -4973,6 +4980,14 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev)
 	ctrl |= (cap & PCI_ACS_CR);
 	ctrl |= (cap & PCI_ACS_UF);
 
+	/* Enable Translation Blocking for external devices */
+	if (dev->external_facing || dev->untrusted) {
+		if (cap & PCI_ACS_TB)
+			ctrl |= PCI_ACS_TB;
+		else if (dev->external_facing)
+			pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n");
+	}
+
 	pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl);
 
 	pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n");
-- 
2.27.0.212.ge8ba1cc988-goog

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
  2020-07-07 22:46 ` [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices Rajat Jain via iommu
@ 2020-07-10 20:29   ` Bjorn Helgaas
  2020-07-10 21:28     ` Raj, Ashok
  2020-09-16 21:46   ` Bjorn Helgaas
  1 sibling, 1 reply; 16+ messages in thread
From: Bjorn Helgaas @ 2020-07-10 20:29 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Todd Broch, linux-pci, lalithambika.krishnakumar,
	Heikki Krogerus, Diego Rivas, Jean-Philippe Brucker,
	Furquan Shaikh, Raj Ashok, Saravana Kannan, linux-acpi,
	Christian Kellner, Mattias Nissler, Jesse Barnes, Len Brown,
	Rajat Jain, Prashant Malani, Suzuki K Poulose, Aaron Durbin,
	Alex Williamson, Bjorn Helgaas, Mika Westerberg, Bernie Keany,
	Duncan Laurie, Greg Kroah-Hartman, Rafael J. Wysocki,
	linux-kernel, iommu, Arnd Bergmann, oohall, Benson Leung,
	David Woodhouse, Alex Levin

On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote:
> When enabling ACS, enable translation blocking for external facing ports
> and untrusted devices.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> v4: Add braces to avoid warning from kernel robot
>     print warning for only external-facing devices.
> v3: print warning if ACS_TB not supported on external-facing/untrusted ports.
>     Minor code comments fixes.
> v2: Commit log change
> 
>  drivers/pci/pci.c    |  8 ++++++++
>  drivers/pci/quirks.c | 15 +++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 73a8627822140..a5a6bea7af7ce 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev)
>  	/* Upstream Forwarding */
>  	ctrl |= (cap & PCI_ACS_UF);
>  
> +	/* Enable Translation Blocking for external devices */
> +	if (dev->external_facing || dev->untrusted) {
> +		if (cap & PCI_ACS_TB)
> +			ctrl |= PCI_ACS_TB;
> +		else if (dev->external_facing)
> +			pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n");
> +	}

IIUC, this means that external devices can *never* use ATS and can
never cache translations.  And (I guess, I'm not an expert) it can
also never use the Page Request Services?

Is this what we want?  Do we have any idea how many external devices
this will affect or how much of a performance impact they will see?

Do we need some kind of override or mechanism to authenticate certain
devices so they can use ATS and PRI?

If we do decide this is the right thing to do, I think we need to
expand the commit log a bit, because this is potentially a significant
user-visible change.

>  	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
>  }
>  
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index b341628e47527..bb22b46c1d719 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev)
>  	}
>  }
>  
> +/*
> + * Currently this quirk does the equivalent of
> + * PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF
> + *
> + * TODO: This quirk also needs to do equivalent of PCI_ACS_TB,
> + * if dev->external_facing || dev->untrusted
> + */
>  static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev)
>  {
>  	if (!pci_quirk_intel_pch_acs_match(dev))
> @@ -4973,6 +4980,14 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev)
>  	ctrl |= (cap & PCI_ACS_CR);
>  	ctrl |= (cap & PCI_ACS_UF);
>  
> +	/* Enable Translation Blocking for external devices */
> +	if (dev->external_facing || dev->untrusted) {
> +		if (cap & PCI_ACS_TB)
> +			ctrl |= PCI_ACS_TB;
> +		else if (dev->external_facing)
> +			pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n");
> +	}
> +
>  	pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl);
>  
>  	pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n");
> -- 
> 2.27.0.212.ge8ba1cc988-goog
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 1/4] PCI: Move pci_enable_acs() and its dependencies up in pci.c
  2020-07-07 22:46 [PATCH v4 1/4] PCI: Move pci_enable_acs() and its dependencies up in pci.c Rajat Jain via iommu
                   ` (2 preceding siblings ...)
  2020-07-07 22:46 ` [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices Rajat Jain via iommu
@ 2020-07-10 20:39 ` Bjorn Helgaas
  3 siblings, 0 replies; 16+ messages in thread
From: Bjorn Helgaas @ 2020-07-10 20:39 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Todd Broch, linux-pci, lalithambika.krishnakumar,
	Heikki Krogerus, Diego Rivas, Jean-Philippe Brucker,
	Furquan Shaikh, Raj Ashok, Saravana Kannan, linux-acpi,
	Christian Kellner, Mattias Nissler, Jesse Barnes, Len Brown,
	Rajat Jain, Prashant Malani, Suzuki K Poulose, Aaron Durbin,
	Alex Williamson, Bjorn Helgaas, Mika Westerberg, Bernie Keany,
	Duncan Laurie, Greg Kroah-Hartman, Rafael J. Wysocki,
	linux-kernel, iommu, Arnd Bergmann, oohall, Benson Leung,
	David Woodhouse, Alex Levin

On Tue, Jul 07, 2020 at 03:46:01PM -0700, Rajat Jain wrote:
> Move pci_enable_acs() and the functions it depends on, further up in the
> source code to avoid having to forward declare it when we make it static
> in near future (next patch).
> 
> No functional changes intended.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>

Applied patches 1-3 to pci/enumeration for v5.9, thanks!

I held off on patch 4 (enabling PCI_ACS_TB) until we have a little
more conversation on the impact of it.

> ---
> v4: Same as v3
> v3: Initial version of the patch, created per Bjorn's suggestion
> 
>  drivers/pci/pci.c | 254 +++++++++++++++++++++++-----------------------
>  1 file changed, 127 insertions(+), 127 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index ce096272f52b1..eec625f0e594e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -777,6 +777,133 @@ int pci_wait_for_pending(struct pci_dev *dev, int pos, u16 mask)
>  	return 0;
>  }
>  
> +static int pci_acs_enable;
> +
> +/**
> + * pci_request_acs - ask for ACS to be enabled if supported
> + */
> +void pci_request_acs(void)
> +{
> +	pci_acs_enable = 1;
> +}
> +
> +static const char *disable_acs_redir_param;
> +
> +/**
> + * pci_disable_acs_redir - disable ACS redirect capabilities
> + * @dev: the PCI device
> + *
> + * For only devices specified in the disable_acs_redir parameter.
> + */
> +static void pci_disable_acs_redir(struct pci_dev *dev)
> +{
> +	int ret = 0;
> +	const char *p;
> +	int pos;
> +	u16 ctrl;
> +
> +	if (!disable_acs_redir_param)
> +		return;
> +
> +	p = disable_acs_redir_param;
> +	while (*p) {
> +		ret = pci_dev_str_match(dev, p, &p);
> +		if (ret < 0) {
> +			pr_info_once("PCI: Can't parse disable_acs_redir parameter: %s\n",
> +				     disable_acs_redir_param);
> +
> +			break;
> +		} else if (ret == 1) {
> +			/* Found a match */
> +			break;
> +		}
> +
> +		if (*p != ';' && *p != ',') {
> +			/* End of param or invalid format */
> +			break;
> +		}
> +		p++;
> +	}
> +
> +	if (ret != 1)
> +		return;
> +
> +	if (!pci_dev_specific_disable_acs_redir(dev))
> +		return;
> +
> +	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
> +	if (!pos) {
> +		pci_warn(dev, "cannot disable ACS redirect for this hardware as it does not have ACS capabilities\n");
> +		return;
> +	}
> +
> +	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
> +
> +	/* P2P Request & Completion Redirect */
> +	ctrl &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC);
> +
> +	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> +
> +	pci_info(dev, "disabled ACS redirect\n");
> +}
> +
> +/**
> + * pci_std_enable_acs - enable ACS on devices using standard ACS capabilities
> + * @dev: the PCI device
> + */
> +static void pci_std_enable_acs(struct pci_dev *dev)
> +{
> +	int pos;
> +	u16 cap;
> +	u16 ctrl;
> +
> +	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
> +	if (!pos)
> +		return;
> +
> +	pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
> +	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
> +
> +	/* Source Validation */
> +	ctrl |= (cap & PCI_ACS_SV);
> +
> +	/* P2P Request Redirect */
> +	ctrl |= (cap & PCI_ACS_RR);
> +
> +	/* P2P Completion Redirect */
> +	ctrl |= (cap & PCI_ACS_CR);
> +
> +	/* Upstream Forwarding */
> +	ctrl |= (cap & PCI_ACS_UF);
> +
> +	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> +}
> +
> +/**
> + * pci_enable_acs - enable ACS if hardware support it
> + * @dev: the PCI device
> + */
> +void pci_enable_acs(struct pci_dev *dev)
> +{
> +	if (!pci_acs_enable)
> +		goto disable_acs_redir;
> +
> +	if (!pci_dev_specific_enable_acs(dev))
> +		goto disable_acs_redir;
> +
> +	pci_std_enable_acs(dev);
> +
> +disable_acs_redir:
> +	/*
> +	 * Note: pci_disable_acs_redir() must be called even if ACS was not
> +	 * enabled by the kernel because it may have been enabled by
> +	 * platform firmware.  So if we are told to disable it, we should
> +	 * always disable it after setting the kernel's default
> +	 * preferences.
> +	 */
> +	pci_disable_acs_redir(dev);
> +}
> +
>  /**
>   * pci_restore_bars - restore a device's BAR values (e.g. after wake-up)
>   * @dev: PCI device to have its BARs restored
> @@ -3230,133 +3357,6 @@ void pci_configure_ari(struct pci_dev *dev)
>  	}
>  }
>  
> -static int pci_acs_enable;
> -
> -/**
> - * pci_request_acs - ask for ACS to be enabled if supported
> - */
> -void pci_request_acs(void)
> -{
> -	pci_acs_enable = 1;
> -}
> -
> -static const char *disable_acs_redir_param;
> -
> -/**
> - * pci_disable_acs_redir - disable ACS redirect capabilities
> - * @dev: the PCI device
> - *
> - * For only devices specified in the disable_acs_redir parameter.
> - */
> -static void pci_disable_acs_redir(struct pci_dev *dev)
> -{
> -	int ret = 0;
> -	const char *p;
> -	int pos;
> -	u16 ctrl;
> -
> -	if (!disable_acs_redir_param)
> -		return;
> -
> -	p = disable_acs_redir_param;
> -	while (*p) {
> -		ret = pci_dev_str_match(dev, p, &p);
> -		if (ret < 0) {
> -			pr_info_once("PCI: Can't parse disable_acs_redir parameter: %s\n",
> -				     disable_acs_redir_param);
> -
> -			break;
> -		} else if (ret == 1) {
> -			/* Found a match */
> -			break;
> -		}
> -
> -		if (*p != ';' && *p != ',') {
> -			/* End of param or invalid format */
> -			break;
> -		}
> -		p++;
> -	}
> -
> -	if (ret != 1)
> -		return;
> -
> -	if (!pci_dev_specific_disable_acs_redir(dev))
> -		return;
> -
> -	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
> -	if (!pos) {
> -		pci_warn(dev, "cannot disable ACS redirect for this hardware as it does not have ACS capabilities\n");
> -		return;
> -	}
> -
> -	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
> -
> -	/* P2P Request & Completion Redirect */
> -	ctrl &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC);
> -
> -	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> -
> -	pci_info(dev, "disabled ACS redirect\n");
> -}
> -
> -/**
> - * pci_std_enable_acs - enable ACS on devices using standard ACS capabilities
> - * @dev: the PCI device
> - */
> -static void pci_std_enable_acs(struct pci_dev *dev)
> -{
> -	int pos;
> -	u16 cap;
> -	u16 ctrl;
> -
> -	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
> -	if (!pos)
> -		return;
> -
> -	pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
> -	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
> -
> -	/* Source Validation */
> -	ctrl |= (cap & PCI_ACS_SV);
> -
> -	/* P2P Request Redirect */
> -	ctrl |= (cap & PCI_ACS_RR);
> -
> -	/* P2P Completion Redirect */
> -	ctrl |= (cap & PCI_ACS_CR);
> -
> -	/* Upstream Forwarding */
> -	ctrl |= (cap & PCI_ACS_UF);
> -
> -	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> -}
> -
> -/**
> - * pci_enable_acs - enable ACS if hardware support it
> - * @dev: the PCI device
> - */
> -void pci_enable_acs(struct pci_dev *dev)
> -{
> -	if (!pci_acs_enable)
> -		goto disable_acs_redir;
> -
> -	if (!pci_dev_specific_enable_acs(dev))
> -		goto disable_acs_redir;
> -
> -	pci_std_enable_acs(dev);
> -
> -disable_acs_redir:
> -	/*
> -	 * Note: pci_disable_acs_redir() must be called even if ACS was not
> -	 * enabled by the kernel because it may have been enabled by
> -	 * platform firmware.  So if we are told to disable it, we should
> -	 * always disable it after setting the kernel's default
> -	 * preferences.
> -	 */
> -	pci_disable_acs_redir(dev);
> -}
> -
>  static bool pci_acs_flags_enabled(struct pci_dev *pdev, u16 acs_flags)
>  {
>  	int pos;
> -- 
> 2.27.0.212.ge8ba1cc988-goog
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
  2020-07-10 20:29   ` Bjorn Helgaas
@ 2020-07-10 21:28     ` Raj, Ashok
  2020-07-10 22:53       ` Rajat Jain via iommu
  0 siblings, 1 reply; 16+ messages in thread
From: Raj, Ashok @ 2020-07-10 21:28 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Todd Broch, linux-pci, lalithambika.krishnakumar,
	Heikki Krogerus, Diego Rivas, Rajat Jain, Jean-Philippe Brucker,
	Furquan Shaikh, Arnd Bergmann, Saravana Kannan, linux-acpi,
	Christian Kellner, Mattias Nissler, Jesse Barnes, Len Brown,
	Rajat Jain, Prashant Malani, Suzuki K Poulose, Aaron Durbin,
	Alex Williamson, Bjorn Helgaas, Mika Westerberg, Benson Leung,
	Ashok Raj, Duncan Laurie, Greg Kroah-Hartman, Rafael J. Wysocki,
	linux-kernel, iommu, oohall, Bernie Keany, David Woodhouse,
	Alex Levin

Hi Bjorn


On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote:
> On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote:
> > When enabling ACS, enable translation blocking for external facing ports
> > and untrusted devices.
> > 
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> > v4: Add braces to avoid warning from kernel robot
> >     print warning for only external-facing devices.
> > v3: print warning if ACS_TB not supported on external-facing/untrusted ports.
> >     Minor code comments fixes.
> > v2: Commit log change
> > 
> >  drivers/pci/pci.c    |  8 ++++++++
> >  drivers/pci/quirks.c | 15 +++++++++++++++
> >  2 files changed, 23 insertions(+)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 73a8627822140..a5a6bea7af7ce 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> >  	/* Upstream Forwarding */
> >  	ctrl |= (cap & PCI_ACS_UF);
> >  
> > +	/* Enable Translation Blocking for external devices */
> > +	if (dev->external_facing || dev->untrusted) {
> > +		if (cap & PCI_ACS_TB)
> > +			ctrl |= PCI_ACS_TB;
> > +		else if (dev->external_facing)
> > +			pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n");
> > +	}
> 
> IIUC, this means that external devices can *never* use ATS and can
> never cache translations.  And (I guess, I'm not an expert) it can
> also never use the Page Request Services?

Yep, sounds like it.

> 
> Is this what we want?  Do we have any idea how many external devices
> this will affect or how much of a performance impact they will see?
> 
> Do we need some kind of override or mechanism to authenticate certain
> devices so they can use ATS and PRI?

Sounds like we would need some form of an allow-list to start with so we
can have something in the interim. 

I suppose a future platform might have a facilty to ensure ATS is secure and
authenticated we could enable for all of devices in the system, in addition
to PCI CMA/IDE. 

I think having a global override to enable all devices so platform can
switch to current behavior, or maybe via a cmdline switch.. as much as we
have a billion of those, it still gives an option in case someone needs it.



> 
> If we do decide this is the right thing to do, I think we need to
> expand the commit log a bit, because this is potentially a significant
> user-visible change.
> 
> >  	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> >  }
> >  
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index b341628e47527..bb22b46c1d719 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev)
> >  	}
> >  }
> >  
> > +/*
> > + * Currently this quirk does the equivalent of
> > + * PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF
> > + *
> > + * TODO: This quirk also needs to do equivalent of PCI_ACS_TB,
> > + * if dev->external_facing || dev->untrusted
> > + */
> >  static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev)
> >  {
> >  	if (!pci_quirk_intel_pch_acs_match(dev))
> > @@ -4973,6 +4980,14 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev)
> >  	ctrl |= (cap & PCI_ACS_CR);
> >  	ctrl |= (cap & PCI_ACS_UF);
> >  
> > +	/* Enable Translation Blocking for external devices */
> > +	if (dev->external_facing || dev->untrusted) {
> > +		if (cap & PCI_ACS_TB)
> > +			ctrl |= PCI_ACS_TB;
> > +		else if (dev->external_facing)
> > +			pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n");
> > +	}
> > +
> >  	pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl);
> >  
> >  	pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n");
> > -- 
> > 2.27.0.212.ge8ba1cc988-goog
> > 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
  2020-07-10 21:28     ` Raj, Ashok
@ 2020-07-10 22:53       ` Rajat Jain via iommu
  2020-07-10 23:32         ` Bjorn Helgaas
  2020-07-11 19:53         ` Bjorn Helgaas
  0 siblings, 2 replies; 16+ messages in thread
From: Rajat Jain via iommu @ 2020-07-10 22:53 UTC (permalink / raw)
  To: Raj, Ashok
  Cc: Todd Broch, linux-pci, Krishnakumar, Lalithambika,
	Heikki Krogerus, Diego Rivas, Jean-Philippe Brucker,
	Furquan Shaikh, Arnd Bergmann, Saravana Kannan,
	ACPI Devel Maling List, Bjorn Helgaas, Christian Kellner,
	Mattias Nissler, Jesse Barnes, Len Brown, Rajat Jain,
	Prashant Malani, Suzuki K Poulose, Aaron Durbin, Alex Williamson,
	Bjorn Helgaas, Mika Westerberg, Bernie Keany, Duncan Laurie,
	Greg Kroah-Hartman, Rafael J. Wysocki, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	Oliver O'Halloran, Benson Leung, David Woodhouse, Alex Levin

Hello,

On Fri, Jul 10, 2020 at 2:29 PM Raj, Ashok <ashok.raj@intel.com> wrote:
>
> Hi Bjorn
>
>
> On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote:
> > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote:
> > > When enabling ACS, enable translation blocking for external facing ports
> > > and untrusted devices.
> > >
> > > Signed-off-by: Rajat Jain <rajatja@google.com>
> > > ---
> > > v4: Add braces to avoid warning from kernel robot
> > >     print warning for only external-facing devices.
> > > v3: print warning if ACS_TB not supported on external-facing/untrusted ports.
> > >     Minor code comments fixes.
> > > v2: Commit log change
> > >
> > >  drivers/pci/pci.c    |  8 ++++++++
> > >  drivers/pci/quirks.c | 15 +++++++++++++++
> > >  2 files changed, 23 insertions(+)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 73a8627822140..a5a6bea7af7ce 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> > >     /* Upstream Forwarding */
> > >     ctrl |= (cap & PCI_ACS_UF);
> > >
> > > +   /* Enable Translation Blocking for external devices */
> > > +   if (dev->external_facing || dev->untrusted) {
> > > +           if (cap & PCI_ACS_TB)
> > > +                   ctrl |= PCI_ACS_TB;
> > > +           else if (dev->external_facing)
> > > +                   pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n");
> > > +   }
> >
> > IIUC, this means that external devices can *never* use ATS
> and can
> > never cache translations.

Yes, but it already exists today (and this patch doesn't change that):
521376741b2c2 "PCI/ATS: Only enable ATS for trusted devices"

IMHO any external device trying to send ATS traffic despite having ATS
disabled should count as a bad intent. And this patch is trying to
plug that loophole, by blocking the AT traffic from devices that we do
not expect to see AT from anyway.

Do you see any case where this is not true?

>  And (I guess, I'm not an expert) it can
> > also never use the Page Request Services?
>
> Yep, sounds like it.

Yes, from spec "Address Translation Services" Rev 1.1:
"...a device that supports ATS need not support PRI, but PRI is
dependent on ATS’s capabilities."
(So no ATS = No PRI).

>
> >
> > Is this what we want?  Do we have any idea how many external devices
> > this will affect or how much of a performance impact they will see?
> >
> > Do we need some kind of override or mechanism to authenticate certain
> > devices so they can use ATS and PRI?
>
> Sounds like we would need some form of an allow-list to start with so we
> can have something in the interim.

I assume what is being referred to, is an escape hatch to enable ATS
on certain given "external-facing" ports (and devices downstream on
that port). Do we really think a *per-port* control for ATS may be
needed? I can add if there is consensus about this.

>
> I suppose a future platform might have a facilty to ensure ATS is secure and
> authenticated we could enable for all of devices in the system, in addition
> to PCI CMA/IDE.
>
> I think having a global override to enable all devices so platform can
> switch to current behavior, or maybe via a cmdline switch.. as much as we
> have a billion of those, it still gives an option in case someone needs it.

Currently:

pci.noats => No ATS on all PCI devices.
(Absense of pci.noats): ATS on all PCI devices, EXCEPT external devices.

I can look to add another parameter that is synonymous to
"trust-external-pci-devices" that can keep ATS enabled on external
ports as well. I think this is better than an allow-list of only
certain ports, because most likely an admin will trust all its
external ports, or not. Also, we can add this global override and may
be add a more granular control later, if and when really needed.

Thanks,

Rajat

>
>
>
> >
> > If we do decide this is the right thing to do, I think we need to
> > expand the commit log a bit, because this is potentially a significant
> > user-visible change.
> >
> > >     pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> > >  }
> > >
> > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > > index b341628e47527..bb22b46c1d719 100644
> > > --- a/drivers/pci/quirks.c
> > > +++ b/drivers/pci/quirks.c
> > > @@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev)
> > >     }
> > >  }
> > >
> > > +/*
> > > + * Currently this quirk does the equivalent of
> > > + * PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF
> > > + *
> > > + * TODO: This quirk also needs to do equivalent of PCI_ACS_TB,
> > > + * if dev->external_facing || dev->untrusted
> > > + */
> > >  static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev)
> > >  {
> > >     if (!pci_quirk_intel_pch_acs_match(dev))
> > > @@ -4973,6 +4980,14 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev)
> > >     ctrl |= (cap & PCI_ACS_CR);
> > >     ctrl |= (cap & PCI_ACS_UF);
> > >
> > > +   /* Enable Translation Blocking for external devices */
> > > +   if (dev->external_facing || dev->untrusted) {
> > > +           if (cap & PCI_ACS_TB)
> > > +                   ctrl |= PCI_ACS_TB;
> > > +           else if (dev->external_facing)
> > > +                   pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n");
> > > +   }
> > > +
> > >     pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl);
> > >
> > >     pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n");
> > > --
> > > 2.27.0.212.ge8ba1cc988-goog
> > >
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
  2020-07-10 22:53       ` Rajat Jain via iommu
@ 2020-07-10 23:32         ` Bjorn Helgaas
  2020-07-11 19:53         ` Bjorn Helgaas
  1 sibling, 0 replies; 16+ messages in thread
From: Bjorn Helgaas @ 2020-07-10 23:32 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Todd Broch, linux-pci, Krishnakumar, Lalithambika,
	Heikki Krogerus, Diego Rivas, Jean-Philippe Brucker,
	Furquan Shaikh, Raj, Ashok, Saravana Kannan,
	ACPI Devel Maling List, Christian Kellner, Mattias Nissler,
	Jesse Barnes, Len Brown, Rajat Jain, Prashant Malani,
	Suzuki K Poulose, Aaron Durbin, Alex Williamson, Bjorn Helgaas,
	Mika Westerberg, Bernie Keany, Duncan Laurie, Greg Kroah-Hartman,
	Rafael J. Wysocki, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	Arnd Bergmann, Oliver O'Halloran, Benson Leung,
	David Woodhouse, Alex Levin

On Fri, Jul 10, 2020 at 03:53:59PM -0700, Rajat Jain wrote:
> On Fri, Jul 10, 2020 at 2:29 PM Raj, Ashok <ashok.raj@intel.com> wrote:
> > On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote:
> > > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote:
> > > > When enabling ACS, enable translation blocking for external facing ports
> > > > and untrusted devices.
> > > >
> > > > Signed-off-by: Rajat Jain <rajatja@google.com>
> > > > ---
> > > > v4: Add braces to avoid warning from kernel robot
> > > >     print warning for only external-facing devices.
> > > > v3: print warning if ACS_TB not supported on external-facing/untrusted ports.
> > > >     Minor code comments fixes.
> > > > v2: Commit log change
> > > >
> > > >  drivers/pci/pci.c    |  8 ++++++++
> > > >  drivers/pci/quirks.c | 15 +++++++++++++++
> > > >  2 files changed, 23 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index 73a8627822140..a5a6bea7af7ce 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> > > >     /* Upstream Forwarding */
> > > >     ctrl |= (cap & PCI_ACS_UF);
> > > >
> > > > +   /* Enable Translation Blocking for external devices */
> > > > +   if (dev->external_facing || dev->untrusted) {
> > > > +           if (cap & PCI_ACS_TB)
> > > > +                   ctrl |= PCI_ACS_TB;
> > > > +           else if (dev->external_facing)
> > > > +                   pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n");
> > > > +   }
> > >
> > > IIUC, this means that external devices can *never* use ATS
> > and can
> > > never cache translations.
> 
> Yes, but it already exists today (and this patch doesn't change that):
> 521376741b2c2 "PCI/ATS: Only enable ATS for trusted devices"

If you get in the habit of using the commit reference style from
Documentation/process/submitting-patches.rst it saves me the trouble
of fixing them.  I use this:

  gsr is aliased to `git --no-pager show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"'

> IMHO any external device trying to send ATS traffic despite having
> ATS disabled should count as a bad intent. And this patch is trying
> to plug that loophole, by blocking the AT traffic from devices that
> we do not expect to see AT from anyway.

That's exactly the sort of assertion I was looking for.  If we can get
something like this explanation into the commit log, and if Ashok and
Alex are OK with this, we'll be much closer.

It sounds like this is just enforcing a restriction we already have,
i.e., enabling PCI_ACS_TB blocks translated requests from devices that
aren't supposed to be generating them.

> Do you see any case where this is not true?
> 
> >  And (I guess, I'm not an expert) it can
> > > also never use the Page Request Services?
> >
> > Yep, sounds like it.
> 
> Yes, from spec "Address Translation Services" Rev 1.1:
> "...a device that supports ATS need not support PRI, but PRI is
> dependent on ATS’s capabilities."
> (So no ATS = No PRI).
> 
> > > Is this what we want?  Do we have any idea how many external
> > > devices this will affect or how much of a performance impact
> > > they will see?
> > >
> > > Do we need some kind of override or mechanism to authenticate
> > > certain devices so they can use ATS and PRI?
> >
> > Sounds like we would need some form of an allow-list to start with
> > so we can have something in the interim.
> 
> I assume what is being referred to, is an escape hatch to enable ATS
> on certain given "external-facing" ports (and devices downstream on
> that port). Do we really think a *per-port* control for ATS may be
> needed? I can add if there is consensus about this.
> 
> > I suppose a future platform might have a facilty to ensure ATS is
> > secure and authenticated we could enable for all of devices in the
> > system, in addition to PCI CMA/IDE.
> >
> > I think having a global override to enable all devices so platform
> > can switch to current behavior, or maybe via a cmdline switch.. as
> > much as we have a billion of those, it still gives an option in
> > case someone needs it.
> 
> Currently:
> 
> pci.noats => No ATS on all PCI devices.
> (Absense of pci.noats): ATS on all PCI devices, EXCEPT external devices.

You mean the "pci=noats" kernel command line parameter, right?

> I can look to add another parameter that is synonymous to
> "trust-external-pci-devices" that can keep ATS enabled on external
> ports as well. I think this is better than an allow-list of only
> certain ports, because most likely an admin will trust all its
> external ports, or not. Also, we can add this global override and
> may be add a more granular control later, if and when really needed.

I think this would be new functionality that we don't have today, and
we don't have anything that actually *needs* it AFAIK, so I wouldn't
bother.

Bjorn
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
  2020-07-10 22:53       ` Rajat Jain via iommu
  2020-07-10 23:32         ` Bjorn Helgaas
@ 2020-07-11 19:53         ` Bjorn Helgaas
  2020-07-12  0:08           ` Rajat Jain
  2020-07-14 20:19           ` Rajat Jain via iommu
  1 sibling, 2 replies; 16+ messages in thread
From: Bjorn Helgaas @ 2020-07-11 19:53 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Todd Broch, linux-pci, Krishnakumar, Lalithambika,
	Heikki Krogerus, Diego Rivas, Jean-Philippe Brucker,
	Furquan Shaikh, Raj, Ashok, Saravana Kannan,
	ACPI Devel Maling List, Christian Kellner, Mattias Nissler,
	Jesse Barnes, Len Brown, Rajat Jain, Prashant Malani,
	Suzuki K Poulose, Aaron Durbin, Alex Williamson, Bjorn Helgaas,
	Mika Westerberg, Bernie Keany, Duncan Laurie, Greg Kroah-Hartman,
	Rafael J. Wysocki, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	Arnd Bergmann, Oliver O'Halloran, Benson Leung,
	David Woodhouse, Alex Levin

On Fri, Jul 10, 2020 at 03:53:59PM -0700, Rajat Jain wrote:
> On Fri, Jul 10, 2020 at 2:29 PM Raj, Ashok <ashok.raj@intel.com> wrote:
> > On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote:
> > > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote:
> > > > When enabling ACS, enable translation blocking for external facing ports
> > > > and untrusted devices.
> > > >
> > > > Signed-off-by: Rajat Jain <rajatja@google.com>
> > > > ---
> > > > v4: Add braces to avoid warning from kernel robot
> > > >     print warning for only external-facing devices.
> > > > v3: print warning if ACS_TB not supported on external-facing/untrusted ports.
> > > >     Minor code comments fixes.
> > > > v2: Commit log change
> > > >
> > > >  drivers/pci/pci.c    |  8 ++++++++
> > > >  drivers/pci/quirks.c | 15 +++++++++++++++
> > > >  2 files changed, 23 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index 73a8627822140..a5a6bea7af7ce 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> > > >     /* Upstream Forwarding */
> > > >     ctrl |= (cap & PCI_ACS_UF);
> > > >
> > > > +   /* Enable Translation Blocking for external devices */
> > > > +   if (dev->external_facing || dev->untrusted) {
> > > > +           if (cap & PCI_ACS_TB)
> > > > +                   ctrl |= PCI_ACS_TB;
> > > > +           else if (dev->external_facing)
> > > > +                   pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n");
> > > > +   }
> > >
> > > IIUC, this means that external devices can *never* use ATS and
> > > can never cache translations.
> 
> Yes, but it already exists today (and this patch doesn't change that):
> 521376741b2c2 "PCI/ATS: Only enable ATS for trusted devices"
> 
> IMHO any external device trying to send ATS traffic despite having ATS
> disabled should count as a bad intent. And this patch is trying to
> plug that loophole, by blocking the AT traffic from devices that we do
> not expect to see AT from anyway.

Thinking about this some more, I wonder if Linux should:

  - Explicitly disable ATS for every device at enumeration-time, e.g.,
    in pci_init_capabilities(), 

  - Enable PCI_ACS_TB for every device (not just external-facing or
    untrusted ones),

  - Disable PCI_ACS_TB for the relevant devices along the path only
    when enabling ATS.

One nice thing about doing that is that the "untrusted" test would be
only in pci_enable_ats(), and we wouldn't need one in
pci_std_enable_acs().

It's possible BIOS gives us devices with ATS enabled, and this might
break them, but that seems like something we'd want to find out about.

Bjorn
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
  2020-07-11 19:53         ` Bjorn Helgaas
@ 2020-07-12  0:08           ` Rajat Jain
  2020-07-12  2:58             ` Bjorn Helgaas
  2020-07-14 20:19           ` Rajat Jain via iommu
  1 sibling, 1 reply; 16+ messages in thread
From: Rajat Jain @ 2020-07-12  0:08 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Todd Broch, linux-pci, Krishnakumar, Lalithambika,
	Heikki Krogerus, Diego Rivas, Rajat Jain, Jean-Philippe Brucker,
	Furquan Shaikh, Raj, Ashok, Saravana Kannan,
	ACPI Devel Maling List, Christian Kellner, Mattias Nissler,
	Jesse Barnes, Len Brown, Prashant Malani, Suzuki K Poulose,
	Aaron Durbin, Alex Williamson, Bjorn Helgaas, Mika Westerberg,
	Benson Leung, Duncan Laurie, Greg Kroah-Hartman,
	Rafael J. Wysocki, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	Arnd Bergmann, Oliver O'Halloran, Bernie Keany,
	David Woodhouse, Alex Levin

On Sat, Jul 11, 2020 at 12:53 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jul 10, 2020 at 03:53:59PM -0700, Rajat Jain wrote:
> > On Fri, Jul 10, 2020 at 2:29 PM Raj, Ashok <ashok.raj@intel.com> wrote:
> > > On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote:
> > > > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote:
> > > > > When enabling ACS, enable translation blocking for external facing ports
> > > > > and untrusted devices.
> > > > >
> > > > > Signed-off-by: Rajat Jain <rajatja@google.com>
> > > > > ---
> > > > > v4: Add braces to avoid warning from kernel robot
> > > > >     print warning for only external-facing devices.
> > > > > v3: print warning if ACS_TB not supported on external-facing/untrusted ports.
> > > > >     Minor code comments fixes.
> > > > > v2: Commit log change
> > > > >
> > > > >  drivers/pci/pci.c    |  8 ++++++++
> > > > >  drivers/pci/quirks.c | 15 +++++++++++++++
> > > > >  2 files changed, 23 insertions(+)
> > > > >
> > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > index 73a8627822140..a5a6bea7af7ce 100644
> > > > > --- a/drivers/pci/pci.c
> > > > > +++ b/drivers/pci/pci.c
> > > > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> > > > >     /* Upstream Forwarding */
> > > > >     ctrl |= (cap & PCI_ACS_UF);
> > > > >
> > > > > +   /* Enable Translation Blocking for external devices */
> > > > > +   if (dev->external_facing || dev->untrusted) {
> > > > > +           if (cap & PCI_ACS_TB)
> > > > > +                   ctrl |= PCI_ACS_TB;
> > > > > +           else if (dev->external_facing)
> > > > > +                   pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n");
> > > > > +   }
> > > >
> > > > IIUC, this means that external devices can *never* use ATS and
> > > > can never cache translations.
> >
> > Yes, but it already exists today (and this patch doesn't change that):
> > 521376741b2c2 "PCI/ATS: Only enable ATS for trusted devices"
> >
> > IMHO any external device trying to send ATS traffic despite having ATS
> > disabled should count as a bad intent. And this patch is trying to
> > plug that loophole, by blocking the AT traffic from devices that we do
> > not expect to see AT from anyway.
>
> Thinking about this some more, I wonder if Linux should:
>
>   - Explicitly disable ATS for every device at enumeration-time, e.g.,
>     in pci_init_capabilities(),
>
>   - Enable PCI_ACS_TB for every device (not just external-facing or
>     untrusted ones),
>
>   - Disable PCI_ACS_TB for the relevant devices along the path only
>     when enabling ATS.
>
> One nice thing about doing that is that the "untrusted" test would be
> only in pci_enable_ats(), and we wouldn't need one in
> pci_std_enable_acs().

Yes, this could work.

I think I had thought about this but I'm blanking out on why I had
given it up. I think it was because of the possibility that some
bridges may have "Translation blocking" disabled, even if not all
their descendents were trusted enough to enable ATS on them. But now
thinking about this again, as long as we retain the policy of not
enabling ATS on external devices (and thus enable TB for sure on
them), this should not be a problem. WDYT?

>
> It's possible BIOS gives us devices with ATS enabled, and this might
> break them, but that seems like something we'd want to find out about.
>

Why would they break? We'd disable ATS on each device as we enumerate
them, so they'd be functional, just with ATS disabled until it is
enabled again on internal devices as needed. Which would be WAI
behavior?

Thanks,
,
Rajat




> Bjorn
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
  2020-07-12  0:08           ` Rajat Jain
@ 2020-07-12  2:58             ` Bjorn Helgaas
  2020-07-13  8:01               ` Joerg Roedel
  0 siblings, 1 reply; 16+ messages in thread
From: Bjorn Helgaas @ 2020-07-12  2:58 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Todd Broch, linux-pci, Krishnakumar, Lalithambika,
	Heikki Krogerus, Diego Rivas, Rajat Jain, Jean-Philippe Brucker,
	Furquan Shaikh, Raj, Ashok, Saravana Kannan,
	ACPI Devel Maling List, Christian Kellner, Mattias Nissler,
	Jesse Barnes, Len Brown, Prashant Malani, Suzuki K Poulose,
	Aaron Durbin, Alex Williamson, Bjorn Helgaas, Mika Westerberg,
	Benson Leung, Duncan Laurie, Greg Kroah-Hartman,
	Rafael J. Wysocki, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	Arnd Bergmann, Oliver O'Halloran, Bernie Keany,
	David Woodhouse, Alex Levin

On Sat, Jul 11, 2020 at 05:08:51PM -0700, Rajat Jain wrote:
> On Sat, Jul 11, 2020 at 12:53 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Jul 10, 2020 at 03:53:59PM -0700, Rajat Jain wrote:
> > > On Fri, Jul 10, 2020 at 2:29 PM Raj, Ashok <ashok.raj@intel.com> wrote:
> > > > On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote:
> > > > > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote:
> > > > > > When enabling ACS, enable translation blocking for external facing ports
> > > > > > and untrusted devices.
> > > > > >
> > > > > > Signed-off-by: Rajat Jain <rajatja@google.com>
> > > > > > ---
> > > > > > v4: Add braces to avoid warning from kernel robot
> > > > > >     print warning for only external-facing devices.
> > > > > > v3: print warning if ACS_TB not supported on external-facing/untrusted ports.
> > > > > >     Minor code comments fixes.
> > > > > > v2: Commit log change
> > > > > >
> > > > > >  drivers/pci/pci.c    |  8 ++++++++
> > > > > >  drivers/pci/quirks.c | 15 +++++++++++++++
> > > > > >  2 files changed, 23 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > > index 73a8627822140..a5a6bea7af7ce 100644
> > > > > > --- a/drivers/pci/pci.c
> > > > > > +++ b/drivers/pci/pci.c
> > > > > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> > > > > >     /* Upstream Forwarding */
> > > > > >     ctrl |= (cap & PCI_ACS_UF);
> > > > > >
> > > > > > +   /* Enable Translation Blocking for external devices */
> > > > > > +   if (dev->external_facing || dev->untrusted) {
> > > > > > +           if (cap & PCI_ACS_TB)
> > > > > > +                   ctrl |= PCI_ACS_TB;
> > > > > > +           else if (dev->external_facing)
> > > > > > +                   pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n");
> > > > > > +   }
> > > > >
> > > > > IIUC, this means that external devices can *never* use ATS and
> > > > > can never cache translations.
> > >
> > > Yes, but it already exists today (and this patch doesn't change that):
> > > 521376741b2c2 "PCI/ATS: Only enable ATS for trusted devices"
> > >
> > > IMHO any external device trying to send ATS traffic despite having ATS
> > > disabled should count as a bad intent. And this patch is trying to
> > > plug that loophole, by blocking the AT traffic from devices that we do
> > > not expect to see AT from anyway.
> >
> > Thinking about this some more, I wonder if Linux should:
> >
> >   - Explicitly disable ATS for every device at enumeration-time, e.g.,
> >     in pci_init_capabilities(),
> >
> >   - Enable PCI_ACS_TB for every device (not just external-facing or
> >     untrusted ones),
> >
> >   - Disable PCI_ACS_TB for the relevant devices along the path only
> >     when enabling ATS.
> >
> > One nice thing about doing that is that the "untrusted" test would be
> > only in pci_enable_ats(), and we wouldn't need one in
> > pci_std_enable_acs().
> 
> Yes, this could work.
> 
> I think I had thought about this but I'm blanking out on why I had
> given it up. I think it was because of the possibility that some
> bridges may have "Translation blocking" disabled, even if not all
> their descendents were trusted enough to enable ATS on them. But now
> thinking about this again, as long as we retain the policy of not
> enabling ATS on external devices (and thus enable TB for sure on
> them), this should not be a problem. WDYT?

I think I would feel better if we always enabled Translation Blocking
except when we actually need it for ATS.  But I'm not confident about
how all the pieces of ATS work, so I could be missing something.

> > It's possible BIOS gives us devices with ATS enabled, and this
> > might break them, but that seems like something we'd want to find
> > out about.
> 
> Why would they break? We'd disable ATS on each device as we
> enumerate them, so they'd be functional, just with ATS disabled
> until it is enabled again on internal devices as needed. Which would
> be WAI behavior?

If BIOS handed off with ATS enabled and we somehow relied on it being
already enabled, something might break if we start disabling ATS.
Just a theoretical possibility, doesn't seem likely to me.

Bjorn
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
  2020-07-12  2:58             ` Bjorn Helgaas
@ 2020-07-13  8:01               ` Joerg Roedel
  0 siblings, 0 replies; 16+ messages in thread
From: Joerg Roedel @ 2020-07-13  8:01 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Todd Broch, linux-pci, Krishnakumar, Lalithambika,
	Heikki Krogerus, Diego Rivas, Rajat Jain, Jean-Philippe Brucker,
	Furquan Shaikh, Raj, Ashok, Saravana Kannan,
	ACPI Devel Maling List, Christian Kellner, Mattias Nissler,
	Jesse Barnes, Len Brown, Rajat Jain, Prashant Malani,
	Suzuki K Poulose, Aaron Durbin, Alex Williamson, Bjorn Helgaas,
	Mika Westerberg, Bernie Keany, Duncan Laurie, Greg Kroah-Hartman,
	Rafael J. Wysocki, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	Arnd Bergmann, Oliver O'Halloran, Benson Leung,
	David Woodhouse, Alex Levin

On Sat, Jul 11, 2020 at 09:58:38PM -0500, Bjorn Helgaas wrote:
> If BIOS handed off with ATS enabled and we somehow relied on it being
> already enabled, something might break if we start disabling ATS.
> Just a theoretical possibility, doesn't seem likely to me.

I don't think this will be a problem. When the BIOS enables ATS for a
device it also needs to enable the IOMMU already, an we are not handling
an already enabled IOMMU (outside of kdump kernels) very well.


Regards,

	Joerg
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
  2020-07-11 19:53         ` Bjorn Helgaas
  2020-07-12  0:08           ` Rajat Jain
@ 2020-07-14 20:19           ` Rajat Jain via iommu
  2020-09-15 23:01             ` Rajat Jain via iommu
  1 sibling, 1 reply; 16+ messages in thread
From: Rajat Jain via iommu @ 2020-07-14 20:19 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Todd Broch, linux-pci, Krishnakumar, Lalithambika,
	Heikki Krogerus, Diego Rivas, Jean-Philippe Brucker,
	Furquan Shaikh, Raj, Ashok, Saravana Kannan,
	ACPI Devel Maling List, Christian Kellner, Mattias Nissler,
	Jesse Barnes, Len Brown, Rajat Jain, Prashant Malani,
	Suzuki K Poulose, Aaron Durbin, Alex Williamson, Bjorn Helgaas,
	Mika Westerberg, Bernie Keany, Duncan Laurie, Greg Kroah-Hartman,
	Rafael J. Wysocki, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	Arnd Bergmann, Oliver O'Halloran, Benson Leung,
	David Woodhouse, Alex Levin

On Sat, Jul 11, 2020 at 12:53 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jul 10, 2020 at 03:53:59PM -0700, Rajat Jain wrote:
> > On Fri, Jul 10, 2020 at 2:29 PM Raj, Ashok <ashok.raj@intel.com> wrote:
> > > On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote:
> > > > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote:
> > > > > When enabling ACS, enable translation blocking for external facing ports
> > > > > and untrusted devices.
> > > > >
> > > > > Signed-off-by: Rajat Jain <rajatja@google.com>
> > > > > ---
> > > > > v4: Add braces to avoid warning from kernel robot
> > > > >     print warning for only external-facing devices.
> > > > > v3: print warning if ACS_TB not supported on external-facing/untrusted ports.
> > > > >     Minor code comments fixes.
> > > > > v2: Commit log change
> > > > >
> > > > >  drivers/pci/pci.c    |  8 ++++++++
> > > > >  drivers/pci/quirks.c | 15 +++++++++++++++
> > > > >  2 files changed, 23 insertions(+)
> > > > >
> > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > index 73a8627822140..a5a6bea7af7ce 100644
> > > > > --- a/drivers/pci/pci.c
> > > > > +++ b/drivers/pci/pci.c
> > > > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> > > > >     /* Upstream Forwarding */
> > > > >     ctrl |= (cap & PCI_ACS_UF);
> > > > >
> > > > > +   /* Enable Translation Blocking for external devices */
> > > > > +   if (dev->external_facing || dev->untrusted) {
> > > > > +           if (cap & PCI_ACS_TB)
> > > > > +                   ctrl |= PCI_ACS_TB;
> > > > > +           else if (dev->external_facing)
> > > > > +                   pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n");
> > > > > +   }
> > > >
> > > > IIUC, this means that external devices can *never* use ATS and
> > > > can never cache translations.
> >
> > Yes, but it already exists today (and this patch doesn't change that):
> > 521376741b2c2 "PCI/ATS: Only enable ATS for trusted devices"
> >
> > IMHO any external device trying to send ATS traffic despite having ATS
> > disabled should count as a bad intent. And this patch is trying to
> > plug that loophole, by blocking the AT traffic from devices that we do
> > not expect to see AT from anyway.
>
> Thinking about this some more, I wonder if Linux should:
>
>   - Explicitly disable ATS for every device at enumeration-time, e.g.,
>     in pci_init_capabilities(),
>
>   - Enable PCI_ACS_TB for every device (not just external-facing or
>     untrusted ones),
>
>   - Disable PCI_ACS_TB for the relevant devices along the path only
>     when enabling ATS.
>
> One nice thing about doing that is that the "untrusted" test would be
> only in pci_enable_ats(), and we wouldn't need one in
> pci_std_enable_acs().

Sent out v5 with this approach here:
https://patchwork.kernel.org/patch/11663515/

Thanks,

Rajat

>
>
> It's possible BIOS gives us devices with ATS enabled, and this might
> break them, but that seems like something we'd want to find out about.
>
> Bjorn
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
  2020-07-14 20:19           ` Rajat Jain via iommu
@ 2020-09-15 23:01             ` Rajat Jain via iommu
  0 siblings, 0 replies; 16+ messages in thread
From: Rajat Jain via iommu @ 2020-09-15 23:01 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Todd Broch, linux-pci, Krishnakumar, Lalithambika,
	Heikki Krogerus, Diego Rivas, Jean-Philippe Brucker,
	Furquan Shaikh, Raj, Ashok, Saravana Kannan,
	ACPI Devel Maling List, Christian Kellner, Mattias Nissler,
	Jesse Barnes, Len Brown, Rajat Jain, Prashant Malani,
	Suzuki K Poulose, Aaron Durbin, Alex Williamson, Bjorn Helgaas,
	Mika Westerberg, Bernie Keany, Duncan Laurie, Greg Kroah-Hartman,
	Rafael J. Wysocki, Linux Kernel Mailing List,
	open list:AMD IOMMU (AMD-VI),
	Arnd Bergmann, Oliver O'Halloran, Benson Leung,
	David Woodhouse, Alex Levin

Hello Bjorn,


On Tue, Jul 14, 2020 at 1:19 PM Rajat Jain <rajatja@google.com> wrote:
>
> On Sat, Jul 11, 2020 at 12:53 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Fri, Jul 10, 2020 at 03:53:59PM -0700, Rajat Jain wrote:
> > > On Fri, Jul 10, 2020 at 2:29 PM Raj, Ashok <ashok.raj@intel.com> wrote:
> > > > On Fri, Jul 10, 2020 at 03:29:22PM -0500, Bjorn Helgaas wrote:
> > > > > On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote:
> > > > > > When enabling ACS, enable translation blocking for external facing ports
> > > > > > and untrusted devices.
> > > > > >
> > > > > > Signed-off-by: Rajat Jain <rajatja@google.com>
> > > > > > ---
> > > > > > v4: Add braces to avoid warning from kernel robot
> > > > > >     print warning for only external-facing devices.
> > > > > > v3: print warning if ACS_TB not supported on external-facing/untrusted ports.
> > > > > >     Minor code comments fixes.
> > > > > > v2: Commit log change
> > > > > >
> > > > > >  drivers/pci/pci.c    |  8 ++++++++
> > > > > >  drivers/pci/quirks.c | 15 +++++++++++++++
> > > > > >  2 files changed, 23 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > > index 73a8627822140..a5a6bea7af7ce 100644
> > > > > > --- a/drivers/pci/pci.c
> > > > > > +++ b/drivers/pci/pci.c
> > > > > > @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> > > > > >     /* Upstream Forwarding */
> > > > > >     ctrl |= (cap & PCI_ACS_UF);
> > > > > >
> > > > > > +   /* Enable Translation Blocking for external devices */
> > > > > > +   if (dev->external_facing || dev->untrusted) {
> > > > > > +           if (cap & PCI_ACS_TB)
> > > > > > +                   ctrl |= PCI_ACS_TB;
> > > > > > +           else if (dev->external_facing)
> > > > > > +                   pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n");
> > > > > > +   }
> > > > >
> > > > > IIUC, this means that external devices can *never* use ATS and
> > > > > can never cache translations.
> > >
> > > Yes, but it already exists today (and this patch doesn't change that):
> > > 521376741b2c2 "PCI/ATS: Only enable ATS for trusted devices"
> > >
> > > IMHO any external device trying to send ATS traffic despite having ATS
> > > disabled should count as a bad intent. And this patch is trying to
> > > plug that loophole, by blocking the AT traffic from devices that we do
> > > not expect to see AT from anyway.
> >
> > Thinking about this some more, I wonder if Linux should:
> >
> >   - Explicitly disable ATS for every device at enumeration-time, e.g.,
> >     in pci_init_capabilities(),
> >
> >   - Enable PCI_ACS_TB for every device (not just external-facing or
> >     untrusted ones),
> >
> >   - Disable PCI_ACS_TB for the relevant devices along the path only
> >     when enabling ATS.
> >
> > One nice thing about doing that is that the "untrusted" test would be
> > only in pci_enable_ats(), and we wouldn't need one in
> > pci_std_enable_acs().
>
> Sent out v5 with this approach here:
> https://patchwork.kernel.org/patch/11663515/

Any feedback on the patch above? It has been waiting for feedback....

Thanks & Best Regards,,

Rajat


>
> Thanks,
>
> Rajat
>
> >
> >
> > It's possible BIOS gives us devices with ATS enabled, and this might
> > break them, but that seems like something we'd want to find out about.
> >
> > Bjorn
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices
  2020-07-07 22:46 ` [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices Rajat Jain via iommu
  2020-07-10 20:29   ` Bjorn Helgaas
@ 2020-09-16 21:46   ` Bjorn Helgaas
  1 sibling, 0 replies; 16+ messages in thread
From: Bjorn Helgaas @ 2020-09-16 21:46 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Todd Broch, linux-pci, lalithambika.krishnakumar,
	Heikki Krogerus, Diego Rivas, Jean-Philippe Brucker,
	Furquan Shaikh, Raj Ashok, Saravana Kannan, linux-acpi,
	Christian Kellner, Mattias Nissler, Jesse Barnes, Len Brown,
	Rajat Jain, Prashant Malani, Suzuki K Poulose, Aaron Durbin,
	Alex Williamson, Bjorn Helgaas, Mika Westerberg, Bernie Keany,
	Duncan Laurie, Greg Kroah-Hartman, Rafael J. Wysocki,
	linux-kernel, iommu, Arnd Bergmann, oohall, Benson Leung,
	David Woodhouse, Alex Levin

On Tue, Jul 07, 2020 at 03:46:04PM -0700, Rajat Jain wrote:
> When enabling ACS, enable translation blocking for external facing ports
> and untrusted devices.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>

Applied (slightly modified) to pci/acs for v5.10, thanks!

I think the warning is superfluous because every external_facing
device is a Root Port or Switch Downstream Port, and if those support
ACS at all, they are required to support Translation Blocking.  So we
should only see the warning if the device is defective, and I don't
think we need to go out of our way to look for those.

> ---
> v4: Add braces to avoid warning from kernel robot
>     print warning for only external-facing devices.
> v3: print warning if ACS_TB not supported on external-facing/untrusted ports.
>     Minor code comments fixes.
> v2: Commit log change
> 
>  drivers/pci/pci.c    |  8 ++++++++
>  drivers/pci/quirks.c | 15 +++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 73a8627822140..a5a6bea7af7ce 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -876,6 +876,14 @@ static void pci_std_enable_acs(struct pci_dev *dev)
>  	/* Upstream Forwarding */
>  	ctrl |= (cap & PCI_ACS_UF);
>  
> +	/* Enable Translation Blocking for external devices */
> +	if (dev->external_facing || dev->untrusted) {
> +		if (cap & PCI_ACS_TB)
> +			ctrl |= PCI_ACS_TB;
> +		else if (dev->external_facing)
> +			pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n");
> +	}
> +
>  	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
>  }
>  
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index b341628e47527..bb22b46c1d719 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4934,6 +4934,13 @@ static void pci_quirk_enable_intel_rp_mpc_acs(struct pci_dev *dev)
>  	}
>  }
>  
> +/*
> + * Currently this quirk does the equivalent of
> + * PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF
> + *
> + * TODO: This quirk also needs to do equivalent of PCI_ACS_TB,
> + * if dev->external_facing || dev->untrusted
> + */
>  static int pci_quirk_enable_intel_pch_acs(struct pci_dev *dev)
>  {
>  	if (!pci_quirk_intel_pch_acs_match(dev))
> @@ -4973,6 +4980,14 @@ static int pci_quirk_enable_intel_spt_pch_acs(struct pci_dev *dev)
>  	ctrl |= (cap & PCI_ACS_CR);
>  	ctrl |= (cap & PCI_ACS_UF);
>  
> +	/* Enable Translation Blocking for external devices */
> +	if (dev->external_facing || dev->untrusted) {
> +		if (cap & PCI_ACS_TB)
> +			ctrl |= PCI_ACS_TB;
> +		else if (dev->external_facing)
> +			pci_warn(dev, "ACS: No Translation Blocking on external-facing dev\n");
> +	}
> +
>  	pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl);
>  
>  	pci_info(dev, "Intel SPT PCH root port ACS workaround enabled\n");
> -- 
> 2.27.0.212.ge8ba1cc988-goog
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-09-16 21:46 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-07 22:46 [PATCH v4 1/4] PCI: Move pci_enable_acs() and its dependencies up in pci.c Rajat Jain via iommu
2020-07-07 22:46 ` [PATCH v4 2/4] PCI: Keep the ACS capability offset in device Rajat Jain via iommu
2020-07-07 22:46 ` [PATCH v4 3/4] PCI: Treat "external-facing" devices as internal Rajat Jain via iommu
2020-07-07 22:46 ` [PATCH v4 4/4] PCI/ACS: Enable PCI_ACS_TB for untrusted/external-facing devices Rajat Jain via iommu
2020-07-10 20:29   ` Bjorn Helgaas
2020-07-10 21:28     ` Raj, Ashok
2020-07-10 22:53       ` Rajat Jain via iommu
2020-07-10 23:32         ` Bjorn Helgaas
2020-07-11 19:53         ` Bjorn Helgaas
2020-07-12  0:08           ` Rajat Jain
2020-07-12  2:58             ` Bjorn Helgaas
2020-07-13  8:01               ` Joerg Roedel
2020-07-14 20:19           ` Rajat Jain via iommu
2020-09-15 23:01             ` Rajat Jain via iommu
2020-09-16 21:46   ` Bjorn Helgaas
2020-07-10 20:39 ` [PATCH v4 1/4] PCI: Move pci_enable_acs() and its dependencies up in pci.c Bjorn Helgaas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).