linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/11] PCI/ATS: Device-tree support and other improvements
@ 2020-03-11 12:44 Jean-Philippe Brucker
  2020-03-11 12:44 ` [PATCH v2 01/11] dt-bindings: PCI: generic: Add ats-supported property Jean-Philippe Brucker
                   ` (10 more replies)
  0 siblings, 11 replies; 20+ messages in thread
From: Jean-Philippe Brucker @ 2020-03-11 12:44 UTC (permalink / raw)
  To: bhelgaas, will, robh+dt, joro, baolu.lu, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu
  Cc: lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list,
	Jean-Philippe Brucker

Enable ATS on device-tree based systems, and factor the common ATS
enablement checks into pci_enable_ats().

Since v1 [1] I added acks and review tags, simplified patch 3 and tried
to clarify the comment in patch 2.

I'd like acks or comments on the following patches:
* PCI on patches 2, 3 and 5
* Arm SMMUv3 on patch 7
* Intel VT-d on patch 8
* arm64 DT on patch 10 

Thanks,
Jean

[1] https://lore.kernel.org/linux-iommu/20200213165049.508908-1-jean-philippe@linaro.org/

Jean-Philippe Brucker (11):
  dt-bindings: PCI: generic: Add ats-supported property
  PCI: Add ats_supported host bridge flag
  PCI: OF: Check whether the host bridge supports ATS
  ACPI/IORT: Check ATS capability in root complex node
  PCI/ATS: Gather checks into pci_ats_supported()
  iommu/amd: Use pci_ats_supported()
  iommu/arm-smmu-v3: Use pci_ats_supported()
  iommu/vt-d: Use pci_ats_supported()
  ACPI/IORT: Drop ATS fwspec flag
  arm64: dts: fast models: Enable PCIe ATS for Base RevC FVP
  Documentation: Generalize the "pci=noats" boot parameter

 .../admin-guide/kernel-parameters.txt         |  4 +-
 .../bindings/pci/host-generic-pci.yaml        |  6 +++
 arch/arm64/boot/dts/arm/fvp-base-revc.dts     |  1 +
 drivers/acpi/arm64/iort.c                     | 38 +++++++++++++------
 drivers/acpi/pci_root.c                       |  3 ++
 drivers/iommu/amd_iommu.c                     | 12 ++----
 drivers/iommu/arm-smmu-v3.c                   | 18 ++-------
 drivers/iommu/intel-iommu.c                   |  9 ++---
 drivers/pci/ats.c                             | 30 ++++++++++++++-
 drivers/pci/controller/pci-host-common.c      | 11 ++++++
 drivers/pci/probe.c                           |  8 ++++
 include/linux/acpi_iort.h                     |  8 ++++
 include/linux/iommu.h                         |  4 --
 include/linux/pci-ats.h                       |  3 ++
 include/linux/pci.h                           |  1 +
 15 files changed, 109 insertions(+), 47 deletions(-)

-- 
2.25.1


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

* [PATCH v2 01/11] dt-bindings: PCI: generic: Add ats-supported property
  2020-03-11 12:44 [PATCH v2 00/11] PCI/ATS: Device-tree support and other improvements Jean-Philippe Brucker
@ 2020-03-11 12:44 ` Jean-Philippe Brucker
  2020-03-11 12:44 ` [PATCH v2 02/11] PCI: Add ats_supported host bridge flag Jean-Philippe Brucker
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Jean-Philippe Brucker @ 2020-03-11 12:44 UTC (permalink / raw)
  To: bhelgaas, will, robh+dt, joro, baolu.lu, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu
  Cc: lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list,
	Jean-Philippe Brucker, Rob Herring

Add a way for firmware to tell the OS that ATS is supported by the PCI
root complex. An endpoint with ATS enabled may send Translation Requests
and Translated Memory Requests, which look just like Normal Memory
Requests with a non-zero AT field. So a root controller that ignores the
AT field may simply forward the request to the IOMMU as a Normal Memory
Request, which could end badly. In any case, the endpoint will be
unusable.

The ats-supported property allows the OS to only enable ATS in endpoints
if the root controller can handle ATS requests. Only add the property to
pcie-host-ecam-generic for the moment. For non-generic root controllers,
availability of ATS can be inferred from the compatible string.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 Documentation/devicetree/bindings/pci/host-generic-pci.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml
index 47353d0cd394..7d40edd7f1ef 100644
--- a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml
+++ b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml
@@ -107,6 +107,12 @@ properties:
 
   dma-coherent: true
 
+  ats-supported:
+    description:
+      Indicates that a PCIe host controller supports ATS, and can handle Memory
+      Requests with Address Type (AT).
+    type: boolean
+
 required:
   - compatible
   - reg
-- 
2.25.1


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

* [PATCH v2 02/11] PCI: Add ats_supported host bridge flag
  2020-03-11 12:44 [PATCH v2 00/11] PCI/ATS: Device-tree support and other improvements Jean-Philippe Brucker
  2020-03-11 12:44 ` [PATCH v2 01/11] dt-bindings: PCI: generic: Add ats-supported property Jean-Philippe Brucker
@ 2020-03-11 12:44 ` Jean-Philippe Brucker
  2020-03-12 21:21   ` Bjorn Helgaas
  2020-03-11 12:44 ` [PATCH v2 03/11] PCI: OF: Check whether the host bridge supports ATS Jean-Philippe Brucker
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 20+ messages in thread
From: Jean-Philippe Brucker @ 2020-03-11 12:44 UTC (permalink / raw)
  To: bhelgaas, will, robh+dt, joro, baolu.lu, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu
  Cc: lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list,
	Jean-Philippe Brucker

Each vendor has their own way of describing whether a host bridge
supports ATS.  The Intel and AMD ACPI tables selectively enable or
disable ATS per device or sub-tree, while Arm has a single bit for each
host bridge.  For those that need it, add an ats_supported bit to the
host bridge structure.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
v1->v2: try to improve the comment
---
 drivers/pci/probe.c | 8 ++++++++
 include/linux/pci.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 512cb4312ddd..b5e36f06b40a 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -598,6 +598,14 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge)
 	bridge->native_shpc_hotplug = 1;
 	bridge->native_pme = 1;
 	bridge->native_ltr = 1;
