[PATCH v2 0/5] iommu/amd: Refactors for ATS robustness

[PATCH v2 0/5] iommu/amd: Refactors for ATS robustness

[Pranjal Shrivastava]

This series forms the second half of the subsystem-wide ATS robustness
updates. The first part (focusing on the core subsystem, Intel, and ARM
SMMUv3) has already been merged upstream [1]. This half addresses the
AMD IOMMU driver and standardizes the PCI ATS API.

In v2, the series has been significantly restructured based on feedback
on v1. 

[v2]

 - Patch 1 Refctors the probe path, isolating capabilities into 
   iommu_init_device_caps().
 - Patch 2 renames iommu_ignore_device() to iommu_disable_device_dma().
   Following Jason's suggestion, it invalidates the hardware DTE by
   clearing the Valid bit (lower 128 bits) followed by the upper
   128 bits.  
 - Patch 3 splits the probe error paths to ensure that devices with
   config failures (like PD_MODE_NONE or ATS mismatches) can preserve
   their rlookup_table entries, successfully keeping IRQ remapping 
   functional for bypassed devices.
 - Patch 4 implements the "Fail Hard" pattern for ATS in the AMD driver,
   failing the probe and throwing a WARN_ON() upon ATS configuration or
   enablement failures.
 - Patch 5 enforces the checking of pci_ats_supported() prior to calling
   pci_prepare_ats() across the entire kernel PCI subsystem.

[v1]
 - https://lore.kernel.org/all/20260601134204.2150602-1-praan@google.com/

Thanks,
Praan

[1] https://lore.kernel.org/all/20260615235037.259909-1-praan@google.com/

Pranjal Shrivastava (5):
  iommu/amd: Refactor device probe and capability initialization
  iommu/amd: Fix DTE clearing and rename iommu_ignore_device()
  iommu/amd: Split probe error paths to preserve IRQ remapping
  iommu/amd: Fail probe on ATS configuration failure
  PCI/ATS: Mandate checking pci_ats_supported() before pci_prepare_ats()

 drivers/iommu/amd/iommu.c | 196 +++++++++++++++++++++++---------------
 drivers/pci/ats.c         |   6 +-
 2 files changed, 122 insertions(+), 80 deletions(-)

-- 
2.55.0.691.gc56d675ccc-goog

[PATCH v2 1/5] iommu/amd: Refactor device probe and capability initialization

[Pranjal Shrivastava]

Restructure the device probe path to improve readability and prepare for
cleaner error handling. Refactor check_device() into iommu_lookup_device
to explicitly validate and return the amd_iommu ptr & devid. Refactor
iommu_init_device() to return the allocated dev_data. Consolidate all
PCI cap inits (MSI domains, PASID, ATS) into a new helper:
iommu_init_device_caps().

Suggested-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/amd/iommu.c | 114 +++++++++++++++++++-------------------
 1 file changed, 56 insertions(+), 58 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 4c31294fabc5..9b8ad131ba79 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -675,7 +675,8 @@ static void pdev_disable_caps(struct pci_dev *pdev)
  * This function checks if the driver got a valid device from the caller to
  * avoid dereferencing invalid pointers.
  */
-static bool check_device(struct device *dev)
+static bool iommu_lookup_device(struct device *dev,
+				struct amd_iommu **iommu_out, u16 *devid_out)
 {
 	struct amd_iommu_pci_seg *pci_seg;
 	struct amd_iommu *iommu;
@@ -690,7 +691,7 @@ static bool check_device(struct device *dev)
 	devid = PCI_SBDF_TO_DEVID(sbdf);
 
 	iommu = rlookup_amd_iommu(dev);
-	if (!iommu)
+	if (!iommu || !iommu->iommu.ops)
 		return false;
 
 	/* Out of our scope? */
@@ -698,47 +699,33 @@ static bool check_device(struct device *dev)
 	if (devid > pci_seg->last_bdf)
 		return false;
 
+	*iommu_out = iommu;
+	*devid_out = devid;
 	return true;
 }
 
