Skip to contentSkip to conversation

Allow ATS to be always on for certain ATS-capable devices

PATCH Superseded Nicolin Chen 7 participants 30 messages Apr 27 – May 25

Subsystems (3)

v4 of 6 Superseded Latest is v6 →

lore
Share & tools
lore
#0 lore raw

v4 cover Allow ATS to be always on for certain ATS-capable devices

PCI ATS function is controlled by the IOMMU driver calling pci_enable_ats() and pci_disable_ats() helpers. In general, IOMMU driver only enables ATS when a translation channel is enabled on a PASID, typically for an SVA use case. When a device's RID is IOMMU bypassed and its PASIDs are not running SVA use case, ATS is always disabled.

However, certain PCIe devices require non-PASID ATS on the RID, even if the RID is IOMMU bypassed. E.g. CXL.cache capability requires ATS to access the physical memory; some pre-CXL NVIDIA GPUs also require the ATS to be always on even when their RIDs are IOMMU bypassed.

Provide a helper function to detect CXL.cache capability and scan through a pre-CXL device ID list.

As the initial use case, call the helper in ARM SMMUv3 driver and adapt the driver accordingly with a per-device ats_always_on flag.

This is on Github:
https://github.com/nicolinc/iommufd/commits/pci_ats_always_on-v4/

Changelog
v4
* Rebase on v7.1-rc1
* Added Reviewed/Tested/Acked-by lines
* Update commit messages and inline comments
* [pci-quirks] Add range-based scan for NVIDIA GPUs
* [smmu] Add missing arm_smmu_remove_master() in error path
* [pci-ats] Don't init "cap=0"; check pci_read_config_word error
v3
https://lore.kernel.org/all/[email protected]/
* Add Reviewed-by from Jonathan
* Update function kdocs of PCI APIs
* Simplify boolean return/variable computations
v2
https://lore.kernel.org/all/[email protected]/
* s/non-CXL/pre-CXL
* Rebase on v7.0-rc1
* Update inline comments and commit message
* Add WARN_ON back at !ptr in arm_smmu_clear_cd()
* Add NVIDIA CX10 Family NVlink-C2C to the pre-CXL list
* Do not add boolean parameter to arm_smmu_attach_dev_ste()
v1
https://lore.kernel.org/all/[email protected]/

Nicolin Chen (3):
PCI: Allow ATS to be always on for CXL.cache capable devices
PCI: Allow ATS to be always on for pre-CXL devices iommu/arm-smmu-v3: Allow ATS to be always on

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 +
 drivers/pci/pci.h                           |  9 +++
 include/linux/pci-ats.h                     |  3 +
 include/uapi/linux/pci_regs.h               |  1 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 75 ++++++++++++++++++---
 drivers/pci/ats.c                           | 44 ++++++++++++
 drivers/pci/quirks.c                        | 38 +++++++++++
 7 files changed, 163 insertions(+), 8 deletions(-)
Signature · 1 line
2.43.0
#1 lore raw

PATCH v4 1/3 PCI: Allow ATS to be always on for CXL.cache capable devices

Controlled by the IOMMU driver, ATS is usually enabled "on demand" when a given PASID on a device is attached to an I/O page table. This is working even when a device has no translation on its RID (i.e., the RID is IOMMU bypassed).

However, certain PCIe devices require non-PASID ATS on their RID even when the RID is IOMMU bypassed. Call this "always on".

For example, CXL spec r4.0 notes in sec 3.2.5.13 Memory Type on CXL.cache:
"To source requests on CXL.cache, devices need to get the Host Physical
Address (HPA) from the Host by means of an ATS request on CXL.io."

In other words, the CXL.cache capability requires ATS; otherwise, it can't access host physical memory.

Introduce a new pci_ats_always_on() helper for the IOMMU driver to scan a PCI device and shift ATS policies between "on demand" and "always on".

Add the support for CXL.cache devices first. Pre-CXL devices will be added in quirks.c file.

Note that pci_ats_always_on() validates against pci_ats_supported(), so we ensure that untrusted devices (e.g. external ports) will not be always on. This maintains the existing ATS security policy regarding potential side- channel attacks via ATS.

---
 include/linux/pci-ats.h       |  3 +++
 include/uapi/linux/pci_regs.h |  1 +
 drivers/pci/ats.c             | 43 +++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+)
include/linux/pci-ats.h +3 -0
index 75c6c86cf09dc..d14ba727d38b3 100644
@@ -12,6 +12,7 @@ int pci_prepare_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);
+bool pci_ats_always_on(struct pci_dev *dev);
 #else /* CONFIG_PCI_ATS */
 static inline bool pci_ats_supported(struct pci_dev *d)
 { return false; }
@@ -24,6 +25,8 @@ static inline int pci_ats_queue_depth(struct pci_dev *d)
 { return -ENODEV; }
 static inline int pci_ats_page_aligned(struct pci_dev *dev)
 { return 0; }
+static inline bool pci_ats_always_on(struct pci_dev *dev)
+{ return false; }
 #endif /* CONFIG_PCI_ATS */
 
 #ifdef CONFIG_PCI_PRI
include/uapi/linux/pci_regs.h +1 -0
index 14f634ab9350d..6ac45be1008b8 100644
@@ -1349,6 +1349,7 @@
 /* CXL r4.0, 8.1.3: PCIe DVSEC for CXL Device */
 #define PCI_DVSEC_CXL_DEVICE				0
 #define  PCI_DVSEC_CXL_CAP				0xA
+#define   PCI_DVSEC_CXL_CACHE_CAPABLE			_BITUL(0)
 #define   PCI_DVSEC_CXL_MEM_CAPABLE			_BITUL(2)
 #define   PCI_DVSEC_CXL_HDM_COUNT			__GENMASK(5, 4)
 #define  PCI_DVSEC_CXL_CTRL				0xC
drivers/pci/ats.c +43 -0

49 lines not on this page · raw message

Signature · 1 line
2.43.0
Reviewed-by on v4 1/3
#2 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

Quoted 26 lines · Nicolin Chen

Controlled by the IOMMU driver, ATS is usually enabled "on demand" when a given PASID on a device is attached to an I/O page table. This is working even when a device has no translation on its RID (i.e., the RID is IOMMU bypassed).

However, certain PCIe devices require non-PASID ATS on their RID even when the RID is IOMMU bypassed. Call this "always on".

For example, CXL spec r4.0 notes in sec 3.2.5.13 Memory Type on CXL.cache:
"To source requests on CXL.cache, devices need to get the Host Physical
Address (HPA) from the Host by means of an ATS request on CXL.io."

In other words, the CXL.cache capability requires ATS; otherwise, it can't access host physical memory.

Introduce a new pci_ats_always_on() helper for the IOMMU driver to scan a PCI device and shift ATS policies between "on demand" and "always on".

Add the support for CXL.cache devices first. Pre-CXL devices will be added in quirks.c file.

Note that pci_ats_always_on() validates against pci_ats_supported(), so we ensure that untrusted devices (e.g. external ports) will not be always on. This maintains the existing ATS security policy regarding potential side- channel attacks via ATS.

Quoted 9 lines
Quoted 93 lines
---
 include/linux/pci-ats.h       |  3 +++
 include/uapi/linux/pci_regs.h |  1 +
 drivers/pci/ats.c             | 43 +++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+)
include/linux/pci-ats.h +3 -0
index 75c6c86cf09dc..d14ba727d38b3 100644
@@ -12,6 +12,7 @@ int pci_prepare_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);
+bool pci_ats_always_on(struct pci_dev *dev);
 #else /* CONFIG_PCI_ATS */
 static inline bool pci_ats_supported(struct pci_dev *d)
 { return false; }
@@ -24,6 +25,8 @@ static inline int pci_ats_queue_depth(struct pci_dev *d)
 { return -ENODEV; }
 static inline int pci_ats_page_aligned(struct pci_dev *dev)
 { return 0; }
+static inline bool pci_ats_always_on(struct pci_dev *dev)
+{ return false; }
 #endif /* CONFIG_PCI_ATS */
 
 #ifdef CONFIG_PCI_PRI
include/uapi/linux/pci_regs.h +1 -0
index 14f634ab9350d..6ac45be1008b8 100644
@@ -1349,6 +1349,7 @@
 /* CXL r4.0, 8.1.3: PCIe DVSEC for CXL Device */
 #define PCI_DVSEC_CXL_DEVICE				0
 #define  PCI_DVSEC_CXL_CAP				0xA