+
+	/*
+	 * Some systems (ACPI IORT, device-tree) declare ATS support at the host
+	 * bridge, and clear this bit when ATS isn't supported. Others (ACPI
+	 * DMAR and IVRS) declare ATS support with a smaller granularity, and
+	 * need this bit set.
+	 */
+	bridge->ats_supported = 1;
 }
 
 struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 3840a541a9de..9fe2e84d74d7 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -511,6 +511,7 @@ struct pci_host_bridge {
 	unsigned int	native_pme:1;		/* OS may use PCIe PME */
 	unsigned int	native_ltr:1;		/* OS may use PCIe LTR */
 	unsigned int	preserve_config:1;	/* Preserve FW resource setup */
+	unsigned int	ats_supported:1;
 
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
-- 
2.25.1


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

* [PATCH v2 03/11] PCI: OF: Check whether the host bridge supports ATS
  2020-03-11 12:44 [PATCH v2 00/11] PCI/ATS: Device-tree support and other improvements Jean-Philippe Brucker
  2020-03-11 12:44 ` [PATCH v2 01/11] dt-bindings: PCI: generic: Add ats-supported property Jean-Philippe Brucker
  2020-03-11 12:44 ` [PATCH v2 02/11] PCI: Add ats_supported host bridge flag Jean-Philippe Brucker
@ 2020-03-11 12:44 ` Jean-Philippe Brucker
  2020-03-12 20:45   ` Bjorn Helgaas
  2020-03-11 12:44 ` [PATCH v2 04/11] ACPI/IORT: Check ATS capability in root complex node Jean-Philippe Brucker
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 20+ messages in thread
From: Jean-Philippe Brucker @ 2020-03-11 12:44 UTC (permalink / raw)
  To: bhelgaas, will, robh+dt, joro, baolu.lu, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu
  Cc: lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list,
	Jean-Philippe Brucker

When setting up a generic host on a device-tree based system, copy the
ats-supported flag into the pci_host_bridge structure.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
v1->v2: keep the helper in pci-host-common.c
---
 drivers/pci/controller/pci-host-common.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
index 250a3fc80ec6..2e800bc6ae7a 100644
--- a/drivers/pci/controller/pci-host-common.c
+++ b/drivers/pci/controller/pci-host-common.c
@@ -54,6 +54,16 @@ static struct pci_config_window *gen_pci_init(struct device *dev,
 	return ERR_PTR(err);
 }
 
+static void of_pci_host_check_ats(struct pci_host_bridge *bridge)
+{
+	struct device_node *np = bridge->bus->dev.of_node;
+
+	if (!np)
+		return;
+
+	bridge->ats_supported = of_property_read_bool(np, "ats-supported");
+}
+
 int pci_host_common_probe(struct platform_device *pdev,
 			  struct pci_ecam_ops *ops)
 {
@@ -92,6 +102,7 @@ int pci_host_common_probe(struct platform_device *pdev,
 		return ret;
 	}
 
+	of_pci_host_check_ats(bridge);
 	platform_set_drvdata(pdev, bridge->bus);
 	return 0;
 }
-- 
2.25.1


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

* [PATCH v2 04/11] ACPI/IORT: Check ATS capability in root complex node
  2020-03-11 12:44 [PATCH v2 00/11] PCI/ATS: Device-tree support and other improvements Jean-Philippe Brucker
                   ` (2 preceding siblings ...)
  2020-03-11 12:44 ` [PATCH v2 03/11] PCI: OF: Check whether the host bridge supports ATS Jean-Philippe Brucker
@ 2020-03-11 12:44 ` Jean-Philippe Brucker
  2020-03-12 20:58   ` Bjorn Helgaas
  2020-03-11 12:45 ` [PATCH v2 05/11] PCI/ATS: Gather checks into pci_ats_supported() Jean-Philippe Brucker
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 20+ messages in thread
From: Jean-Philippe Brucker @ 2020-03-11 12:44 UTC (permalink / raw)
  To: bhelgaas, will, robh+dt, joro, baolu.lu, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu
  Cc: lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list,
	Jean-Philippe Brucker

When initializing a PCI root bridge, copy its "ATS supported" attribute
into the root bridge.

Acked-by: Hanjun Guo <guohanjun@huawei.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/acpi/arm64/iort.c | 27 +++++++++++++++++++++++++++
 drivers/acpi/pci_root.c   |  3 +++
 include/linux/acpi_iort.h |  8 ++++++++
 3 files changed, 38 insertions(+)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index ed3d2d1a7ae9..d99d7f5b51e1 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -1633,6 +1633,33 @@ static void __init iort_enable_acs(struct acpi_iort_node *iort_node)
 		}
 	}
 }
+
+static acpi_status iort_match_host_bridge_callback(struct acpi_iort_node *node,
+						   void *context)
+{
+	struct acpi_iort_root_complex *pci_rc;
+	struct pci_host_bridge *host_bridge = context;
+
+	pci_rc = (struct acpi_iort_root_complex *)node->node_data;
+
+	return pci_domain_nr(host_bridge->bus) == pci_rc->pci_segment_number ?
+		AE_OK : AE_NOT_FOUND;
+}
+
+void iort_pci_host_bridge_setup(struct pci_host_bridge *host_bridge)
+{
+	struct acpi_iort_node *node;
+	struct acpi_iort_root_complex *pci_rc;
+
+	node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
+			      iort_match_host_bridge_callback, host_bridge);
+	if (!node)
+		return;
+
+	pci_rc = (struct acpi_iort_root_complex *)node->node_data;
+	host_bridge->ats_supported = !!(pci_rc->ats_attribute &
+					ACPI_IORT_ATS_SUPPORTED);
+}
 #else
 static inline void iort_enable_acs(struct acpi_iort_node *iort_node) { }
 #endif
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index d1e666ef3fcc..eb2fb8f17c0b 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -6,6 +6,7 @@
  *  Copyright (C) 2001, 2002 Paul Diefenbaugh <paul.s.diefenbaugh@intel.com>
  */
 
+#include <linux/acpi_iort.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -917,6 +918,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
 		host_bridge->native_ltr = 0;
 
+	iort_pci_host_bridge_setup(host_bridge);
+
 	/*
 	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
 	 * exists and returns 0, we must preserve any PCI resource
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index 8e7e2ec37f1b..7b06871cc3aa 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -10,6 +10,7 @@
 #include <linux/acpi.h>
 #include <linux/fwnode.h>
 #include <linux/irqdomain.h>
+#include <linux/pci.h>
 
 #define IORT_IRQ_MASK(irq)		(irq & 0xffffffffULL)
 #define IORT_IRQ_TRIGGER_MASK(irq)	((irq >> 32) & 0xffffffffULL)
@@ -55,4 +56,11 @@ int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head)
 { return 0; }
 #endif
 
+#if defined(CONFIG_ACPI_IORT) && defined(CONFIG_PCI)
+void iort_pci_host_bridge_setup(struct pci_host_bridge *host_bridge);
+#else
+static inline
+void iort_pci_host_bridge_setup(struct pci_host_bridge *host_bridge) { }
+#endif
+
 #endif /* __ACPI_IORT_H__ */
-- 
2.25.1


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