-static int iommu_init_device(struct amd_iommu *iommu, struct device *dev)
+static struct iommu_dev_data *iommu_init_device(struct amd_iommu *iommu,
+						struct device *dev, u16 devid)
 {
 	struct iommu_dev_data *dev_data;
-	int devid, sbdf;
-
-	if (dev_iommu_priv_get(dev))
-		return 0;
 
-	sbdf = get_device_sbdf_id(dev);
-	if (sbdf < 0)
-		return sbdf;
-
-	devid = PCI_SBDF_TO_DEVID(sbdf);
 	dev_data = find_dev_data(iommu, devid);
 	if (!dev_data)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	dev_data->dev = dev;
 
 	/*
-	 * The dev_iommu_priv_set() needes to be called before setup_aliases.
+	 * The dev_iommu_priv_set() needs to be called before setup_aliases.
 	 * Otherwise, subsequent call to dev_iommu_priv_get() will fail.
 	 */
 	dev_iommu_priv_set(dev, dev_data);
 	setup_aliases(iommu, dev);
 
-	/*
-	 * By default we use passthrough mode for IOMMUv2 capable device.
-	 * But if amd_iommu=force_isolation is set (e.g. to debug DMA to
-	 * invalid address), we ignore the capability for the device so
-	 * it'll be forced to go into translation mode.
-	 */
-	if ((iommu_default_passthrough() || !amd_iommu_force_isolation) &&
-	    dev_is_pci(dev) && amd_iommu_gt_ppr_supported()) {
-		dev_data->flags = pdev_get_caps(to_pci_dev(dev));
-	}
+	/* Wait for DTE updates to go through */
+	iommu_completion_wait(iommu);
 
-	return 0;
+	return dev_data;
 }
 
 static void iommu_ignore_device(struct amd_iommu *iommu, struct device *dev)
@@ -2477,49 +2464,70 @@ static void detach_device(struct device *dev)
 	mutex_unlock(&dev_data->mutex);
 }
 
