[PATCH v3 00/13] iommu/arm-smmu-v3: Add PRI support

[PATCH v3 00/13] iommu/arm-smmu-v3: Add PRI support

[Nicolin Chen]

The SMMUv3 driver doesn't handle events on the PRI queue or respond to IOPF
faults. This series adds the missing pieces, using the IOPF infrastructure,
to convert PRI page requests into iopf_faults and issue CMDQ_OP_PRI_RESP.

The iopf_queue_flush_dev() contract requires the driver to first drain the
hardware PRI queue and synchronize using a threaded IRQ handler before the
IOPF software flush. This drove the additional commits compared to v1:
 - arm_smmu_drain_queue() drains the hardware queue by counting the
   entries that the threaded IRQ handler consumes
 - arm_smmu_attach_release() moves the teardown outside the global lock
 - synchronize_irq() closes the gap before the final flush

Note that, although this series is only about PRIQ and EVTQ, the Q_POS and
arm_smmu_drain_queue() helpers will be shared with Pranj's RPM series, per
discussion below for CMDQ to use as well:
https://lore.kernel.org/all/ao3ljjiXhnSJte5A@google.com/

Also, two patches in this series have some conflict with the kdump series:
https://lore.kernel.org/all/cover.1788130528.git.nicolinc@nvidia.com/

As the kdump one is still under review, I leave those as-is, since Sashiko
review relies on a clean rebase on linux-next to scan the series. Once the
kdump series gets merged, I will rebase those kdump related stuff. For now,
I think the series is good to collect review comments.

This is on Github:
https://github.com/nicolinc/iommufd/commits/smmuv3_pri-v3

FWIW, engineers on the NVIDIA side have managed to verify the PRI feature.

Changelog
v3:
 * Rebase on v7.3-rc1
 * Drop FEAT_SVA gate on IOPF queue allocation
 * Discard partial PRI faults on PRIQ overflow
 * Refuse PRI on a PCIe master with stall_enabled
 * Merge and ratelimit the unexpected PRI request prints
 * Disable EVTQ/PRIQ/combined IRQs before iopf_queue_free()
 * Reclaim a leaked IOPF enrollment in arm_smmu_release_device()
 * Leak master_domain on a drain timeout instead of risking a UAF
 * Fix the IOMMU_PAGE_RESP code mapping in arm_smmu_page_response()
 * Rework arm_smmu_drain_queue() into a counting-based sleeping poll
 * Split the fault event drain and the fault work flush into two patches
 * Drop IOMMU_FAULT_PAGE_REQUEST_STALLS_TRANS; dispatch on per-master state
v2:
 https://lore.kernel.org/all/cover.1779944354.git.nicolinc@nvidia.com/
 * Allocate evtq.iopf for ARM_SMMU_FEAT_PRI
 * Pick up Jean's PRI stubs and PRI export patches
 * Enable PRI for PCI devices in arm_smmu_probe_device()
 * Add arm_smmu_drain_queue_for_iopf() for EVTQ and PRIQ
 * Add arm_smmu_attach_release() to rework the IOPF drain
 * Add IOMMU_FAULT_PAGE_REQUEST_STALLS_TRANS for STALL mode
 * Gate pci_enable_pri() on FEAT_PRI plus a non-NULL evtq.iopf
 * Deny unrecognised-StreamID PRG_LAST in arm_smmu_handle_ppr()
 * Disable PRI when no IRQ handler is registered (unique or combined IRQ)
v1:
 https://lore.kernel.org/all/cover.1772568590.git.nicolinc@nvidia.com/

Jean-Philippe Brucker (2):
  PCI/ATS: Add PRI stubs
  PCI/ATS: Export pci_enable_pri() and pci_reset_pri()

Malak Marrid (1):
  iommu/arm-smmu-v3: Submit CMDQ_OP_PRI_RESP for IOPF event

Nicolin Chen (10):
  iommu/arm-smmu-v3: Add arm_smmu_attach_release()
  iommu/arm-smmu-v3: Add Q_POS() macro
  iommu/arm-smmu-v3: Drain in-flight fault events on domain detach
  iommu/arm-smmu-v3: Flush in-flight fault work on domain detach
  iommu/arm-smmu-v3: Allocate IOPF queue without FEAT_SVA
  iommu/arm-smmu-v3: Disable the queue IRQs before disabling the SMMU
  iommu/arm-smmu-v3: Disable PRI when no IRQ handler is registered
  iommu/arm-smmu-v3: Support PRI Page Request in arm_smmu_handle_ppr()
  iommu/arm-smmu-v3: Allocate IOPF queue for ARM_SMMU_FEAT_PRI
  iommu/arm-smmu-v3: Enable PRI for PCI device in
    arm_smmu_probe_device()

 drivers/iommu/arm/Kconfig                     |   2 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |   5 +
 include/linux/pci-ats.h                       |   5 +
 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     |   1 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 417 +++++++++++++++---
 drivers/pci/ats.c                             |   2 +
 6 files changed, 382 insertions(+), 50 deletions(-)


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.43.0

[PATCH v3 01/13] iommu/arm-smmu-v3: Add arm_smmu_attach_release()

[Nicolin Chen]

The IOPF teardown is done in arm_smmu_remove_master_domain() when releasing
the master_domain on detach, under the global arm_smmu_asid_lock mutex. But
the teardown must drain any in-flight IOPF (for the old domain), before the
master_domain is freed via iopf_queue_flush_dev() calling flush_workqueue()
that can block on user-faulting page-fault handlers. Doing so while holding
the arm_smmu_asid_lock would stall any unrelated attachment in the system.

Split the teardown out of arm_smmu_remove_master_domain(), to a new helper
arm_smmu_attach_release() that runs after arm_smmu_asid_lock is released.

Since no other device would use the old master_domain that is being freed,
it's safe to move out of arm_smmu_asid_lock (still under the protection of
iommu_group->mutex).

Note: this is a pure refactor; no functional change.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 ++
 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     |  1 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 33 +++++++++++++++----
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 50f8321e979ce..5b89bad71c102 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -1204,12 +1204,14 @@ struct arm_smmu_attach_state {
 	struct arm_smmu_vmaster *vmaster;
 	struct arm_smmu_inv_state old_domain_invst;
 	struct arm_smmu_inv_state new_domain_invst;
+	struct arm_smmu_master_domain *old_master_domain;
 	bool ats_enabled;
 };
 
 int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
 			    struct iommu_domain *new_domain);
 void arm_smmu_attach_commit(struct arm_smmu_attach_state *state);
+void arm_smmu_attach_release(struct arm_smmu_attach_state *state);
 void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master,
 				  const struct arm_smmu_ste *target);
 
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
index 25982bdbcbd9a..fce026efa44f1 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
@@ -194,6 +194,7 @@ static int arm_smmu_attach_dev_nested(struct iommu_domain *domain,
 	arm_smmu_install_ste_for_dev(master, &ste);
 	arm_smmu_attach_commit(&state);
 	mutex_unlock(&arm_smmu_asid_lock);
+	arm_smmu_attach_release(&state);
 	return 0;
 }
 
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 5732f3ba0122d..99baa59b39c9d 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3285,9 +3285,9 @@ arm_smmu_master_build_invs(struct arm_smmu_master *master, bool ats_enabled,
 	return master->build_invs;
 }
 
-static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
-					  struct iommu_domain *domain,
-					  ioasid_t ssid)
+static struct arm_smmu_master_domain *
+arm_smmu_remove_master_domain(struct arm_smmu_master *master,
+			      struct iommu_domain *domain, ioasid_t ssid)
 {
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain_devices(domain);
 	struct arm_smmu_master_domain *master_domain;
@@ -3295,7 +3295,7 @@ static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
 	unsigned long flags;
 
 	if (!smmu_domain)
-		return;
+		return NULL;
 
 	if (domain->type == IOMMU_DOMAIN_NESTED)
 		nested_ats_flush = to_smmu_nested_domain(domain)->enable_ats;
@@ -3310,8 +3310,23 @@ static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
 	}
 	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
 
+	/* arm_smmu_attach_release() will free it */
+	return master_domain;
+}
+
+/* Release the old master_domain detached by arm_smmu_remove_master_domain() */
+void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
+{
+	struct arm_smmu_master_domain *master_domain = state->old_master_domain;
+	struct arm_smmu_master *master = state->master;
+
+	iommu_group_mutex_assert(master->dev);
+
+	if (!master_domain)
+		return;
 	arm_smmu_disable_iopf(master, master_domain);
 	kfree(master_domain);
+	state->old_master_domain = NULL;
 }
 
 /*
@@ -3609,7 +3624,8 @@ void arm_smmu_attach_commit(struct arm_smmu_attach_state *state)
 		arm_smmu_atc_inv_master(master, IOMMU_NO_PASID);
 	}
 
-	arm_smmu_remove_master_domain(master, state->old_domain, state->ssid);
+	state->old_master_domain = arm_smmu_remove_master_domain(
+		master, state->old_domain, state->ssid);
 	arm_smmu_install_old_domain_invs(state);
 	master->ats_enabled = state->ats_enabled;
 }
@@ -3684,6 +3700,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev,
 
 	arm_smmu_attach_commit(&state);
 	mutex_unlock(&arm_smmu_asid_lock);
+	arm_smmu_attach_release(&state);
 	return 0;
 }
 
@@ -3784,6 +3801,7 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
 
 out_unlock:
 	mutex_unlock(&arm_smmu_asid_lock);
+	arm_smmu_attach_release(&state);
 	return ret;
 }
 
@@ -3804,9 +3822,11 @@ static int arm_smmu_blocking_set_dev_pasid(struct iommu_domain *new_domain,
 	arm_smmu_clear_cd(master, pasid);
 	if (master->ats_enabled)
 		arm_smmu_atc_inv_master(master, pasid);
-	arm_smmu_remove_master_domain(master, &smmu_domain->domain, pasid);
+	state.old_master_domain = arm_smmu_remove_master_domain(
+		master, &smmu_domain->domain, pasid);
 	arm_smmu_install_old_domain_invs(&state);
 	mutex_unlock(&arm_smmu_asid_lock);
+	arm_smmu_attach_release(&state);
 
 	/*
 	 * When the last user of the CD table goes away downgrade the STE back
@@ -3869,6 +3889,7 @@ static void arm_smmu_attach_dev_ste(struct iommu_domain *domain,
 	arm_smmu_install_ste_for_dev(master, ste);
 	arm_smmu_attach_commit(&state);
 	mutex_unlock(&arm_smmu_asid_lock);
+	arm_smmu_attach_release(&state);
 
 	/*
 	 * This has to be done after removing the master from the
-- 
2.43.0

[PATCH v3 02/13] iommu/arm-smmu-v3: Add Q_POS() macro

[Nicolin Chen]

A queue position, the wrap bit combined with the index, is Q_WRP | Q_IDX.
It is a (max_n_shift + 1)-bit value that wraps at twice the queue capacity.
queue_inc_cons(), queue_sync_cons_ovf(), and queue_inc_prod_n() currently
compute such positions by open-coding the two macros at each call site.

Add a Q_POS() macro and switch the open-coded sites to it.

A subsequent change will apply Q_POS() to a position difference, to count
the entries that a queue pointer moved past.

No functional change intended.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  2 ++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 11 +++++------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 5b89bad71c102..de7e4284658a1 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -187,6 +187,8 @@ struct arm_vsmmu;
 
 #define Q_IDX(llq, p)			((p) & ((1 << (llq)->max_n_shift) - 1))
 #define Q_WRP(llq, p)			((p) & (1 << (llq)->max_n_shift))
+/* A position is Q_WRP | Q_IDX, wrapping at twice the queue capacity */
+#define Q_POS(llq, p)			(Q_WRP(llq, p) | Q_IDX(llq, p))
 #define Q_OVERFLOW_FLAG			(1U << 31)
 #define Q_OVF(p)			((p) & Q_OVERFLOW_FLAG)
 #define Q_ENT(q, p)			((q)->base +			\
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 99baa59b39c9d..e00b6c88214f5 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -181,8 +181,8 @@ static void queue_sync_cons_out(struct arm_smmu_queue *q)
 
 static void queue_inc_cons(struct arm_smmu_ll_queue *q)
 {
-	u32 cons = (Q_WRP(q, q->cons) | Q_IDX(q, q->cons)) + 1;
-	q->cons = Q_OVF(q->cons) | Q_WRP(q, cons) | Q_IDX(q, cons);
+	u32 cons = Q_POS(q, q->cons) + 1;
+	q->cons = Q_OVF(q->cons) | Q_POS(q, cons);
 }
 
 static void queue_sync_cons_ovf(struct arm_smmu_queue *q)
@@ -192,8 +192,7 @@ static void queue_sync_cons_ovf(struct arm_smmu_queue *q)
 	if (likely(Q_OVF(llq->prod) == Q_OVF(llq->cons)))
 		return;
 
-	llq->cons = Q_OVF(llq->prod) | Q_WRP(llq, llq->cons) |
-		      Q_IDX(llq, llq->cons);
+	llq->cons = Q_OVF(llq->prod) | Q_POS(llq, llq->cons);
 	queue_sync_cons_out(q);
 }
 
