Re: [PATCH v5 04/15] iommu/arm-smmu-v3: Drain in-flight fault events on domain detach

[Nicolin Chen <nicolinc@nvidia.com>]

On Wed, Sep 23, 2026 at 03:37:05PM -0300, Jason Gunthorpe wrote:
> > [ ... 73 lines skipped ... ]
> > +static int arm_smmu_wait_for_queue_drained(struct arm_smmu_device *smmu,
> > +					   struct arm_smmu_queue *q,
> > +					   bool until_empty)
> > +{
> 
> Nothing uses until_empty = false ?

This series (PRIQ/EVTQ) use 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?

The RPM series (CMDQ) uses until_empty=true, as it knows that there
are no new entries.

> > +	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