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_wait_for_queue_drained() 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(),
Q_DIFF() and arm_smmu_wait_for_queue_drained() helpers will be shared with
Pranj's RPM series, per discussion below for CMDQ to use as well:
https://lore.kernel.org/all/20260908171712.356645-4-praan@google.com/
This is on Github:
https://github.com/nicolinc/iommufd/commits/smmuv3_pri-v5
FWIW, engineers on the NVIDIA side have managed to verify the PRI feature.
Changelog
v5:
* Add review tags from Jonathan
* Gate the PASID Stop Marker discard on SSV
* Follow coding-assistants.rst for the Assisted-by tags
* Sample the drain wait early in arm_smmu_wait_for_queue_drained(), for
Sashiko finding:
https://sashiko.dev/#/patchset/cover.1789081084.git.nicolinc%40nvidia.com?part=4
v4:
https://lore.kernel.org/all/cover.1789081084.git.nicolinc@nvidia.com/
* Add review tags from Jonathan
* Simplify verbose commit messages
* Use fsleep() in the drain poll loop
* Drop the goto in arm_smmu_set_pasid()
* Drop the WARN_ON() on a drain timeout
* Gate the evtq/priq drain on using_iopf
* Add a prm pointer in arm_smmu_page_response()
* Run impl_ops->device_disable() before SMMU shutdown
* Track a drain timeout in a bool instead of an errno
* Add a Q_DIFF() macro for the queue position differences
* Rename arm_smmu_drain_queue() to arm_smmu_wait_for_queue_drained()
* Split the fault report and the overflow discard into two patches
v3:
https://lore.kernel.org/all/cover.1788222485.git.nicolinc@nvidia.com/
* 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 (12):
iommu/arm-smmu-v3: Disable the impl before disabling the SMMU on
shutdown
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: Discard partial PRI faults on PRIQ overflow
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 | 7 +
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 | 444 +++++++++++++++---
drivers/pci/ats.c | 2 +
6 files changed, 405 insertions(+), 56 deletions(-)
base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
--
2.43.0
arm_smmu_device_shutdown() only calls arm_smmu_device_disable(), and never
the impl_ops->device_disable() hook. On tegra241-cmdqv, that hook quiesces
every VINTF through tegra241_vintf_hw_deinit(), which issues a CMD_SYNC on
the shared CMDQ while it is still enabled.
arm_smmu_disable_action() already runs this hook before disabling the SMMU,
to cover an unbind or a probe failure, and no devres release ever follows a
reboot or a kexec to run it again. Have arm_smmu_device_shutdown() call it
directly, instead of reimplementing part of what it already does.
Fixes: 9ff145a25c5c ("iommu/tegra241-cmdqv: Fix CMD_SYNC use-after-free on teardown")
Cc: stable@vger.kernel.org
Reviewed-by: Jonathan Cameron <redacted>
Assisted-by: LLM
Signed-off-by: Nicolin Chen <redacted>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
When a device leaves a domain, fault events for the old domain may remain
in the SMMU event queue or the IOPF workqueue. If the IOMMU core frees that
domain before those events are handled, the work may use freed memory.
Start with the hardware queue by using arm_smmu_wait_for_queue_drained() to
count entries consumed by the threaded IRQ handler, and poll the EVTQ when
an IOPF-enabled attachment ends. This prevents a pending IRQ from queuing
old-domain work after the drain. Its until_empty mode can drain the CMDQ as
well during suspend and runtime PM.
queue_poll() cannot be used because it is an atomic busy-wait that expects
hardware to consume entries. The EVTQ and PRIQ are drained by threaded IRQ
handlers, so a busy-wait could starve a handler sharing the same CPU on a
non-preemptible kernel. The new helper sleeps, and might_sleep() catches an
atomic-context caller even when the queue is already empty.
Sample the deadline ahead of the queue state that it judges, and honour it
only after the exit conditions, so a drain that completes while this poll
is preempted still returns success instead of a spurious timeout. This is
the same ordering that poll_timeout_us() guarantees.
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.
Fixes: cfea71aea921 ("iommu/arm-smmu-v3: Put iopf enablement in the domain attach path")
Cc: stable@vger.kernel.org # v6.16
Reviewed-by: Jonathan Cameron <redacted>
Assisted-by: LLM
Signed-off-by: Nicolin Chen <redacted>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 +
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 101 ++++++++++++++++++++
2 files changed, 103 insertions(+)
@@ -948,6 +948,97 @@ static int arm_smmu_cmdq_batch_submit(struct arm_smmu_device *smmu,cmds->num,true);}+/**+*arm_smmu_wait_for_queue_drained-WaitforanSMMUqueuetobedrained+*@smmu:theSMMUdevice+*@q:thequeuetobedrained+*@until_empty:targetselection+*+*With@until_empty==true(forCMDQ),exitoncethequeueisobservedempty:+*+*cons0consprod+*|||+*---+###################+===================================+--->+*|<---------undrained==0?--------->|+*+*With@until_empty==false(forEVTQ/PRIQ),exitonce"drained"reachesits+*target:"pending"(i.e.prod0-cons0,frozenattheentrytime):+*+*cons0consprod0(prod)+*|<----drained---->|||+*---+###################+=====================+=============+--->+*|<---------------pending--------------->|+*+*Notethatadrainedentryisdequeued,butnotnecessarilyhandled:the+*EVTQ/PRIQcallersmustfollowupwithasynchronize_irq()towaitforthe+*threadedIRQhandlertofinishhandlingthedequeuedentries.+*+*Context:Processcontext;maysleep.+*Return:0onsuccessoranegativeerrnoontimeout.+*/+staticintarm_smmu_wait_for_queue_drained(structarm_smmu_device*smmu,+structarm_smmu_queue*q,+booluntil_empty)+{+ktime_ttimeout=ktime_add_us(ktime_get(),ARM_SMMU_POLL_TIMEOUT_US);+u32cons,prod,pending;+u32drained=0;++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_DIFF(&q->llq,cons,prod);++while(true){+u32prev,undrained;+boolexpired;++/*+*Samplethedeadlineaheadofthequeuestateitjudges,but+*breakonlyaftertheexitconditionsbelow,soaqueuethat+*drainedduringalongpreemptionstillexitswithasuccess.+*/+expired=ktime_compare(ktime_get(),timeout)>0;++/* Accumulate the entries consumed since the last poll */+prev=cons;+cons=readl_relaxed(q->cons_reg);+drained+=Q_DIFF(&q->llq,prev,cons);++prod=readl_relaxed(q->prod_reg);+undrained=Q_DIFF(&q->llq,cons,prod);++/* Exit on an empty queue, regardless of until_empty */+if(!undrained)+return0;++/* Snapshot mode: exit once the pending entries are drained */+if(!until_empty&&drained>=pending)+return0;++/*+*Atimeoutmeanstheconsumermightbestuck.Intheory,ifit+*moves2*qsizeentriesormorewithinasinglepollinterval+*Q_DIFF()willwrapandundercountdrained:thatcouldtrigger+*aspuriouswarningtoo,ifthequeuewasneveronceobserved+*empty.Yet,thatmuchconsumptioninsuchashortintervalis+*unrealistic.+*/+if(expired)+break;++/* The consumer might be a threaded IRQ handler. Yield to it */+fsleep(100);+}++dev_warn_ratelimited(smmu->dev,+"queue drain timed out at prod=0x%x cons=0x%x\n",+prod,cons);+return-ETIMEDOUT;+}+staticvoidarm_smmu_page_response(structdevice*dev,structiopf_fault*unused,structiommu_page_response*resp){
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.
A later change will add an IOPF workqueue flush to that teardown, which can
block on a user-faulting page-fault handler. Holding the arm_smmu_asid_lock
across it would stall every 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.
No functional change: the old master_domain belongs to no other device, so
freeing it outside the lock stays safe, still under iommu_group->mutex.
Reviewed-by: Jonathan Cameron <redacted>
Signed-off-by: Nicolin Chen <redacted>
---
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 | 44 ++++++++++++++-----
3 files changed, 36 insertions(+), 11 deletions(-)
@@ -3310,8 +3310,24 @@ 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 */+returnmaster_domain;+}++/* Release the old master_domain detached by arm_smmu_remove_master_domain() */+voidarm_smmu_attach_release(structarm_smmu_attach_state*state)+{+structarm_smmu_master_domain*master_domain=state->old_master_domain;+structarm_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;}/*
After the hardware queue is drained, an event may still be moving from the
IRQ thread to the IOPF workqueue, while earlier IOPF work is still running.
Synchronize the EVTQ and combined IRQs, then call iopf_queue_flush_dev().
This finishes all old-domain work before the IOMMU core frees the domain.
Skip synchronize_irq() after a drain timeout because a stuck consumer can
otherwise leave it waiting forever.
If arm_smmu_wait_for_queue_drained() times out, fault work may still be in
flight, and iopf_queue_remove_device() would free iopf groups that the 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 <redacted>
Signed-off-by: Barak Biber <redacted>
Co-developed-by: Stefan Kaestle <redacted>
Signed-off-by: Stefan Kaestle <redacted>
Signed-off-by: Malak Marrid <redacted>
Reviewed-by: Jonathan Cameron <redacted>
Assisted-by: LLM
Signed-off-by: Nicolin Chen <redacted>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 46 +++++++++++++++++++--
1 file changed, 43 insertions(+), 3 deletions(-)
@@ -3422,8 +3423,38 @@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)*whichtheIOMMUcoremightfreeoncethisreturns.Drainthehardware*eventq,sothatapendingeventcannotturnintonewfaultwork.*/-if(master_domain->using_iopf&&master->stall_enabled)-arm_smmu_wait_for_queue_drained(smmu,&smmu->evtq.q,false);+if(master_domain->using_iopf&&master->stall_enabled){+timed_out=arm_smmu_wait_for_queue_drained(smmu,&smmu->evtq.q,+false);+/*+*EnsurependingeventshavereachedtheIOPFqueue,unless+*thedraintimedout:astuckconsumerwouldalsoblockan+*unboundedwait_event()insidethesynchronize_irq().+*/+if(!timed_out){+if(smmu->evtq.q.irq)+synchronize_irq(smmu->evtq.q.irq);+/* Pending events might be in the combined_irq handler */+if(smmu->combined_irq)+synchronize_irq(smmu->combined_irq);+}+}++/* Lastly, flush the fault work that the drained events queued */+if(master_domain->using_iopf){+iopf_queue_flush_dev(master->dev);++/*+*Atimed-outdrainmayleavefaultworkinflight,and+*iopf_queue_remove_device()wouldfreeiopfgroupsthat+*suchworkstillreferences.Skiptheiopfteardownand+*leakmaster_domain,ratherthanriskaUAF.+*/+if(WARN_ON(timed_out)){+state->old_master_domain=NULL;+return;+}+}arm_smmu_disable_iopf(master,master_domain);kfree(master_domain);
arm_smmu_setup_irqs() can leave the PRIQ without any registered handler if
its IRQ is missing, its request fails, or the combined IRQ request fails.
These paths warn but leave ARM_SMMU_FEAT_PRI set.
This function itself would then go on to enable IRQ_CTRL_PRIQ_IRQEN, even
though no handler will drain the queue. A later patch also enables PRI on
such endpoints. The caller already enabled CR0_PRIQEN before this function
even runs.
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 doesn't service page requests: strip FEAT_PRI when detecting
features, so downstream PRI setup is skipped.
Reviewed-by: Jonathan Cameron <redacted>
Assisted-by: LLM
Signed-off-by: Nicolin Chen <redacted>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
@@ -4944,9 +4944,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 */
@@ -4981,6 +4983,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;}}elsearm_smmu_setup_unique_irqs(smmu);
@@ -5161,6 +5164,10 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu)returnret;}+/* 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);
arm_smmu_init_queues() allocates the IOPF queue only for FEAT_SVA together
with FEAT_STALLS. This queue has 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 can
oops on mutex_lock(&queue->lock).
Allocate the queue whenever FEAT_STALLS is present. Also select IOMMU_IOPF
so iopf_queue_alloc() is always available without CONFIG_ARM_SMMU_V3_SVA.
Since IOPF does not depend on SVA, drop the SVA guards from its enable and
disable paths.
Fixes: 395ad89d11fd9 ("iommu/arm-smmu-v3: Add stall support for platform devices")
Cc: stable@vger.kernel.org # v5.14
Reviewed-by: Jonathan Cameron <redacted>
Assisted-by: LLM
Signed-off-by: Nicolin Chen <redacted>
---
drivers/iommu/arm/Kconfig | 1 +
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 +--------
2 files changed, 2 insertions(+), 8 deletions(-)
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. arm_smmu_device_shutdown() needs no change of its
own here, since it already just calls arm_smmu_disable_action().
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: LLM
Signed-off-by: Nicolin Chen <redacted>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 36 ++++++++++++++++++---
1 file changed, 31 insertions(+), 5 deletions(-)
@@ -4914,8 +4914,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");}
@@ -4938,12 +4940,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;}}
@@ -4971,8 +4978,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;+}}elsearm_smmu_setup_unique_irqs(smmu);
@@ -4999,10 +5008,22 @@ static int arm_smmu_device_disable(struct arm_smmu_device *smmu)returnret;}+/* Quiesce the queue IRQ threads, e.g. before disabling the SMMU */+staticvoidarm_smmu_disable_irqs(structarm_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);+}+staticvoidarm_smmu_disable_action(void*data){structarm_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);
@@ -5149,18 +5170,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");-returnret;+gotoerr_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");-returnret;+gotoerr_disable_irqs;}}return0;++err_disable_irqs:+/* The probe error path cannot tell if the IRQs were requested */+arm_smmu_disable_irqs(smmu);+returnret;}#define IIDR_IMPLEMENTER_ARM 0x43b
From: Jean-Philippe Brucker <redacted>
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 <redacted>
[nicolinc: drop stale or already-exported APIs]
Signed-off-by: Nicolin Chen <redacted>
Reviewed-by: Jonathan Cameron <redacted>
---
drivers/pci/ats.c | 2 ++
1 file changed, 2 insertions(+)
From: Malak Marrid <redacted>
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.
Both EVTQ stall events and PRIQ requests reach the IOPF layer with the same
IOMMU_FAULT_PAGE_REQ, but these two fault models are mutually exclusive. A
stalling master holds a transaction until CMDQ_OP_RESUME and has no reason
to send a page request. A later patch rejects PRI for such a master, so the
response command can be selected from stall_enabled or pri_enabled.
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 <redacted>
Signed-off-by: Barak Biber <redacted>
Co-developed-by: Stefan Kaestle <redacted>
Signed-off-by: Stefan Kaestle <redacted>
Signed-off-by: Malak Marrid <redacted>
Reviewed-by: Jonathan Cameron <redacted>
Signed-off-by: Nicolin Chen <redacted>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 +
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 74 +++++++++++++++------
2 files changed, 56 insertions(+), 19 deletions(-)
@@ -1017,6 +1017,7 @@ struct arm_smmu_master {/* Locked by the iommu core using the group mutex */structarm_smmu_ctx_desc_cfgcd_table;unsignedintnum_streams;+boolpri_enabled:1;boolats_enabled:1;boolste_ats_enabled:1;boolstall_enabled;
@@ -1039,32 +1039,68 @@ static int arm_smmu_wait_for_queue_drained(struct arm_smmu_device *smmu,return-ETIMEDOUT;}-staticvoidarm_smmu_page_response(structdevice*dev,structiopf_fault*unused,+staticvoidarm_smmu_page_response(structdevice*dev,structiopf_fault*evt,structiommu_page_response*resp){structarm_smmu_master*master=dev_iommu_priv_get(dev);-u8resume_resp;+structarm_smmu_cmdcmd;+intsid;-if(WARN_ON(!master->stall_enabled))+if(WARN_ON_ONCE(evt->fault.type!=IOMMU_FAULT_PAGE_REQ))return;-switch(resp->code){-caseIOMMU_PAGE_RESP_INVALID:-caseIOMMU_PAGE_RESP_FAILURE:-resume_resp=CMDQ_RESUME_0_RESP_ABORT;-break;-caseIOMMU_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){+u8resume_resp;++switch(resp->code){+caseIOMMU_PAGE_RESP_INVALID:+caseIOMMU_PAGE_RESP_FAILURE:+resume_resp=CMDQ_RESUME_0_RESP_ABORT;+break;+caseIOMMU_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);+}elseif(master->pri_enabled){+structiommu_fault_page_request*prm=&evt->fault.prm;+enumpri_resppri_resp;+boolssv;++/* PCIe allows only one PRG Response per group */+if(!(prm->flags&IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))+return;+switch(resp->code){+caseIOMMU_PAGE_RESP_SUCCESS:+pri_resp=PRI_RESP_SUCC;+break;+caseIOMMU_PAGE_RESP_FAILURE:+/* 0b00 ResponseFailure: a permanent non-paging error */+pri_resp=PRI_RESP_DENY;+break;+caseIOMMU_PAGE_RESP_INVALID:+/* 0b01 InvalidRequest: page-in unsuccessful */+pri_resp=PRI_RESP_FAIL;+break;+default:+WARN_ON(true);+return;+}+ssv=!!(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'tsendaSYNC,itdoesn'tdoanythingforRESUMEorPRI_RESP.*RESUMEconsumptionguaranteesthatthestalledtransactionwillbe
@@ -3220,7 +3256,7 @@ static int arm_smmu_enable_iopf(struct arm_smmu_master *master,*device-specificfaulthandlersanddon'tneedIOPF,sothisisnota*failure.*/-if(!master->stall_enabled)+if(!master->stall_enabled&&!master->pri_enabled)return0;/* We're not keeping track of SIDs in fault events */
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 only 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.
Nothing can arrive in between, as an active overflow inhibits new entries
from being written to the PRI queue until a final CONS write acknowledges
it (IHI0070 8.1).
Suggested-by: Harsha Vardhan V <redacted>
Reviewed-by: Jonathan Cameron <redacted>
Assisted-by: LLM
Signed-off-by: Nicolin Chen <redacted>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
For a PRI-enabled master, convert every PRIQ entry into an iopf_fault and
report it through iommu_report_device_fault().
For an unknown StreamID or a master without PRI enabled, keep the existing
LAST-page DENY path. Merge its two messages into one ratelimited line so a
faulty device cannot 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.
When SSV is clear, the same LRW encoding is a PRI Page Request that expects
a response, so let it fall through.
Note that master->pri_enabled will only be set by a later change, once all
the PRI paths are ready.
Co-developed-by: Barak Biber <redacted>
Signed-off-by: Barak Biber <redacted>
Co-developed-by: Stefan Kaestle <redacted>
Signed-off-by: Stefan Kaestle <redacted>
Signed-off-by: Malak Marrid <redacted>
Signed-off-by: Nicolin Chen <redacted>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 50 +++++++++++++++++++--
1 file changed, 47 insertions(+), 3 deletions(-)
PRI handling is now ready. Enable PRI when probing a PCI device, and drain
the PRIQ with the EVTQ when an IOPF-enabled attachment ends. Queued fault
work may still refer to the old domain when the detach call returns.
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.
A stalling master resolves its faults synchronously, so it has no reason at
all to issue a page request. Refuse to enable PRI on such a master, so that
page_response() can keep dispatching on the 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.
Reviewed-by: Jonathan Cameron <redacted>
Signed-off-by: Nicolin Chen <redacted>
---
drivers/iommu/arm/Kconfig | 1 +
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 52 +++++++++++++++++++++
2 files changed, 53 insertions(+)
@@ -3526,6 +3526,20 @@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)}}+/* Same as the eventq drain above, for the hardware priq */+if(master_domain->using_iopf&&master->pri_enabled){+timed_out|=arm_smmu_wait_for_queue_drained(+smmu,&smmu->priq.q,false);+/* Ensure pending requests have reached the IOPF queue */+if(!timed_out){+if(smmu->priq.q.irq)+synchronize_irq(smmu->priq.q.irq);+/* Pending requests might be in the combined_irq handler */+if(smmu->combined_irq)+synchronize_irq(smmu->combined_irq);+}+}+/* Lastly, flush the fault work that the drained events queued */if(master_domain->using_iopf){iopf_queue_flush_dev(master->dev);
@@ -4458,6 +4472,40 @@ static int arm_smmu_master_prepare_ats(struct arm_smmu_master *master)returnarm_smmu_alloc_cd_tables(master);}+staticvoidarm_smmu_master_enable_pri(structarm_smmu_master*master)+{+structarm_smmu_device*smmu=master->smmu;+structpci_dev*pdev;+unsignedintreqs;++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;++/* A stalling master resolves its faults without any page request */+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");+}+staticstructiommu_device*arm_smmu_probe_device(structdevice*dev){intret;
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.
Reviewed-by: Jonathan Cameron <redacted>
Assisted-by: LLM
Signed-off-by: Nicolin Chen <redacted>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 build a Q_DIFF() on it, to count the entries that
a queue pointer moved past.
No functional change intended.
Assisted-by: LLM
Signed-off-by: Nicolin Chen <redacted>
---
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(-)
From: Jean-Philippe Brucker <redacted>
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 <redacted>
[nicolinc: drop stale APIs]
Signed-off-by: Nicolin Chen <redacted>
Reviewed-by: Jonathan Cameron <redacted>
---
include/linux/pci-ats.h | 5 +++++
1 file changed, 5 insertions(+)
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 build a Q_DIFF() on it, to count the entries that
a queue pointer moved past.
No functional change intended.
Assisted-by: LLM
Signed-off-by: Nicolin Chen <redacted>
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. arm_smmu_device_shutdown() needs no change of its
own here, since it already just calls arm_smmu_disable_action().
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: LLM
Signed-off-by: Nicolin Chen <redacted>
I'm still not loving the disable_irq() in here just because to me it
feels like it may end up masking stuff we ought to handle more carefully but
I don't mind that much
Reviewed-by: Jonathan Cameron <redacted>
For a PRI-enabled master, convert every PRIQ entry into an iopf_fault and
report it through iommu_report_device_fault().
For an unknown StreamID or a master without PRI enabled, keep the existing
LAST-page DENY path. Merge its two messages into one ratelimited line so a
faulty device cannot 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.
When SSV is clear, the same LRW encoding is a PRI Page Request that expects
a response, so let it fall through.
Note that master->pri_enabled will only be set by a later change, once all
the PRI paths are ready.
Co-developed-by: Barak Biber <redacted>
Signed-off-by: Barak Biber <redacted>
Co-developed-by: Stefan Kaestle <redacted>
Signed-off-by: Stefan Kaestle <redacted>
Signed-off-by: Malak Marrid <redacted>
Signed-off-by: Nicolin Chen <redacted>
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.
A later change will add an IOPF workqueue flush to that teardown, which can
block on a user-faulting page-fault handler. Holding the arm_smmu_asid_lock
across it would stall every unrelated attachment in the system.
[...]
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
--
Jason
arm_smmu_device_shutdown() only calls arm_smmu_device_disable(), and never
the impl_ops->device_disable() hook. On tegra241-cmdqv, that hook quiesces
every VINTF through tegra241_vintf_hw_deinit(), which issues a CMD_SYNC on
the shared CMDQ while it is still enabled.
arm_smmu_disable_action() already runs this hook before disabling the SMMU,
to cover an unbind or a probe failure, and no devres release ever follows a
reboot or a kexec to run it again. Have arm_smmu_device_shutdown() call it
directly, instead of reimplementing part of what it already does.
[...]
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
--
Jason
From: Jason Gunthorpe <jgg@nvidia.com> Date: 2026-09-23 18:37:48
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.
This seems like a sashiko hallucination?
There are no PRIs during probe. The initial STE table does not permit
it.
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.
That's fscking devm again. Do not mix devm and non devm CALLS. It
doesn't work:
ret = arm_smmu_device_reset(smmu);
if (ret) {
arm_smmu_device_disable(smmu); <-------------------
return ret;
}
/* Register last so it unwinds first, while the CMDQ is still up. */
ret = devm_add_action_or_reset(smmu->dev, arm_smmu_disable_action, smmu);
if (ret)
return ret;
When devm is devm'ing properly the existing devm irq handlers will be
torn down before disable_action and we don't need to open code a
disable_irq
That just leaves shutdown, and I'm not sure how that is supposed to
work, we can't just halt the SMMU while there are any threads running
around still poking at it. I forget the context shutdown runs in, but
if concurrency is shut down already then its fine to not disable the
irqs.
If not, it doesn't work safely for lots of reasons, and disable_irq is
not going to fix it enough.
Maybe that is something to tackle after the power management series as
shutdown is sort of like power down.
--
Jason
This doesn't need to hold this mutex when calling report, the irq
should be synchronized when the master is removed to keep dev stable.
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
--
Jason
Nothing uses until_empty = false ?
Is that for the power management series? How does it make sense?
Shouldn't we already know the queue is not seeing new entries in that
case?
+ ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
+ u32 cons, prod, pending;
+ u32 drained = 0;
+
+ 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_DIFF(&q->llq, cons, prod);
+
+ while (true) {
+ u32 prev, undrained;
+ bool expired;
+
+ /*
+ * Sample the deadline ahead of the queue state it judges, but
+ * break only after the exit conditions below, so a queue that
+ * drained during a long preemption still exits with a success.
+ */
+ expired = ktime_compare(ktime_get(), timeout) > 0;
+
+ /* Accumulate the entries consumed since the last poll */
+ prev = cons;
+ cons = readl_relaxed(q->cons_reg);
+ drained += Q_DIFF(&q->llq, prev, cons);
+
+ prod = readl_relaxed(q->prod_reg);
+ undrained = Q_DIFF(&q->llq, cons, prod);
I'm not sure how this all can work, the queue is running on its own
with some other CPU handling interrupts.
You can't do this sort of Q_DIFF math unless you've somehow guaranteed
one side of the queue is stable for this logic. If both pointers are
moving forward then the points pointers can progress and wrap without
this noticing that happened. That will lock up.
Can you just replace this whole function with:
static void arm_smmu_irq_thread_fence(struct arm_smmu_device *smmu,
unsigned int irq)
{
if (!irq)
return;
irq_wake_thread(irq, smmu);
synchronize_irq(irq);
}
?
This forces the thread to run and waits for it to finish. Since the
thread fully drains the queue at the moment it starts, that should be
sufficient?
But I wonder if the point of this has been lost? Prior to calling the
driver attach functions the core code already changes the xarray:
curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
XA_ZERO_ENTRY, GFP_KERNEL);
That immediately makes the threaded IRQ safe since it calls
iommu_attach_handle_get() which now fails.
So all that is needed is to synchronize_irq() to make sure the irq
thread sees the xa update
Then to flush the workqueue that iommu_report_device_fault() pushes
into.
We don't need to do anything with the HW queue.
--
Jason
From: Jason Gunthorpe <jgg@nvidia.com> Date: 2026-09-23 18:37:50
quoted hunk
[ ... 54 lines skipped ... ]
@@ -3422,8 +3423,38 @@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state) * which the IOMMU core might free once this returns. Drain the hardware * eventq, so that a pending event cannot turn into new fault work. */- if (master_domain->using_iopf && master->stall_enabled)- arm_smmu_wait_for_queue_drained(smmu, &smmu->evtq.q, false);+ if (master_domain->using_iopf && master->stall_enabled) {+ timed_out = arm_smmu_wait_for_queue_drained(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 (!timed_out) {+ if (smmu->evtq.q.irq)+ synchronize_irq(smmu->evtq.q.irq);+ /* Pending events might be in the combined_irq handler */+ if (smmu->combined_irq)+ synchronize_irq(smmu->combined_irq);+ }
I think this logic is overkill. If we can't flush the queue we are
probably going to crash next. Chuck a warn on and hope for the
best. We don't need this fancy half attempt at recovery.
--
Jason
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.
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
--
Jason
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 only 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.
[...]
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
--
Jason
[Severity: Medium]
This is a pre-existing issue, but does hardcoding the SID to streams[0].id
cause the wrong stream to be resumed on multi-SID devices?
This seems right. Stall mode should encode the SID in the iopf_fault
event, maybe in the private data and recover it here.
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.
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
--
Jason
From: Jason Gunthorpe <jgg@nvidia.com> Date: 2026-09-23 18:40:57
arm_smmu_init_queues() allocates the IOPF queue only for FEAT_SVA together
with FEAT_STALLS. This queue has no SVA dependency, and FEAT_STALLS alone
can produce events that need IOPF.
Until iommufd there was no other way to create faults in the first
place. Doesn't matter if the HW supports it.
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 can
oops on mutex_lock(&queue->lock).
Yeah the guards got mismatched. There should have been a
arm_smmu_iopf_enabled(smmu)
if (!IS_ENABLED(CONFIG_ARM_SMMU_V3_SVA)) return false
if (!(smmu->features & ARM_SMMU_FEAT_SVA)) return false
And that is used everywhere. The fact one place used the feat and the
other used the CONFIG is the cause of this crash.
quoted hunk
[ ... 32 lines skipped ... ]
@@ -3215,9 +3215,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
@@ -3249,9 +3246,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;
I think this is fine
quoted hunk
@@ -4667,8 +4661,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;
But here it would still be nice to support the non-SVA optimization..
I'd change the kconfig to move the select IOMMU_IOPF down to the
iommufd and SVA blocks
Then guard this with an IS_ENABLED(CONFIG_IOMMU_IOPF)
That way you can choose to compile out IOPF if you don't want to use
any SMMU features that rely on it.
--
Jason
arm_smmu_setup_irqs() can leave the PRIQ without any registered handler if
its IRQ is missing, its request fails, or the combined IRQ request fails.
These paths warn but leave ARM_SMMU_FEAT_PRI set.
This function itself would then go on to enable IRQ_CTRL_PRIQ_IRQEN, even
though no handler will drain the queue. A later patch also enables PRI on
such endpoints. The caller already enabled CR0_PRIQEN before this function
even runs.
[...]
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
--
Jason
Is that for the power management series? How does it make sense?
Shouldn't we already know the queue is not seeing new entries in that
case?
The RPM series (CMDQ) uses until_empty=true, as it knows that there
are no new entries.
quoted
+ ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US);
+ u32 cons, prod, pending;
+ u32 drained = 0;
+
+ 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_DIFF(&q->llq, cons, prod);
+
+ while (true) {
+ u32 prev, undrained;
+ bool expired;
+
+ /*
+ * Sample the deadline ahead of the queue state it judges, but
+ * break only after the exit conditions below, so a queue that
+ * drained during a long preemption still exits with a success.
+ */
+ expired = ktime_compare(ktime_get(), timeout) > 0;
+
+ /* Accumulate the entries consumed since the last poll */
+ prev = cons;
+ cons = readl_relaxed(q->cons_reg);
+ drained += Q_DIFF(&q->llq, prev, cons);
+
+ prod = readl_relaxed(q->prod_reg);
+ undrained = Q_DIFF(&q->llq, cons, prod);
I'm not sure how this all can work, the queue is running on its own
with some other CPU handling interrupts.
You can't do this sort of Q_DIFF math unless you've somehow guaranteed
one side of the queue is stable for this logic. If both pointers are
moving forward then the points pointers can progress and wrap without
this noticing that happened. That will lock up.
One side of the queue (pointers) is actually stable. EVTQ/PRIQ uses
the snapshot mode (until_empty=false):
* 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 --------------->|
"cons0", "prod0", and "pending" are frozen/stable at the start; the
function only needs to track a moving "cons".
Arguably the other model for CMDQ (until_empty=true) does have two
moving pointers in theory:
* With @until_empty == true (for CMDQ), exit once the queue is observed empty:
*
* cons0 cons prod
* | | |
* ---+###################+===================================+--->
* |<--------- undrained==0? --------->|
But in its practical use case (RPM), as you pointed out, there would
not be new entries. IOW, its "prod" is fixed/stable; only "cons" is
moving.
Can you just replace this whole function with:
static void arm_smmu_irq_thread_fence(struct arm_smmu_device *smmu,
unsigned int irq)
{
if (!irq)
return;
irq_wake_thread(irq, smmu);
synchronize_irq(irq);
}
?
This forces the thread to run and waits for it to finish. Since the
thread fully drains the queue at the moment it starts, that should be
sufficient?
Ah, that seems legit to me. The point is to make sure no pending IRQ
on the HW queues, so everything is drained. And this likely suffices.
Yea, I will try that one!
But I wonder if the point of this has been lost? Prior to calling the
driver attach functions the core code already changes the xarray:
curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
XA_ZERO_ENTRY, GFP_KERNEL);
That immediately makes the threaded IRQ safe since it calls
iommu_attach_handle_get() which now fails.
I am not sure about that. Looking at iommufd_hwpt_replace_device(),
there can be a old_handle != NULL, in which case the cmpxchg() would
not change the xarray?
So all that is needed is to synchronize_irq() to make sure the irq
thread sees the xa update
Then to flush the workqueue that iommu_report_device_fault() pushes
into.
We don't need to do anything with the HW queue.
FWIW, the idea of HW drain came from intel_iommu_drain_pasid_prq()..
Nicolin
From: Jason Gunthorpe <jgg@nvidia.com> Date: 2026-09-23 23:39:41
On Wed, Sep 23, 2026 at 03:33:06PM -0700, Nicolin Chen wrote:
quoted
I'm not sure how this all can work, the queue is running on its own
with some other CPU handling interrupts.
You can't do this sort of Q_DIFF math unless you've somehow guaranteed
one side of the queue is stable for this logic. If both pointers are
moving forward then the points pointers can progress and wrap without
this noticing that happened. That will lock up.
One side of the queue (pointers) is actually stable. EVTQ/PRIQ uses
the snapshot mode (until_empty=false):
It isn't stable, just because this reads it once doesn't mean the
actual values are not changing, which is the point.
If one of the pointers is held stable then the HW cannot advance its
value past it.
If both are advancing all bets are off and you have no idea how the
values are related to each other since everything is modulo the ring
size.
For instance you can read cons0=10, then you read prod=15, then you
next read prod=11. What does that mean? It means since cons was
actually advancing prod & cons went around the whole ring and
wrapped.
You could only do tricks like this if you had full 64 bit counters,
not truncated versions with modulo that can wrap quickly.
quoted
But I wonder if the point of this has been lost? Prior to calling the
driver attach functions the core code already changes the xarray:
curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
XA_ZERO_ENTRY, GFP_KERNEL);
That immediately makes the threaded IRQ safe since it calls
iommu_attach_handle_get() which now fails.
Hmm, actually that's a sneaky cmpxchg that is only doing reserve..
I am not sure about that. Looking at iommufd_hwpt_replace_device(),
there can be a old_handle != NULL, in which case the cmpxchg() would
not change the xarray?
I think this is wrong, there is no way it can work like this where the
attach continues to see the to-be-detached domain across the
flushes. No amount of flushing can fix it.
Somehow we broke it :\
quoted
So all that is needed is to synchronize_irq() to make sure the irq
thread sees the xa update
Then to flush the workqueue that iommu_report_device_fault() pushes
into.
We don't need to do anything with the HW queue.
FWIW, the idea of HW drain came from intel_iommu_drain_pasid_prq()..
Yeah, but I think they might have over done it too..
Jason
On Wed, Sep 23, 2026 at 03:37:05PM -0300, Jason Gunthorpe wrote:
quoted
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.
This seems like a sashiko hallucination?
There are no PRIs during probe. The initial STE table does not permit
it.
Checked my long rework history. Yes... my bad..
quoted
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.
That's fscking devm again. Do not mix devm and non devm CALLS. It
doesn't work:
ret = arm_smmu_device_reset(smmu);
if (ret) {
arm_smmu_device_disable(smmu); <-------------------
return ret;
}
/* Register last so it unwinds first, while the CMDQ is still up. */
ret = devm_add_action_or_reset(smmu->dev, arm_smmu_disable_action, smmu);
if (ret)
return ret;
This seems requiring a reorder.
When devm is devm'ing properly the existing devm irq handlers will be
torn down before disable_action and we don't need to open code a
disable_irq
I see. Yes.
That just leaves shutdown, and I'm not sure how that is supposed to
work, we can't just halt the SMMU while there are any threads running
around still poking at it. I forget the context shutdown runs in, but
if concurrency is shut down already then its fine to not disable the
irqs.
If not, it doesn't work safely for lots of reasons, and disable_irq is
not going to fix it enough.
Maybe that is something to tackle after the power management series as
shutdown is sort of like power down.
On Wed, Sep 23, 2026 at 08:39:20PM -0300, Jason Gunthorpe wrote:
On Wed, Sep 23, 2026 at 03:33:06PM -0700, Nicolin Chen wrote:
quoted
quoted
I'm not sure how this all can work, the queue is running on its own
with some other CPU handling interrupts.
You can't do this sort of Q_DIFF math unless you've somehow guaranteed
one side of the queue is stable for this logic. If both pointers are
moving forward then the points pointers can progress and wrap without
this noticing that happened. That will lock up.
One side of the queue (pointers) is actually stable. EVTQ/PRIQ uses
the snapshot mode (until_empty=false):
It isn't stable, just because this reads it once doesn't mean the
actual values are not changing, which is the point.
If one of the pointers is held stable then the HW cannot advance its
value past it.
Oh, HW pointers cannot be stable, as EVTQ and EVTQ are shared with
other devices that could constantly add new entries onto the queue.
The stunt was to pick a snapshot of CONS/PROD, while HW CONS/PROD
are still advancing. And it would only wait for the length of that
snapshot. The device is detached, so any event after that must be
irrelevant.
If both are advancing all bets are off and you have no idea how the
values are related to each other since everything is modulo the ring
size.
For instance you can read cons0=10, then you read prod=15, then you
next read prod=11. What does that mean? It means since cons was
actually advancing prod & cons went around the whole ring and
wrapped.
You could only do tricks like this if you had full 64 bit counters,
not truncated versions with modulo that can wrap quickly.
The Q_DIFF was calculated including the WRAP bits. So, it wouldn't
be a problem when any pointer wraps (once).
There is a problem, however, if one of them wraps twice (i.e. 2 x
queue size): then it would miss the exit at the target length even
if it is already much longer; and the penalty would be a timeout,
yet at that moment the queue is definitely drained.
Or maybe I am still missing a key point?
quoted
quoted
But I wonder if the point of this has been lost? Prior to calling the
driver attach functions the core code already changes the xarray:
curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
XA_ZERO_ENTRY, GFP_KERNEL);
That immediately makes the threaded IRQ safe since it calls
iommu_attach_handle_get() which now fails.
Hmm, actually that's a sneaky cmpxchg that is only doing reserve..
quoted
I am not sure about that. Looking at iommufd_hwpt_replace_device(),
there can be a old_handle != NULL, in which case the cmpxchg() would
not change the xarray?
I think this is wrong, there is no way it can work like this where the
attach continues to see the to-be-detached domain across the
flushes. No amount of flushing can fix it.
Somehow we broke it :\
Hmm, I will take a deeper look.
quoted
quoted
So all that is needed is to synchronize_irq() to make sure the irq
thread sees the xa update
Then to flush the workqueue that iommu_report_device_fault() pushes
into.
We don't need to do anything with the HW queue.
FWIW, the idea of HW drain came from intel_iommu_drain_pasid_prq()..
Yeah, but I think they might have over done it too..
From: Jason Gunthorpe <jgg@nvidia.com> Date: 2026-09-24 14:04:25
On Wed, Sep 23, 2026 at 06:42:42PM -0700, Nicolin Chen wrote:
There is a problem, however, if one of them wraps twice (i.e. 2 x
queue size): then it would miss the exit at the target length even
if it is already much longer; and the penalty would be a timeout,
yet at that moment the queue is definitely drained.
Yes, we this routine should not hit timeouts, that's really bad. The
point is you can hit a false timeout because the algorithm does not
work.
quoted
quoted
FWIW, the idea of HW drain came from intel_iommu_drain_pasid_prq()..
Yeah, but I think they might have over done it too..
I see.
The HW queue may need flushing on the device hot remove path, that
depends how they implemented things. Like on ARM you had a mutex
around the whole thing dev pointer so we don't even need that..
Jason