@@ -218,8 +217,8 @@ static int queue_sync_prod_in(struct arm_smmu_queue *q)
 
 static u32 queue_inc_prod_n(struct arm_smmu_ll_queue *q, int n)
 {
-	u32 prod = (Q_WRP(q, q->prod) | Q_IDX(q, q->prod)) + n;
-	return Q_OVF(q->prod) | Q_WRP(q, prod) | Q_IDX(q, prod);
+	u32 prod = Q_POS(q, q->prod) + n;
+	return Q_OVF(q->prod) | Q_POS(q, prod);
 }
 
 static void queue_poll_init(struct arm_smmu_device *smmu,
-- 
2.43.0

[PATCH v3 03/13] iommu/arm-smmu-v3: Drain in-flight fault events on domain detach

[Nicolin Chen]

When a device is switching away from a domain, either through a detach or a
replace operation, in-flight stall events for the old domain might still be
on the SMMU's hardware event queue or on the IOMMU core's IOPF queue. Thus,
if the IOMMU core swaps the device's attach_handle and frees the old domain
before those handlers complete, the IOPF work might hit use-after-free.

Two queues need to be drained: the SMMU hardware event queue and the IOMMU
core IOPF software workqueue. Start with the former: add a counting-based
arm_smmu_drain_queue() helper, and poll the evtq on a domain detach, so a
pending IRQ won't let the threaded handler run after the drain and queue a
fault referencing the domain being freed. Its until_empty mode serves the
suspend and runtime PM routines that would drain the CMDQ. Any timed-out
drain fires a WARN_ON as well, since reaching the timeout would take some
stuck consumer in any realistic case.

The existing queue_poll() API is not reusable for such a drain: it is the
atomic busy-wait for the command issuing paths, and it assumes a hardware
consumer making progress. A drain caller is sleepable, in contrast, while
the EVTQ/PRIQ consumer is a threaded IRQ handler that needs the CPU: such
a busy wait would starve the handler throughout an entire timeout, whenever
the waiter and the handler shared one CPU on a non-preemptible kernel. So,
this new sleeping helper is marked with a might_sleep() as well, given that
an atomic-context misuse would otherwise hide behind an empty queue.

Note that a drained event is dequeued, but not necessarily handled, since
queue_remove_raw() moves the MMIO CONS before the threaded IRQ handler gets
to push the event onto the IOPF workqueue. A subsequent change will invoke
synchronize_irq() and iopf_queue_flush_dev() to close that gap, and it will
act on the errno of a timed-out drain too.

The drain runs before the IOMMU core swaps the device's attach handle, so a
fault event generated on the new STE during this window resolves to the old
handle, completing with IOMMU_PAGE_RESP_INVALID that resumes the stall with
abort: the impact is bounded to that one failed transaction.

Also run the drain for every stall-capable master, even when the departing
attachment did not enable IOPF: such a stall event has to be aborted while
it still resolves to the old attach handle, otherwise the threaded handler
could pick it up right after the handle swap, mistakenly resuming it as if
it were a valid page fault against a new domain.

Fixes: cfea71aea921 ("iommu/arm-smmu-v3: Put iopf enablement in the domain attach path")
Cc: stable@vger.kernel.org # v6.16
Assisted-by: Claude:claude-fable-5
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 92 +++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index e00b6c88214f5..d255ff2519f9d 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -948,6 +948,86 @@ static int arm_smmu_cmdq_batch_submit(struct arm_smmu_device *smmu,
 					   cmds->num, true);
 }
 
+/**
+ * arm_smmu_drain_queue - Drain an SMMU queue
+ * @smmu: the SMMU device
+ * @q: the queue to drain
+ * @until_empty: target selection
+ *
+ * With @until_empty == true (for CMDQ), exit once the queue is observed empty:
+ *
+ *   cons0                cons                                prod
+ *     |                   |                                   |
+ *  ---+###################+=====================+=============+--->
+ *                         |<--------- undrained==0? --------->|
+ *
+ * With @until_empty == false (for EVTQ/PRIQ), exit once "drained" reaches its
+ * target: "pending" (i.e. prod0 - cons0, frozen at the entry time):
+ *
+ *   cons0                cons                 prod0         (prod)
+ *     |<---- drained ---->|                     |             |
+ *  ---+###################+=====================+=============+--->
+ *     |<--------------- pending --------------->|
+ *
+ * Note that a drained entry is dequeued, but not necessarily handled: the
+ * EVTQ/PRIQ callers must follow up with a synchronize_irq() to wait for the
+ * threaded IRQ handler to finish handling the dequeued entries.
+ *
+ * Context: Process context; may sleep.
+ * Return: 0 on success or a negative errno on timeout.
+ */
+static int arm_smmu_drain_queue(struct arm_smmu_device *smmu,
+				struct arm_smmu_queue *q, bool until_empty)
+{
+	ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
+	u32 cons, prod, prev, undrained;
+	u32 drained = 0, pending;
+
+	might_sleep();
+
+	cons = readl_relaxed(q->cons_reg);
+	prod = readl_relaxed(q->prod_reg);
+	/* The exit target: the number of entries in the queue at entry */
+	pending = Q_POS(&q->llq, prod - cons);
+
+	while (true) {
+		/* Accumulate the entries consumed since the last poll */
+		prev = cons;
+		cons = readl_relaxed(q->cons_reg);
+		drained += Q_POS(&q->llq, cons - prev);
+
+		prod = readl_relaxed(q->prod_reg);
+		undrained = Q_POS(&q->llq, prod - cons);
+
+		/* Exit on an empty queue, regardless of until_empty */
+		if (!undrained)
+			return 0;
+
+		/* Snapshot mode: exit once the pending entries are drained */
+		if (!until_empty && drained >= pending)
+			return 0;
+
+		/*
+		 * A timeout means the consumer might be stuck. In theory, if it
+		 * moves 2 * qsize entries or more within a single poll interval
+		 * Q_POS() would wrap and undercount drained: that could trigger
+		 * a spurious warning too, if the queue was never once observed
+		 * empty. Yet, that much consumption in such a short interval is
+		 * unrealistic. WARN it only, as a stuck consumer is a real bug.
+		 */
+		if (WARN_ON(ktime_compare(ktime_get(), timeout) > 0))
+			break;
+
+		/* The consumer might be a threaded IRQ handler. Yield to it */
+		usleep_range(100, 200);
+	}
+
+	dev_warn_ratelimited(smmu->dev,
+			     "queue drain timed out at prod=0x%x cons=0x%x\n",
+			     prod, cons);
+	return -ETIMEDOUT;
+}
+
 static void arm_smmu_page_response(struct device *dev, struct iopf_fault *unused,
 				   struct iommu_page_response *resp)
 {
@@ -3318,11 +3398,23 @@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
 {
 	struct arm_smmu_master_domain *master_domain = state->old_master_domain;
 	struct arm_smmu_master *master = state->master;
+	struct arm_smmu_device *smmu = master->smmu;
 
+	lockdep_assert_not_held(&arm_smmu_asid_lock);
 	iommu_group_mutex_assert(master->dev);
 
 	if (!master_domain)
 		return;
+
+	/*
+	 * Drain the hardware eventq, while stale events still resolve to the
+	 * old attach handle. Otherwise, the threaded handler could pick one
+	 * up once the IOMMU core swaps the handle, mistakenly resuming it
+	 * against the next domain.
+	 */
+	if (master->stall_enabled)
+		arm_smmu_drain_queue(smmu, &smmu->evtq.q, false);
+
 	arm_smmu_disable_iopf(master, master_domain);
 	kfree(master_domain);
 	state->old_master_domain = NULL;
-- 
2.43.0

[PATCH v3 04/13] iommu/arm-smmu-v3: Flush in-flight fault work on domain detach

[Nicolin Chen]

arm_smmu_attach_release() polls the hardware event queue to drain all the
in-flight stall events for an old domain that the IOMMU core might free at
any moment. However, a drained event is dequeued, yet it is not necessarily
handled, and the IOPF work for a handled one can still be running.

So, first synchronize_irq() on the evtq and the combined IRQs following the
drain, in order to guarantee that every dequeued event has reached the IOPF
workqueue, since synchronize_irq() waits for an in-flight IRQ thread. Skip
the waits on a timed-out drain though, since a stuck consumer would block
the unbounded wait_event() inside synchronize_irq() as well.

Then, invoke iopf_queue_flush_dev() to finally drain the IOPF workqueue, as
the fault work references the old domain via its attach handle, while the
IOMMU core might free that old domain once the detach call returns.

If arm_smmu_drain_queue() times out, fault work may still be in flight, and
iopf_queue_remove_device() would free iopf groups that the queued work also
references. Skip the iopf teardown and leak the master_domain, rather than
risk a use-after-free.

The skip also leaks the iopf refcount, keeping the device enrolled on the
IOPF queue, which would strand its fault parameter on the queue list once
the device teardown frees dev->iommu, crashing a later iopf_queue_free().
Reclaim the enrollment in arm_smmu_release_device(), where all the attach
handles are gone so a straggler report cannot queue a new fault group.

Note that a residual race window remains between an iopf_queue_flush_dev()
and iopf_queue_remove_device(): a fault arriving in between still resolves
to the old attach handle, as the IOMMU core publishes a handle change only
after the driver ops return. This window predates the drain narrowing it,
and is only closable by an ordering fix in the IOMMU core. Furthermore, a
timed-out drain shares exactly the same window, given that it must keep the
device enrolled on the IOPF queue, where iopf_queue_remove_device() would
free the iopf groups that any in-flight fault work still references.

Fixes: cfea71aea921 ("iommu/arm-smmu-v3: Put iopf enablement in the domain attach path")
Cc: stable@vger.kernel.org # v6.16
Co-developed-by: Barak Biber <bbiber@nvidia.com>
Signed-off-by: Barak Biber <bbiber@nvidia.com>
Co-developed-by: Stefan Kaestle <skaestle@nvidia.com>
Signed-off-by: Stefan Kaestle <skaestle@nvidia.com>
Signed-off-by: Malak Marrid <mmarrid@nvidia.com>
Assisted-by: Claude:claude-fable-5
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 48 +++++++++++++++++++--
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index d255ff2519f9d..1851ee7ec15b9 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3399,6 +3399,7 @@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
 	struct arm_smmu_master_domain *master_domain = state->old_master_domain;
 	struct arm_smmu_master *master = state->master;
 	struct arm_smmu_device *smmu = master->smmu;
+	int ret = 0;
 
 	lockdep_assert_not_held(&arm_smmu_asid_lock);
 	iommu_group_mutex_assert(master->dev);
@@ -3412,8 +3413,40 @@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
 	 * up once the IOMMU core swaps the handle, mistakenly resuming it
 	 * against the next domain.
 	 */
-	if (master->stall_enabled)
-		arm_smmu_drain_queue(smmu, &smmu->evtq.q, false);
+	if (master->stall_enabled) {
+		ret = arm_smmu_drain_queue(smmu, &smmu->evtq.q, false);
+		/*
+		 * Ensure pending events have reached the IOPF queue, unless
+		 * the drain timed out: a stuck consumer would also block an
+		 * unbounded wait_event() inside the synchronize_irq().
+		 */
+		if (!ret && smmu->evtq.q.irq)
+			synchronize_irq(smmu->evtq.q.irq);
+		/* Pending events might be in the combined_irq handler */
+		if (!ret && smmu->combined_irq)
+			synchronize_irq(smmu->combined_irq);
+	}
+
+	/*
+	 * Only IOPF-enabled attachments queue fault work, and such work
+	 * references the old domain via its attach handle. Flush it, as
+	 * the IOMMU core might free the old domain once this returns.
+	 */
+	if (master_domain->using_iopf) {
+		/* Lastly, drain the IOPF queue */
+		iopf_queue_flush_dev(master->dev);
+
+		/*
+		 * A timed-out drain may leave fault work in flight, and
+		 * iopf_queue_remove_device() would free iopf groups that
+		 * such work still references. Skip the iopf teardown and
+		 * leak master_domain, rather than risk a UAF.
+		 */
+		if (WARN_ON(ret)) {
+			state->old_master_domain = NULL;
+			return;
+		}
+	}
 
 	arm_smmu_disable_iopf(master, master_domain);
 	kfree(master_domain);
@@ -4397,7 +4430,16 @@ static void arm_smmu_release_device(struct device *dev)
 {
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
 
-	WARN_ON(master->iopf_refcount);
+	/*
+	 * A timed-out drain in arm_smmu_attach_release() leaks the refcount,
+	 * keeping the device on the IOPF queue. Reclaim it here, since every
+	 * attach handle is gone: a straggler fault can no longer queue a new
+	 * fault group, so the queue turns stable once flushed.
+	 */
+	if (WARN_ON(master->iopf_refcount)) {
+		iopf_queue_flush_dev(dev);
+		iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
+	}
 
 	arm_smmu_disable_pasid(master);
 	arm_smmu_remove_master(master);
-- 
2.43.0

[PATCH v3 05/13] iommu/arm-smmu-v3: Allocate IOPF queue without FEAT_SVA

[Nicolin Chen]

arm_smmu_init_queues() currently allocates smmu->evtq.iopf only when both
FEAT_SVA and FEAT_STALLS are advertised by the SMMU. The FEAT_SVA gate is
unnecessary: iopf_queue_alloc() is a plain workqueue allocator with no SVA
dependency, and FEAT_STALLS alone can produce events that need IOPF.

On HW with FEAT_STALLS but no FEAT_SVA, a master with the "dma-can-stall"
device-tree property has master->stall_enabled set at probe. Then also with
CONFIG_ARM_SMMU_V3_SVA=y, arm_smmu_enable_iopf() sees stall_enabled, calls
iopf_queue_add_device(NULL, ...) at fault-handling domain attach, which now
oopses on mutex_lock(&queue->lock).

Drop the FEAT_SVA term so the IOPF workqueue is allocated whenever the SMMU
can produce IOPF events, and select IOMMU_IOPF from ARM_SMMU_V3 as well, so
iopf_queue_alloc() has a real implementation instead of the NULL-returning
stub when CONFIG_ARM_SMMU_V3_SVA is unset.

IOPF is orthogonal to SVA anyway. So, drop the CONFIG_ARM_SMMU_V3_SVA gates
in arm_smmu_enable_iopf() and arm_smmu_disable_iopf().

Fixes: 395ad89d11fd9 ("iommu/arm-smmu-v3: Add stall support for platform devices")
Cc: stable@vger.kernel.org # v5.14
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/Kconfig                   | 1 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 +--------
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/arm/Kconfig b/drivers/iommu/arm/Kconfig
index 5fac08b89deea..b848a4253677d 100644
--- a/drivers/iommu/arm/Kconfig
+++ b/drivers/iommu/arm/Kconfig
@@ -78,6 +78,7 @@ config ARM_SMMU_V3
 	depends on ARM64
 	select IOMMU_API
 	select IOMMU_IO_PGTABLE_LPAE
+	select IOMMU_IOPF
 	select GENERIC_MSI_IRQ
 	select IOMMUFD_DRIVER if IOMMUFD
 	help
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 1851ee7ec15b9..352c916b2a572 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3204,9 +3204,6 @@ static int arm_smmu_enable_iopf(struct arm_smmu_master *master,
 
 	iommu_group_mutex_assert(master->dev);
 
-	if (!IS_ENABLED(CONFIG_ARM_SMMU_V3_SVA))
-		return -EOPNOTSUPP;
-
 	/*
 	 * Drivers for devices supporting PRI or stall require iopf others have
 	 * device-specific fault handlers and don't need IOPF, so this is not a
@@ -3238,9 +3235,6 @@ static void arm_smmu_disable_iopf(struct arm_smmu_master *master,
 {
 	iommu_group_mutex_assert(master->dev);
 
-	if (!IS_ENABLED(CONFIG_ARM_SMMU_V3_SVA))
-		return;
-
 	if (!master_domain || !master_domain->using_iopf)
 		return;
 
@@ -4659,8 +4653,7 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
 	if (ret)
 		return ret;
 
-	if ((smmu->features & ARM_SMMU_FEAT_SVA) &&
-	    (smmu->features & ARM_SMMU_FEAT_STALLS)) {
+	if (smmu->features & ARM_SMMU_FEAT_STALLS) {
 		smmu->evtq.iopf = iopf_queue_alloc(dev_name(smmu->dev));
 		if (!smmu->evtq.iopf)
 			return -ENOMEM;
-- 
2.43.0

[PATCH v3 06/13] iommu/arm-smmu-v3: Submit CMDQ_OP_PRI_RESP for IOPF event

[Nicolin Chen]

From: Malak Marrid <mmarrid@nvidia.com>

To handle IOMMU_FAULT_PAGE_REQ from the PRI queue, arm_smmu_page_response()
must issue a CMDQ_OP_PRI_RESP back to the SMMU.

A stall event in the EVTQ and a PRI request in the PRIQ both surface to the
IOPF infrastructure with fault.type == IOMMU_FAULT_PAGE_REQ. SMMUv3 forbids
the Stall model on PCIe streams (PCIe must use Terminate), and PRI is only
enabled on PCIe masters, so stall_enabled and pri_enabled never co-occur on
a single master. arm_smmu_page_response() can therefore key on the master
state: CMDQ_OP_RESUME for stall_enabled, CMDQ_OP_PRI_RESP for pri_enabled,
mapping IOMMU_PAGE_RESP_* to the PRI response codes.

Note that a CMD_PRI_RESP.Resp encodes 0b00 as ResponseFailure (a permanent
non-paging error), 0b01 as InvalidRequest (page-in unsuccessful), and 0b10
as Success. So IOMMU_PAGE_RESP_FAILURE maps to PRI_RESP_DENY (0b00) while
IOMMU_PAGE_RESP_INVALID maps to PRI_RESP_FAIL (0b01), following the codes
rather than the similarity of the enum names.

Extend arm_smmu_enable_iopf() to also proceed for a PRI-enabled master, so
that attaching a fault-capable domain would set up IOPF for it. Note that
a later change will set master->pri_enabled, once all PRI paths are ready.

Note: streams[0].id remains the RID because arm_smmu_enable_iopf() rejects
num_streams != 1.

Co-developed-by: Barak Biber <bbiber@nvidia.com>
Signed-off-by: Barak Biber <bbiber@nvidia.com>
Co-developed-by: Stefan Kaestle <skaestle@nvidia.com>
Signed-off-by: Stefan Kaestle <skaestle@nvidia.com>
Signed-off-by: Malak Marrid <mmarrid@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 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, 57 insertions(+), 19 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index de7e4284658a1..92d21ded07ae9 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -1015,6 +1015,7 @@ struct arm_smmu_master {
 	/* Locked by the iommu core using the group mutex */
 	struct arm_smmu_ctx_desc_cfg	cd_table;
 	unsigned int			num_streams;
+	bool				pri_enabled : 1;
 	bool				ats_enabled : 1;
 	bool				ste_ats_enabled : 1;
 	bool				stall_enabled;
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 352c916b2a572..64540cfb73244 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1028,32 +1028,69 @@ static int arm_smmu_drain_queue(struct arm_smmu_device *smmu,
 	return -ETIMEDOUT;
 }
 
-static void arm_smmu_page_response(struct device *dev, struct iopf_fault *unused,
+static void arm_smmu_page_response(struct device *dev, struct iopf_fault *evt,
 				   struct iommu_page_response *resp)
 {
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
-	u8 resume_resp;
+	struct arm_smmu_cmd cmd;
+	int sid;
 
-	if (WARN_ON(!master->stall_enabled))
+	if (WARN_ON_ONCE(evt->fault.type != IOMMU_FAULT_PAGE_REQ))
 		return;
 
-	switch (resp->code) {
-	case IOMMU_PAGE_RESP_INVALID:
-	case IOMMU_PAGE_RESP_FAILURE:
-		resume_resp = CMDQ_RESUME_0_RESP_ABORT;
-		break;
-	case IOMMU_PAGE_RESP_SUCCESS:
-		resume_resp = CMDQ_RESUME_0_RESP_RETRY;
-		break;
-	default:
-		resume_resp = CMDQ_RESUME_0_RESP_TERM;
-		break;
+	/* IOPF is gated to num_streams == 1 in arm_smmu_enable_iopf() */
+	sid = master->streams[0].id;
+
+	if (master->stall_enabled) {
+		u8 resume_resp;
+
+		switch (resp->code) {
+		case IOMMU_PAGE_RESP_INVALID:
+		case IOMMU_PAGE_RESP_FAILURE:
+			resume_resp = CMDQ_RESUME_0_RESP_ABORT;
+			break;
+		case IOMMU_PAGE_RESP_SUCCESS:
+			resume_resp = CMDQ_RESUME_0_RESP_RETRY;
+			break;
+		default:
+			resume_resp = CMDQ_RESUME_0_RESP_TERM;
+			break;
+		}
+		cmd = arm_smmu_make_cmd_resume(sid, resp->grpid, resume_resp);
+	} else if (master->pri_enabled) {
+		enum pri_resp pri_resp;
+		bool ssv;
+
+		/* PCIe allows only one PRG Response per group */
+		if (!(evt->fault.prm.flags &
+		      IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))
+			return;
+		switch (resp->code) {
+		case IOMMU_PAGE_RESP_SUCCESS:
+			pri_resp = PRI_RESP_SUCC;
+			break;
+		case IOMMU_PAGE_RESP_FAILURE:
+			/* 0b00 ResponseFailure: a permanent non-paging error */
+			pri_resp = PRI_RESP_DENY;
+			break;
+		case IOMMU_PAGE_RESP_INVALID:
+			/* 0b01 InvalidRequest: page-in unsuccessful */
+			pri_resp = PRI_RESP_FAIL;
+			break;
+		default:
+			WARN_ON(true);
+			return;
+		}
+		ssv = !!(evt->fault.prm.flags &
+			 IOMMU_FAULT_PAGE_REQUEST_PASID_VALID);
+		cmd = arm_smmu_make_cmd_pri_resp(sid, resp->pasid, ssv,
+						 resp->grpid, pri_resp);
+	} else {
+		WARN_ON_ONCE(1);
+		return;
 	}
 
-	arm_smmu_cmdq_issue_cmd(master->smmu,
-				arm_smmu_make_cmd_resume(master->streams[0].id,
-							 resp->grpid,
-							 resume_resp));
+	arm_smmu_cmdq_issue_cmd(master->smmu, cmd);
 	/*
 	 * Don't send a SYNC, it doesn't do anything for RESUME or PRI_RESP.
 	 * RESUME consumption guarantees that the stalled transaction will be
@@ -3209,7 +3246,7 @@ static int arm_smmu_enable_iopf(struct arm_smmu_master *master,
 	 * device-specific fault handlers and don't need IOPF, so this is not a
 	 * failure.
 	 */
-	if (!master->stall_enabled)
+	if (!master->stall_enabled && !master->pri_enabled)
 		return 0;
 
 	/* We're not keeping track of SIDs in fault events */
-- 
2.43.0

[PATCH v3 07/13] iommu/arm-smmu-v3: Disable the queue IRQs before disabling the SMMU

[Nicolin Chen]

The EVTQ, PRIQ and combined IRQ handlers are threaded and issue commands of
their own, e.g. a CMDQ_OP_PRI_RESP for a page request. Disabling the SMMU
while one is in flight hands that command to a queue consuming nothing, so
its poll waits out a full timeout.

Two paths disable the SMMU while those IRQs are still requested: a failing
arm_smmu_device_reset() returns to a probe that disables the device itself,
and arm_smmu_disable_action() covers an unbind or any later probe failure.
Both can run after arm_smmu_setup_irqs() requested the IRQs.

Disable those IRQs first in both paths, so that no handler is left running
once the SMMU goes down.

Also clear an IRQ number when its request fails or is skipped for a missing
ARM_SMMU_FEAT_PRI, keeping disable_irq() to the IRQs that this driver truly
owns.

Note that the IOPF queue needs no such care of its own, as devres frees the
IRQs before running the release action of that queue, which came earlier in
arm_smmu_init_queues().

Assisted-by: Claude:claude-opus-5
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 36 ++++++++++++++++++---
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 64540cfb73244..d45d97f356cf0 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -4907,8 +4907,10 @@ static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
 						arm_smmu_evtq_thread,
 						IRQF_ONESHOT,
 						"arm-smmu-v3-evtq", smmu);
-		if (ret < 0)
+		if (ret < 0) {
 			dev_warn(smmu->dev, "failed to enable evtq irq\n");
+			smmu->evtq.q.irq = 0;
+		}
 	} else {
 		dev_warn(smmu->dev, "no evtq irq - events will not be reported!\n");
 	}
@@ -4931,12 +4933,17 @@ static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
 							IRQF_ONESHOT,
 							"arm-smmu-v3-priq",
 							smmu);
-			if (ret < 0)
+			if (ret < 0) {
 				dev_warn(smmu->dev,
 					 "failed to enable priq irq\n");
+				smmu->priq.q.irq = 0;
+			}
 		} else {
 			dev_warn(smmu->dev, "no priq irq - PRI will be broken\n");
 		}
+	} else {
+		/* An unrequested IRQ (e.g. set by DT) must not be disabled */
+		smmu->priq.q.irq = 0;
 	}
 }
 
@@ -4964,8 +4971,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 					arm_smmu_combined_irq_thread,
 					IRQF_ONESHOT,
 					"arm-smmu-v3-combined-irq", smmu);