+static void iommu_init_device_caps(struct iommu_dev_data *dev_data,
+				   struct device *dev,
+				   struct amd_iommu *iommu)
+{
+	if (FEATURE_NUM_INT_REMAP_SUP_2K(amd_iommu_efr2))
+		dev_data->max_irqs = MAX_IRQS_PER_TABLE_2K;
+	else
+		dev_data->max_irqs = MAX_IRQS_PER_TABLE_512;
+
+	amd_iommu_set_pci_msi_domain(dev, iommu);
+
+	if (!dev_is_pci(dev))
+		return;
+
+	/*
+	 * By default we use passthrough mode for IOMMUv2 capable device.
+	 * But if amd_iommu=force_isolation is set (e.g. to debug DMA to
+	 * invalid address), we ignore the capability for the device so
+	 * it'll be forced to go into translation mode.
+	 */
+	if ((iommu_default_passthrough() || !amd_iommu_force_isolation) &&
+	    amd_iommu_gt_ppr_supported()) {
+		dev_data->flags = pdev_get_caps(to_pci_dev(dev));
+	}
+
+	/*
+	 * If IOMMU and device supports PASID then it will contain max
+	 * supported PASIDs, else it will be zero.
+	 */
+	if (amd_iommu_pasid_supported() &&
+	    pdev_pasid_supported(dev_data)) {
+		dev_data->max_pasids = min_t(u32, iommu->iommu.max_pasids,
+					     pci_max_pasids(to_pci_dev(dev)));
+	}
+
+	pci_prepare_ats(to_pci_dev(dev), PAGE_SHIFT);
+}
+
 static struct iommu_device *amd_iommu_probe_device(struct device *dev)
 {
 	struct iommu_device *iommu_dev;
 	struct amd_iommu *iommu;
 	struct iommu_dev_data *dev_data;
 	int ret;
+	u16 devid;
 
-	if (!check_device(dev))
-		return ERR_PTR(-ENODEV);
-
-	iommu = rlookup_amd_iommu(dev);
-	if (!iommu)
-		return ERR_PTR(-ENODEV);
-
-	/* Not registered yet? */
-	if (!iommu->iommu.ops)
+	if (!iommu_lookup_device(dev, &iommu, &devid))
 		return ERR_PTR(-ENODEV);
 
 	if (dev_iommu_priv_get(dev))
 		return &iommu->iommu;
 
-	ret = iommu_init_device(iommu, dev);
-	if (ret) {
+	dev_data = iommu_init_device(iommu, dev, devid);
+	if (IS_ERR(dev_data)) {
+		ret = PTR_ERR(dev_data);
 		dev_err(dev, "Failed to initialize - trying to proceed anyway\n");
 		iommu_dev = ERR_PTR(ret);
 		iommu_ignore_device(iommu, dev);
 		goto out_err;
 	}
 
-	amd_iommu_set_pci_msi_domain(dev, iommu);
+	iommu_init_device_caps(dev_data, dev, iommu);
 	iommu_dev = &iommu->iommu;
 
-	/*
-	 * If IOMMU and device supports PASID then it will contain max
-	 * supported PASIDs, else it will be zero.
-	 */
-	dev_data = dev_iommu_priv_get(dev);
-	if (amd_iommu_pasid_supported() && dev_is_pci(dev) &&
-	    pdev_pasid_supported(dev_data)) {
-		dev_data->max_pasids = min_t(u32, iommu->iommu.max_pasids,
-					     pci_max_pasids(to_pci_dev(dev)));
-	}
-
 	if (amd_iommu_pgtable == PD_MODE_NONE) {
 		pr_warn_once("%s: DMA translation not supported by iommu.\n",
 			     __func__);
@@ -2527,16 +2535,6 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
 		goto out_err;
 	}
 
-	iommu_completion_wait(iommu);
-
-	if (FEATURE_NUM_INT_REMAP_SUP_2K(amd_iommu_efr2))
-		dev_data->max_irqs = MAX_IRQS_PER_TABLE_2K;
-	else
-		dev_data->max_irqs = MAX_IRQS_PER_TABLE_512;
-
-	if (dev_is_pci(dev))
-		pci_prepare_ats(to_pci_dev(dev), PAGE_SHIFT);
-
 out_err:
 	return iommu_dev;
 }
-- 
2.55.0.691.gc56d675ccc-goog

Re: [PATCH v2 1/5] iommu/amd: Refactor device probe and capability initialization

[sashiko-bot]

> Restructure the device probe path to improve readability and prepare for
> cleaner error handling. Refactor check_device() into iommu_lookup_device
> to explicitly validate and return the amd_iommu ptr & devid. Refactor
> iommu_init_device() to return the allocated dev_data. Consolidate all
> PCI cap inits (MSI domains, PASID, ATS) into a new helper:
> iommu_init_device_caps().
> 
> Suggested-by: Vasant Hegde <vasant.hegde@amd.com>
> Signed-off-by: Pranjal Shrivastava <praan@google.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814015647.3370124-1-praan@google.com?part=1

[PATCH v2 2/5] iommu/amd: Fix DTE clearing and rename iommu_ignore_device()

[Pranjal Shrivastava]

The iommu_ignore_device() function currently uses memset() to manually
clear the primary Device Table Entry (DTE), which risks torn writes as
the hardware reads DTEs as atomic 256-bit qwords. Furthermore, clearing
the primary devid in the lookup table before calling setup_aliases()
causes rlookup_amd_iommu() to fail for aliases. This prevents clearing
the DTEs for DMA aliases.

Fix this by replacing the manual memset with a dedicated helper that
invalidates the DTE by clearing the lower 128 bits (having the Valid bit)
first, followed by the upper 128 bits. The cleared state is then
explicitly cloned to all aliases before the lookup tables are nullified.

Rename the function to iommu_disable_device_dma() more accurately
reflects its intent, as we still support IRQ remapping for these devices)

Fixes: 99fc4ac3d297 ("iommu/amd: Introduce per PCI segment alias_table")
Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/all/20260529153216.2AD1E1F00899@smtp.kernel.org/
Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/amd/iommu.c | 53 ++++++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 9b8ad131ba79..911a95527d74 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -728,22 +728,6 @@ static struct iommu_dev_data *iommu_init_device(struct amd_iommu *iommu,
 	return dev_data;
 }
 