+#define   PCI_DVSEC_CXL_CACHE_CAPABLE			_BITUL(0)
 #define   PCI_DVSEC_CXL_MEM_CAPABLE			_BITUL(2)
 #define   PCI_DVSEC_CXL_HDM_COUNT			__GENMASK(5, 4)
 #define  PCI_DVSEC_CXL_CTRL				0xC
drivers/pci/ats.c +43 -0
index ec6c8dbdc5e9c..fc871858b65bc 100644
@@ -205,6 +205,49 @@ int pci_ats_page_aligned(struct pci_dev *pdev)
 	return 0;
 }
 
+/*
+ * CXL r4.0, sec 3.2.5.13 Memory Type on CXL.cache notes: to source requests on
+ * CXL.cache, devices need to get the Host Physical Address (HPA) from the Host
+ * by means of an ATS request on CXL.io.
+ *
+ * In other words, CXL.cache devices cannot access host physical memory without
+ * ATS.
+ */
+static bool pci_cxl_ats_always_on(struct pci_dev *pdev)
+{
+	int offset;
+	u16 cap;
+
+	offset = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL,
+					   PCI_DVSEC_CXL_DEVICE);
+	if (!offset)
+		return false;
+
+	if (pci_read_config_word(pdev, offset + PCI_DVSEC_CXL_CAP, &cap))
+		return false;
+
+	return cap & PCI_DVSEC_CXL_CACHE_CAPABLE;
+}
+
+/**
+ * pci_ats_always_on - Whether the PCI device requires ATS to be always enabled
+ * @pdev: the PCI device
+ *
+ * Returns true, if the PCI device requires ATS for basic functional operation.
+ */
+bool pci_ats_always_on(struct pci_dev *pdev)
+{
+	if (pci_ats_disabled() || !pci_ats_supported(pdev))
+		return false;
+
+	/* A VF inherits its PF's requirement for ATS function */
+	if (pdev->is_virtfn)
+		pdev = pci_physfn(pdev);
+
+	return pci_cxl_ats_always_on(pdev);
+}
+EXPORT_SYMBOL_GPL(pci_ats_always_on);
+
 #ifdef CONFIG_PCI_PRI
 void pci_pri_init(struct pci_dev *pdev)
 {
#3 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

In reply to Nicolin Chen
Quoted 94 lines · Nicolin Chen

Controlled by the IOMMU driver, ATS is usually enabled "on demand" when a given PASID on a device is attached to an I/O page table. This is working even when a device has no translation on its RID (i.e., the RID is IOMMU bypassed).

However, certain PCIe devices require non-PASID ATS on their RID even when the RID is IOMMU bypassed. Call this "always on".

For example, CXL spec r4.0 notes in sec 3.2.5.13 Memory Type on CXL.cache:
"To source requests on CXL.cache, devices need to get the Host Physical
Address (HPA) from the Host by means of an ATS request on CXL.io."

In other words, the CXL.cache capability requires ATS; otherwise, it can't access host physical memory.

Introduce a new pci_ats_always_on() helper for the IOMMU driver to scan a PCI device and shift ATS policies between "on demand" and "always on".

Add the support for CXL.cache devices first. Pre-CXL devices will be added in quirks.c file.

Note that pci_ats_always_on() validates against pci_ats_supported(), so we ensure that untrusted devices (e.g. external ports) will not be always on. This maintains the existing ATS security policy regarding potential side- channel attacks via ATS.

---
 include/linux/pci-ats.h       |  3 +++
 include/uapi/linux/pci_regs.h |  1 +
 drivers/pci/ats.c             | 43 +++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+)
include/linux/pci-ats.h +3 -0
index 75c6c86cf09dc..d14ba727d38b3 100644
@@ -12,6 +12,7 @@ int pci_prepare_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);
+bool pci_ats_always_on(struct pci_dev *dev);
 #else /* CONFIG_PCI_ATS */
 static inline bool pci_ats_supported(struct pci_dev *d)
 { return false; }
@@ -24,6 +25,8 @@ static inline int pci_ats_queue_depth(struct pci_dev *d)
 { return -ENODEV; }
 static inline int pci_ats_page_aligned(struct pci_dev *dev)
 { return 0; }
+static inline bool pci_ats_always_on(struct pci_dev *dev)
+{ return false; }
 #endif /* CONFIG_PCI_ATS */
 
 #ifdef CONFIG_PCI_PRI
include/uapi/linux/pci_regs.h +1 -0
index 14f634ab9350d..6ac45be1008b8 100644
@@ -1349,6 +1349,7 @@
 /* CXL r4.0, 8.1.3: PCIe DVSEC for CXL Device */
 #define PCI_DVSEC_CXL_DEVICE				0
 #define  PCI_DVSEC_CXL_CAP				0xA
+#define   PCI_DVSEC_CXL_CACHE_CAPABLE			_BITUL(0)
 #define   PCI_DVSEC_CXL_MEM_CAPABLE			_BITUL(2)
 #define   PCI_DVSEC_CXL_HDM_COUNT			__GENMASK(5, 4)
 #define  PCI_DVSEC_CXL_CTRL				0xC
drivers/pci/ats.c +12 -0
index ec6c8dbdc5e9c..fc871858b65bc 100644
@@ -205,6 +205,49 @@ int pci_ats_page_aligned(struct pci_dev *pdev)
 	return 0;
 }
 
+/*
+ * CXL r4.0, sec 3.2.5.13 Memory Type on CXL.cache notes: to source requests on
+ * CXL.cache, devices need to get the Host Physical Address (HPA) from the Host
+ * by means of an ATS request on CXL.io.
+ *
+ * In other words, CXL.cache devices cannot access host physical memory without
+ * ATS.
+ */
+static bool pci_cxl_ats_always_on(struct pci_dev *pdev)
+{
+	int offset;
+	u16 cap;

drivers/pci/ats.c int pci_ats_page_aligned(struct pci_dev *pdev) · line 229

Quoted 10 lines
drivers/pci/ats.c +10 -0
+
+	offset = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL,
+					   PCI_DVSEC_CXL_DEVICE);
+	if (!offset)
+		return false;
+
+	if (pci_read_config_word(pdev, offset + PCI_DVSEC_CXL_CAP, &cap))
+		return false;
+
+	return cap & PCI_DVSEC_CXL_CACHE_CAPABLE;

Apologies for coming to this late and forgive me if the following has already been asked and answered. Why not check for actual CXL.cache protocol on the wire being present?

I.e. replace pci_cxl_ats_always_on() with a pdev->is_cxl_cache and this incremental change (compile tested only):