* [PATCH v2 05/11] PCI/ATS: Gather checks into pci_ats_supported()
  2020-03-11 12:44 [PATCH v2 00/11] PCI/ATS: Device-tree support and other improvements Jean-Philippe Brucker
                   ` (3 preceding siblings ...)
  2020-03-11 12:44 ` [PATCH v2 04/11] ACPI/IORT: Check ATS capability in root complex node Jean-Philippe Brucker
@ 2020-03-11 12:45 ` Jean-Philippe Brucker
  2020-03-12 21:01   ` Bjorn Helgaas
  2020-03-11 12:45 ` [PATCH v2 06/11] iommu/amd: Use pci_ats_supported() Jean-Philippe Brucker
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 20+ messages in thread
From: Jean-Philippe Brucker @ 2020-03-11 12:45 UTC (permalink / raw)
  To: bhelgaas, will, robh+dt, joro, baolu.lu, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu
  Cc: lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list,
	Jean-Philippe Brucker

IOMMU drivers need to perform several tests when checking if a device
supports ATS.  Move them all into a new function that returns true when
a device and its host bridge support ATS.

Since pci_enable_ats() now calls pci_ats_supported(), the following
new checks are now common:
* whether a device is trusted.  Devices plugged into external-facing
  ports such as thunderbolt are untrusted.
* whether the host bridge supports ATS, which defaults to true unless
  the firmware description states that ATS isn't supported by the host
  bridge.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/pci/ats.c       | 30 +++++++++++++++++++++++++++++-
 include/linux/pci-ats.h |  3 +++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 390e92f2d8d1..bbfd0d42b8b9 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -30,6 +30,34 @@ void pci_ats_init(struct pci_dev *dev)
 	dev->ats_cap = pos;
 }
 
+/**
+ * pci_ats_supported - check if the device can use ATS
+ * @dev: the PCI device
+ *
+ * Returns true if the device supports ATS and is allowed to use it, false
+ * otherwise.
+ */
+bool pci_ats_supported(struct pci_dev *dev)
+{
+	struct pci_host_bridge *bridge;
+
+	if (!dev->ats_cap)
+		return false;
+
+	if (dev->untrusted)
+		return false;
+
+	bridge = pci_find_host_bridge(dev->bus);
+	if (!bridge)
+		return false;
+
+	if (!bridge->ats_supported)
+		return false;
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(pci_ats_supported);
+
 /**
  * pci_enable_ats - enable the ATS capability
  * @dev: the PCI device
@@ -42,7 +70,7 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
 	u16 ctrl;
 	struct pci_dev *pdev;
 
-	if (!dev->ats_cap)
+	if (!pci_ats_supported(dev))
 		return -EINVAL;
 
 	if (WARN_ON(dev->ats_enabled))
diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
index d08f0869f121..f75c307f346d 100644
--- a/include/linux/pci-ats.h
+++ b/include/linux/pci-ats.h
@@ -6,11 +6,14 @@
 
 #ifdef CONFIG_PCI_ATS
 /* Address Translation Service */
+bool pci_ats_supported(struct pci_dev *dev);
 int pci_enable_ats(struct pci_dev *dev, int ps);
 void pci_disable_ats(struct pci_dev *dev);
 int pci_ats_queue_depth(struct pci_dev *dev);
 int pci_ats_page_aligned(struct pci_dev *dev);
 #else /* CONFIG_PCI_ATS */
+static inline bool pci_ats_supported(struct pci_dev *d)
+{ return false; }
 static inline int pci_enable_ats(struct pci_dev *d, int ps)
 { return -ENODEV; }
 static inline void pci_disable_ats(struct pci_dev *d) { }
-- 
2.25.1


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

* [PATCH v2 06/11] iommu/amd: Use pci_ats_supported()
  2020-03-11 12:44 [PATCH v2 00/11] PCI/ATS: Device-tree support and other improvements Jean-Philippe Brucker
                   ` (4 preceding siblings ...)
  2020-03-11 12:45 ` [PATCH v2 05/11] PCI/ATS: Gather checks into pci_ats_supported() Jean-Philippe Brucker
@ 2020-03-11 12:45 ` Jean-Philippe Brucker
  2020-03-11 12:45 ` [PATCH v2 07/11] iommu/arm-smmu-v3: " Jean-Philippe Brucker
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Jean-Philippe Brucker @ 2020-03-11 12:45 UTC (permalink / raw)
  To: bhelgaas, will, robh+dt, joro, baolu.lu, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu
  Cc: lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list,
	Jean-Philippe Brucker

The pci_ats_supported() function checks if a device supports ATS and is
allowed to use it.  In addition to checking that the device has an ATS
capability and that the global pci=noats is not set
(pci_ats_disabled()), it also checks if a device is untrusted (plugged
into an external-facing port such as thunderbolt).

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/iommu/amd_iommu.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index aac132bd1ef0..084f0b2e132e 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -291,16 +291,15 @@ static struct iommu_group *acpihid_device_group(struct device *dev)
 static bool pci_iommuv2_capable(struct pci_dev *pdev)
 {
 	static const int caps[] = {
-		PCI_EXT_CAP_ID_ATS,
 		PCI_EXT_CAP_ID_PRI,
 		PCI_EXT_CAP_ID_PASID,
 	};
 	int i, pos;
 
-	if (pci_ats_disabled())
+	if (!pci_ats_supported(pdev))
 		return false;
 
-	for (i = 0; i < 3; ++i) {
+	for (i = 0; i < 2; ++i) {
 		pos = pci_find_ext_capability(pdev, caps[i]);
 		if (pos == 0)
 			return false;
@@ -3040,11 +3039,8 @@ int amd_iommu_device_info(struct pci_dev *pdev,
 
 	memset(info, 0, sizeof(*info));
 
-	if (!pci_ats_disabled()) {
-		pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ATS);
-		if (pos)
-			info->flags |= AMD_IOMMU_DEVICE_FLAG_ATS_SUP;
-	}
+	if (pci_ats_supported(pdev))
+		info->flags |= AMD_IOMMU_DEVICE_FLAG_ATS_SUP;
 
 	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
 	if (pos)
-- 
2.25.1


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

* [PATCH v2 07/11] iommu/arm-smmu-v3: Use pci_ats_supported()
  2020-03-11 12:44 [PATCH v2 00/11] PCI/ATS: Device-tree support and other improvements Jean-Philippe Brucker
                   ` (5 preceding siblings ...)
  2020-03-11 12:45 ` [PATCH v2 06/11] iommu/amd: Use pci_ats_supported() Jean-Philippe Brucker
@ 2020-03-11 12:45 ` Jean-Philippe Brucker
  2020-03-18 21:49   ` Will Deacon
  2020-03-11 12:45 ` [PATCH v2 08/11] iommu/vt-d: " Jean-Philippe Brucker
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 20+ messages in thread
From: Jean-Philippe Brucker @ 2020-03-11 12:45 UTC (permalink / raw)
  To: bhelgaas, will, robh+dt, joro, baolu.lu, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu
  Cc: lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list,
	Jean-Philippe Brucker