-static void iommu_ignore_device(struct amd_iommu *iommu, struct device *dev)
-{
-	struct amd_iommu_pci_seg *pci_seg = iommu->pci_seg;
-	struct dev_table_entry *dev_table = get_dev_table(iommu);
-	int devid, sbdf;
-
-	sbdf = get_device_sbdf_id(dev);
-	if (sbdf < 0)
-		return;
-
-	devid = PCI_SBDF_TO_DEVID(sbdf);
-	pci_seg->rlookup_table[devid] = NULL;
-	memset(&dev_table[devid], 0, sizeof(struct dev_table_entry));
-
-	setup_aliases(iommu, dev);
-}
 
 
 /****************************************************************************
@@ -2233,6 +2217,41 @@ static void dev_update_dte(struct iommu_dev_data *dev_data, bool set)
 		clear_dte_entry(iommu, dev_data);
 }
 
+/*
+ * Invalidate a DTE by clearing the Valid bit first.
+ * Note: Not to be used on a fully probed device with
+ * live dev_data.
+ */
+static void amd_iommu_disable_dte(struct dev_table_entry *ptr)
+{
+	struct dev_table_entry new = {};
+
+	write_dte_lower128(ptr, &new);
+	write_dte_upper128(ptr, &new);
+}
+
+static void iommu_disable_device_dma(struct amd_iommu *iommu, struct device *dev)
+{
+	struct amd_iommu_pci_seg *pci_seg = iommu->pci_seg;
+	struct dev_table_entry *dev_table = get_dev_table(iommu);
+	int devid, sbdf;
+
+	sbdf = get_device_sbdf_id(dev);
+	if (sbdf < 0)
+		return;
+
+	devid = PCI_SBDF_TO_DEVID(sbdf);
+
+	/* Clear the primary DTE */
+	amd_iommu_disable_dte(&dev_table[devid]);
+
+	/* Clone the cleared DTE to all aliases before wiping the rlookup */
+	if (dev_iommu_priv_get(dev))
+		clone_aliases(iommu, dev);
+
+	pci_seg->rlookup_table[devid] = NULL;
+}
+
 /*
  * If domain is SVA capable then initialize GCR3 table. Also if domain is
  * in v2 page table mode then update GCR3[0].
@@ -2521,7 +2540,7 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
 		ret = PTR_ERR(dev_data);
 		dev_err(dev, "Failed to initialize - trying to proceed anyway\n");
 		iommu_dev = ERR_PTR(ret);
-		iommu_ignore_device(iommu, dev);
+		iommu_disable_device_dma(iommu, dev);
 		goto out_err;
 	}
 
-- 
2.55.0.691.gc56d675ccc-goog

Re: [PATCH v2 2/5] iommu/amd: Fix DTE clearing and rename iommu_ignore_device()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The condition `if (dev_iommu_priv_get(dev))` in `iommu_disable_device_dma()` always evaluates to false on the probe error path, making the alias DTE cleanup dead code.
- [High] Lockless execution of atomic DTE write helpers risks silent update failures and IRQ remapping corruption.

Pre-existing issues:
- [High] Missing hardware IOMMU cache flush after disabling device DMA.
--

--- Patch [2]: [PATCH v2 2/5] iommu/amd: Fix DTE clearing and rename iommu_ignore_device() ---
commit fe0ff8dabc53d6febbad3289c8dca1c1bf0c2df2
Author: Pranjal Shrivastava <praan@google.com>

iommu/amd: Fix DTE clearing and rename iommu_ignore_device()

This commit replaces the manual memset of the primary Device Table Entry
with a dedicated helper to avoid torn writes. It explicitly clears the
DTE lower and upper bits, and renames the function to
iommu_disable_device_dma.

Closes: https://lore.kernel.org/all/20260529153216.2AD1E1F00899@smtp.kernel.org/

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index ef21fbd9c9a89..91d551f29ed12 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c

[ ... ]

> @@ -2243,6 +2227,41 @@ static void dev_update_dte(struct iommu_dev_data *dev_data, bool set)
>  		clear_dte_entry(iommu, dev_data);
>  }
>  
> +/*
> + * Invalidate a DTE by clearing the Valid bit first.
> + * Note: Not to be used on a fully probed device with
> + * live dev_data.
> + */
> +static void amd_iommu_disable_dte(struct dev_table_entry *ptr)
> +{
> +	struct dev_table_entry new = {};
> +
> +	write_dte_lower128(ptr, &new);
> +	write_dte_upper128(ptr, &new);
> +}

[Severity: High]
Does this sequence need to be protected by a lock?

Looking at write_dte_upper128(), it performs a read-modify-write of the
interrupt mask:

write_dte_upper128(ptr, new)
    old.data128[1] = ptr->data128[1];
    new->data[2] &= ~DTE_DATA2_INTR_MASK;
    new->data[2] |= old.data[2] & DTE_DATA2_INTR_MASK;
    amd_iommu_atomic128_set(&ptr->data128[1], new->data128[1]);