-		if (ret < 0)
+		if (ret < 0) {
 			dev_warn(smmu->dev, "failed to enable combined irq\n");
+			smmu->combined_irq = 0;
+		}
 	} else
 		arm_smmu_setup_unique_irqs(smmu);
 
@@ -4992,10 +5001,22 @@ static int arm_smmu_device_disable(struct arm_smmu_device *smmu)
 	return ret;
 }
 
+/* Quiesce the queue IRQ threads, e.g. before disabling the SMMU */
+static void arm_smmu_disable_irqs(struct arm_smmu_device *smmu)
+{
+	if (smmu->combined_irq)
+		disable_irq(smmu->combined_irq);
+	if (smmu->evtq.q.irq)
+		disable_irq(smmu->evtq.q.irq);
+	if (smmu->priq.q.irq)
+		disable_irq(smmu->priq.q.irq);
+}
+
 static void arm_smmu_disable_action(void *data)
 {
 	struct arm_smmu_device *smmu = data;
 
+	arm_smmu_disable_irqs(smmu);
 	if (smmu->impl_ops && smmu->impl_ops->device_disable)
 		smmu->impl_ops->device_disable(smmu);
 	arm_smmu_device_disable(smmu);
@@ -5142,18 +5163,23 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu)
 				      ARM_SMMU_CR0ACK);
 	if (ret) {
 		dev_err(smmu->dev, "failed to enable SMMU interface\n");
-		return ret;
+		goto err_disable_irqs;
 	}
 
 	if (smmu->impl_ops && smmu->impl_ops->device_reset) {
 		ret = smmu->impl_ops->device_reset(smmu);
 		if (ret) {
 			dev_err(smmu->dev, "failed to reset impl\n");
-			return ret;
+			goto err_disable_irqs;
 		}
 	}
 
 	return 0;
+
+err_disable_irqs:
+	/* The probe error path cannot tell if the IRQs were requested */
+	arm_smmu_disable_irqs(smmu);
+	return ret;
 }
 
 #define IIDR_IMPLEMENTER_ARM		0x43b
-- 
2.43.0

[PATCH v3 08/13] iommu/arm-smmu-v3: Disable PRI when no IRQ handler is registered

[Nicolin Chen]

arm_smmu_setup_irqs() has three failure paths that leave the priq without
a handler: a missing priq IRQ line, devm_request_threaded_irq() failure on
the priq IRQ, and devm_request_threaded_irq() failure on the combined IRQ.
Each path warned but kept ARM_SMMU_FEAT_PRI set in smmu->features.

With FEAT_PRI still set, arm_smmu_setup_irqs() enables IRQ_CTRL_PRIQ_IRQEN
against a handler that does not exist, and a later change would then have
arm_smmu_probe_device() call pci_enable_pri() on PCIe endpoints, allowing
them to issue PRI Page Requests that pile up in the priq with no drainer.
Separately, arm_smmu_device_reset() has already enabled CR0_PRIQEN before
it invoked arm_smmu_setup_irqs(), so the hardware queue would stay enabled
regardless of what setup_irqs decides.

Clear ARM_SMMU_FEAT_PRI in all three failure paths, so that subsequent code
treats PRI as unavailable. And disable CR0_PRIQEN after the setup function
returns.