include/linux/pci.h +3 -2
index 2c4454583c11..45d87af4de63 100644
@@ -483,7 +483,8 @@ struct pci_dev {
 	unsigned int	is_pciehp:1;
 	unsigned int	shpc_managed:1;		/* SHPC owned by shpchp */
 	unsigned int	is_thunderbolt:1;	/* Thunderbolt controller */
-	unsigned int	is_cxl:1;               /* Compute Express Link (CXL) */
+	unsigned int	is_cxl_mem:1;           /* Compute Express Link (CXL.mem) */
+	unsigned int	is_cxl_cache:1;         /* Compute Express Link (CXL.cache) */
 	/*
 	 * Devices marked being untrusted are the ones that can potentially
 	 * execute DMA attacks and similar. They are typically connected
@@ -809,7 +810,7 @@ static inline bool pci_is_display(struct pci_dev *pdev)
 
 static inline bool pcie_is_cxl(struct pci_dev *pci_dev)
 {
-	return pci_dev->is_cxl;
+	return pci_dev->is_cxl_mem || pci_dev->is_cxl_cache;
 }
 
 #define for_each_pci_bridge(dev, bus)				\
drivers/pci/probe.c +2 -3
index b63cd0c310bc..c01f0e8362f1 100644
@@ -1733,9 +1733,8 @@ static void set_pcie_cxl(struct pci_dev *dev)
 	pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS,
 			     &cap);
 
-	dev->is_cxl = FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS_CACHE, cap) ||
-		FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS_MEM, cap);
-
+	dev->is_cxl_cache = FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS_CACHE, cap);
+	dev->is_cxl_mem = FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS_MEM, cap);
 }
 
 static void set_pcie_untrusted(struct pci_dev *dev)
#4 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

Quoted 14 lines · Dan Williams (nvidia)
Quoted 14 lines
(quoted diff) +14 -0
+static bool pci_cxl_ats_always_on(struct pci_dev *pdev)
+{
+	int offset;
+	u16 cap;
+
+	offset = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL,
+					   PCI_DVSEC_CXL_DEVICE);
+	if (!offset)
+		return false;
+
+	if (pci_read_config_word(pdev, offset + PCI_DVSEC_CXL_CAP, &cap))
+		return false;
+
+	return cap & PCI_DVSEC_CXL_CACHE_CAPABLE;

[...]

Quoted 3 lines

Apologies for coming to this late and forgive me if the following has already been asked and answered. Why not check for actual CXL.cache protocol on the wire being present?

Actually it would make the patch smaller. The thing is that this is_cxl property wasn't added when I started the series. So, it's not using it. :)

drivers/pci/probe.c static void set_pcie_cxl(struct pci_dev *dev) · line 1737

Quoted 9 lines
drivers/pci/probe.c +2 -3
@@ -1733,9 +1733,8 @@ static void set_pcie_cxl(struct pci_dev *dev)
 	pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS,
 			     &cap);
 
-	dev->is_cxl = FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS_CACHE, cap) ||
-		FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS_MEM, cap);
-
+	dev->is_cxl_cache = FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS_CACHE, cap);
+	dev->is_cxl_mem = FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS_MEM, cap);

One caveat is that:

Here it checks the cap from:

	PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS (0xE) via
		PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS (0x7)

On the other hand, mine checks from:

	PCI_DVSEC_CXL_CAP (0xA) via
		PCI_DVSEC_CXL_DEVICE (0x0)

The spec mentions in 8.2.1.3.1 DVSEC Flex Bus Port Capability: "
Note: The Mem_Capable, IO_Capable, and Cache_Capable fields are also present in the DVSEC Flex Bus for the device [which is the legacy name for DVSEC 0x0]. This allows for future scalability where multiple devices, each with potentially different capabilities, may be populated behind a single Port.
"

Not arguing that set_pcie_cxl() is wrong, but I am not sure if there would be any side effect to rely on the "legacy name" over DVSEC 0x0.

Is there any CXL expert who can help confirm?

Thanks!
Nicolin

#5 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

Quoted 52 lines · Nicolin Chen
Quoted 14 lines · Dan Williams (nvidia)
Quoted 14 lines
(quoted diff) +14 -0
+static bool pci_cxl_ats_always_on(struct pci_dev *pdev)
+{
+	int offset;
+	u16 cap;
+
+	offset = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL,
+					   PCI_DVSEC_CXL_DEVICE);
+	if (!offset)
+		return false;
+
+	if (pci_read_config_word(pdev, offset + PCI_DVSEC_CXL_CAP, &cap))
+		return false;
+
+	return cap & PCI_DVSEC_CXL_CACHE_CAPABLE;

[...]

Quoted 3 lines

Apologies for coming to this late and forgive me if the following has already been asked and answered. Why not check for actual CXL.cache protocol on the wire being present?

Actually it would make the patch smaller. The thing is that this is_cxl property wasn't added when I started the series. So, it's not using it. :)

drivers/pci/probe.c static void set_pcie_cxl(struct pci_dev *dev) · line 1737

Quoted 9 lines
drivers/pci/probe.c +2 -3
@@ -1733,9 +1733,8 @@ static void set_pcie_cxl(struct pci_dev *dev)
 	pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS,
 			     &cap);
 
-	dev->is_cxl = FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS_CACHE, cap) ||
-		FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS_MEM, cap);
-
+	dev->is_cxl_cache = FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS_CACHE, cap);
+	dev->is_cxl_mem = FIELD_GET(PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS_MEM, cap);

One caveat is that:

Here it checks the cap from:

	PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS (0xE) via
		PCI_DVSEC_CXL_FLEXBUS_PORT_STATUS (0x7)

On the other hand, mine checks from:

	PCI_DVSEC_CXL_CAP (0xA) via
		PCI_DVSEC_CXL_DEVICE (0x0)

The spec mentions in 8.2.1.3.1 DVSEC Flex Bus Port Capability: "
Note: The Mem_Capable, IO_Capable, and Cache_Capable fields are also present in the DVSEC Flex Bus for the device [which is the legacy name for DVSEC 0x0]. This allows for future scalability where multiple devices, each with potentially different capabilities, may be populated behind a single Port.
"

Quoted 4 lines

Not arguing that set_pcie_cxl() is wrong, but I am not sure if there would be any side effect to rely on the "legacy name" over DVSEC 0x0.

Is there any CXL expert who can help confirm?

You appear to be confusing Cache_Capable and Cache_Enabled.

"8.2.1.3.1 DVSEC Flex Bus Port Capability" != "8.2.1.3.3 DVSEC Flex Bus Port Status"

Cache_Capable is only a capability. To check that the device has actually trained the CXL.cache alternate protocol you need to look at the status register.

#6 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

Quoted 7 lines · Dan Williams (nvidia)

You appear to be confusing Cache_Capable and Cache_Enabled.

"8.2.1.3.1 DVSEC Flex Bus Port Capability" != "8.2.1.3.3 DVSEC Flex Bus Port Status"

Cache_Capable is only a capability. To check that the device has actually trained the CXL.cache alternate protocol you need to look at the status register.

The capable is probably a reasonable choice here unless you are confident the status will never change after the device is first discovered? ATS is being set early in the boot sequence.

It is pretty safe to be over eager with the ATS enablement, less safe to get it off when it needs to be on.

Jason

#7 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

Quoted 10 lines · Jason Gunthorpe
Quoted 7 lines · Dan Williams (nvidia)

You appear to be confusing Cache_Capable and Cache_Enabled.

"8.2.1.3.1 DVSEC Flex Bus Port Capability" != "8.2.1.3.3 DVSEC Flex Bus Port Status"

Cache_Capable is only a capability. To check that the device has actually trained the CXL.cache alternate protocol you need to look at the status register.

Quoted 6 lines

The capable is probably a reasonable choice here unless you are confident the status will never change after the device is first discovered? ATS is being set early in the boot sequence.

It is pretty safe to be over eager with the ATS enablement, less safe to get it off when it needs to be on.

True, a reset could turn on CXL.cache. Ok, stick with what you have.

The present state of alternate protocol negotiation is still relevant though for distinguishing CXL protocol errors from other PCIe AER "internal" errors.

Need a bit of fixup work for that to refresh the status bit after reset.

#8 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

In reply to Nicolin Chen
Quoted 18 lines · Nicolin Chen

Controlled by the IOMMU driver, ATS is usually enabled "on demand" when a given PASID on a device is attached to an I/O page table. This is working even when a device has no translation on its RID (i.e., the RID is IOMMU bypassed).

However, certain PCIe devices require non-PASID ATS on their RID even when the RID is IOMMU bypassed. Call this "always on".

For example, CXL spec r4.0 notes in sec 3.2.5.13 Memory Type on CXL.cache:
"To source requests on CXL.cache, devices need to get the Host Physical
Address (HPA) from the Host by means of an ATS request on CXL.io."

In other words, the CXL.cache capability requires ATS; otherwise, it can't access host physical memory.

Introduce a new pci_ats_always_on() helper for the IOMMU driver to scan a PCI device and shift ATS policies between "on demand" and "always on".

Quoted 7 lines

Add the support for CXL.cache devices first. Pre-CXL devices will be added in quirks.c file.

Note that pci_ats_always_on() validates against pci_ats_supported(), so we ensure that untrusted devices (e.g. external ports) will not be always on. This maintains the existing ATS security policy regarding potential side- channel attacks via ATS.

IMO this doesn't really fit in the PCI core. ats.c encapsulates discovery and provides interfaces to access the ATS Capability, but the users of those interfaces are all outside the PCI core.

The decision to enable enable ATS for CXL.cache devices is fine but it's really an IOMMU usage policy, and I think it should be implemented in the IOMMU core. All the pieces needed (pci_ats_disabled(), pci_ats_supported(), pci_find_dvsec_capability()) are already exported.

One motivation for putting this in the PCI core was to use the quirk infrastructure, but this series doesn't use any of that. It doesn't declare any fixups, e.g., DECLARE_PCI_FIXUP_FINAL, and it doesn't update any state cached by the PCI core.

include/uapi/linux/pci_regs.h line 1352

Quoted 6 lines

+++ b/include/uapi/linux/pci_regs.h

include/uapi/linux/pci_regs.h +1 -0
@@ -1349,6 +1349,7 @@
 /* CXL r4.0, 8.1.3: PCIe DVSEC for CXL Device */
 #define PCI_DVSEC_CXL_DEVICE				0
 #define  PCI_DVSEC_CXL_CAP				0xA
+#define   PCI_DVSEC_CXL_CACHE_CAPABLE			_BITUL(0)

This makes good sense, I'm fine with adding
PCI_DVSEC_CXL_CACHE_CAPABLE.

drivers/pci/ats.c int pci_ats_page_aligned(struct pci_dev *pdev) · line 241

Quoted 4 lines
drivers/pci/ats.c +4 -0
+bool pci_ats_always_on(struct pci_dev *pdev)
+{
+	if (pci_ats_disabled() || !pci_ats_supported(pdev))
+		return false;

Isn't this the same as:

if (!pci_ats_supported(pdev))

    return false;

If pci_ats_disabled(), dev->ats_cap should be zero, so pci_ats_supported() should always return false.

Quoted 8 lines
drivers/pci/ats.c +8 -0
+
+	/* A VF inherits its PF's requirement for ATS function */
+	if (pdev->is_virtfn)
+		pdev = pci_physfn(pdev);
+
+	return pci_cxl_ats_always_on(pdev);
+}
+EXPORT_SYMBOL_GPL(pci_ats_always_on);
#9 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

Quoted 4 lines · Bjorn Helgaas

One motivation for putting this in the PCI core was to use the quirk infrastructure, but this series doesn't use any of that. It doesn't declare any fixups, e.g., DECLARE_PCI_FIXUP_FINAL, and it doesn't update any state cached by the PCI core.

It works like the acs quirks that are in the quirks file, which are also arguably only used by iommu too :)