The comments in write_dte_upper128() mention this requires being inside
a spin_lock(&dev_data->dte_lock) context. If this is called locklessly,
could a concurrent modification (such as clone_aliases() from another
device probing) cause the underlying cmpxchg16b to silently fail or
corrupt the IRQ remapping masks?

> +
> +static void iommu_disable_device_dma(struct amd_iommu *iommu, struct device *dev)
> +{
> +	struct amd_iommu_pci_seg *pci_seg = iommu->pci_seg;
> +	struct dev_table_entry *dev_table = get_dev_table(iommu);
> +	int devid, sbdf;
> +
> +	sbdf = get_device_sbdf_id(dev);
> +	if (sbdf < 0)
> +		return;
> +
> +	devid = PCI_SBDF_TO_DEVID(sbdf);
> +
> +	/* Clear the primary DTE */
> +	amd_iommu_disable_dte(&dev_table[devid]);

[Severity: High]
This is a pre-existing issue, but does the IOMMU hardware cache need to be
flushed after clearing the primary DTE in memory here?

Since the DTE is cleared in memory without a subsequent hardware flush
command (like iommu_flush_dte_sync() or iommu_completion_wait()) before
returning, could the IOMMU hardware continue using the old cached DTE and
retain DMA privileges on failure paths?

> +
> +	/* Clone the cleared DTE to all aliases before wiping the rlookup */
> +	if (dev_iommu_priv_get(dev))
> +		clone_aliases(iommu, dev);

[Severity: High]
Will this condition ever be true when called from the probe error path?

Looking at amd_iommu_probe_device(), it calls iommu_disable_device_dma()
only when iommu_init_device() fails:

amd_iommu_probe_device()
    dev_data = iommu_init_device(iommu, dev, devid);
    if (IS_ERR(dev_data)) {
        ...
        goto err_deinit;

err_deinit:
    iommu_disable_device_dma(iommu, dev);

Since dev_iommu_priv_set() is called inside iommu_init_device(), if the
initialization fails, dev_iommu_priv_get(dev) would return NULL. 
Doesn't this mean clone_aliases() is completely skipped during cleanup on
this error path, leaving DMA aliases active?

> +
> +	pci_seg->rlookup_table[devid] = NULL;
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814015647.3370124-1-praan@google.com?part=2

[PATCH v2 3/5] iommu/amd: Split probe error paths to preserve IRQ remapping

[Pranjal Shrivastava]

Split the amd_iommu_probe_device() error paths into err_deinit and
out_err. Proper init failures continue to call iommu_disable_device_dma()
while configuration failures (like PD_MODE_NONE or ATS mismatches) skip
it to preserve the rlookup_table entry required for IRQ remapping.

Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/all/20260529153216.2AD1E1F00899@smtp.kernel.org/
Suggested-by: Ankit Soni <ankit.soni@amd.com>
Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/amd/iommu.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 911a95527d74..808011a700de 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2540,8 +2540,7 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
 		ret = PTR_ERR(dev_data);
 		dev_err(dev, "Failed to initialize - trying to proceed anyway\n");
 		iommu_dev = ERR_PTR(ret);
-		iommu_disable_device_dma(iommu, dev);
-		goto out_err;
+		goto err_deinit;
 	}
 
 	iommu_init_device_caps(dev_data, dev, iommu);
@@ -2554,6 +2553,10 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
 		goto out_err;
 	}
 
+	return iommu_dev;
+
+err_deinit:
+	iommu_disable_device_dma(iommu, dev);
 out_err:
 	return iommu_dev;
 }
-- 
2.55.0.691.gc56d675ccc-goog

Re: [PATCH v2 3/5] iommu/amd: Split probe error paths to preserve IRQ remapping

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Skipping iommu_disable_device_dma() on probe configuration failures leaves the device's DTE dangerously active, leading to memory corruption or IOMMU faults.

Pre-existing issues:
- [High] A hardware-triggered Peripheral Page Request (PPR) can cause a NULL pointer dereference if the device's IOMMU probe previously failed or was aborted.
--