The new pci_ats_supported() function checks if a device supports ATS and
is allowed to use it.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/iommu/arm-smmu-v3.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 4f0a38dae6db..87ae31ef35a1 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2592,26 +2592,14 @@ static void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master)
 	}
 }
 
-#ifdef CONFIG_PCI_ATS
 static bool arm_smmu_ats_supported(struct arm_smmu_master *master)
 {
-	struct pci_dev *pdev;
+	struct device *dev = master->dev;
 	struct arm_smmu_device *smmu = master->smmu;
-	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(master->dev);
-
-	if (!(smmu->features & ARM_SMMU_FEAT_ATS) || !dev_is_pci(master->dev) ||
-	    !(fwspec->flags & IOMMU_FWSPEC_PCI_RC_ATS) || pci_ats_disabled())
-		return false;
 
-	pdev = to_pci_dev(master->dev);
-	return !pdev->untrusted && pdev->ats_cap;
+	return (smmu->features & ARM_SMMU_FEAT_ATS) && dev_is_pci(dev) &&
+		pci_ats_supported(to_pci_dev(dev));
 }
-#else
-static bool arm_smmu_ats_supported(struct arm_smmu_master *master)
-{
-	return false;
-}
-#endif
 
 static void arm_smmu_enable_ats(struct arm_smmu_master *master)
 {
-- 
2.25.1


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

* [PATCH v2 08/11] iommu/vt-d: Use pci_ats_supported()
  2020-03-11 12:44 [PATCH v2 00/11] PCI/ATS: Device-tree support and other improvements Jean-Philippe Brucker
                   ` (6 preceding siblings ...)
  2020-03-11 12:45 ` [PATCH v2 07/11] iommu/arm-smmu-v3: " Jean-Philippe Brucker
@ 2020-03-11 12:45 ` Jean-Philippe Brucker
  2020-03-12  1:44   ` Lu Baolu
  2020-03-11 12:45 ` [PATCH v2 09/11] ACPI/IORT: Drop ATS fwspec flag Jean-Philippe Brucker
                   ` (2 subsequent siblings)
  10 siblings, 1 reply; 20+ messages in thread
From: Jean-Philippe Brucker @ 2020-03-11 12:45 UTC (permalink / raw)
  To: bhelgaas, will, robh+dt, joro, baolu.lu, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu
  Cc: lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list,
	Jean-Philippe Brucker

The pci_ats_supported() function checks if a device supports ATS and is
allowed to use it.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/iommu/intel-iommu.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 6fa6de2b6ad5..17208280ef5c 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1454,8 +1454,7 @@ static void iommu_enable_dev_iotlb(struct device_domain_info *info)
 	    !pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32))
 		info->pri_enabled = 1;
 #endif