I'm not keen on spreading lists of device ids for PCI quirks to iommu files, but it would be OK to move pci_ats_always_on() to iommu_ats_always_on() that calls the PCI quirk function.

Thanks,
Jason

#10 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

Quoted 8 lines · Jason Gunthorpe
Quoted 4 lines · Bjorn Helgaas

One motivation for putting this in the PCI core was to use the quirk infrastructure, but this series doesn't use any of that. It doesn't declare any fixups, e.g., DECLARE_PCI_FIXUP_FINAL, and it doesn't update any state cached by the PCI core.

It works like the acs quirks that are in the quirks file, which are also arguably only used by iommu too :)

True, although ACS has a lot more PCI-specific grunge in it, including all the "pci=config_acs" and "pci=disable_acs_redir" stuff.

Quoted 3 lines

I'm not keen on spreading lists of device ids for PCI quirks to iommu files, but it would be OK to move pci_ats_always_on() to iommu_ats_always_on() that calls the PCI quirk function.

Yeah, I guess it's fair to collect the device IDs in PCI since this is about characteristics of the device.

If we leave stuff in drivers/pci/, I would prefer that part of it be named to be purely informational, i.e., "CXL.cache_enabled" or something similar that would also cover the NVIDIA devices.

"pci_ats_always_on()" doesn't sound right quite to me because it presupposes the policy choice that IOMMU is going to make; that PCI function doesn't actually turn ATS on, and it looks like the question of enabling ATS depends on how the device is actually *used*. E.g., if Cache_Enable is not set, is ATS required?

That raises the question of whether this is the right test:

+ if (pci_read_config_word(pdev, offset + PCI_DVSEC_CXL_CAP, &cap))
+ return false;
+
+ return cap & PCI_DVSEC_CXL_CACHE_CAPABLE;

That just says the device is *capable* of CXL.cache; should it check whether CXL.cache is *enabled* instead?

Bjorn

#11 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

Quoted 17 lines · Bjorn Helgaas
Quoted 8 lines · Jason Gunthorpe
Quoted 4 lines · Bjorn Helgaas

One motivation for putting this in the PCI core was to use the quirk infrastructure, but this series doesn't use any of that. It doesn't declare any fixups, e.g., DECLARE_PCI_FIXUP_FINAL, and it doesn't update any state cached by the PCI core.

It works like the acs quirks that are in the quirks file, which are also arguably only used by iommu too :)

True, although ACS has a lot more PCI-specific grunge in it, including all the "pci=config_acs" and "pci=disable_acs_redir" stuff.

Quoted 3 lines

I'm not keen on spreading lists of device ids for PCI quirks to iommu files, but it would be OK to move pci_ats_always_on() to iommu_ats_always_on() that calls the PCI quirk function.

Quoted 6 lines

Yeah, I guess it's fair to collect the device IDs in PCI since this is about characteristics of the device.

If we leave stuff in drivers/pci/, I would prefer that part of it be named to be purely informational, i.e., "CXL.cache_enabled" or something similar that would also cover the NVIDIA devices.

Yeah, that's fair, so let's rename it to

pci_translated_required()

ie the device requires translated requests to function. This is what CXL.cache implies (IIRC I was told the spec specifically says this)

Requiring translated requests implies you have to enable ATS in the system.

Quoted 3 lines

function doesn't actually turn ATS on, and it looks like the question of enabling ATS depends on how the device is actually *used*. E.g., if Cache_Enable is not set, is ATS required?

We have no way to know..

Quoted 9 lines

That raises the question of whether this is the right test:

+ if (pci_read_config_word(pdev, offset + PCI_DVSEC_CXL_CAP, &cap))
+ return false;
+
+ return cap & PCI_DVSEC_CXL_CACHE_CAPABLE;

That just says the device is *capable* of CXL.cache; should it check whether CXL.cache is *enabled* instead?

No, we talked about this with Dan in one of the versions... it is better to over-enable ATS than under-enable. over-enable at best is a NOP, or maybe a tiny performance loss, under-enable is a functional failure.

If the CXL.cache is not enabled right now it could become enabled later, after the iommu has already called this and made its choice..

Thus lets not try to be too narrow here..

Thanks,
Jason

#12 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

Quoted 27 lines · Jason Gunthorpe
Quoted 17 lines · Bjorn Helgaas
Quoted 8 lines · Jason Gunthorpe
Quoted 4 lines · Bjorn Helgaas

One motivation for putting this in the PCI core was to use the quirk infrastructure, but this series doesn't use any of that. It doesn't declare any fixups, e.g., DECLARE_PCI_FIXUP_FINAL, and it doesn't update any state cached by the PCI core.

It works like the acs quirks that are in the quirks file, which are also arguably only used by iommu too :)

True, although ACS has a lot more PCI-specific grunge in it, including all the "pci=config_acs" and "pci=disable_acs_redir" stuff.

Quoted 3 lines

I'm not keen on spreading lists of device ids for PCI quirks to iommu files, but it would be OK to move pci_ats_always_on() to iommu_ats_always_on() that calls the PCI quirk function.

Quoted 6 lines

Yeah, I guess it's fair to collect the device IDs in PCI since this is about characteristics of the device.

If we leave stuff in drivers/pci/, I would prefer that part of it be named to be purely informational, i.e., "CXL.cache_enabled" or something similar that would also cover the NVIDIA devices.

Yeah, that's fair, so let's rename it to

Quoted 7 lines

pci_translated_required()

ie the device requires translated requests to function. This is what CXL.cache implies (IIRC I was told the spec specifically says this)

Requiring translated requests implies you have to enable ATS in the system.

Perhaps we could let IOMMU drivers check:
pci_cxl_is_cache_capable() || pci_dev_specific_is_pre_cxl()
directly?

Thanks
Nicolin

#13 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

Quoted 7 lines · Nicolin Chen
Quoted 7 lines

Yeah, that's fair, so let's rename it to

pci_translated_required()

ie the device requires translated requests to function. This is what CXL.cache implies (IIRC I was told the spec specifically says this)

Quoted 6 lines
Quoted 2 lines

Requiring translated requests implies you have to enable ATS in the system.

Perhaps we could let IOMMU drivers check:
pci_cxl_is_cache_capable() || pci_dev_specific_is_pre_cxl()
directly?

I'd rather have a single function.

Jason

#14 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

Quoted 9 lines · Jason Gunthorpe
Quoted 7 lines · Nicolin Chen
Quoted 7 lines

Yeah, that's fair, so let's rename it to

pci_translated_required()

ie the device requires translated requests to function. This is what CXL.cache implies (IIRC I was told the spec specifically says this)

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

Requiring translated requests implies you have to enable ATS in the system.

Perhaps we could let IOMMU drivers check:
pci_cxl_is_cache_capable() || pci_dev_specific_is_pre_cxl()
directly?

I'd rather have a single function.

OK. Can we use pci_ats_required()?

CXL spec explicitly used "ATS" when stating the requirement of CXL.cache). And it'd fit into the existing pci_ats_ functions.

Nicolin

#15 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

Quoted 17 lines · Nicolin Chen
Quoted 16 lines · Jason Gunthorpe
Quoted 13 lines · Nicolin Chen
Quoted 9 lines

Yeah, that's fair, so let's rename it to

pci_translated_required()

ie the device requires translated requests to function. This is what CXL.cache implies (IIRC I was told the spec specifically says this)