--- Patch [3]: [PATCH v2 3/5] iommu/amd: Split probe error paths to preserve IRQ remapping ---
commit e8c6c05e87391c978351a97a7ecc5fb4ef5a2b22
Author: Pranjal Shrivastava <praan@google.com>

iommu/amd: Split probe error paths to preserve IRQ remapping

This commit splits the error paths in amd_iommu_probe_device() to skip
calling iommu_disable_device_dma() on configuration failures. This aims to
preserve the rlookup_table entry required for IRQ remapping.

Closes: https://lore.kernel.org/all/20260529153216.2AD1E1F00899@smtp.kernel.org/

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 91d551f29ed12..9ad3412f0ade8 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c

[ ... ]

> @@ -2564,6 +2563,10 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
>  		goto out_err;
>  	}

[Severity: High]
Does skipping iommu_disable_device_dma() on probe configuration failures
leave the device's Device Table Entry (DTE) dangerously active? 

If the DTE is not properly cleared (e.g., clearing the TV and V bits), could
the AMD hardware continue to intercept DMA using stale or uninitialized state?
This might lead to memory corruption if a kexec environment leaves valid page
tables active, or IOMMU target aborts if a normal boot leaves the page table
root at 0, breaking direct DMA fallback for the device.

>  
> +	return iommu_dev;
> +
> +err_deinit:
> +	iommu_disable_device_dma(iommu, dev);
>  out_err:

[Severity: High]
This is a pre-existing issue, but does leaving the DTE active on probe
failures expose a NULL pointer dereference in the hardware PPR handler?

If the hardware generates a Peripheral Page Request (PPR) because the old
kexec PPR configuration was left active, the interrupt handler is called:

drivers/iommu/amd/ppr.c:iommu_call_iopf_notifier() {
    ...
    dev_data = dev_iommu_priv_get(&pdev->dev);
    if (event.fault.prm.pasid == 0 ||
        event.fault.prm.pasid >= dev_data->max_pasids) {
    ...
}

Since the IOMMU core frees dev->iommu on probe failure, dev_data would be
NULL. Won't unconditionally reading dev_data->max_pasids cause a kernel panic
here?

>  	return iommu_dev;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814015647.3370124-1-praan@google.com?part=3

[PATCH v2 4/5] iommu/amd: Fail probe on ATS configuration failure

[Pranjal Shrivastava]

Update the driver to call pci_prepare_ats() after checking if
pci_ats_supported() and fail the probe_device if pci_prepare_ats()
returns an error. Additionally, update pdev_enable_cap_ats() to WARN_ON()
a failure in pci_enable_ats().

Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/amd/iommu.c | 40 +++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 808011a700de..5ccc76bffb88 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -573,10 +573,17 @@ static inline int pdev_enable_cap_ats(struct pci_dev *pdev)
 	if (amd_iommu_iotlb_sup &&
 	    (dev_data->flags & AMD_IOMMU_DEVICE_FLAG_ATS_SUP)) {
 		ret = pci_enable_ats(pdev, PAGE_SHIFT);
-		if (!ret) {
-			dev_data->ats_enabled = 1;
-			dev_data->ats_qdep    = pci_ats_queue_depth(pdev);
-		}
+
+		/*
+		 * pci_enable_ats() should not fail here because earlier
+		 * checks have already verified support & config.
+		 */
+		if (WARN_ON(ret))
+			return ret;
+
+		dev_data->ats_enabled = 1;
+		dev_data->ats_qdep    = pci_ats_queue_depth(pdev);
+		ret = 0;
 	}
 
 	return ret;
@@ -2483,10 +2490,12 @@ static void detach_device(struct device *dev)
 	mutex_unlock(&dev_data->mutex);
 }
 