-	if (!pdev->untrusted && info->ats_supported &&
-	    pci_ats_page_aligned(pdev) &&
+	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
 	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
 		info->ats_enabled = 1;
 		domain_update_iotlb(info->domain);
@@ -2611,10 +2610,8 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
 	if (dev && dev_is_pci(dev)) {
 		struct pci_dev *pdev = to_pci_dev(info->dev);
 
-		if (!pdev->untrusted &&
-		    !pci_ats_disabled() &&
-		    ecap_dev_iotlb_support(iommu->ecap) &&
-		    pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ATS) &&
+		if (ecap_dev_iotlb_support(iommu->ecap) &&
+		    pci_ats_supported(pdev) &&
 		    dmar_find_matched_atsr_unit(pdev))
 			info->ats_supported = 1;
 
-- 
2.25.1


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

* [PATCH v2 09/11] ACPI/IORT: Drop ATS fwspec flag
  2020-03-11 12:44 [PATCH v2 00/11] PCI/ATS: Device-tree support and other improvements Jean-Philippe Brucker
                   ` (7 preceding siblings ...)
  2020-03-11 12:45 ` [PATCH v2 08/11] iommu/vt-d: " Jean-Philippe Brucker
@ 2020-03-11 12:45 ` Jean-Philippe Brucker
  2020-03-11 12:45 ` [PATCH v2 10/11] arm64: dts: fast models: Enable PCIe ATS for Base RevC FVP Jean-Philippe Brucker
  2020-03-11 12:45 ` [PATCH v2 11/11] Documentation: Generalize the "pci=noats" boot parameter Jean-Philippe Brucker
  10 siblings, 0 replies; 20+ messages in thread
From: Jean-Philippe Brucker @ 2020-03-11 12:45 UTC (permalink / raw)
  To: bhelgaas, will, robh+dt, joro, baolu.lu, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu
  Cc: lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list,
	Jean-Philippe Brucker

Now that the ats_supported flag is in the host bridge structure where it
belongs, we can remove it from the per-device fwspec structure.

Acked-by: Hanjun Guo <guohanjun@huawei.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/acpi/arm64/iort.c | 11 -----------
 include/linux/iommu.h     |  4 ----
 2 files changed, 15 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index d99d7f5b51e1..f634641b3699 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -924,14 +924,6 @@ static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
 	return ret;
 }
 
-static bool iort_pci_rc_supports_ats(struct acpi_iort_node *node)
-{
-	struct acpi_iort_root_complex *pci_rc;
-
-	pci_rc = (struct acpi_iort_root_complex *)node->node_data;
-	return pci_rc->ats_attribute & ACPI_IORT_ATS_SUPPORTED;
-}
-
 static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
 			    u32 streamid)
 {
@@ -1026,9 +1018,6 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev)
 		info.node = node;
 		err = pci_for_each_dma_alias(to_pci_dev(dev),
 					     iort_pci_iommu_init, &info);
-
-		if (!err && iort_pci_rc_supports_ats(node))
-			dev->iommu_fwspec->flags |= IOMMU_FWSPEC_PCI_RC_ATS;
 	} else {
 		int i = 0;
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index d1b5f4d98569..1739f8a7a4b4 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -589,15 +589,11 @@ struct iommu_fwspec {
 	const struct iommu_ops	*ops;
 	struct fwnode_handle	*iommu_fwnode;
 	void			*iommu_priv;
-	u32			flags;
 	u32			num_pasid_bits;
 	unsigned int		num_ids;
 	u32			ids[1];
 };
 
-/* ATS is supported */
-#define IOMMU_FWSPEC_PCI_RC_ATS			(1 << 0)
-
 /**
  * struct iommu_sva - handle to a device-mm bond
  */
-- 
2.25.1


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

* [PATCH v2 10/11] arm64: dts: fast models: Enable PCIe ATS for Base RevC FVP
  2020-03-11 12:44 [PATCH v2 00/11] PCI/ATS: Device-tree support and other improvements Jean-Philippe Brucker
                   ` (8 preceding siblings ...)
  2020-03-11 12:45 ` [PATCH v2 09/11] ACPI/IORT: Drop ATS fwspec flag Jean-Philippe Brucker
@ 2020-03-11 12:45 ` Jean-Philippe Brucker
  2020-03-11 12:45 ` [PATCH v2 11/11] Documentation: Generalize the "pci=noats" boot parameter Jean-Philippe Brucker
  10 siblings, 0 replies; 20+ messages in thread
From: Jean-Philippe Brucker @ 2020-03-11 12:45 UTC (permalink / raw)
  To: bhelgaas, will, robh+dt, joro, baolu.lu, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu
  Cc: lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list,
	Jean-Philippe Brucker

Declare that the host controller supports ATS, so the OS can enable it
for ATS-capable PCIe endpoints.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 arch/arm64/boot/dts/arm/fvp-base-revc.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/arm/fvp-base-revc.dts b/arch/arm64/boot/dts/arm/fvp-base-revc.dts
index 335fff762451..9171bf0254bf 100644
--- a/arch/arm64/boot/dts/arm/fvp-base-revc.dts
+++ b/arch/arm64/boot/dts/arm/fvp-base-revc.dts
@@ -170,6 +170,7 @@ pci: pci@40000000 {
 		iommu-map = <0x0 &smmu 0x0 0x10000>;
 
 		dma-coherent;
+		ats-supported;
 	};
 
 	smmu: smmu@2b400000 {
-- 
2.25.1


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

* [PATCH v2 11/11] Documentation: Generalize the "pci=noats" boot parameter
  2020-03-11 12:44 [PATCH v2 00/11] PCI/ATS: Device-tree support and other improvements Jean-Philippe Brucker
                   ` (9 preceding siblings ...)
  2020-03-11 12:45 ` [PATCH v2 10/11] arm64: dts: fast models: Enable PCIe ATS for Base RevC FVP Jean-Philippe Brucker
@ 2020-03-11 12:45 ` Jean-Philippe Brucker
  10 siblings, 0 replies; 20+ messages in thread
From: Jean-Philippe Brucker @ 2020-03-11 12:45 UTC (permalink / raw)
  To: bhelgaas, will, robh+dt, joro, baolu.lu, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu
  Cc: lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list,
	Jean-Philippe Brucker

The "pci=noats" kernel parameter disables PCIe ATS globally, and affects
any ATS-capable IOMMU driver. So rather than adding Arm SMMUv3, which
recently gained ATS support, to the list of relevant build options,
simplify the noats description.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 Documentation/admin-guide/kernel-parameters.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index c07815d230bc..3e17ddb76731 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3610,8 +3610,8 @@
 				on: Turn realloc on
 		realloc		same as realloc=on
 		noari		do not use PCIe ARI.
-		noats		[PCIE, Intel-IOMMU, AMD-IOMMU]
-				do not use PCIe ATS (and IOMMU device IOTLB).
+		noats		[PCIE] Do not use PCIe ATS (and IOMMU device
+				IOTLB).
 		pcie_scan_all	Scan all possible PCIe devices.  Otherwise we
 				only look for one device below a PCIe downstream
 				port.
-- 
2.25.1


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

* Re: [PATCH v2 08/11] iommu/vt-d: Use pci_ats_supported()
  2020-03-11 12:45 ` [PATCH v2 08/11] iommu/vt-d: " Jean-Philippe Brucker
@ 2020-03-12  1:44   ` Lu Baolu
  2020-03-12  7:54     ` Jean-Philippe Brucker
  0 siblings, 1 reply; 20+ messages in thread
From: Lu Baolu @ 2020-03-12  1:44 UTC (permalink / raw)
  To: Jean-Philippe Brucker, bhelgaas, will, robh+dt, joro,
	sudeep.holla, linux-doc, linux-pci, linux-arm-kernel, devicetree,
	linux-acpi, iommu
  Cc: baolu.lu, lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau,
	guohanjun, rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list

Hi Jean,

On 2020/3/11 20:45, Jean-Philippe Brucker wrote:
> The pci_ats_supported() function checks if a device supports ATS and is
> allowed to use it.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>   drivers/iommu/intel-iommu.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 6fa6de2b6ad5..17208280ef5c 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -1454,8 +1454,7 @@ static void iommu_enable_dev_iotlb(struct device_domain_info *info)
>   	    !pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32))
>   		info->pri_enabled = 1;
>   #endif
> -	if (!pdev->untrusted && info->ats_supported &&
> -	    pci_ats_page_aligned(pdev) &&
> +	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
>   	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
>   		info->ats_enabled = 1;
>   		domain_update_iotlb(info->domain);
> @@ -2611,10 +2610,8 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
>   	if (dev && dev_is_pci(dev)) {
>   		struct pci_dev *pdev = to_pci_dev(info->dev);
>   
> -		if (!pdev->untrusted &&
> -		    !pci_ats_disabled() &&

The pci_ats_disabled() couldn't be replaced by pci_ats_supported(). Even
pci_ats_supported() returns true, user still can disable it. Or move
ats_disabled into pci_ats_supported()?

Best regards,
baolu

> -		    ecap_dev_iotlb_support(iommu->ecap) &&
> -		    pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ATS) &&
> +		if (ecap_dev_iotlb_support(iommu->ecap) &&
> +		    pci_ats_supported(pdev) &&
>   		    dmar_find_matched_atsr_unit(pdev))
>   			info->ats_supported = 1;

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

* Re: [PATCH v2 08/11] iommu/vt-d: Use pci_ats_supported()
  2020-03-12  1:44   ` Lu Baolu
@ 2020-03-12  7:54     ` Jean-Philippe Brucker
  2020-03-12  8:18       ` Lu Baolu
  0 siblings, 1 reply; 20+ messages in thread
From: Jean-Philippe Brucker @ 2020-03-12  7:54 UTC (permalink / raw)
  To: Lu Baolu
  Cc: bhelgaas, will, robh+dt, joro, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu,
	lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list

Hi Baolu,

On Thu, Mar 12, 2020 at 09:44:16AM +0800, Lu Baolu wrote:
> Hi Jean,
> 
> On 2020/3/11 20:45, Jean-Philippe Brucker wrote:
> > The pci_ats_supported() function checks if a device supports ATS and is
> > allowed to use it.
> > 
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> >   drivers/iommu/intel-iommu.c | 9 +++------
> >   1 file changed, 3 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > index 6fa6de2b6ad5..17208280ef5c 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -1454,8 +1454,7 @@ static void iommu_enable_dev_iotlb(struct device_domain_info *info)
> >   	    !pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32))
> >   		info->pri_enabled = 1;
> >   #endif
> > -	if (!pdev->untrusted && info->ats_supported &&
> > -	    pci_ats_page_aligned(pdev) &&
> > +	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
> >   	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
> >   		info->ats_enabled = 1;
> >   		domain_update_iotlb(info->domain);
> > @@ -2611,10 +2610,8 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
> >   	if (dev && dev_is_pci(dev)) {
> >   		struct pci_dev *pdev = to_pci_dev(info->dev);
> > -		if (!pdev->untrusted &&
> > -		    !pci_ats_disabled() &&
> 
> The pci_ats_disabled() couldn't be replaced by pci_ats_supported(). Even
> pci_ats_supported() returns true, user still can disable it. Or move
> ats_disabled into pci_ats_supported()?

It is already there, but hidden behind the "if (!dev->ats_cap)":
pci_ats_init() only sets dev->ats_cap after checking that
pci_ats_disabled() returns false.

Thanks,
Jean

> 
> Best regards,
> baolu
> 
> > -		    ecap_dev_iotlb_support(iommu->ecap) &&
> > -		    pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ATS) &&
> > +		if (ecap_dev_iotlb_support(iommu->ecap) &&
> > +		    pci_ats_supported(pdev) &&
> >   		    dmar_find_matched_atsr_unit(pdev))
> >   			info->ats_supported = 1;

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

* Re: [PATCH v2 08/11] iommu/vt-d: Use pci_ats_supported()
  2020-03-12  7:54     ` Jean-Philippe Brucker
@ 2020-03-12  8:18       ` Lu Baolu
  0 siblings, 0 replies; 20+ messages in thread
From: Lu Baolu @ 2020-03-12  8:18 UTC (permalink / raw)
  To: Jean-Philippe Brucker
  Cc: baolu.lu, bhelgaas, will, robh+dt, joro, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu,
	lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list

On 2020/3/12 15:54, Jean-Philippe Brucker wrote:
> Hi Baolu,
> 
> On Thu, Mar 12, 2020 at 09:44:16AM +0800, Lu Baolu wrote:
>> Hi Jean,
>>
>> On 2020/3/11 20:45, Jean-Philippe Brucker wrote:
>>> The pci_ats_supported() function checks if a device supports ATS and is
>>> allowed to use it.
>>>
>>> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>>> ---
>>>    drivers/iommu/intel-iommu.c | 9 +++------
>>>    1 file changed, 3 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>>> index 6fa6de2b6ad5..17208280ef5c 100644
>>> --- a/drivers/iommu/intel-iommu.c
>>> +++ b/drivers/iommu/intel-iommu.c
>>> @@ -1454,8 +1454,7 @@ static void iommu_enable_dev_iotlb(struct device_domain_info *info)
>>>    	    !pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32))
>>>    		info->pri_enabled = 1;
>>>    #endif
>>> -	if (!pdev->untrusted && info->ats_supported &&
>>> -	    pci_ats_page_aligned(pdev) &&
>>> +	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
>>>    	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
>>>    		info->ats_enabled = 1;
>>>    		domain_update_iotlb(info->domain);
>>> @@ -2611,10 +2610,8 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
>>>    	if (dev && dev_is_pci(dev)) {
>>>    		struct pci_dev *pdev = to_pci_dev(info->dev);
>>> -		if (!pdev->untrusted &&
>>> -		    !pci_ats_disabled() &&
>>
>> The pci_ats_disabled() couldn't be replaced by pci_ats_supported(). Even
>> pci_ats_supported() returns true, user still can disable it. Or move
>> ats_disabled into pci_ats_supported()?
> 
> It is already there, but hidden behind the "if (!dev->ats_cap)":
> pci_ats_init() only sets dev->ats_cap after checking that
> pci_ats_disabled() returns false.
>

Ah! Yes.

Acked-by: Lu Baolu <baolu.lu@linux.intel.com>

> Thanks,
> Jean

Best regards,
baolu

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

* Re: [PATCH v2 03/11] PCI: OF: Check whether the host bridge supports ATS
  2020-03-11 12:44 ` [PATCH v2 03/11] PCI: OF: Check whether the host bridge supports ATS Jean-Philippe Brucker
@ 2020-03-12 20:45   ` Bjorn Helgaas
  0 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2020-03-12 20:45 UTC (permalink / raw)
  To: Jean-Philippe Brucker
  Cc: will, robh+dt, joro, baolu.lu, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu,
	lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list

On Wed, Mar 11, 2020 at 01:44:58PM +0100, Jean-Philippe Brucker wrote:
> When setting up a generic host on a device-tree based system, copy the
> ats-supported flag into the pci_host_bridge structure.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> v1->v2: keep the helper in pci-host-common.c
> ---
>  drivers/pci/controller/pci-host-common.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
> index 250a3fc80ec6..2e800bc6ae7a 100644
> --- a/drivers/pci/controller/pci-host-common.c
> +++ b/drivers/pci/controller/pci-host-common.c
> @@ -54,6 +54,16 @@ static struct pci_config_window *gen_pci_init(struct device *dev,
>  	return ERR_PTR(err);
>  }
>  
> +static void of_pci_host_check_ats(struct pci_host_bridge *bridge)
> +{
> +	struct device_node *np = bridge->bus->dev.of_node;
> +
> +	if (!np)
> +		return;
> +
> +	bridge->ats_supported = of_property_read_bool(np, "ats-supported");
> +}
> +
>  int pci_host_common_probe(struct platform_device *pdev,
>  			  struct pci_ecam_ops *ops)
>  {
> @@ -92,6 +102,7 @@ int pci_host_common_probe(struct platform_device *pdev,
>  		return ret;
>  	}
>  
> +	of_pci_host_check_ats(bridge);

I would prefer to write this as a predicate instead of having the
assignment be a side-effect, e.g.,

  bridge->ats_supported = of_pci_host_ats_supported(bridge);

If that works for you,

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

>  	platform_set_drvdata(pdev, bridge->bus);
>  	return 0;
>  }
> -- 
> 2.25.1
> 

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

* Re: [PATCH v2 04/11] ACPI/IORT: Check ATS capability in root complex node
  2020-03-11 12:44 ` [PATCH v2 04/11] ACPI/IORT: Check ATS capability in root complex node Jean-Philippe Brucker
@ 2020-03-12 20:58   ` Bjorn Helgaas
  0 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2020-03-12 20:58 UTC (permalink / raw)
  To: Jean-Philippe Brucker
  Cc: will, robh+dt, joro, baolu.lu, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu,
	lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list

On Wed, Mar 11, 2020 at 01:44:59PM +0100, Jean-Philippe Brucker wrote:
> When initializing a PCI root bridge, copy its "ATS supported" attribute
> into the root bridge.
> 
> Acked-by: Hanjun Guo <guohanjun@huawei.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  drivers/acpi/arm64/iort.c | 27 +++++++++++++++++++++++++++
>  drivers/acpi/pci_root.c   |  3 +++
>  include/linux/acpi_iort.h |  8 ++++++++
>  3 files changed, 38 insertions(+)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index ed3d2d1a7ae9..d99d7f5b51e1 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -1633,6 +1633,33 @@ static void __init iort_enable_acs(struct acpi_iort_node *iort_node)
>  		}
>  	}
>  }
> +
> +static acpi_status iort_match_host_bridge_callback(struct acpi_iort_node *node,
> +						   void *context)
> +{
> +	struct acpi_iort_root_complex *pci_rc;
> +	struct pci_host_bridge *host_bridge = context;
> +
> +	pci_rc = (struct acpi_iort_root_complex *)node->node_data;
> +
> +	return pci_domain_nr(host_bridge->bus) == pci_rc->pci_segment_number ?
> +		AE_OK : AE_NOT_FOUND;
> +}
> +
> +void iort_pci_host_bridge_setup(struct pci_host_bridge *host_bridge)
> +{
> +	struct acpi_iort_node *node;
> +	struct acpi_iort_root_complex *pci_rc;
> +
> +	node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
> +			      iort_match_host_bridge_callback, host_bridge);
> +	if (!node)
> +		return;
> +
> +	pci_rc = (struct acpi_iort_root_complex *)node->node_data;
> +	host_bridge->ats_supported = !!(pci_rc->ats_attribute &
> +					ACPI_IORT_ATS_SUPPORTED);
> +}
>  #else
>  static inline void iort_enable_acs(struct acpi_iort_node *iort_node) { }
>  #endif
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index d1e666ef3fcc..eb2fb8f17c0b 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -6,6 +6,7 @@
>   *  Copyright (C) 2001, 2002 Paul Diefenbaugh <paul.s.diefenbaugh@intel.com>
>   */
>  
> +#include <linux/acpi_iort.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
> @@ -917,6 +918,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
>  		host_bridge->native_ltr = 0;
>  
> +	iort_pci_host_bridge_setup(host_bridge);