Requiring translated requests implies you have to enable ATS in the system.

Perhaps we could let IOMMU drivers check:
pci_cxl_is_cache_capable() || pci_dev_specific_is_pre_cxl()
directly?

Quoted 6 lines
Quoted 1 line

I'd rather have a single function.

OK. Can we use pci_ats_required()?

CXL spec explicitly used "ATS" when stating the requirement of CXL.cache). And it'd fit into the existing pci_ats_ functions.

OK by me.

You already have a comment in the code about the CXL.cache requirement; thanks for that.

I don't know enough about CXL to know what's behind the ATS requirement. It sounds like it's more than a simple performance optimization. If you happen to know the reason, it might be worth a short comment about that too.

Please add a one-line comment in the code about why we check Cache_Capable instead of Cache_Enable, i.e., even if CXL.cache is not enabled now, it may be enabled later.

Bjorn

#16 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

Quoted 4 lines · Bjorn Helgaas

I don't know enough about CXL to know what's behind the ATS requirement. It sounds like it's more than a simple performance optimization. If you happen to know the reason, it might be worth a short comment about that too.

At the core of this is underlying physical interconnect protocols that only work with translated addresses.

Ie CXL.cache only has a definition for translated physical in its protocol spec. The use of true physical only is due to the cache coherence shootdown protocol..

It is why I suggested 'pci_translated_required()' earlier, there are a few more than CXl.cache why a device might need translated physical addresses only.

ATS is the only way for a device to get those addresses, so ats_required is fine too, but it sort of glosses over what is driving it.

Jason

Reviewed-by on v4 1/3
#17 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

In reply to Nicolin Chen
Quoted 4 lines · Nicolin Chen

Controlled by the IOMMU driver, ATS is usually enabled "on demand" when a given PASID on a device is attached to an I/O page table. This is working even when a device has no translation on its RID (i.e., the RID is IOMMU bypassed).

nit: this description seems not accurate. Intel iommu driver enables ATS in the probe_device() phase. mind tweak a bit to avoid misleading message. :)

Quoted 13 lines

However, certain PCIe devices require non-PASID ATS on their RID even when the RID is IOMMU bypassed. Call this "always on".

For example, CXL spec r4.0 notes in sec 3.2.5.13 Memory Type on CXL.cache:
"To source requests on CXL.cache, devices need to get the Host Physical
Address (HPA) from the Host by means of an ATS request on CXL.io."

In other words, the CXL.cache capability requires ATS; otherwise, it can't access host physical memory.

Introduce a new pci_ats_always_on() helper for the IOMMU driver to scan a PCI device and shift ATS policies between "on demand" and "always on".

Quoted 8 lines

Add the support for CXL.cache devices first. Pre-CXL devices will be added in quirks.c file.

Note that pci_ats_always_on() validates against pci_ats_supported(), so we ensure that untrusted devices (e.g. external ports) will not be always on. This maintains the existing ATS security policy regarding potential side- channel attacks via ATS.

Quoted 102 lines
---
  include/linux/pci-ats.h       |  3 +++
  include/uapi/linux/pci_regs.h |  1 +
  drivers/pci/ats.c             | 43 +++++++++++++++++++++++++++++++++++
  3 files changed, 47 insertions(+)
include/linux/pci-ats.h +3 -0
index 75c6c86cf09dc..d14ba727d38b3 100644
@@ -12,6 +12,7 @@ int pci_prepare_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);
+bool pci_ats_always_on(struct pci_dev *dev);
  #else /* CONFIG_PCI_ATS */
  static inline bool pci_ats_supported(struct pci_dev *d)
  { return false; }
@@ -24,6 +25,8 @@ static inline int pci_ats_queue_depth(struct pci_dev *d)
  { return -ENODEV; }
  static inline int pci_ats_page_aligned(struct pci_dev *dev)
  { return 0; }
+static inline bool pci_ats_always_on(struct pci_dev *dev)
+{ return false; }
  #endif /* CONFIG_PCI_ATS */
 
  #ifdef CONFIG_PCI_PRI
include/uapi/linux/pci_regs.h +1 -0
index 14f634ab9350d..6ac45be1008b8 100644
@@ -1349,6 +1349,7 @@
  /* CXL r4.0, 8.1.3: PCIe DVSEC for CXL Device */
  #define PCI_DVSEC_CXL_DEVICE				0
  #define  PCI_DVSEC_CXL_CAP				0xA
+#define   PCI_DVSEC_CXL_CACHE_CAPABLE			_BITUL(0)
  #define   PCI_DVSEC_CXL_MEM_CAPABLE			_BITUL(2)
  #define   PCI_DVSEC_CXL_HDM_COUNT			__GENMASK(5, 4)
  #define  PCI_DVSEC_CXL_CTRL				0xC
drivers/pci/ats.c +43 -0
index ec6c8dbdc5e9c..fc871858b65bc 100644
@@ -205,6 +205,49 @@ int pci_ats_page_aligned(struct pci_dev *pdev)
  	return 0;
  }
 
+/*
+ * CXL r4.0, sec 3.2.5.13 Memory Type on CXL.cache notes: to source requests on
+ * CXL.cache, devices need to get the Host Physical Address (HPA) from the Host
+ * by means of an ATS request on CXL.io.
+ *
+ * In other words, CXL.cache devices cannot access host physical memory without
+ * ATS.
+ */
+static bool pci_cxl_ats_always_on(struct pci_dev *pdev)
+{
+	int offset;
+	u16 cap;
+
+	offset = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL,
+					   PCI_DVSEC_CXL_DEVICE);
+	if (!offset)
+		return false;
+
+	if (pci_read_config_word(pdev, offset + PCI_DVSEC_CXL_CAP, &cap))
+		return false;
+
+	return cap & PCI_DVSEC_CXL_CACHE_CAPABLE;
+}
+
+/**
+ * pci_ats_always_on - Whether the PCI device requires ATS to be always enabled
+ * @pdev: the PCI device
+ *
+ * Returns true, if the PCI device requires ATS for basic functional operation.
+ */
+bool pci_ats_always_on(struct pci_dev *pdev)
+{
+	if (pci_ats_disabled() || !pci_ats_supported(pdev))
+		return false;
+
+	/* A VF inherits its PF's requirement for ATS function */
+	if (pdev->is_virtfn)
+		pdev = pci_physfn(pdev);
+
+	return pci_cxl_ats_always_on(pdev);
+}
+EXPORT_SYMBOL_GPL(pci_ats_always_on);
+
  #ifdef CONFIG_PCI_PRI
  void pci_pri_init(struct pci_dev *pdev)
  {
#18 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

Quoted 9 lines · Yi Liu
Quoted 4 lines · Nicolin Chen

Controlled by the IOMMU driver, ATS is usually enabled "on demand" when a given PASID on a device is attached to an I/O page table. This is working even when a device has no translation on its RID (i.e., the RID is IOMMU bypassed).

nit: this description seems not accurate. Intel iommu driver enables ATS in the probe_device() phase. mind tweak a bit to avoid misleading message. :)

It probably shouldn't do this, it should follow ARM and have it dynamic during domain attach.

For security we need ATS disabled for blocking domains at a minimum.

Jason

#19 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

Quoted 7 lines · Jason Gunthorpe
Quoted 6 lines · Yi Liu
Quoted 4 lines · Nicolin Chen

Controlled by the IOMMU driver, ATS is usually enabled "on demand" when a given PASID on a device is attached to an I/O page table. This is working even when a device has no translation on its RID (i.e., the RID is IOMMU bypassed).

Quoted 6 lines
Quoted 3 lines

nit: this description seems not accurate. Intel iommu driver enables ATS in the probe_device() phase. mind tweak a bit to avoid misleading message. :)

It probably shouldn't do this, it should follow ARM and have it dynamic during domain attach.

Agreed that making it dynamic during domain attach is a better direction. However, even framing it that way, the description tying ATS enablement to PASID attachment is still architecturally specific to ARM SMMUv3, and doesn't hold as a general statement. :)

Quoted 1 line

For security we need ATS disabled for blocking domains at a minimum.

Agreed on the security model.

One more data point worth discussing: today Intel's IOMMU driver enables ATS at probe time, which has two effects — enabling the PCI ATS capability on the device, and setting the DTE bit in the scalable-mode PASID-table entry. When a RID or PASID is subsequently attached to a blocking domain, the corresponding PASID-table entry has its Present (P) bit cleared.

Per the VT-d spec (condition SPT.2), with P=0:

- Translation Requests (with or without PASID) complete successfully, but return R=W=U=S=0 to the device — effectively a no-access result.
- Untranslated Requests receive UR.
- Translated Requests are N/A.

So while neither the PCI ATS capability nor the DTE bit is explicitly cleared when a blocking domain is attached, ATS-related transactions don't produce any usable result from the device's perspective.

Does this hardware behavior satisfy the security expectation you have in mind? Or do you still require that both the DTE bit and the PCI ATS capability be explicitly disabled when a blocking domain is in effect?

Regards,
Yi Liu

#20 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

Quoted 3 lines · Yi Liu

Does this hardware behavior satisfy the security expectation you have in mind? Or do you still require that both the DTE bit and the PCI ATS capability be explicitly disabled when a blocking domain is in effect?

If the HW rejects translated TLPs then you should be clearing the ATS enable bit in the device config space prior to rejecting them

But it does seem secure enough as-is.

Jason

#21 lore raw

Re: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

Quoted 10 lines · Jason Gunthorpe
Quoted 3 lines · Yi Liu

Does this hardware behavior satisfy the security expectation you have in mind? Or do you still require that both the DTE bit and the PCI ATS capability be explicitly disabled when a blocking domain is in effect?

If the HW rejects translated TLPs then you should be clearing the ATS enable bit in the device config space prior to rejecting them

But it does seem secure enough as-is.

got it. thanks for the thoughts.

#22 lore raw

RE: [PATCH v4 1/3] PCI: Allow ATS to be always on for CXL.cache capable devices

In reply to Jason Gunthorpe
Quoted 5 lines

From: Jason Gunthorpe <[email protected]>
Sent: Thursday, May 21, 2026 9:06 PM

On Thu, May 21, 2026 at 03:31:46PM +0800, Yi Liu wrote:

Quoted 9 lines
Quoted 3 lines

Does this hardware behavior satisfy the security expectation you have in mind? Or do you still require that both the DTE bit and the PCI ATS capability be explicitly disabled when a blocking domain is in effect?

If the HW rejects translated TLPs then you should be clearing the ATS enable bit in the device config space prior to rejecting them

But it does seem secure enough as-is.

yeah we need disable ATS explicitly instead of relying on some implicit behavior...

#23 lore raw

PATCH v4 2/3 PCI: Allow ATS to be always on for pre-CXL devices

In reply to Nicolin Chen

Some NVIDIA GPU/NIC devices, though they don't implement CXL config space, have many CXL-like properties. Call this kind "pre-CXL".

Similar to CXL.cache capability, these pre-CXL devices also require the ATS function even when their RIDs are IOMMU bypassed, i.e. keep ATS "always on" v.s. "on demand" when a non-zero PASID line gets enabled in SVA use cases.

Introduce pci_dev_specific_ats_always_on() quirk function to scan a list of IDs for these devices. Then, include it in pci_ats_always_on().

---
 drivers/pci/pci.h    |  9 +++++++++
 drivers/pci/ats.c    |  3 ++-
 drivers/pci/quirks.c | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 1 deletion(-)
drivers/pci/pci.h +9 -0
index 4a14f88e543a2..4e0077478cd7a 100644
@@ -1155,6 +1155,15 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, bool probe)
 }
 #endif
 
+#if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_PCI_ATS)
+bool pci_dev_specific_ats_always_on(struct pci_dev *dev);
+#else
+static inline bool pci_dev_specific_ats_always_on(struct pci_dev *dev)
+{
+	return false;
+}
+#endif
+
 #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64)
 int acpi_get_rc_resources(struct device *dev, const char *hid, u16 segment,
 			  struct resource *res);
drivers/pci/ats.c +2 -1
index fc871858b65bc..3846447ea322f 100644
@@ -244,7 +244,8 @@ bool pci_ats_always_on(struct pci_dev *pdev)
 	if (pdev->is_virtfn)
 		pdev = pci_physfn(pdev);
 
-	return pci_cxl_ats_always_on(pdev);
+	return pci_cxl_ats_always_on(pdev) ||
+	       pci_dev_specific_ats_always_on(pdev);
 }
 EXPORT_SYMBOL_GPL(pci_ats_always_on);
 
drivers/pci/quirks.c +38 -0
index caaed1a01dc02..887babba97cc7 100644
@@ -5715,6 +5715,44 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1457, quirk_intel_e2000_no_ats);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1459, quirk_intel_e2000_no_ats);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145a, quirk_intel_e2000_no_ats);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145c, quirk_intel_e2000_no_ats);
+
+static bool quirk_nvidia_gpu_ats_always_on(struct pci_dev *pdev)
+{
+	switch (pdev->device) {
+	case 0x2e00 ... 0x2e3f: /* GB20B */
+		return true;
+	}
+	return false;
+}
+
+static const struct pci_dev_ats_always_on {
+	u16 vendor;
+	u16 device;
+	bool (*ats_always_on)(struct pci_dev *dev);
+} pci_dev_ats_always_on[] = {
+	/* NVIDIA GPUs */
+	{ PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, quirk_nvidia_gpu_ats_always_on },
+	/* NVIDIA CX10 Family NVlink-C2C */
+	{ PCI_VENDOR_ID_MELLANOX, 0x2101, NULL },
+	{ 0 }
+};
+
+/* Some pre-CXL devices require ATS when it is IOMMU-bypassed */
+bool pci_dev_specific_ats_always_on(struct pci_dev *pdev)
+{
+	const struct pci_dev_ats_always_on *i;
+
+	for (i = pci_dev_ats_always_on; i->vendor; i++) {
+		if (i->vendor != pdev->vendor)
+			continue;
+		if (i->ats_always_on && i->ats_always_on(pdev))
+			return true;
+		if (!i->ats_always_on && i->device == pdev->device)
+			return true;
+	}
+
+	return false;
+}
 #endif /* CONFIG_PCI_ATS */
 
 /* Freescale PCIe doesn't support MSI in RC mode */
Signature · 1 line
2.43.0
Reviewed-by on v4 2/3
#24 lore raw

Re: [PATCH v4 2/3] PCI: Allow ATS to be always on for pre-CXL devices

Quoted 10 lines · Nicolin Chen

Some NVIDIA GPU/NIC devices, though they don't implement CXL config space, have many CXL-like properties. Call this kind "pre-CXL".

Similar to CXL.cache capability, these pre-CXL devices also require the ATS function even when their RIDs are IOMMU bypassed, i.e. keep ATS "always on" v.s. "on demand" when a non-zero PASID line gets enabled in SVA use cases.

Introduce pci_dev_specific_ats_always_on() quirk function to scan a list of IDs for these devices. Then, include it in pci_ats_always_on().

Quoted 7 lines
Quoted 89 lines
---
 drivers/pci/pci.h    |  9 +++++++++
 drivers/pci/ats.c    |  3 ++-
 drivers/pci/quirks.c | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 1 deletion(-)
drivers/pci/pci.h +9 -0
index 4a14f88e543a2..4e0077478cd7a 100644
@@ -1155,6 +1155,15 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, bool probe)
 }
 #endif
 
+#if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_PCI_ATS)
+bool pci_dev_specific_ats_always_on(struct pci_dev *dev);
+#else
+static inline bool pci_dev_specific_ats_always_on(struct pci_dev *dev)
+{
+	return false;
+}
+#endif
+
 #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64)
 int acpi_get_rc_resources(struct device *dev, const char *hid, u16 segment,
 			  struct resource *res);
drivers/pci/ats.c +2 -1
index fc871858b65bc..3846447ea322f 100644
@@ -244,7 +244,8 @@ bool pci_ats_always_on(struct pci_dev *pdev)
 	if (pdev->is_virtfn)
 		pdev = pci_physfn(pdev);
 
-	return pci_cxl_ats_always_on(pdev);
+	return pci_cxl_ats_always_on(pdev) ||
+	       pci_dev_specific_ats_always_on(pdev);
 }
 EXPORT_SYMBOL_GPL(pci_ats_always_on);
 