A kdump kernel cannot service page requests: strip FEAT_PRI when detecting
features, so downstream PRI setup is skipped.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index d45d97f356cf0..60f21591de9e8 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -4937,9 +4937,11 @@ static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
 				dev_warn(smmu->dev,
 					 "failed to enable priq irq\n");
 				smmu->priq.q.irq = 0;
+				smmu->features &= ~ARM_SMMU_FEAT_PRI;
 			}
 		} else {
 			dev_warn(smmu->dev, "no priq irq - PRI will be broken\n");
+			smmu->features &= ~ARM_SMMU_FEAT_PRI;
 		}
 	} else {
 		/* An unrequested IRQ (e.g. set by DT) must not be disabled */
@@ -4974,6 +4976,7 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 		if (ret < 0) {
 			dev_warn(smmu->dev, "failed to enable combined irq\n");
 			smmu->combined_irq = 0;
+			smmu->features &= ~ARM_SMMU_FEAT_PRI;
 		}
 	} else
 		arm_smmu_setup_unique_irqs(smmu);
@@ -5154,6 +5157,10 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu)
 		return ret;
 	}
 
+	/* arm_smmu_setup_irqs() might have unset the ARM_SMMU_FEAT_PRI */
+	if (!(smmu->features & ARM_SMMU_FEAT_PRI))
+		enables &= ~CR0_PRIQEN;
+
 	if (is_kdump_kernel())
 		enables &= ~(CR0_EVTQEN | CR0_PRIQEN);
 
@@ -5295,7 +5302,7 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
 	}
 
 	/* Boolean feature flags */
-	if (IS_ENABLED(CONFIG_PCI_PRI) && reg & IDR0_PRI)
+	if (IS_ENABLED(CONFIG_PCI_PRI) && reg & IDR0_PRI && !is_kdump_kernel())
 		smmu->features |= ARM_SMMU_FEAT_PRI;
 
 	if (IS_ENABLED(CONFIG_PCI_ATS) && reg & IDR0_ATS)
-- 
2.43.0

[PATCH v3 09/13] iommu/arm-smmu-v3: Support PRI Page Request in arm_smmu_handle_ppr()

[Nicolin Chen]

Now, arm_smmu_page_response() can issue CMDQ_OP_PRI_RESP for page requests
from the IOPF infrastructure. Forward PRI requests from the priq into the
IOPF infrastructure for PRI-enabled masters by building an iopf_fault from
the priq entry and calling iommu_report_device_fault().

For an unrecognised StreamID or a master without master->pri_enabled, fall
through to the existing "unexpected PRI request" log + LAST-page DENY path
to release the credit per the PCIe PRI spec. Merge its two info prints in
the path into one single ratelimited line, as a malfunctioning device can
spam unrecognised requests and easily flood the kernel log.

Discard any PASID Stop Marker (LRW = 0b100) prior to the fault report and
the DENY fallback both, because a Stop Marker does not expect a response.
The IOPF infrastructure requires the driver to discard it, as per the doc
at iommu_report_device_fault(). This also matches the intel-iommu code.

Note that master->pri_enabled will only be set by a later change, once all
the PRI paths are ready.

On PRIQ overflow, partial requests stored via report_partial_fault() whose
LAST-page entry was lost stay in iopf_param->partial. Drop them by calling
iopf_queue_discard_partial(), matching intel-iommu's handling. Do it after
the queue is fully drained, since the visible entries all precede the loss
point: a group whose LAST-page entry is still in the queue gets assembled
before the discard, rather than losing its stored partials to it. Nor can
a new arrival race against the discard, since an active overflow inhibits
new entries from being written to the PRI queue (IHI0070 8.1), up until a
final CONS write acknowledges it.

