Re: [PATCH v2 7/7] iommu/arm-smmu-v3: Block ATS upon an ATC invalidation timeout

[Shuai Xue]

On 3/19/26 11:26 AM, Nicolin Chen wrote:
> On Thu, Mar 19, 2026 at 10:56:43AM +0800, Shuai Xue wrote:
> > On 3/18/26 3:15 AM, Nicolin Chen wrote:
> > > For batched ATC_INV commands, SMMU hardware only reports a timeout at the
> > > CMD_SYNC, which could follow the batch issued for multiple devices. So, it
> > > isn't straightforward to identify which command in a batch resulted in the
> > > timeout. Fortunately, the invs array has a sorted list of ATC entries. So,
> > > the issued batch must be sorted as well. This makes it possible to bisect
> > > the batch to retry the command per Stream ID and identify the master.
> >
> > Nit: The implementation is a linear per-SID retry, not a binary
> > search / bisection. Suggest rewording to:
> >
> >    "retry the ATC_INV command for each unique Stream ID in the batch
> >     to identify the unresponsive master"
>
> You are right. And that sounds OK.
>
> > > +	step = arm_smmu_get_step_for_sid(smmu, sid);
> > > +	WRITE_ONCE(step->data[1],
> > > +		   READ_ONCE(step->data[1]) & cpu_to_le64(~STRTAB_STE_1_EATS));
> >
> >
> > This non-atomic read-modify-write on step->data[1] can race with the
> > normal STE installation path (arm_smmu_write_entry → entry_set →
> > WRITE_ONCE).
> >
> > The error path runs from:
> >
> >    __arm_smmu_domain_inv_range()  (data path, no group->mutex)
> >      → arm_smmu_cmdq_batch_retry()
> >        → arm_smmu_master_disable_ats()
> >          → arm_smmu_disable_eats_for_sid()   ← NO locks on STE
> >
> > The normal STE path runs from:
> >
> >    iommu_attach_device()
> >      → mutex_lock(&group->mutex)
> >        → arm_smmu_attach_dev()
> >          → mutex_lock(&arm_smmu_asid_lock)
> >            → arm_smmu_install_ste_for_dev()
> >              → arm_smmu_write_entry()        ← holds both mutexes
> >
> > Since the error path holds neither group->mutex nor arm_smmu_asid_lock,
> > the following race is possible:
>
> Because invalidations can be in atomic context so we can't hold
> those mutex locks.
>
> >    CPU A (error path):             CPU B (attach path):
> >    READ data[1] = X
> >                                    WRITE data[1] = Y (new STE config)
> >    WRITE data[1] = X & ~EATS
> >                                    // Y is lost
> >
> > This could clobber a concurrent STE update from the attach path.
>
> Oh, that's true. Maybe this:
> 	__le64 new, old = READ_ONCE(step->data[1]);
> 	[...]
> 	do {
> 		new = old & cpu_to_le64(~STRTAB_STE_1_EATS);
> 	} while (!try_cmpxchg64(&step->data[1], &old, new));
> ?

Yes, the cmpxchg loop looks correct to me.

Thanks.
Shuai