-static void iommu_init_device_caps(struct iommu_dev_data *dev_data,
-				   struct device *dev,
-				   struct amd_iommu *iommu)
+static int iommu_init_device_caps(struct iommu_dev_data *dev_data,
+				  struct device *dev,
+				  struct amd_iommu *iommu)
 {
+	int ret;
+
 	if (FEATURE_NUM_INT_REMAP_SUP_2K(amd_iommu_efr2))
 		dev_data->max_irqs = MAX_IRQS_PER_TABLE_2K;
 	else
@@ -2495,7 +2504,7 @@ static void iommu_init_device_caps(struct iommu_dev_data *dev_data,
 	amd_iommu_set_pci_msi_domain(dev, iommu);
 
 	if (!dev_is_pci(dev))
-		return;
+		return 0;
 
 	/*
 	 * By default we use passthrough mode for IOMMUv2 capable device.
@@ -2518,7 +2527,13 @@ static void iommu_init_device_caps(struct iommu_dev_data *dev_data,
 					     pci_max_pasids(to_pci_dev(dev)));
 	}
 
-	pci_prepare_ats(to_pci_dev(dev), PAGE_SHIFT);
+	if (pci_ats_supported(to_pci_dev(dev))) {
+		ret = pci_prepare_ats(to_pci_dev(dev), PAGE_SHIFT);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 static struct iommu_device *amd_iommu_probe_device(struct device *dev)
@@ -2543,7 +2558,12 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
 		goto err_deinit;
 	}
 
-	iommu_init_device_caps(dev_data, dev, iommu);
+	ret = iommu_init_device_caps(dev_data, dev, iommu);
+	if (ret) {
+		iommu_dev = ERR_PTR(ret);
+		goto out_err;
+	}
+
 	iommu_dev = &iommu->iommu;
 
 	if (amd_iommu_pgtable == PD_MODE_NONE) {
-- 
2.55.0.691.gc56d675ccc-goog

Re: [PATCH v2 4/5] iommu/amd: Fail probe on ATS configuration failure

[sashiko-bot]

> Update the driver to call pci_prepare_ats() after checking if
> pci_ats_supported() and fail the probe_device if pci_prepare_ats()
> returns an error. Additionally, update pdev_enable_cap_ats() to WARN_ON()
> a failure in pci_enable_ats().
> 
> Signed-off-by: Pranjal Shrivastava <praan@google.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814015647.3370124-1-praan@google.com?part=4

[PATCH v2 5/5] PCI/ATS: Mandate checking pci_ats_supported() before pci_prepare_ats()

[Pranjal Shrivastava]

Currently, pci_prepare_ats() internally calls pci_ats_supported() and
returns -EINVAL if the device does not support ATS. While this provides
a silent safety check, it conflates support detection with configuration.

Update pci_prepare_ats() to wrap the internal pci_ats_supported check in
a WARN_ON(). This mandates all callers to call pci_prepare_ats() only if
the function supports ATS.

Update the function documentation to mention that callers must verify
ATS support (via pci_ats_supported()) before calling pci_prepare_ats().

Suggested-by: Baolu Lu <baolu.lu@linux.intel.com>
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/pci/ats.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 96efa00d9743..f18c06b73eb2 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -53,7 +53,9 @@ EXPORT_SYMBOL_GPL(pci_ats_supported);
  * @ps: the IOMMU page shift
  *
  * This must be done by the IOMMU driver on the PF before any VFs are created to
- * ensure that the VF can have ATS enabled.
+ * ensure that the VF can have ATS enabled. Callers must verify that ATS is
+ * supported by the device (e.g. via pci_ats_supported()) before calling this
+ * function.
  *
  * Returns 0 on success, or negative on failure.
  */
@@ -61,7 +63,7 @@ int pci_prepare_ats(struct pci_dev *dev, int ps)
 {
 	u16 ctrl;
 
-	if (!pci_ats_supported(dev))
+	if (WARN_ON(!pci_ats_supported(dev)))
 		return -EINVAL;
 
 	if (WARN_ON(dev->ats_enabled))
-- 
2.55.0.691.gc56d675ccc-goog

Re: [PATCH v2 5/5] PCI/ATS: Mandate checking pci_ats_supported() before pci_prepare_ats()

[sashiko-bot]

> Currently, pci_prepare_ats() internally calls pci_ats_supported() and
> returns -EINVAL if the device does not support ATS. While this provides
> a silent safety check, it conflates support detection with configuration.
> 
> Update pci_prepare_ats() to wrap the internal pci_ats_supported check in
> a WARN_ON(). This mandates all callers to call pci_prepare_ats() only if
> the function supports ATS.
> 
> Update the function documentation to mention that callers must verify
> ATS support (via pci_ats_supported()) before calling pci_prepare_ats().
> 
> Suggested-by: Baolu Lu <baolu.lu@linux.intel.com>
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Pranjal Shrivastava <praan@google.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814015647.3370124-1-praan@google.com?part=5