drivers/pci/quirks.c +38 -0
index caaed1a01dc02..887babba97cc7 100644
@@ -5715,6 +5715,44 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1457, quirk_intel_e2000_no_ats);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1459, quirk_intel_e2000_no_ats);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145a, quirk_intel_e2000_no_ats);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145c, quirk_intel_e2000_no_ats);
+
+static bool quirk_nvidia_gpu_ats_always_on(struct pci_dev *pdev)
+{
+	switch (pdev->device) {
+	case 0x2e00 ... 0x2e3f: /* GB20B */
+		return true;
+	}
+	return false;
+}
+
+static const struct pci_dev_ats_always_on {
+	u16 vendor;
+	u16 device;
+	bool (*ats_always_on)(struct pci_dev *dev);
+} pci_dev_ats_always_on[] = {
+	/* NVIDIA GPUs */
+	{ PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, quirk_nvidia_gpu_ats_always_on },
+	/* NVIDIA CX10 Family NVlink-C2C */
+	{ PCI_VENDOR_ID_MELLANOX, 0x2101, NULL },
+	{ 0 }
+};
+
+/* Some pre-CXL devices require ATS when it is IOMMU-bypassed */
+bool pci_dev_specific_ats_always_on(struct pci_dev *pdev)
+{
+	const struct pci_dev_ats_always_on *i;
+
+	for (i = pci_dev_ats_always_on; i->vendor; i++) {
+		if (i->vendor != pdev->vendor)
+			continue;
+		if (i->ats_always_on && i->ats_always_on(pdev))
+			return true;
+		if (!i->ats_always_on && i->device == pdev->device)
+			return true;
+	}
+
+	return false;
+}
 #endif /* CONFIG_PCI_ATS */
 
 /* Freescale PCIe doesn't support MSI in RC mode */
Reviewed-by on v4 2/3
#25 lore raw

Re: [PATCH v4 2/3] PCI: Allow ATS to be always on for pre-CXL devices

In reply to Nicolin Chen
Quoted 17 lines · Nicolin Chen

Some NVIDIA GPU/NIC devices, though they don't implement CXL config space, have many CXL-like properties. Call this kind "pre-CXL".

Similar to CXL.cache capability, these pre-CXL devices also require the ATS function even when their RIDs are IOMMU bypassed, i.e. keep ATS "always on" v.s. "on demand" when a non-zero PASID line gets enabled in SVA use cases.

Introduce pci_dev_specific_ats_always_on() quirk function to scan a list of IDs for these devices. Then, include it in pci_ats_always_on().

Quoted 5 lines
---
  drivers/pci/pci.h    |  9 +++++++++
  drivers/pci/ats.c    |  3 ++-
  drivers/pci/quirks.c | 38 ++++++++++++++++++++++++++++++++++++++
  3 files changed, 49 insertions(+), 1 deletion(-)
Quoted 83 lines
drivers/pci/pci.h +9 -0
index 4a14f88e543a2..4e0077478cd7a 100644
@@ -1155,6 +1155,15 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, bool probe)
  }
  #endif
 
+#if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_PCI_ATS)
+bool pci_dev_specific_ats_always_on(struct pci_dev *dev);
+#else
+static inline bool pci_dev_specific_ats_always_on(struct pci_dev *dev)
+{
+	return false;
+}
+#endif
+
  #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64)
  int acpi_get_rc_resources(struct device *dev, const char *hid, u16 segment,
  			  struct resource *res);
drivers/pci/ats.c +2 -1
index fc871858b65bc..3846447ea322f 100644
@@ -244,7 +244,8 @@ bool pci_ats_always_on(struct pci_dev *pdev)
  	if (pdev->is_virtfn)
  		pdev = pci_physfn(pdev);
 
-	return pci_cxl_ats_always_on(pdev);
+	return pci_cxl_ats_always_on(pdev) ||
+	       pci_dev_specific_ats_always_on(pdev);
  }
  EXPORT_SYMBOL_GPL(pci_ats_always_on);
 
drivers/pci/quirks.c +38 -0
index caaed1a01dc02..887babba97cc7 100644
@@ -5715,6 +5715,44 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1457, quirk_intel_e2000_no_ats);
  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1459, quirk_intel_e2000_no_ats);
  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145a, quirk_intel_e2000_no_ats);
  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x145c, quirk_intel_e2000_no_ats);
+
+static bool quirk_nvidia_gpu_ats_always_on(struct pci_dev *pdev)
+{
+	switch (pdev->device) {
+	case 0x2e00 ... 0x2e3f: /* GB20B */
+		return true;
+	}
+	return false;
+}
+
+static const struct pci_dev_ats_always_on {
+	u16 vendor;
+	u16 device;
+	bool (*ats_always_on)(struct pci_dev *dev);
+} pci_dev_ats_always_on[] = {
+	/* NVIDIA GPUs */
+	{ PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, quirk_nvidia_gpu_ats_always_on },
+	/* NVIDIA CX10 Family NVlink-C2C */
+	{ PCI_VENDOR_ID_MELLANOX, 0x2101, NULL },
+	{ 0 }
+};
+
+/* Some pre-CXL devices require ATS when it is IOMMU-bypassed */
+bool pci_dev_specific_ats_always_on(struct pci_dev *pdev)
+{
+	const struct pci_dev_ats_always_on *i;
+
+	for (i = pci_dev_ats_always_on; i->vendor; i++) {
+		if (i->vendor != pdev->vendor)
+			continue;
+		if (i->ats_always_on && i->ats_always_on(pdev))
+			return true;
+		if (!i->ats_always_on && i->device == pdev->device)
+			return true;
+	}
+
+	return false;
+}
  #endif /* CONFIG_PCI_ATS */
 
  /* Freescale PCIe doesn't support MSI in RC mode */
#26 lore raw

Re: [PATCH v4 2/3] PCI: Allow ATS to be always on for pre-CXL devices

In reply to Nicolin Chen
Quoted 9 lines · Nicolin Chen

Some NVIDIA GPU/NIC devices, though they don't implement CXL config space, have many CXL-like properties. Call this kind "pre-CXL".

Similar to CXL.cache capability, these pre-CXL devices also require the ATS function even when their RIDs are IOMMU bypassed, i.e. keep ATS "always on" v.s. "on demand" when a non-zero PASID line gets enabled in SVA use cases.
...

+/* Some pre-CXL devices require ATS when it is IOMMU-bypassed */

I guess these devices are purely PCIe, with no actual CXL transactions, so a hint here about what leads to the ATS requirement would be useful. It sounds like an actual functional requirement, not just a performance optimization.

Quoted 21 lines
drivers/pci/quirks.c +15 -0
+bool pci_dev_specific_ats_always_on(struct pci_dev *pdev)
+{
+	const struct pci_dev_ats_always_on *i;
+
+	for (i = pci_dev_ats_always_on; i->vendor; i++) {
+		if (i->vendor != pdev->vendor)
+			continue;
+		if (i->ats_always_on && i->ats_always_on(pdev))
+			return true;
+		if (!i->ats_always_on && i->device == pdev->device)
+			return true;
+	}
+
+	return false;
+}
 #endif /* CONFIG_PCI_ATS */
 
 /* Freescale PCIe doesn't support MSI in RC mode */
Signature · 1 line
2.43.0
#27 lore raw

Re: [PATCH v4 2/3] PCI: Allow ATS to be always on for pre-CXL devices

Quoted 9 lines · Bjorn Helgaas
Quoted 7 lines · Nicolin Chen

Some NVIDIA GPU/NIC devices, though they don't implement CXL config space, have many CXL-like properties. Call this kind "pre-CXL".

Similar to CXL.cache capability, these pre-CXL devices also require the ATS function even when their RIDs are IOMMU bypassed, i.e. keep ATS "always on" v.s. "on demand" when a non-zero PASID line gets enabled in SVA use cases.
...

Quoted 5 lines
Quoted 1 line

+/* Some pre-CXL devices require ATS when it is IOMMU-bypassed */

I guess these devices are purely PCIe, with no actual CXL transactions, so a hint here about what leads to the ATS requirement would be useful.

Not quite, they are not actually "real" PCIe devices, these are devices that present to the system as PCIe, can issue CXL like .cache operations and don't use PCIe electrical.

Jason

#28 lore raw

PATCH v4 3/3 iommu/arm-smmu-v3: Allow ATS to be always on

In reply to Nicolin Chen

When a device's default substream attaches to an identity domain, the SMMU driver currently sets the device's STE between two modes:

Mode 1: Cfg=Translate, S1DSS=Bypass, EATS=1
Mode 2: Cfg=bypass (EATS is ignored by HW)

When there is an active PASID (non-default substream), mode 1 is used. And when there is no PASID support or no active PASID, mode 2 is used.

The driver will also downgrade an STE from mode 1 to mode 2, when the last active substream becomes inactive.

However, there are PCIe devices that demand ATS to be always on. For these devices, their STEs have to use the mode 1 as HW ignores EATS with mode 2.

Change the driver accordingly:
- always use the mode 1
- never downgrade to mode 2
- allocate and retain a CD table (see note below)