Co-developed-by: Barak Biber <bbiber@nvidia.com>
Signed-off-by: Barak Biber <bbiber@nvidia.com>
Co-developed-by: Stefan Kaestle <skaestle@nvidia.com>
Signed-off-by: Stefan Kaestle <skaestle@nvidia.com>
Signed-off-by: Malak Marrid <mmarrid@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 59 +++++++++++++++++++--
 1 file changed, 55 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 60f21591de9e8..502b5c7673bc9 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2368,6 +2368,7 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 
 static void arm_smmu_handle_ppr(struct arm_smmu_device *smmu, u64 *evt)
 {
+	struct arm_smmu_master *master;
 	u32 sid, ssid;
 	u16 grpid;
 	bool ssv, last;
@@ -2378,9 +2379,47 @@ static void arm_smmu_handle_ppr(struct arm_smmu_device *smmu, u64 *evt)
 	last = FIELD_GET(PRIQ_0_PRG_LAST, evt[0]);
 	grpid = FIELD_GET(PRIQ_1_PRG_IDX, evt[1]);
 
-	dev_info(smmu->dev, "unexpected PRI request received:\n");
-	dev_info(smmu->dev,
-		 "\tsid 0x%08x.0x%05x: [%u%s] %sprivileged %s%s%s access at iova 0x%016llx\n",
+	/*
+	 * A PASID Stop Marker (LRW = 0b100) does not expect a response and
+	 * must be discarded before fault reporting: see the documentation
+	 * at iommu_report_device_fault().
+	 */
+	if (last && !(evt[0] & (PRIQ_0_PERM_READ | PRIQ_0_PERM_WRITE)))
+		return;
+
+	mutex_lock(&smmu->streams_mutex);
+	master = arm_smmu_find_master(smmu, sid);
+	if (master && master->pri_enabled) {
+		struct iopf_fault iopf_fault = {};
+		struct iommu_fault *fault = &iopf_fault.fault;
+
+		fault->type = IOMMU_FAULT_PAGE_REQ;
+		if (last)
+			fault->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE;
+		if (ssv) {
+			fault->prm.flags |=
+				IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
+			fault->prm.pasid = ssid;
+		}
+		fault->prm.grpid = grpid;
+		if (evt[0] & PRIQ_0_PERM_READ)
+			fault->prm.perm |= IOMMU_FAULT_PERM_READ;
+		if (evt[0] & PRIQ_0_PERM_WRITE)
+			fault->prm.perm |= IOMMU_FAULT_PERM_WRITE;
+		if (evt[0] & PRIQ_0_PERM_EXEC)
+			fault->prm.perm |= IOMMU_FAULT_PERM_EXEC;
+		if (evt[0] & PRIQ_0_PERM_PRIV)
+			fault->prm.perm |= IOMMU_FAULT_PERM_PRIV;
+		fault->prm.addr = FIELD_GET(PRIQ_1_ADDR_MASK, evt[1]) << 12;
+
+		iommu_report_device_fault(master->dev, &iopf_fault);
+		mutex_unlock(&smmu->streams_mutex);
+		return;
+	}
+	mutex_unlock(&smmu->streams_mutex);
+
+	dev_info_ratelimited(smmu->dev,
+		 "unexpected PRI request: sid 0x%08x.0x%05x: [%u%s] %sprivileged %s%s%s access at iova 0x%016llx\n",
 		 sid, ssid, grpid, last ? "L" : "",
 		 evt[0] & PRIQ_0_PERM_PRIV ? "" : "un",
 		 evt[0] & PRIQ_0_PERM_READ ? "R" : "",
@@ -2400,15 +2439,27 @@ static irqreturn_t arm_smmu_priq_thread(int irq, void *dev)
 	struct arm_smmu_queue *q = &smmu->priq.q;
 	struct arm_smmu_ll_queue *llq = &q->llq;
 	u64 evt[PRIQ_ENT_DWORDS];
+	bool overflow = false;
 
 	do {
 		while (!queue_remove_raw(q, evt))
 			arm_smmu_handle_ppr(smmu, evt);
 
-		if (queue_sync_prod_in(q) == -EOVERFLOW)
+		if (queue_sync_prod_in(q) == -EOVERFLOW) {
 			dev_err(smmu->dev, "PRIQ overflow detected -- requests lost\n");
+			overflow = true;
+		}
 	} while (!queue_empty(llq));
 
+	/*
+	 * Discard the partial faults after the drain, so any group with its
+	 * LAST-page entry visible in the queue gets assembled beforehand. An
+	 * active overflow condition inhibits new entries from being written
+	 * to the PRI queue, until it gets acknowledged below.
+	 */
+	if (overflow)
+		iopf_queue_discard_partial(smmu->evtq.iopf);
+
 	/* Sync our overflow flag, as we believe we're up to speed */
 	queue_sync_cons_ovf(q);
 	return IRQ_HANDLED;
-- 
2.43.0

[PATCH v3 10/13] iommu/arm-smmu-v3: Allocate IOPF queue for ARM_SMMU_FEAT_PRI

[Nicolin Chen]

After the previous change drops the FEAT_SVA gate, arm_smmu_init_queues()
allocates smmu->evtq.iopf when the SMMU advertises FEAT_STALLS. PRI support
is being added on top of the IOPF infrastructure, so extend the allocation
condition to cover SMMUs that advertise FEAT_PRI without FEAT_STALLS.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 502b5c7673bc9..47c95b691503b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -4741,7 +4741,7 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
 	if (ret)
 		return ret;
 
-	if (smmu->features & ARM_SMMU_FEAT_STALLS) {
+	if (smmu->features & (ARM_SMMU_FEAT_STALLS | ARM_SMMU_FEAT_PRI)) {
 		smmu->evtq.iopf = iopf_queue_alloc(dev_name(smmu->dev));
 		if (!smmu->evtq.iopf)
 			return -ENOMEM;
-- 
2.43.0

[PATCH v3 11/13] PCI/ATS: Add PRI stubs

[Nicolin Chen]

From: Jean-Philippe Brucker <jean-philippe@linaro.org>

The SMMUv3 driver, which may be built without CONFIG_PCI, will shortly gain
PRI support. Partially revert commit c6e9aefbf9db ("PCI/ATS: Remove unused
PRI and PASID stubs") to re-introduce the PRI stubs, avoiding more #ifdefs
in the SMMU driver.

Link: https://lore.kernel.org/iommu/20201112125519.3987595-8-jean-philippe@linaro.org/
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
[nicolinc: drop stale APIs]
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 include/linux/pci-ats.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
index f3723b6861294..07d24c1c3acdb 100644
--- a/include/linux/pci-ats.h
+++ b/include/linux/pci-ats.h
@@ -36,6 +36,11 @@ int pci_reset_pri(struct pci_dev *pdev);
 int pci_prg_resp_pasid_required(struct pci_dev *pdev);
 bool pci_pri_supported(struct pci_dev *pdev);
 #else
+static inline int pci_enable_pri(struct pci_dev *pdev, u32 reqs)
+{ return -ENODEV; }
+static inline void pci_disable_pri(struct pci_dev *pdev) { }
+static inline int pci_reset_pri(struct pci_dev *pdev)
+{ return -ENODEV; }
 static inline bool pci_pri_supported(struct pci_dev *pdev)
 { return false; }
 #endif /* CONFIG_PCI_PRI */
-- 
2.43.0

[PATCH v3 12/13] PCI/ATS: Export pci_enable_pri() and pci_reset_pri()

[Nicolin Chen]

From: Jean-Philippe Brucker <jean-philippe@linaro.org>

Export pci_enable_pri() and pci_reset_pri() so the SMMUv3 driver, which is
tristate, can use them from its probe path when built as a module.

Link: https://lore.kernel.org/iommu/20201112125519.3987595-9-jean-philippe@linaro.org/
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
[nicolinc: drop stale or already-exported APIs]
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/pci/ats.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 9cb23780093dd..b6de3c858d390 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -320,6 +320,7 @@ int pci_enable_pri(struct pci_dev *pdev, u32 reqs)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(pci_enable_pri);
 
 /**
  * pci_disable_pri - Disable PRI capability
@@ -399,6 +400,7 @@ int pci_reset_pri(struct pci_dev *pdev)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(pci_reset_pri);
 
 /**
  * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit
-- 
2.43.0

[PATCH v3 13/13] iommu/arm-smmu-v3: Enable PRI for PCI device in arm_smmu_probe_device()

[Nicolin Chen]

Now PRI requests can be correctly handled. Enable the PCI cap when probing
a PCI device. Also flush the priq in arm_smmu_attach_release().

Drain the priq for any PRI-enabled master following the same rationale as
the eventq drain: a stale page request must be answered while it can still
resolve to the old attach handle, even when the old attachment did not set
up IOPF, or else the threaded handler could route it to the next domain.

Note that PRI is enabled at the probe time, while ATS gets toggled by the
attach/detach routines, so a master could have PRI enabled when its ATS is
disabled. PCIe (Base 6.3, Table 10-14) sets no ATS precondition on the PRI
Enable bit. Its only ordering rule is that the interface must have gotten
successfully Stopped prior to an enabling, which pci_enable_pri() already
checks using PCI_PRI_STATUS_STOPPED. Also, a PRI-enabled device would not
issue a page request until it starts to use ATS.

Set the per-device outstanding request budget to the full priq depth, same
as intel-iommu's per-device PRQ_DEPTH choice. A fixed per-device cap won't
prevent multiple PRI-capable devices from potentially exceeding the priq's
capacity; priq overflow is recoverable per the SMMUv3 spec, and it is rare
in practice.

Select PCI_PRI in Kconfig like other IOMMUs, gated on PCI so the build can
stay clean for non-PCI ARM SMMUv3 configurations.

SMMUv3 forbids the Stall model on PCIe streams. Refuse to enable PRI on a
PCIe master that came with stall_enabled, so page_response() can dispatch
on master state unambiguously.

Refuse to enable PRI as well on any master reporting num_streams != 1, as
arm_smmu_enable_iopf() rejects multi-stream masters, so IOPF cannot be set
up for them; keeping PRI enabled would let a PRI request arrive on an alias
StreamID and get a PRI_RESP_DENY issued against streams[0] by the driver's
error-response path.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/Kconfig                   |  1 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 49 +++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/drivers/iommu/arm/Kconfig b/drivers/iommu/arm/Kconfig
index b848a4253677d..a31d04f5b031a 100644
--- a/drivers/iommu/arm/Kconfig
+++ b/drivers/iommu/arm/Kconfig
@@ -80,6 +80,7 @@ config ARM_SMMU_V3
 	select IOMMU_IO_PGTABLE_LPAE
 	select IOMMU_IOPF
 	select GENERIC_MSI_IRQ
+	select PCI_PRI if PCI
 	select IOMMUFD_DRIVER if IOMMUFD
 	help
 	  Support for implementations of the ARM System MMU architecture
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 47c95b691503b..d35d814900cb2 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3509,6 +3509,17 @@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
 			synchronize_irq(smmu->combined_irq);
 	}
 
+	/* Same as the eventq drain above, for the hardware priq */
+	if (master->pri_enabled) {
+		ret |= arm_smmu_drain_queue(smmu, &smmu->priq.q, false);
+		/* Ensure pending requests have reached the IOPF queue */
+		if (!ret && smmu->priq.q.irq)
+			synchronize_irq(smmu->priq.q.irq);
+		/* Pending requests might be in the combined_irq handler */
+		if (!ret && smmu->combined_irq)
+			synchronize_irq(smmu->combined_irq);
+	}
+
 	/*
 	 * Only IOPF-enabled attachments queue fault work, and such work
 	 * references the old domain via its attach handle. Flush it, as
@@ -4446,6 +4457,40 @@ static int arm_smmu_master_prepare_ats(struct arm_smmu_master *master)
 	return arm_smmu_alloc_cd_tables(master);
 }
 
+static void arm_smmu_master_enable_pri(struct arm_smmu_master *master)
+{
+	struct arm_smmu_device *smmu = master->smmu;
+	struct pci_dev *pdev;
+	unsigned int reqs;
+
+	if (!(smmu->features & ARM_SMMU_FEAT_PRI) || !smmu->evtq.iopf)
+		return;
+	if (!dev_is_pci(master->dev))
+		return;
+	pdev = to_pci_dev(master->dev);
+
+	if (!pci_pri_supported(pdev))
+		return;
+
+	/* SMMUv3 forbids the Stall model on PCIe streams */
+	if (master->stall_enabled) {
+		pci_warn(pdev, "stall enabled, skip PRI\n");
+		return;
+	}
+
+	if (master->num_streams != 1) {
+		pci_warn(pdev, "multi-SID master, skip PRI\n");
+		return;
+	}
+
+	reqs = 1 << smmu->priq.q.llq.max_n_shift;
+
+	if (!pci_reset_pri(pdev) && !pci_enable_pri(pdev, reqs))
+		master->pri_enabled = true;
+	else
+		pci_warn(pdev, "failed to enable PRI\n");
+}
+
 static struct iommu_device *arm_smmu_probe_device(struct device *dev)
 {
 	int ret;
@@ -4498,6 +4543,8 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
 	if (ret)
 		goto err_disable_pasid;
 
+	arm_smmu_master_enable_pri(master);
+
 	return &smmu->iommu;
 
 err_disable_pasid:
@@ -4523,6 +4570,8 @@ static void arm_smmu_release_device(struct device *dev)
 		iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
 	}
 
+	if (master->pri_enabled)
+		pci_disable_pri(to_pci_dev(master->dev));
 	arm_smmu_disable_pasid(master);
 	arm_smmu_remove_master(master);
 	if (arm_smmu_cdtab_allocated(&master->cd_table))
-- 
2.43.0

Re: [PATCH v3 02/13] iommu/arm-smmu-v3: Add Q_POS() macro

[sashiko-bot]

> A queue position, the wrap bit combined with the index, is Q_WRP | Q_IDX.
> It is a (max_n_shift + 1)-bit value that wraps at twice the queue capacity.
> queue_inc_cons(), queue_sync_cons_ovf(), and queue_inc_prod_n() currently
> compute such positions by open-coding the two macros at each call site.
> 
> Add a Q_POS() macro and switch the open-coded sites to it.
> 
> A subsequent change will apply Q_POS() to a position difference, to count
> the entries that a queue pointer moved past.
> 
> No functional change intended.
> 
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788222485.git.nicolinc@nvidia.com?part=2

Re: [PATCH v3 11/13] PCI/ATS: Add PRI stubs

[sashiko-bot]

> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> 
> The SMMUv3 driver, which may be built without CONFIG_PCI, will shortly gain
> PRI support. Partially revert commit c6e9aefbf9db ("PCI/ATS: Remove unused
> PRI and PASID stubs") to re-introduce the PRI stubs, avoiding more #ifdefs
> in the SMMU driver.
> 
> Link: https://lore.kernel.org/iommu/20201112125519.3987595-8-jean-philippe@linaro.org/
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> [nicolinc: drop stale APIs]
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788222485.git.nicolinc@nvidia.com?part=11

Re: [PATCH v3 10/13] iommu/arm-smmu-v3: Allocate IOPF queue for ARM_SMMU_FEAT_PRI

[sashiko-bot]

> After the previous change drops the FEAT_SVA gate, arm_smmu_init_queues()
> allocates smmu->evtq.iopf when the SMMU advertises FEAT_STALLS. PRI support
> is being added on top of the IOPF infrastructure, so extend the allocation
> condition to cover SMMUs that advertise FEAT_PRI without FEAT_STALLS.
> 
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788222485.git.nicolinc@nvidia.com?part=10

Re: [PATCH v3 12/13] PCI/ATS: Export pci_enable_pri() and pci_reset_pri()

[sashiko-bot]

> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> 
> Export pci_enable_pri() and pci_reset_pri() so the SMMUv3 driver, which is
> tristate, can use them from its probe path when built as a module.
> 
> Link: https://lore.kernel.org/iommu/20201112125519.3987595-9-jean-philippe@linaro.org/
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> [nicolinc: drop stale or already-exported APIs]
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788222485.git.nicolinc@nvidia.com?part=12

Re: [PATCH v3 05/13] iommu/arm-smmu-v3: Allocate IOPF queue without FEAT_SVA

[sashiko-bot]

> arm_smmu_init_queues() currently allocates smmu->evtq.iopf only when both
> FEAT_SVA and FEAT_STALLS are advertised by the SMMU. The FEAT_SVA gate is
> unnecessary: iopf_queue_alloc() is a plain workqueue allocator with no SVA
> dependency, and FEAT_STALLS alone can produce events that need IOPF.
> 
> On HW with FEAT_STALLS but no FEAT_SVA, a master with the "dma-can-stall"
> device-tree property has master->stall_enabled set at probe. Then also with
> CONFIG_ARM_SMMU_V3_SVA=y, arm_smmu_enable_iopf() sees stall_enabled, calls
> iopf_queue_add_device(NULL, ...) at fault-handling domain attach, which now
> oopses on mutex_lock(&queue->lock).
> 
> Drop the FEAT_SVA term so the IOPF workqueue is allocated whenever the SMMU
> can produce IOPF events, and select IOMMU_IOPF from ARM_SMMU_V3 as well, so
> iopf_queue_alloc() has a real implementation instead of the NULL-returning
> stub when CONFIG_ARM_SMMU_V3_SVA is unset.
> 
> IOPF is orthogonal to SVA anyway. So, drop the CONFIG_ARM_SMMU_V3_SVA gates
> in arm_smmu_enable_iopf() and arm_smmu_disable_iopf().
> 
> Fixes: 395ad89d11fd9 ("iommu/arm-smmu-v3: Add stall support for platform devices")
> Cc: stable@vger.kernel.org # v5.14
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788222485.git.nicolinc@nvidia.com?part=5

Re: [PATCH v3 01/13] iommu/arm-smmu-v3: Add arm_smmu_attach_release()

[sashiko-bot]

> The IOPF teardown is done in arm_smmu_remove_master_domain() when releasing
> the master_domain on detach, under the global arm_smmu_asid_lock mutex. But
> the teardown must drain any in-flight IOPF (for the old domain), before the
> master_domain is freed via iopf_queue_flush_dev() calling flush_workqueue()
> that can block on user-faulting page-fault handlers. Doing so while holding
> the arm_smmu_asid_lock would stall any unrelated attachment in the system.
> 
> Split the teardown out of arm_smmu_remove_master_domain(), to a new helper
> arm_smmu_attach_release() that runs after arm_smmu_asid_lock is released.
> 
> Since no other device would use the old master_domain that is being freed,
> it's safe to move out of arm_smmu_asid_lock (still under the protection of
> iommu_group->mutex).
> 
> Note: this is a pure refactor; no functional change.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788222485.git.nicolinc@nvidia.com?part=1

Re: [PATCH v3 08/13] iommu/arm-smmu-v3: Disable PRI when no IRQ handler is registered

[sashiko-bot]

> arm_smmu_setup_irqs() has three failure paths that leave the priq without
> a handler: a missing priq IRQ line, devm_request_threaded_irq() failure on
> the priq IRQ, and devm_request_threaded_irq() failure on the combined IRQ.
> Each path warned but kept ARM_SMMU_FEAT_PRI set in smmu->features.
> 
> With FEAT_PRI still set, arm_smmu_setup_irqs() enables IRQ_CTRL_PRIQ_IRQEN
> against a handler that does not exist, and a later change would then have
> arm_smmu_probe_device() call pci_enable_pri() on PCIe endpoints, allowing
> them to issue PRI Page Requests that pile up in the priq with no drainer.
> Separately, arm_smmu_device_reset() has already enabled CR0_PRIQEN before
> it invoked arm_smmu_setup_irqs(), so the hardware queue would stay enabled
> regardless of what setup_irqs decides.
> 
> Clear ARM_SMMU_FEAT_PRI in all three failure paths, so that subsequent code
> treats PRI as unavailable. And disable CR0_PRIQEN after the setup function
> returns.
> 
> A kdump kernel cannot service page requests: strip FEAT_PRI when detecting
> features, so downstream PRI setup is skipped.
> 
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788222485.git.nicolinc@nvidia.com?part=8

Re: [PATCH v3 03/13] iommu/arm-smmu-v3: Drain in-flight fault events on domain detach

[sashiko-bot]

> When a device is switching away from a domain, either through a detach or a
> replace operation, in-flight stall events for the old domain might still be
> on the SMMU's hardware event queue or on the IOMMU core's IOPF queue. Thus,
> if the IOMMU core swaps the device's attach_handle and frees the old domain
> before those handlers complete, the IOPF work might hit use-after-free.
> 
> Two queues need to be drained: the SMMU hardware event queue and the IOMMU
> core IOPF software workqueue. Start with the former: add a counting-based
> arm_smmu_drain_queue() helper, and poll the evtq on a domain detach, so a
> pending IRQ won't let the threaded handler run after the drain and queue a
> fault referencing the domain being freed. Its until_empty mode serves the
> suspend and runtime PM routines that would drain the CMDQ. Any timed-out
> drain fires a WARN_ON as well, since reaching the timeout would take some
> stuck consumer in any realistic case.
> 
> [ ... ]
> 
> Fixes: cfea71aea921 ("iommu/arm-smmu-v3: Put iopf enablement in the domain attach path")
> Cc: stable@vger.kernel.org # v6.16
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788222485.git.nicolinc@nvidia.com?part=3

Re: [PATCH v3 09/13] iommu/arm-smmu-v3: Support PRI Page Request in arm_smmu_handle_ppr()

[sashiko-bot]

> Now, arm_smmu_page_response() can issue CMDQ_OP_PRI_RESP for page requests
> from the IOPF infrastructure. Forward PRI requests from the priq into the
> IOPF infrastructure for PRI-enabled masters by building an iopf_fault from
> the priq entry and calling iommu_report_device_fault().
> 
> For an unrecognised StreamID or a master without master->pri_enabled, fall
> through to the existing "unexpected PRI request" log + LAST-page DENY path
> to release the credit per the PCIe PRI spec. Merge its two info prints in
> the path into one single ratelimited line, as a malfunctioning device can
> spam unrecognised requests and easily flood the kernel log.
> 
> Discard any PASID Stop Marker (LRW = 0b100) prior to the fault report and
> the DENY fallback both, because a Stop Marker does not expect a response.
> The IOPF infrastructure requires the driver to discard it, as per the doc
> at iommu_report_device_fault(). This also matches the intel-iommu code.
> [ ... ]
> Signed-off-by: Barak Biber <bbiber@nvidia.com>
> Co-developed-by: Stefan Kaestle <skaestle@nvidia.com>
> Signed-off-by: Stefan Kaestle <skaestle@nvidia.com>
> Signed-off-by: Malak Marrid <mmarrid@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788222485.git.nicolinc@nvidia.com?part=9

Re: [PATCH v3 13/13] iommu/arm-smmu-v3: Enable PRI for PCI device in arm_smmu_probe_device()

[sashiko-bot]

> Now PRI requests can be correctly handled. Enable the PCI cap when probing
> a PCI device. Also flush the priq in arm_smmu_attach_release().
> 
> Drain the priq for any PRI-enabled master following the same rationale as
> the eventq drain: a stale page request must be answered while it can still
> resolve to the old attach handle, even when the old attachment did not set
> up IOPF, or else the threaded handler could route it to the next domain.
> 
> Note that PRI is enabled at the probe time, while ATS gets toggled by the
> attach/detach routines, so a master could have PRI enabled when its ATS is
> disabled. PCIe (Base 6.3, Table 10-14) sets no ATS precondition on the PRI
> Enable bit. Its only ordering rule is that the interface must have gotten
> successfully Stopped prior to an enabling, which pci_enable_pri() already
> checks using PCI_PRI_STATUS_STOPPED. Also, a PRI-enabled device would not
> issue a page request until it starts to use ATS.
> [ ... ]
> up for them; keeping PRI enabled would let a PRI request arrive on an alias
> StreamID and get a PRI_RESP_DENY issued against streams[0] by the driver's
> error-response path.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788222485.git.nicolinc@nvidia.com?part=13

Re: [PATCH v3 06/13] iommu/arm-smmu-v3: Submit CMDQ_OP_PRI_RESP for IOPF event

[sashiko-bot]

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

Pre-existing issues:
- [High] If a multi-stream master device (`num_streams > 1`) has `stall_enabled` set, any stalled transaction on a stream other than index 0 will be replied to with the wrong Stream ID, permanently locking up the stalled stream and the interconnect.
--

commit e5bd1a5ac6f06fe386135f8eef6843d3096e660e
Author: Malak Marrid <mmarrid@nvidia.com>

iommu/arm-smmu-v3: Submit CMDQ_OP_PRI_RESP for IOPF event

This commit updates arm_smmu_page_response() to issue a CMDQ_OP_PRI_RESP
for IOMMU_FAULT_PAGE_REQ events when PRI is enabled. It maps IOMMU page
response codes to PRI response codes and extends arm_smmu_enable_iopf()
to allow PRI-enabled masters.

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 352c916b2a572..64540cfb73244 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c

[ ... ]

> @@ -1028,32 +1028,69 @@ static int arm_smmu_drain_queue(struct arm_smmu_device *smmu,
>  static void arm_smmu_page_response(struct device *dev, struct iopf_fault *evt,
>  				   struct iommu_page_response *resp)
>  {
>  	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> +	struct arm_smmu_cmd cmd;
> +	int sid;
>  
> +	if (WARN_ON_ONCE(evt->fault.type != IOMMU_FAULT_PAGE_REQ))
>  		return;
>  
> +	/* IOPF is gated to num_streams == 1 in arm_smmu_enable_iopf() */
> +	sid = master->streams[0].id;

[Severity: High]
This is a pre-existing issue, but does this hardcoded stream ID assignment
handle multi-stream devices correctly?

If a multi-stream master device (num_streams > 1) has the dma-can-stall
property enabled, arm_smmu_probe_device() unconditionally sets
master->stall_enabled = true.

If a translation fault occurs on any stream ID other than index 0, the
hardware generates a stall event. Because arm_smmu_enable_iopf() rejects
num_streams != 1, the IOMMU fault framework fails to find a registered
IOPF handler and falls back to an error response via
arm_smmu_page_response().

Since this function hardcodes sid = master->streams[0].id for the
CMDQ_OP_RESUME command, the actual faulting stream never receives a resume
command.

Can this result in a hardware interconnect lockup because the SMMU
permanently stalls the transaction on the faulting stream?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788222485.git.nicolinc@nvidia.com?part=6

Re: [PATCH v3 04/13] iommu/arm-smmu-v3: Flush in-flight fault work on domain detach

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Pre-existing UAF on the IOMMU core domain during a race window between IOPF queue flush and IOMMU core handle update.
--

commit fe39d3d0018e082fcdaca3fbd4d2a4bd526c0f92
Author: Nicolin Chen <nicolinc@nvidia.com>

    iommu/arm-smmu-v3: Flush in-flight fault work on domain detach

This commit adds synchronization and flushing of the IOPF queue during domain
detach. It ensures that all in-flight stall events and fault work are handled
or safely leaked in case of timeouts.

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index d255ff2519f9d..1851ee7ec15b9 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3412,8 +3413,40 @@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
[ ... ]
> +	/*
> +	 * Only IOPF-enabled attachments queue fault work, and such work
> +	 * references the old domain via its attach handle. Flush it, as
> +	 * the IOMMU core might free the old domain once this returns.
> +	 */
> +	if (master_domain->using_iopf) {
> +		/* Lastly, drain the IOPF queue */
> +		iopf_queue_flush_dev(master->dev);

[Severity: Critical]
This is a pre-existing issue, but does flushing the IOPF queue here leave a
race window open for new faults to be resolved to the old domain before the
function returns and the IOMMU core updates the attach handle?

During a domain transition, the hardware is reconfigured to the new domain,
and arm_smmu_attach_release() calls iopf_queue_flush_dev(). If the device
immediately generates a new fault, the EVTQ IRQ thread reads it and calls
iommu_report_device_fault().

Because the IOMMU core only updates the attach handle after the driver's
operation returns, the new fault is incorrectly resolved to the old domain's
handle and queued as fault work.

The IOMMU core then frees the old domain, and the queued work executes,
dereferencing the freed domain via
group->attach_handle->domain->iopf_handler(group).

Can this result in a Use-After-Free of the iommu_domain structure by an
untrusted guest or device generating faults immediately after a domain
transition?

> +
> +		/*
> +		 * A timed-out drain may leave fault work in flight, and
> +		 * iopf_queue_remove_device() would free iopf groups that
> +		 * such work still references. Skip the iopf teardown and
> +		 * leak master_domain, rather than risk a UAF.
> +		 */
> +		if (WARN_ON(ret)) {
> +			state->old_master_domain = NULL;
> +			return;
> +		}
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788222485.git.nicolinc@nvidia.com?part=4

Re: [PATCH v3 07/13] iommu/arm-smmu-v3: Disable the queue IRQs before disabling the SMMU

[sashiko-bot]

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

Pre-existing issues:
- [High] The system shutdown path disables the SMMU without quiescing queue IRQs, leading to an infinite loop in the kernel thread.
--

commit 723b797a0d5bc30d8469f53716a51e3b4d1bb03c
Author: Nicolin Chen <nicolinc@nvidia.com>

iommu/arm-smmu-v3: Disable the queue IRQs before disabling the SMMU

This commit addresses an issue where disabling the SMMU while queue IRQs are
running leads to commands being issued to a queue consuming nothing. It
ensures that the queue IRQ threads are quiesced by disabling the IRQs before
the SMMU is disabled in device reset and disable action paths.

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 64540cfb73244..d45d97f356cf0 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
[ ... ]
> @@ -4992,10 +5001,22 @@ static int arm_smmu_device_disable(struct arm_smmu_device *smmu)
>  	return ret;
>  }
>  
> +/* Quiesce the queue IRQ threads, e.g. before disabling the SMMU */
> +static void arm_smmu_disable_irqs(struct arm_smmu_device *smmu)
> +{
> +	if (smmu->combined_irq)
> +		disable_irq(smmu->combined_irq);
> +	if (smmu->evtq.q.irq)
> +		disable_irq(smmu->evtq.q.irq);
> +	if (smmu->priq.q.irq)
> +		disable_irq(smmu->priq.q.irq);
> +}
> +
>  static void arm_smmu_disable_action(void *data)
>  {
>  	struct arm_smmu_device *smmu = data;
>  
> +	arm_smmu_disable_irqs(smmu);

[Severity: High]
This is a pre-existing issue, but since this patch explicitly aims to fix
the vulnerability where the SMMU is disabled without quiescing the IRQs,
does the system shutdown path also need to be updated to call
arm_smmu_disable_irqs()?

Looking at arm_smmu_device_shutdown() which is called during system shutdown:

static void arm_smmu_device_shutdown(struct platform_device *pdev)
{
	struct arm_smmu_device *smmu = platform_get_drvdata(pdev);

	arm_smmu_device_disable(smmu);
}

The shutdown path directly disables the SMMU without first quiescing the
threaded IRQs. Because devm callbacks do not run on shutdown, the IRQ handlers
remain active while the hardware is disabled.

If a pending PRIQ or EVTQ event triggers, the threaded handler
(e.g., arm_smmu_priq_thread) executes and calls arm_smmu_page_response(),
which attempts to insert a PRI_RESP command into the CMDQ without waiting for
a SYNC. Since the SMMU is hardware-disabled, it will not process commands and
the CMDQ software ring buffer quickly fills up. Once full,
__arm_smmu_cmdq_issue_cmdlist() enters a while (!queue_has_space(...)) loop
which either spins tightly or times out every second, looping indefinitely
without any breakout condition.

Could we also add a call to arm_smmu_disable_irqs(smmu) in
arm_smmu_device_shutdown() to prevent this infinite loop?

>  	if (smmu->impl_ops && smmu->impl_ops->device_disable)
>  		smmu->impl_ops->device_disable(smmu);
>  	arm_smmu_device_disable(smmu);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788222485.git.nicolinc@nvidia.com?part=7

Re: [PATCH v3 06/13] iommu/arm-smmu-v3: Submit CMDQ_OP_PRI_RESP for IOPF event

[Jonathan Cameron]

> To handle IOMMU_FAULT_PAGE_REQ from the PRI queue, arm_smmu_page_response()
> must issue a CMDQ_OP_PRI_RESP back to the SMMU.
> 
> A stall event in the EVTQ and a PRI request in the PRIQ both surface to the
> IOPF infrastructure with fault.type == IOMMU_FAULT_PAGE_REQ. SMMUv3 forbids
> the Stall model on PCIe streams (PCIe must use Terminate), and PRI is only

There are those systems that annoy some because they smell like PCIe
(present PCIe software interfaces) but aren't and use stall mode. However, it
is nonsense to use PRI with stall mode. So, instead I'd just argue that for
stall the fault handling is done synchronously from a device point of
view so a PRI request makes no sense rather htan associating this with
PCIe as such.

Hopefully someone with such a system (Huawei folk) are testing this
and can confirm nothing breaks.

> enabled on PCIe masters, so stall_enabled and pri_enabled never co-occur on
> a single master. arm_smmu_page_response() can therefore key on the master
> state: CMDQ_OP_RESUME for stall_enabled, CMDQ_OP_PRI_RESP for pri_enabled,
> mapping IOMMU_PAGE_RESP_* to the PRI response codes.
> 
> Note that a CMD_PRI_RESP.Resp encodes 0b00 as ResponseFailure (a permanent
> non-paging error), 0b01 as InvalidRequest (page-in unsuccessful), and 0b10
> as Success. So IOMMU_PAGE_RESP_FAILURE maps to PRI_RESP_DENY (0b00) while
> IOMMU_PAGE_RESP_INVALID maps to PRI_RESP_FAIL (0b01), following the codes
> rather than the similarity of the enum names.
> 
> Extend arm_smmu_enable_iopf() to also proceed for a PRI-enabled master, so
> that attaching a fault-capable domain would set up IOPF for it. Note that
> a later change will set master->pri_enabled, once all PRI paths are ready.
> 
> Note: streams[0].id remains the RID because arm_smmu_enable_iopf() rejects
> num_streams != 1.
> 
> Co-developed-by: Barak Biber <bbiber@nvidia.com>
> Signed-off-by: Barak Biber <bbiber@nvidia.com>
> Co-developed-by: Stefan Kaestle <skaestle@nvidia.com>
> Signed-off-by: Stefan Kaestle <skaestle@nvidia.com>
> Signed-off-by: Malak Marrid <mmarrid@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
One trivial comment inline.  Given I've mostly forgotten how all this
works, this tag might not worth that much!

Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 352c916b2a57..64540cfb7324 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1028,32 +1028,69 @@ static int arm_smmu_drain_queue(struct arm_smmu_device *smmu,
>  	return -ETIMEDOUT;
>  }
>  
> -static void arm_smmu_page_response(struct device *dev, struct iopf_fault *unused,
> +static void arm_smmu_page_response(struct device *dev, struct iopf_fault *evt,
>  				   struct iommu_page_response *resp)
>  {
...
> +	} else if (master->pri_enabled) {
> +		enum pri_resp pri_resp;
> +		bool ssv;
> +
> +		/* PCIe allows only one PRG Response per group */
> +		if (!(evt->fault.prm.flags &
> +		      IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))

Go long on that line.  It is worth it for readability and it's only 81
chars.

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Re: [PATCH v3 09/13] iommu/arm-smmu-v3: Support PRI Page Request in arm_smmu_handle_ppr()

[Jonathan Cameron]

> Now, arm_smmu_page_response() can issue CMDQ_OP_PRI_RESP for page requests
> from the IOPF infrastructure. Forward PRI requests from the priq into the
> IOPF infrastructure for PRI-enabled masters by building an iopf_fault from
> the priq entry and calling iommu_report_device_fault().
> 
> For an unrecognised StreamID or a master without master->pri_enabled, fall
> through to the existing "unexpected PRI request" log + LAST-page DENY path
> to release the credit per the PCIe PRI spec. Merge its two info prints in

Either don't bother mentioning pcie spec explicitly or expand that
to a useful reference - so section number, section title.

> the path into one single ratelimited line, as a malfunctioning device can
> spam unrecognised requests and easily flood the kernel log.
> 
> Discard any PASID Stop Marker (LRW = 0b100) prior to the fault report and
> the DENY fallback both, because a Stop Marker does not expect a response.
> The IOPF infrastructure requires the driver to discard it, as per the doc
> at iommu_report_device_fault(). This also matches the intel-iommu code.
> 
> Note that master->pri_enabled will only be set by a later change, once all
> the PRI paths are ready.
> 
> On PRIQ overflow, partial requests stored via report_partial_fault() whose
> LAST-page entry was lost stay in iopf_param->partial. Drop them by calling
> iopf_queue_discard_partial(), matching intel-iommu's handling. Do it after
> the queue is fully drained, since the visible entries all precede the loss
> point: a group whose LAST-page entry is still in the queue gets assembled
> before the discard, rather than losing its stored partials to it. Nor can
> a new arrival race against the discard, since an active overflow inhibits
> new entries from being written to the PRI queue (IHI0070 8.1), up until a
> final CONS write acknowledges it.

Kind of a general comment.  I'm guessing AI assisted heavily in these.
Whilst I haven't found much that definitely doesn't belong in a patch
description, they do end up rather more verbose than ideal.  Maybe
poke it to be more terse in English language choices at least?

> 
> Co-developed-by: Barak Biber <bbiber@nvidia.com>
> Signed-off-by: Barak Biber <bbiber@nvidia.com>
> Co-developed-by: Stefan Kaestle <skaestle@nvidia.com>
> Signed-off-by: Stefan Kaestle <skaestle@nvidia.com>
> Signed-off-by: Malak Marrid <mmarrid@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

This looks fine to me, but running low on time today and
I didn't check all the flags etc, so no tag.

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Re: [PATCH v3 11/13] PCI/ATS: Add PRI stubs

[Jonathan Cameron]

> The SMMUv3 driver, which may be built without CONFIG_PCI, will shortly gain
> PRI support. Partially revert commit c6e9aefbf9db ("PCI/ATS: Remove unused
> PRI and PASID stubs") to re-introduce the PRI stubs, avoiding more #ifdefs
> in the SMMU driver.
> 
> Link: https://lore.kernel.org/iommu/20201112125519.3987595-8-jean-philippe@linaro.org/
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> [nicolinc: drop stale APIs]
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Re: [PATCH v3 04/13] iommu/arm-smmu-v3: Flush in-flight fault work on domain detach

[Jonathan Cameron]

> arm_smmu_attach_release() polls the hardware event queue to drain all the
> in-flight stall events for an old domain that the IOMMU core might free at
> any moment. However, a drained event is dequeued, yet it is not necessarily
> handled, and the IOPF work for a handled one can still be running.
> 
> So, first synchronize_irq() on the evtq and the combined IRQs following the
> drain, in order to guarantee that every dequeued event has reached the IOPF
> workqueue, since synchronize_irq() waits for an in-flight IRQ thread. Skip
> the waits on a timed-out drain though, since a stuck consumer would block

drop the "though" - doesn't read well or add anything.

> the unbounded wait_event() inside synchronize_irq() as well.
> 
> Then, invoke iopf_queue_flush_dev() to finally drain the IOPF workqueue, as
> the fault work references the old domain via its attach handle, while the
> IOMMU core might free that old domain once the detach call returns.
> 
> If arm_smmu_drain_queue() times out, fault work may still be in flight, and
> iopf_queue_remove_device() would free iopf groups that the queued work also
> references. Skip the iopf teardown and leak the master_domain, rather than
> risk a use-after-free.
> 
> The skip also leaks the iopf refcount, keeping the device enrolled on the
> IOPF queue, which would strand its fault parameter on the queue list once
> the device teardown frees dev->iommu, crashing a later iopf_queue_free().
> Reclaim the enrollment in arm_smmu_release_device(), where all the attach
> handles are gone so a straggler report cannot queue a new fault group.
> 
> Note that a residual race window remains between an iopf_queue_flush_dev()
> and iopf_queue_remove_device(): a fault arriving in between still resolves
> to the old attach handle, as the IOMMU core publishes a handle change only
> after the driver ops return. This window predates the drain narrowing it,
> and is only closable by an ordering fix in the IOMMU core. Furthermore, a
> timed-out drain shares exactly the same window, given that it must keep the
> device enrolled on the IOPF queue, where iopf_queue_remove_device() would
> free the iopf groups that any in-flight fault work still references.
> 
> Fixes: cfea71aea921 ("iommu/arm-smmu-v3: Put iopf enablement in the domain attach path")
> Cc: stable@vger.kernel.org # v6.16
> Co-developed-by: Barak Biber <bbiber@nvidia.com>
> Signed-off-by: Barak Biber <bbiber@nvidia.com>
> Co-developed-by: Stefan Kaestle <skaestle@nvidia.com>
> Signed-off-by: Stefan Kaestle <skaestle@nvidia.com>
> Signed-off-by: Malak Marrid <mmarrid@nvidia.com>
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index d255ff2519f9..1851ee7ec15b 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3399,6 +3399,7 @@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
>  	struct arm_smmu_master_domain *master_domain = state->old_master_domain;
>  	struct arm_smmu_master *master = state->master;
>  	struct arm_smmu_device *smmu = master->smmu;
> +	int ret = 0;
>  
>  	lockdep_assert_not_held(&arm_smmu_asid_lock);
>  	iommu_group_mutex_assert(master->dev);
> @@ -3412,8 +3413,40 @@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
>  	 * up once the IOMMU core swaps the handle, mistakenly resuming it
>  	 * against the next domain.
>  	 */
> -	if (master->stall_enabled)
> -		arm_smmu_drain_queue(smmu, &smmu->evtq.q, false);
> +	if (master->stall_enabled) {
> +		ret = arm_smmu_drain_queue(smmu, &smmu->evtq.q, false);
> +		/*
> +		 * Ensure pending events have reached the IOPF queue, unless
> +		 * the drain timed out: a stuck consumer would also block an
> +		 * unbounded wait_event() inside the synchronize_irq().
> +		 */
> +		if (!ret && smmu->evtq.q.irq)
> +			synchronize_irq(smmu->evtq.q.irq);
> +		/* Pending events might be in the combined_irq handler */
> +		if (!ret && smmu->combined_irq)
> +			synchronize_irq(smmu->combined_irq);

I'd group the two things that happen if we didn't time out.

		if (!ret) { /* Didn't time out */
			if (smmu->evtq.q.irq)
				synchronize_irq(smmu->evt.q.irq);
			if (smmu->combined_irq)
				synchronize_irq(smmu->combined_irq);
		}
		
> +	}

Thanks,
Jonathan

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Re: [PATCH v3 10/13] iommu/arm-smmu-v3: Allocate IOPF queue for ARM_SMMU_FEAT_PRI

[Jonathan Cameron]

> After the previous change drops the FEAT_SVA gate, arm_smmu_init_queues()
> allocates smmu->evtq.iopf when the SMMU advertises FEAT_STALLS. PRI support
> is being added on top of the IOPF infrastructure, so extend the allocation
> condition to cover SMMUs that advertise FEAT_PRI without FEAT_STALLS.
> 
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Re: [PATCH v3 05/13] iommu/arm-smmu-v3: Allocate IOPF queue without FEAT_SVA

[Jonathan Cameron]

> arm_smmu_init_queues() currently allocates smmu->evtq.iopf only when both
> FEAT_SVA and FEAT_STALLS are advertised by the SMMU. The FEAT_SVA gate is
> unnecessary: iopf_queue_alloc() is a plain workqueue allocator with no SVA
> dependency, and FEAT_STALLS alone can produce events that need IOPF.
> 
> On HW with FEAT_STALLS but no FEAT_SVA, a master with the "dma-can-stall"
> device-tree property has master->stall_enabled set at probe. Then also with
> CONFIG_ARM_SMMU_V3_SVA=y, arm_smmu_enable_iopf() sees stall_enabled, calls
> iopf_queue_add_device(NULL, ...) at fault-handling domain attach, which now
> oopses on mutex_lock(&queue->lock).

When you say 'now' you mean after other patches in this series?
If so then I'd make that more explicit.  If not, drop the 'now'.

> 
> Drop the FEAT_SVA term so the IOPF workqueue is allocated whenever the SMMU
> can produce IOPF events, and select IOMMU_IOPF from ARM_SMMU_V3 as well, so
> iopf_queue_alloc() has a real implementation instead of the NULL-returning
> stub when CONFIG_ARM_SMMU_V3_SVA is unset.
> 
> IOPF is orthogonal to SVA anyway. So, drop the CONFIG_ARM_SMMU_V3_SVA gates
> in arm_smmu_enable_iopf() and arm_smmu_disable_iopf().
> 
> Fixes: 395ad89d11fd9 ("iommu/arm-smmu-v3: Add stall support for platform devices")
> Cc: stable@vger.kernel.org # v5.14
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
>
You had me at the IOPF and SVA being orthogonal part.

Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Re: [PATCH v3 08/13] iommu/arm-smmu-v3: Disable PRI when no IRQ handler is registered

[Jonathan Cameron]

> arm_smmu_setup_irqs() has three failure paths that leave the priq without
> a handler: a missing priq IRQ line, devm_request_threaded_irq() failure on
> the priq IRQ, and devm_request_threaded_irq() failure on the combined IRQ.
> Each path warned but kept ARM_SMMU_FEAT_PRI set in smmu->features.
> 
> With FEAT_PRI still set, arm_smmu_setup_irqs() enables IRQ_CTRL_PRIQ_IRQEN
> against a handler that does not exist, and a later change would then have
> arm_smmu_probe_device() call pci_enable_pri() on PCIe endpoints, allowing
> them to issue PRI Page Requests that pile up in the priq with no drainer.
> Separately, arm_smmu_device_reset() has already enabled CR0_PRIQEN before
> it invoked arm_smmu_setup_irqs(), so the hardware queue would stay enabled
> regardless of what setup_irqs decides.
> 
> Clear ARM_SMMU_FEAT_PRI in all three failure paths, so that subsequent code
> treats PRI as unavailable. And disable CR0_PRIQEN after the setup function
> returns.
> 
> A kdump kernel cannot service page requests: strip FEAT_PRI when detecting
> features, so downstream PRI setup is skipped.

Dumb question - why can't a kdump kernel service page requests?
I'm nowhere near familiar enough with that stuff and I suspect I'm not
the only one who might be in that state and reading this series.
Configuration choice, or a fundamental thing?

Otherwise, seems fine to me.

Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Re: [PATCH v3 12/13] PCI/ATS: Export pci_enable_pri() and pci_reset_pri()

[Jonathan Cameron]

> Export pci_enable_pri() and pci_reset_pri() so the SMMUv3 driver, which is
> tristate, can use them from its probe path when built as a module.
> 
> Link: https://lore.kernel.org/iommu/20201112125519.3987595-9-jean-philippe@linaro.org/
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> [nicolinc: drop stale or already-exported APIs]
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Re: [PATCH v3 00/13] iommu/arm-smmu-v3: Add PRI support

[Jonathan Cameron]

Hi,

This is somewhat of a drive by review as I've been a bit snowed under
(blame the AI deluge if you like).  I'll try to come back to it but
don't wait on me.

Great to see this moving forward.

> The SMMUv3 driver doesn't handle events on the PRI queue or respond to IOPF
> faults. This series adds the missing pieces, using the IOPF infrastructure,
> to convert PRI page requests into iopf_faults and issue CMDQ_OP_PRI_RESP.
> 
> The iopf_queue_flush_dev() contract requires the driver to first drain the
> hardware PRI queue and synchronize using a threaded IRQ handler before the
> IOPF software flush. This drove the additional commits compared to v1:
>  - arm_smmu_drain_queue() drains the hardware queue by counting the
>    entries that the threaded IRQ handler consumes
>  - arm_smmu_attach_release() moves the teardown outside the global lock
>  - synchronize_irq() closes the gap before the final flush
> 
> Note that, although this series is only about PRIQ and EVTQ, the Q_POS and
> arm_smmu_drain_queue() helpers will be shared with Pranj's RPM series, per
> discussion below for CMDQ to use as well:
> https://lore.kernel.org/all/ao3ljjiXhnSJte5A@google.com/
> 
> Also, two patches in this series have some conflict with the kdump series:
> https://lore.kernel.org/all/cover.1788130528.git.nicolinc@nvidia.com/
> 
> As the kdump one is still under review, I leave those as-is, since Sashiko
> review relies on a clean rebase on linux-next to scan the series. Once the
> kdump series gets merged, I will rebase those kdump related stuff. For now,
> I think the series is good to collect review comments.
> 
> This is on Github:
> https://github.com/nicolinc/iommufd/commits/smmuv3_pri-v3
> 
> FWIW, engineers on the NVIDIA side have managed to verify the PRI feature.

I don't suppose you can share what they used to test?

Thanks,

Jonathan

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Re: [PATCH v3 13/13] iommu/arm-smmu-v3: Enable PRI for PCI device in arm_smmu_probe_device()

[Jonathan Cameron]

> Now PRI requests can be correctly handled. Enable the PCI cap when probing
> a PCI device. Also flush the priq in arm_smmu_attach_release().
> 
> Drain the priq for any PRI-enabled master following the same rationale as
> the eventq drain: a stale page request must be answered while it can still
> resolve to the old attach handle, even when the old attachment did not set
> up IOPF, or else the threaded handler could route it to the next domain.
> 
> Note that PRI is enabled at the probe time, while ATS gets toggled by the
> attach/detach routines, so a master could have PRI enabled when its ATS is
> disabled. PCIe (Base 6.3, Table 10-14) sets no ATS precondition on the PRI
> Enable bit. Its only ordering rule is that the interface must have gotten
> successfully Stopped prior to an enabling, which pci_enable_pri() already
> checks using PCI_PRI_STATUS_STOPPED. Also, a PRI-enabled device would not
> issue a page request until it starts to use ATS.
> 
> Set the per-device outstanding request budget to the full priq depth, same
> as intel-iommu's per-device PRQ_DEPTH choice. A fixed per-device cap won't
> prevent multiple PRI-capable devices from potentially exceeding the priq's
> capacity; priq overflow is recoverable per the SMMUv3 spec, and it is rare
> in practice.
> 
> Select PCI_PRI in Kconfig like other IOMMUs, gated on PCI so the build can
> stay clean for non-PCI ARM SMMUv3 configurations.
> 
> SMMUv3 forbids the Stall model on PCIe streams. Refuse to enable PRI on a
> PCIe master that came with stall_enabled, so page_response() can dispatch
> on master state unambiguously.

As earlier, we have exceptions in tree for Stall mode on PCIe streams
(lets not reopen that fun arguement) so I'd focus this on PRI not
making any sense if stall mode is in use as that has alternative
handling for page faults.


> 
> Refuse to enable PRI as well on any master reporting num_streams != 1, as
> arm_smmu_enable_iopf() rejects multi-stream masters, so IOPF cannot be set
> up for them; keeping PRI enabled would let a PRI request arrive on an alias
> StreamID and get a PRI_RESP_DENY issued against streams[0] by the driver's
> error-response path.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
A few minor things in here.

Thanks,
Jonathan

>
> diff --git a/drivers/iommu/arm/Kconfig b/drivers/iommu/arm/Kconfig
> index b848a4253677..a31d04f5b031 100644
> --- a/drivers/iommu/arm/Kconfig
> +++ b/drivers/iommu/arm/Kconfig
> @@ -80,6 +80,7 @@ config ARM_SMMU_V3
>  	select IOMMU_IO_PGTABLE_LPAE
>  	select IOMMU_IOPF
>  	select GENERIC_MSI_IRQ
> +	select PCI_PRI if PCI
>  	select IOMMUFD_DRIVER if IOMMUFD
>  	help
>  	  Support for implementations of the ARM System MMU architecture
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 47c95b691503..d35d814900cb 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3509,6 +3509,17 @@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
>  			synchronize_irq(smmu->combined_irq);
>  	}
>  
> +	/* Same as the eventq drain above, for the hardware priq */
> +	if (master->pri_enabled) {
> +		ret |= arm_smmu_drain_queue(smmu, &smmu->priq.q, false);
> +		/* Ensure pending requests have reached the IOPF queue */
> +		if (!ret && smmu->priq.q.irq)

Similar to before, I'd factor out the if (!ret)
Using |= on return values is never particularly nice though safe
as used here.  Still I'd use another variable to store that things
timed out already so we are skipping these.

> +			synchronize_irq(smmu->priq.q.irq);
> +		/* Pending requests might be in the combined_irq handler */
> +		if (!ret && smmu->combined_irq)
> +			synchronize_irq(smmu->combined_irq);
> +	}
> +
>  	/*
>  	 * Only IOPF-enabled attachments queue fault work, and such work
>  	 * references the old domain via its attach handle. Flush it, as
> @@ -4446,6 +4457,40 @@ static int arm_smmu_master_prepare_ats(struct arm_smmu_master *master)
>  	return arm_smmu_alloc_cd_tables(master);
>  }
>  
> +static void arm_smmu_master_enable_pri(struct arm_smmu_master *master)
> +{
> +	struct arm_smmu_device *smmu = master->smmu;
> +	struct pci_dev *pdev;
> +	unsigned int reqs;
> +
> +	if (!(smmu->features & ARM_SMMU_FEAT_PRI) || !smmu->evtq.iopf)
> +		return;
> +	if (!dev_is_pci(master->dev))
> +		return;
> +	pdev = to_pci_dev(master->dev);
> +
> +	if (!pci_pri_supported(pdev))
> +		return;
> +
> +	/* SMMUv3 forbids the Stall model on PCIe streams */

Again, I'd tweak the wording given we have quite a few examples
in tree that do stall mode on PCIe smelling streams.

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Re: [PATCH v3 07/13] iommu/arm-smmu-v3: Disable the queue IRQs before disabling the SMMU

[Jonathan Cameron]

> The EVTQ, PRIQ and combined IRQ handlers are threaded and issue commands of
> their own, e.g. a CMDQ_OP_PRI_RESP for a page request. Disabling the SMMU
> while one is in flight hands that command to a queue consuming nothing, so
> its poll waits out a full timeout.
> 
> Two paths disable the SMMU while those IRQs are still requested: a failing
> arm_smmu_device_reset() returns to a probe that disables the device itself,
> and arm_smmu_disable_action() covers an unbind or any later probe failure.
> Both can run after arm_smmu_setup_irqs() requested the IRQs.
> 
> Disable those IRQs first in both paths, so that no handler is left running
> once the SMMU goes down.

Why this soluton rather than a flag to stop them queuing new work + a
synchronize_irq() to deal with threads in flight.

irq disables always worry me a little as they tend to be patching over
something nastier.  I think this works though so I'm not going to
strongly object.

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Re: [PATCH v3 01/13] iommu/arm-smmu-v3: Add arm_smmu_attach_release()

[Jonathan Cameron]

> The IOPF teardown is done in arm_smmu_remove_master_domain() when releasing
> the master_domain on detach, under the global arm_smmu_asid_lock mutex. But
> the teardown must drain any in-flight IOPF (for the old domain), before the
> master_domain is freed via iopf_queue_flush_dev() calling flush_workqueue()
> that can block on user-faulting page-fault handlers. Doing so while holding
> the arm_smmu_asid_lock would stall any unrelated attachment in the system.
> 
> Split the teardown out of arm_smmu_remove_master_domain(), to a new helper
> arm_smmu_attach_release() that runs after arm_smmu_asid_lock is released.
> 
> Since no other device would use the old master_domain that is being freed,
> it's safe to move out of arm_smmu_asid_lock (still under the protection of
> iommu_group->mutex).

This is a lot of text if the next bit about being a refactor only
is accurate.  Seems that not blocking attachments is the issue and
to me that is a functional and useful change. However, is that
in this patch?  Anyhow to me this needs a rewrite to focus on just
what is actually changing here rather than the eventual picture.


> 
> Note: this is a pure refactor; no functional change.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> index 25982bdbcbd9..fce026efa44f 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> @@ -194,6 +194,7 @@ static int arm_smmu_attach_dev_nested(struct iommu_domain *domain,
>  	arm_smmu_install_ste_for_dev(master, &ste);
>  	arm_smmu_attach_commit(&state);
>  	mutex_unlock(&arm_smmu_asid_lock);
> +	arm_smmu_attach_release(&state);
>  	return 0;
>  }
>  
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 5732f3ba0122..99baa59b39c9 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3285,9 +3285,9 @@ arm_smmu_master_build_invs(struct arm_smmu_master *master, bool ats_enabled,
>  	return master->build_invs;
>  }
>  
> -static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
> -					  struct iommu_domain *domain,
> -					  ioasid_t ssid)
> +static struct arm_smmu_master_domain *
> +arm_smmu_remove_master_domain(struct arm_smmu_master *master,
> +			      struct iommu_domain *domain, ioasid_t ssid)
>  {
>  	struct arm_smmu_domain *smmu_domain = to_smmu_domain_devices(domain);
>  	struct arm_smmu_master_domain *master_domain;
> @@ -3295,7 +3295,7 @@ static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
>  	unsigned long flags;
>  
>  	if (!smmu_domain)
> -		return;
> +		return NULL;
>  
>  	if (domain->type == IOMMU_DOMAIN_NESTED)
>  		nested_ats_flush = to_smmu_nested_domain(domain)->enable_ats;
> @@ -3310,8 +3310,23 @@ static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
>  	}
>  	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>  
> +	/* arm_smmu_attach_release() will free it */
> +	return master_domain;
> +}
> +
> +/* Release the old master_domain detached by arm_smmu_remove_master_domain() */
> +void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
> +{
> +	struct arm_smmu_master_domain *master_domain = state->old_master_domain;
> +	struct arm_smmu_master *master = state->master;
> +
> +	iommu_group_mutex_assert(master->dev);
> +
> +	if (!master_domain)

I guess this makes sense in later patches, but for now the local
variable seems more confusing than anything.

> +		return;

I'd add a blank line here to separate the sanity checks from bulk
code.


>  	arm_smmu_disable_iopf(master, master_domain);
>  	kfree(master_domain);
> +	state->old_master_domain = NULL;
>  }
>  

> @@ -3784,6 +3801,7 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
>
This path is hit from a failure of arm_smmu_attach_prepare()
At that point the old domain hasn't been detached.

Now it doesn't matter because of what is currently done in release,
but from a code flow / what that function is documented to be for
this seems wrong to me.  I'd separate the good and the bad
paths in the function.

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Re: [PATCH v3 03/13] iommu/arm-smmu-v3: Drain in-flight fault events on domain detach

[Jonathan Cameron]

> When a device is switching away from a domain, either through a detach or a
> replace operation, in-flight stall events for the old domain might still be
> on the SMMU's hardware event queue or on the IOMMU core's IOPF queue. Thus,
> if the IOMMU core swaps the device's attach_handle and frees the old domain
> before those handlers complete, the IOPF work might hit use-after-free.
> 
> Two queues need to be drained: the SMMU hardware event queue and the IOMMU
> core IOPF software workqueue. Start with the former: add a counting-based
> arm_smmu_drain_queue() helper, and poll the evtq on a domain detach, so a
> pending IRQ won't let the threaded handler run after the drain and queue a
> fault referencing the domain being freed. Its until_empty mode serves the
> suspend and runtime PM routines that would drain the CMDQ. Any timed-out
> drain fires a WARN_ON as well, since reaching the timeout would take some
> stuck consumer in any realistic case.
> 
> The existing queue_poll() API is not reusable for such a drain: it is the
> atomic busy-wait for the command issuing paths, and it assumes a hardware
> consumer making progress. A drain caller is sleepable, in contrast, while
> the EVTQ/PRIQ consumer is a threaded IRQ handler that needs the CPU: such
> a busy wait would starve the handler throughout an entire timeout, whenever
> the waiter and the handler shared one CPU on a non-preemptible kernel. So,
> this new sleeping helper is marked with a might_sleep() as well, given that
> an atomic-context misuse would otherwise hide behind an empty queue.
> 
> Note that a drained event is dequeued, but not necessarily handled, since
> queue_remove_raw() moves the MMIO CONS before the threaded IRQ handler gets
> to push the event onto the IOPF workqueue. A subsequent change will invoke
> synchronize_irq() and iopf_queue_flush_dev() to close that gap, and it will
> act on the errno of a timed-out drain too.
> 
> The drain runs before the IOMMU core swaps the device's attach handle, so a
> fault event generated on the new STE during this window resolves to the old
> handle, completing with IOMMU_PAGE_RESP_INVALID that resumes the stall with
> abort: the impact is bounded to that one failed transaction.
> 
> Also run the drain for every stall-capable master, even when the departing
> attachment did not enable IOPF: such a stall event has to be aborted while
> it still resolves to the old attach handle, otherwise the threaded handler
> could pick it up right after the handle swap, mistakenly resuming it as if
> it were a valid page fault against a new domain.
>

Useful perhaps to call out if this has been seen in real systems or
not. I agree with the analysis but would rather hope drivers are
well behaved in ensuring all traffic is done, adn this is hardeninging
/ handling of naught hardware activity (all good if so!)

> Fixes: cfea71aea921 ("iommu/arm-smmu-v3: Put iopf enablement in the domain attach path")
> Cc: stable@vger.kernel.org # v6.16
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index e00b6c88214f..d255ff2519f9 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -948,6 +948,86 @@ static int arm_smmu_cmdq_batch_submit(struct arm_smmu_device *smmu,
>  					   cmds->num, true);
>  }
>  
> +/**
> + * arm_smmu_drain_queue - Drain an SMMU queue
> + * @smmu: the SMMU device
> + * @q: the queue to drain
> + * @until_empty: target selection
> + *
> + * With @until_empty == true (for CMDQ), exit once the queue is observed empty:
> + *
> + *   cons0                cons                                prod
> + *     |                   |                                   |
> + *  ---+###################+=====================+=============+--->
> + *                         |<--------- undrained==0? --------->|
                                                    ^
What is the + indicating?  Seems where prod0 that isn't relevant here
would have been - that is a little confusing so maybe drop?

> + *
> + * With @until_empty == false (for EVTQ/PRIQ), exit once "drained" reaches its
> + * target: "pending" (i.e. prod0 - cons0, frozen at the entry time):
> + *
> + *   cons0                cons                 prod0         (prod)
> + *     |<---- drained ---->|                     |             |
> + *  ---+###################+=====================+=============+--->
> + *     |<--------------- pending --------------->|
> + *
> + * Note that a drained entry is dequeued, but not necessarily handled: the
> + * EVTQ/PRIQ callers must follow up with a synchronize_irq() to wait for the
> + * threaded IRQ handler to finish handling the dequeued entries.
> + *
> + * Context: Process context; may sleep.
> + * Return: 0 on success or a negative errno on timeout.
> + */
> +static int arm_smmu_drain_queue(struct arm_smmu_device *smmu,
> +				struct arm_smmu_queue *q, bool until_empty)

That name suggests this is doing the draining rather than waiting
for it to happen elsewhere.

> +{
> +	ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
> +	u32 cons, prod, prev, undrained;
> +	u32 drained = 0, pending;

Pet irritation. Prefer splitting the elements that assign and those
that don't onto seeprate lines.  Here that just means moving pending
up one line.

> +
> +	might_sleep();
> +
> +	cons = readl_relaxed(q->cons_reg);
> +	prod = readl_relaxed(q->prod_reg);
> +	/* The exit target: the number of entries in the queue at entry */
> +	pending = Q_POS(&q->llq, prod - cons);

Applying a macro called Q_POS to a difference is a bit confusing to
me given the output isn't a position of anything. Maybe just needs
a wrapper Q_DIFF(q->llq, prod, cons)  Can use Q_POS underneath
but avoid that naming out here well away from the macro definitions.

> +
> +	while (true) {

Maybe pull defintion of prev and undrained in here so it is clear
they aren't state maintained across iternations.

> +		/* Accumulate the entries consumed since the last poll */
> +		prev = cons;
> +		cons = readl_relaxed(q->cons_reg);
> +		drained += Q_POS(&q->llq, cons - prev);
> +
> +		prod = readl_relaxed(q->prod_reg);
> +		undrained = Q_POS(&q->llq, prod - cons);
> +
> +		/* Exit on an empty queue, regardless of until_empty */
> +		if (!undrained)

Given you don't use undrained again (maybe in later patches, in which
case ignore me.)
		if (Q_DIFF(&q->llq, prod, cons) == 0)
perhaps.  This one entirely up to you as maybe the named local does
help with readability a little.

> +			return 0;
> +
> +		/* Snapshot mode: exit once the pending entries are drained */
> +		if (!until_empty && drained >= pending)
> +			return 0;
> +
> +		/*
> +		 * A timeout means the consumer might be stuck. In theory, if it
> +		 * moves 2 * qsize entries or more within a single poll interval
> +		 * Q_POS() would wrap and undercount drained: that could trigger
> +		 * a spurious warning too, if the queue was never once observed
> +		 * empty. Yet, that much consumption in such a short interval is
> +		 * unrealistic. WARN it only, as a stuck consumer is a real bug.

I don't like 'unrealisitic' based defenses (even though I agree it is pretty
unlikely).  Is there a way to bound this?  Maybe future systems will
be much quicker.

> +		 */
> +		if (WARN_ON(ktime_compare(ktime_get(), timeout) > 0))

Why WARN_ON here then a dev_warn_ratelimited() below? 

> +			break;
> +
> +		/* The consumer might be a threaded IRQ handler. Yield to it */
> +		usleep_range(100, 200);

fsleep() perhaps then we don't get to argue why that slack.

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>