Similar comment as on the OF side.

You mentioned at [1] that "it's important that we only enable ATS if
the host bridge supports it".  That should be captured in a commit log
and comment somewhere here.

That suggests to me that we should not set

  bridge->ats_supported = 1;

by default in pci_init_host_bridge(), but rather leave it zero as it
is by default, and then do things like:

  if (iort_pci_host_bridge_ats_supported(bridge))
    bridge->ats_supported = 1;

  if (of_pci_host_bridge_ats_supported(bridge))
    bridge->ats_supported = 1;

I don't know what you do about IVRS and DMAR, which don't appear in
this series except in the comment.

[1] https://lore.kernel.org/r/20200213165049.508908-1-jean-philippe@linaro.org

>  	/*
>  	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
>  	 * exists and returns 0, we must preserve any PCI resource
> diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> index 8e7e2ec37f1b..7b06871cc3aa 100644
> --- a/include/linux/acpi_iort.h
> +++ b/include/linux/acpi_iort.h
> @@ -10,6 +10,7 @@
>  #include <linux/acpi.h>
>  #include <linux/fwnode.h>
>  #include <linux/irqdomain.h>
> +#include <linux/pci.h>
>  
>  #define IORT_IRQ_MASK(irq)		(irq & 0xffffffffULL)
>  #define IORT_IRQ_TRIGGER_MASK(irq)	((irq >> 32) & 0xffffffffULL)
> @@ -55,4 +56,11 @@ int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head)
>  { return 0; }
>  #endif
>  
> +#if defined(CONFIG_ACPI_IORT) && defined(CONFIG_PCI)
> +void iort_pci_host_bridge_setup(struct pci_host_bridge *host_bridge);
> +#else
> +static inline
> +void iort_pci_host_bridge_setup(struct pci_host_bridge *host_bridge) { }
> +#endif
> +
>  #endif /* __ACPI_IORT_H__ */
> -- 
> 2.25.1
> 

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