Note that these devices might not support PASID, i.e. doing non-PASID ATS. In such a case, the ssid_bits is set to 0. However, s1cdmax must be set to a !0 value in order to keep the S1DSS field effective. Thus, when a master requires ats_always_on, set its s1cdmax to at least 1, meaning that the CD table will have a dummy entry (SSID=1) that will never be used.

Now for these devices, arm_smmu_cdtab_allocated() will always return true, v.s. false prior to this change. When its default substream is attached to an IDENTITY domain, its first CD is NULL in the table, which is a totally valid case. Thus, add "!master->ats_always_on" to the condition.

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 75 ++++++++++++++++++---
 2 files changed, 68 insertions(+), 8 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -0
index ef42df4753ec4..8c3600f4364c5 100644
@@ -943,6 +943,7 @@ struct arm_smmu_master {
 	bool				ats_enabled : 1;
 	bool				ste_ats_enabled : 1;
 	bool				stall_enabled;
+	bool				ats_always_on;
 	unsigned int			ssid_bits;
 	unsigned int			iopf_refcount;
 };
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +67 -8

121 lines not on this page · raw message

Signature · 1 line
2.43.0
Reviewed-by on v4 3/3
#29 lore raw

Re: [PATCH v4 3/3] iommu/arm-smmu-v3: Allow ATS to be always on

Quoted 31 lines · Nicolin Chen

When a device's default substream attaches to an identity domain, the SMMU driver currently sets the device's STE between two modes:

Mode 1: Cfg=Translate, S1DSS=Bypass, EATS=1
Mode 2: Cfg=bypass (EATS is ignored by HW)

When there is an active PASID (non-default substream), mode 1 is used. And when there is no PASID support or no active PASID, mode 2 is used.

The driver will also downgrade an STE from mode 1 to mode 2, when the last active substream becomes inactive.

However, there are PCIe devices that demand ATS to be always on. For these devices, their STEs have to use the mode 1 as HW ignores EATS with mode 2.

Change the driver accordingly:
- always use the mode 1
- never downgrade to mode 2
- allocate and retain a CD table (see note below)

Note that these devices might not support PASID, i.e. doing non-PASID ATS. In such a case, the ssid_bits is set to 0. However, s1cdmax must be set to a !0 value in order to keep the S1DSS field effective. Thus, when a master requires ats_always_on, set its s1cdmax to at least 1, meaning that the CD table will have a dummy entry (SSID=1) that will never be used.

Now for these devices, arm_smmu_cdtab_allocated() will always return true, v.s. false prior to this change. When its default substream is attached to an IDENTITY domain, its first CD is NULL in the table, which is a totally valid case. Thus, add "!master->ats_always_on" to the condition.

Quoted 6 lines
Quoted 149 lines
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 75 ++++++++++++++++++---
 2 files changed, 68 insertions(+), 8 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -0
index ef42df4753ec4..8c3600f4364c5 100644
@@ -943,6 +943,7 @@ struct arm_smmu_master {
 	bool				ats_enabled : 1;
 	bool				ste_ats_enabled : 1;
 	bool				stall_enabled;
+	bool				ats_always_on;
 	unsigned int			ssid_bits;
 	unsigned int			iopf_refcount;
 };
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +67 -8
index e8d7dbe495f03..d478f148cd34b 100644
@@ -1742,8 +1742,11 @@ void arm_smmu_clear_cd(struct arm_smmu_master *master, ioasid_t ssid)
 	if (!arm_smmu_cdtab_allocated(&master->cd_table))
 		return;
 	cdptr = arm_smmu_get_cd_ptr(master, ssid);
-	if (WARN_ON(!cdptr))
+	if (!cdptr) {
+		/* Only ats_always_on allows a NULL CD on default substream */
+		WARN_ON(!master->ats_always_on || ssid);
 		return;
+	}
 	arm_smmu_write_cd_entry(master, ssid, cdptr, &target);
 }
 
@@ -1756,6 +1759,22 @@ static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master)
 	struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
 
 	cd_table->s1cdmax = master->ssid_bits;
+
+	/*
+	 * When a device doesn't support PASID (non default SSID), ssid_bits is
+	 * set to 0. This also sets S1CDMAX to 0, which disables the substreams
+	 * and ignores the S1DSS field.
+	 *
+	 * On the other hand, if a device demands ATS to be always on even when
+	 * its default substream is IOMMU bypassed, it has to use EATS that is
+	 * only effective with an STE (CFG=S1translate, S1DSS=Bypass). For such
+	 * use cases, S1CDMAX has to be !0, in order to make use of S1DSS/EATS.
+	 *
+	 * Set S1CDMAX no lower than 1. This would add a dummy substream in the
+	 * CD table but it should never be used by an actual CD.
+	 */
+	if (master->ats_always_on)
+		cd_table->s1cdmax = max_t(u8, cd_table->s1cdmax, 1);
 	max_contexts = 1 << cd_table->s1cdmax;
 
 	if (!(smmu->features & ARM_SMMU_FEAT_2_LVL_CDTAB) ||
@@ -3851,7 +3870,8 @@ static int arm_smmu_blocking_set_dev_pasid(struct iommu_domain *new_domain,
 	 * When the last user of the CD table goes away downgrade the STE back
 	 * to a non-cd_table one, by re-attaching its sid_domain.
 	 */
-	if (!arm_smmu_ssids_in_use(&master->cd_table)) {
+	if (!master->ats_always_on &&
+	    !arm_smmu_ssids_in_use(&master->cd_table)) {
 		struct iommu_domain *sid_domain =
 			iommu_driver_get_domain_for_dev(master->dev);
 
@@ -3875,6 +3895,8 @@ static void arm_smmu_attach_dev_ste(struct iommu_domain *domain,
 		.old_domain = old_domain,
 		.ssid = IOMMU_NO_PASID,
 	};
+	bool ats_always_on = master->ats_always_on &&
+			     s1dss != STRTAB_STE_1_S1DSS_TERMINATE;
 
 	/*
 	 * Do not allow any ASID to be changed while are working on the STE,
@@ -3886,7 +3908,7 @@ static void arm_smmu_attach_dev_ste(struct iommu_domain *domain,
 	 * If the CD table is not in use we can use the provided STE, otherwise
 	 * we use a cdtable STE with the provided S1DSS.
 	 */
-	if (arm_smmu_ssids_in_use(&master->cd_table)) {
+	if (ats_always_on || arm_smmu_ssids_in_use(&master->cd_table)) {
 		/*
 		 * If a CD table has to be present then we need to run with ATS
 		 * on because we have to assume a PASID is using ATS. For
@@ -4215,6 +4237,42 @@ static void arm_smmu_remove_master(struct arm_smmu_master *master)
 	kfree(master->build_invs);
 }
 
+static int arm_smmu_master_prepare_ats(struct arm_smmu_master *master)
+{
+	bool s1p = master->smmu->features & ARM_SMMU_FEAT_TRANS_S1;
+	unsigned int stu = __ffs(master->smmu->pgsize_bitmap);
+	struct pci_dev *pdev;
+	int ret;
+
+	if (!arm_smmu_ats_supported(master))
+		return 0;
+
+	pdev = to_pci_dev(master->dev);
+
+	if (!pci_ats_always_on(pdev))
+		goto out_prepare;
+
+	/*
+	 * S1DSS is required for ATS to be always on for identity domain cases.
+	 * However, the S1DSS field is ignored if !IDR0_S1P or !IDR1_SSIDSIZE.
+	 */
+	if (!s1p || !master->smmu->ssid_bits) {
+		dev_info_once(master->dev,
+			      "SMMU doesn't support ATS to be always on\n");
+		goto out_prepare;
+	}
+
+	master->ats_always_on = true;
+
+	ret = arm_smmu_alloc_cd_tables(master);
+	if (ret)
+		return ret;
+
+out_prepare:
+	pci_prepare_ats(pdev, stu);
+	return 0;
+}
+
 static struct iommu_device *arm_smmu_probe_device(struct device *dev)
 {
 	int ret;
@@ -4263,14 +4321,15 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
 	    smmu->features & ARM_SMMU_FEAT_STALL_FORCE)
 		master->stall_enabled = true;
 
-	if (dev_is_pci(dev)) {
-		unsigned int stu = __ffs(smmu->pgsize_bitmap);
-
-		pci_prepare_ats(to_pci_dev(dev), stu);
-	}
+	ret = arm_smmu_master_prepare_ats(master);
+	if (ret)
+		goto err_disable_pasid;
 
 	return &smmu->iommu;
 
+err_disable_pasid:
+	arm_smmu_disable_pasid(master);
+	arm_smmu_remove_master(master);
 err_free_master:
 	kfree(master);
 	return ERR_PTR(ret);