Re: [PATCH v4 03/10] iommu/arm-smmu-v3: Store IOTLB cache tags in struct arm_smmu_attach_state

[Nicolin Chen <nicolinc@nvidia.com>]

On Thu, Apr 09, 2026 at 08:42:23PM -0300, Jason Gunthorpe wrote:
> On Thu, Mar 19, 2026 at 12:51:49PM -0700, Nicolin Chen wrote:
> > So far, an IOTLB tag (ASID or VMID) has been stored in the arm_smmu_domain
> > +static int __arm_smmu_domain_find_iotlb_tag(struct arm_smmu_domain *smmu_domain,
> > +					    struct arm_smmu_inv *tag)
> > +{
> > +	struct arm_smmu_invs *invs = rcu_dereference_protected(
> > +		smmu_domain->invs, lockdep_is_held(&arm_smmu_asid_lock));
> > +	size_t i;
> > +
> > +	arm_smmu_inv_assert_iotlb_tag(tag);
> > +
> > +	for (i = 0; i != invs->num_invs; i++) {
> > +		if (invs->inv[i].type == tag->type &&
> > +		    invs->inv[i].smmu == tag->smmu &&
> > +		    READ_ONCE(invs->inv[i].users)) {
> > +			*tag = invs->inv[i];
> 
> This users thing has become to hard to understand and it isn't how it
> should be.
> 
> All writers *with the possibility of concurrent access* need to use
> WRITE_ONCE since there is a RCU reader. IIRC that is just
> arm_smmu_invs_unref()
> 
> The one in arm_smmu_invs_merge() is just writing to newly allocated
> memory so it shouldn't be marked.
> 
> Only readers *with the possibility of concurrent access* should be
> marked with READ_ONCE. IIRC this is just the invalidation walker.

I added a cleanup patch to the beginning of the series and corrected
all the new reads/writes too.

> Places like this have to be protected by a lock or the whole thing is
> wrong, so it should have a lockdep annoation.

Hmm, is the lockdep_is_held() in rcu_dereference_protected() enough?

> Now what is the locking supposed to be? It looks wrong, it probably
> wants to be arm_smmu_asid_lock, but arm_smmu_mm_release() doesn't grab
> it.
> 
> But why does arm_smmu_mm_release() need a tag in the first place? ASID
> isn't going to be used when EPD0|EPD1 is set, so the tag can just be
> 0. Probably make a patch with that change early on..

I see. I added a cleanup patch.

> All the locking is important because this:
> 
> > +/* Find an existing IOTLB cache tag in smmu_domain->invs (users counter != 0) */
> 
> Must be held as an invarient into the caller, meaning the caller must
> hold arm_smmu_asid_lock while it has an active tag on the stack, and
> that should be documented here. As well as a lockdep of course.
> 
> From what I can tell the final result is correct (aside from
> arm_smmu_mm_release), just under documented.

Re-checking the locking in these functions and updating the kdocs.

Thanks
Nicolin