* Re: [PATCH v2 05/11] PCI/ATS: Gather checks into pci_ats_supported()
  2020-03-11 12:45 ` [PATCH v2 05/11] PCI/ATS: Gather checks into pci_ats_supported() Jean-Philippe Brucker
@ 2020-03-12 21:01   ` Bjorn Helgaas
  0 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2020-03-12 21:01 UTC (permalink / raw)
  To: Jean-Philippe Brucker
  Cc: will, robh+dt, joro, baolu.lu, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu,
	lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list

On Wed, Mar 11, 2020 at 01:45:00PM +0100, Jean-Philippe Brucker wrote:
> IOMMU drivers need to perform several tests when checking if a device
> supports ATS.  Move them all into a new function that returns true when
> a device and its host bridge support ATS.
> 
> Since pci_enable_ats() now calls pci_ats_supported(), the following
> new checks are now common:
> * whether a device is trusted.  Devices plugged into external-facing
>   ports such as thunderbolt are untrusted.
> * whether the host bridge supports ATS, which defaults to true unless
>   the firmware description states that ATS isn't supported by the host
>   bridge.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  drivers/pci/ats.c       | 30 +++++++++++++++++++++++++++++-
>  include/linux/pci-ats.h |  3 +++
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index 390e92f2d8d1..bbfd0d42b8b9 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -30,6 +30,34 @@ void pci_ats_init(struct pci_dev *dev)
>  	dev->ats_cap = pos;
>  }
>  
> +/**
> + * pci_ats_supported - check if the device can use ATS
> + * @dev: the PCI device
> + *
> + * Returns true if the device supports ATS and is allowed to use it, false
> + * otherwise.
> + */
> +bool pci_ats_supported(struct pci_dev *dev)
> +{
> +	struct pci_host_bridge *bridge;
> +
> +	if (!dev->ats_cap)
> +		return false;
> +
> +	if (dev->untrusted)
> +		return false;
> +
> +	bridge = pci_find_host_bridge(dev->bus);
> +	if (!bridge)
> +		return false;
> +
> +	if (!bridge->ats_supported)
> +		return false;
> +
> +	return true;

I assume this is the same as

  return bridge->ats_supported;

Only "assuming" because I'm not a C language lawyer, but I assume it
does the obvious conversion from unsigned:1 to bool.

> +}

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

* Re: [PATCH v2 02/11] PCI: Add ats_supported host bridge flag
  2020-03-11 12:44 ` [PATCH v2 02/11] PCI: Add ats_supported host bridge flag Jean-Philippe Brucker
@ 2020-03-12 21:21   ` Bjorn Helgaas
  0 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2020-03-12 21:21 UTC (permalink / raw)
  To: Jean-Philippe Brucker
  Cc: will, robh+dt, joro, baolu.lu, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu,
	lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list

