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

[Jason Gunthorpe]

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.

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

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

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.

> +int arm_smmu_find_iotlb_tag(struct iommu_domain *domain,
> +			    struct arm_smmu_device *smmu,
> +			    struct arm_smmu_inv *tag)
> +{
> +	struct arm_smmu_domain *smmu_domain = to_smmu_domain_devices(domain);
> +
> +	if (WARN_ON(!smmu_domain))
> +		return -EINVAL;
> +
> +	/* Decide the type of the iotlb cache tag */
> +	switch (smmu_domain->stage) {
> +	case ARM_SMMU_DOMAIN_SVA:
> +	case ARM_SMMU_DOMAIN_S1:
> +		tag->type = INV_TYPE_S1_ASID;
> +		break;
> +	case ARM_SMMU_DOMAIN_S2:
> +		tag->type = INV_TYPE_S2_VMID;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	tag->smmu = smmu;
> +
> +	return __arm_smmu_domain_find_iotlb_tag(smmu_domain, tag);

This is the only caller it probably doesn't need a special __
function..

> +/* Allocate a new IOTLB cache tag (users counter == 0) */
> +static int arm_smmu_alloc_iotlb_tag(struct iommu_domain *domain,
> +				    struct arm_smmu_device *smmu,
> +				    struct arm_smmu_inv *tag)
> +{
> +	struct arm_smmu_domain *smmu_domain = to_smmu_domain_devices(domain);
> +	int ret;
> +
> +	/* Only allocate if there is no IOTLB cache tag to re-use */
> +	ret = arm_smmu_find_iotlb_tag(domain, smmu, tag);
> +	if (!ret || ret != -ENOENT)
> +		return ret;

Lets not call the function 'alloc_iotlb_tag' if it doesn't always
allocate.. 'get_iotlb_tag' more implies the find or allocate behavior.

Again the locking is important and the caller must ensure it holds the
asid_lock while the tag is alive on the stack. Mention it in the kdoc.

> +
> +	/* FIXME replace with an actual allocation from the bitmap */
> +	if (tag->type == INV_TYPE_S1_ASID)
> +		tag->id = smmu_domain->cd.asid;
> +	else
> +		tag->id = smmu_domain->s2_cfg.vmid;

I don't usually put FIXMEs that will be fixed in the next patches.

Jason