Re: [PATCH v4 18/24] iommu/arm-smmu-v3: Introduce master->ats_broken flag

[Nicolin Chen <nicolinc@nvidia.com>]

Thanks for the reply.

This is indeed a very complex and sophisticated topic..

On Fri, Jun 05, 2026 at 04:42:59PM -0300, Jason Gunthorpe wrote:
> I don't see any of these options as appealing. We have to maintain a
> few key invariants, and I think it cannot be done without a way to
> find all the domains that are using the STE.
> 
> One way or another you have to be using the invs list rw locks to
> synchronize the EATS state changes.
> 
> It is okayish to be sloppy when turning EATS off, but when turning it
> back on we do need to cycle through every invs list and toggle its
> lock to ensure that the invalidations are synchronized before
> EATS=enable happens.

I think the core guarantees that "cycle through every invs list"
happens: a PCI reset calls reset_prepare() blocking all the RID
and PASID domains and removing ATS entries from every invs list,
and then calls reset_done() that re-attach RID/PASID domains so
freshly new ATS entries will be installed before EATS=enable.

So, I think the enable path is not an issue, though the disable
path or the invalidation path would need "a way to find all the
domains that are using the STE".

> Given you must have a way to go from STE -> master -> all invs lists
> I'm not sure either option really makes such a large difference.
> 
> If so then adjusting the invs to disable the ATS is pretty simple, run
> over the xarray and set them all off. Yes you could find the master
> through a SID lookup with some locking adjustment.
> > 
> > (1) Per-invs marker: INV_TYPE_ATS_BROKEN + master_domains
> >     disable_ats() in the timeout path walks master->master_domains
> >     and flips matching ATS invs entries to the BROKEN type.
> > 
> >   + invs walker is free (one case label in the existing type switch).
> >   + No lock or pointer deref in the invs walker.
> >   + No master pointer stored in invs; no lifetime concern.
> > 
> >   - disable_ats() walks every (master, domain) and marks each invs
> >     set; the list needs locking usable from atomic.
> 
> This doesn't seem so bad

Yea, the only thing is that the disable path has to deal with a
complexity from going through a per-device domain list. Maybe it
can reuse iommu_group->pasid_array by taking xa_lock?

> > (2) Per-master flag + streams_lock
> >     invs walker resolves SID -> master via streams_lock and reads
> >     master->ats_broken.
> > 
> >   + Single source of truth on the master.
> >   + disable_ats() is one WRITE_ONCE.
> >   + atc_inv_master early-skips via one READ_ONCE.
> >   + attach gates ats_enabled on the flag; a concurrent quarantine
> >     race can be closed by a short post-attach re-check in commit()
> >   + No master pointer in invs; no lifetime concern.
> > 
> >   - invs walker pays streams_lock + rb_find(SID) per ATS entry on
> >     every invalidation. Measurable on ATS-heavy workloads.
> 
> Doesn't consider how to enable

The enable side is core-driven: when reset_done() re-attaches
the device from blocked_domain back to its RID/PASID domains,
the new attach_dev callback (old_domain == blocked_domain) can
clear the per-master flag. If the device is still broken, then
arm_smmu_atc_inv_master() at the end of attach_commit() times
out and re-triggers quarantine.

The flaw lives in the invalidation path as it must translate
every SID to master using streams_lock + rb_find(SID) per ATS
entry, which make it very less attractive.

> > (3) Per-master flag + inv->master pointer (v4)
> >     invs entry carries a master pointer; the invs walker reads
> >     cur->master->ats_broken directly.
> > 
> >   + invs walker is one READ_ONCE through a cached pointer.
> >   + disable_ats is one WRITE_ONCE.
> >   + atc_inv_master early-skip via one READ_ONCE.
> >   + attach gate + post-attach re-check, same as (2).
> > 
> >   - invs holds a master ptr, so release_device must synchronize_rcu()
> >     before freeing the master to drain walkers under rcu_read_lock().
> >     We dropped this from v4 for that reason.
> 
> synchronize_rcu is not right because you have to have gone through the
> rwlock so there can be no readers.

Ah, I think you are right! When release_device() is invoked, the
device must be already in the release (blocked) domain. So there
should be no domain->invs in the system holding its ATS entries.
And the enable part would work as (2).

In this case, (3) seems the best? It's fast on every aspect.

And I think it would fit we plan to generalize the invs design:

struct inv {
	struct arm_smmu_device *smmu; // => struct iommu_device *iommu;
	struct arm_smmu_master *master; // => void *priv;
					//    (dev->iommu->priv)

Thanks
Nicolin