On Wed, Mar 11, 2020 at 01:44:57PM +0100, Jean-Philippe Brucker wrote:
> Each vendor has their own way of describing whether a host bridge
> supports ATS.  The Intel and AMD ACPI tables selectively enable or
> disable ATS per device or sub-tree, while Arm has a single bit for each
> host bridge.  For those that need it, add an ats_supported bit to the
> host bridge structure.

Can you mention the specific ACPI tables here in the commit log?

Maybe elaborate on the "for those that need it" bit?  I'm not sure if
you need it for the cases where DT or ACPI tells us directly for the
host bridge, or if you need it for the more selective cases?

I guess in one sense you *always* need it since you check the cached
bit later.

I don't understand the implications of this, especially the selective
situation.  Given your comment from the first posting, I thought this
was a property of the host bridge, so I don't know what it means to
say some devices support ATS but others don't.

> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> v1->v2: try to improve the comment
> ---
>  drivers/pci/probe.c | 8 ++++++++
>  include/linux/pci.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 512cb4312ddd..b5e36f06b40a 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -598,6 +598,14 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge)
>  	bridge->native_shpc_hotplug = 1;
>  	bridge->native_pme = 1;
>  	bridge->native_ltr = 1;
> +
> +	/*
> +	 * Some systems (ACPI IORT, device-tree) declare ATS support at the host
> +	 * bridge, and clear this bit when ATS isn't supported. Others (ACPI
> +	 * DMAR and IVRS) declare ATS support with a smaller granularity, and
> +	 * need this bit set.
> +	 */
> +	bridge->ats_supported = 1;
>  }
>  
>  struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 3840a541a9de..9fe2e84d74d7 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -511,6 +511,7 @@ struct pci_host_bridge {
>  	unsigned int	native_pme:1;		/* OS may use PCIe PME */
>  	unsigned int	native_ltr:1;		/* OS may use PCIe LTR */
>  	unsigned int	preserve_config:1;	/* Preserve FW resource setup */
> +	unsigned int	ats_supported:1;
>  
>  	/* Resource alignment requirements */
>  	resource_size_t (*align_resource)(struct pci_dev *dev,
> -- 
> 2.25.1
> 

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

* Re: [PATCH v2 07/11] iommu/arm-smmu-v3: Use pci_ats_supported()
  2020-03-11 12:45 ` [PATCH v2 07/11] iommu/arm-smmu-v3: " Jean-Philippe Brucker
@ 2020-03-18 21:49   ` Will Deacon
  0 siblings, 0 replies; 20+ messages in thread
From: Will Deacon @ 2020-03-18 21:49 UTC (permalink / raw)
  To: Jean-Philippe Brucker
  Cc: bhelgaas, robh+dt, joro, baolu.lu, sudeep.holla, linux-doc,
	linux-pci, linux-arm-kernel, devicetree, linux-acpi, iommu,
	lorenzo.pieralisi, corbet, mark.rutland, liviu.dudau, guohanjun,
	rjw, lenb, robin.murphy, dwmw2, amurray, frowand.list

On Wed, Mar 11, 2020 at 01:45:02PM +0100, Jean-Philippe Brucker wrote:
> The new pci_ats_supported() function checks if a device supports ATS and
> is allowed to use it.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  drivers/iommu/arm-smmu-v3.c | 18 +++---------------
>  1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 4f0a38dae6db..87ae31ef35a1 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2592,26 +2592,14 @@ static void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master)
>  	}
>  }
>  
> -#ifdef CONFIG_PCI_ATS
>  static bool arm_smmu_ats_supported(struct arm_smmu_master *master)
>  {
> -	struct pci_dev *pdev;
> +	struct device *dev = master->dev;
>  	struct arm_smmu_device *smmu = master->smmu;
> -	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(master->dev);
> -
> -	if (!(smmu->features & ARM_SMMU_FEAT_ATS) || !dev_is_pci(master->dev) ||
> -	    !(fwspec->flags & IOMMU_FWSPEC_PCI_RC_ATS) || pci_ats_disabled())
> -		return false;
>  
> -	pdev = to_pci_dev(master->dev);
> -	return !pdev->untrusted && pdev->ats_cap;
> +	return (smmu->features & ARM_SMMU_FEAT_ATS) && dev_is_pci(dev) &&
> +		pci_ats_supported(to_pci_dev(dev));
>  }
> -#else
> -static bool arm_smmu_ats_supported(struct arm_smmu_master *master)
> -{
> -	return false;
> -}
> -#endif

Acked-by: Will Deacon <will@kernel.org>

Cheers for doing this.

Will

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

end of thread, other threads:[~2020-03-18 21:49 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-11 12:44 [PATCH v2 00/11] PCI/ATS: Device-tree support and other improvements Jean-Philippe Brucker
2020-03-11 12:44 ` [PATCH v2 01/11] dt-bindings: PCI: generic: Add ats-supported property Jean-Philippe Brucker
2020-03-11 12:44 ` [PATCH v2 02/11] PCI: Add ats_supported host bridge flag Jean-Philippe Brucker
2020-03-12 21:21   ` Bjorn Helgaas
2020-03-11 12:44 ` [PATCH v2 03/11] PCI: OF: Check whether the host bridge supports ATS Jean-Philippe Brucker
2020-03-12 20:45   ` Bjorn Helgaas
2020-03-11 12:44 ` [PATCH v2 04/11] ACPI/IORT: Check ATS capability in root complex node Jean-Philippe Brucker
2020-03-12 20:58   ` Bjorn Helgaas
2020-03-11 12:45 ` [PATCH v2 05/11] PCI/ATS: Gather checks into pci_ats_supported() Jean-Philippe Brucker
2020-03-12 21:01   ` Bjorn Helgaas
2020-03-11 12:45 ` [PATCH v2 06/11] iommu/amd: Use pci_ats_supported() Jean-Philippe Brucker
2020-03-11 12:45 ` [PATCH v2 07/11] iommu/arm-smmu-v3: " Jean-Philippe Brucker
2020-03-18 21:49   ` Will Deacon
2020-03-11 12:45 ` [PATCH v2 08/11] iommu/vt-d: " Jean-Philippe Brucker
2020-03-12  1:44   ` Lu Baolu
2020-03-12  7:54     ` Jean-Philippe Brucker
2020-03-12  8:18       ` Lu Baolu
2020-03-11 12:45 ` [PATCH v2 09/11] ACPI/IORT: Drop ATS fwspec flag Jean-Philippe Brucker
2020-03-11 12:45 ` [PATCH v2 10/11] arm64: dts: fast models: Enable PCIe ATS for Base RevC FVP Jean-Philippe Brucker
2020-03-11 12:45 ` [PATCH v2 11/11] Documentation: Generalize the "pci=noats" boot parameter Jean-Philippe Brucker

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).