Re: [PATCH rfcv1 6/8] iommu/arm-smmu-v3: Populate smmu_domain->invs when attaching masters

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Aug 13, 2025 at 06:25:37PM -0700, Nicolin Chen wrote:
> +typedef struct arm_smmu_invs *(*invs_fn)(struct arm_smmu_invs *old_invs,
> +					 struct arm_smmu_invs *invs);

no reason to pass in fn, this always just calls it as the last thing
so the caller can do it..

> +static struct arm_smmu_invs *arm_smmu_build_invs(
> +	struct arm_smmu_invs *old_invs, struct arm_smmu_domain *smmu_domain,
> +	struct arm_smmu_master *master, bool ats, ioasid_t ssid, invs_fn fn)
> +{
> +	const bool e2h = master->smmu->features & ARM_SMMU_FEAT_E2H;
> +	const bool nesting = smmu_domain->nest_parent;
> +	struct arm_smmu_inv *cur = master->invs->inv;
> +	size_t num_invs = 1;
> +	size_t i;
> +
> +	switch (smmu_domain->stage) {
> +	case ARM_SMMU_DOMAIN_SVA:
> +	case ARM_SMMU_DOMAIN_S1:
> +		cur->smmu = master->smmu;
> +		cur->type = INV_TYPE_S1_ASID;
> +		cur->id = smmu_domain->cd.asid;
> +		cur->size_opcode = e2h ? CMDQ_OP_TLBI_EL2_VA :
> +					 CMDQ_OP_TLBI_NH_VA;
> +		cur->nsize_opcode = e2h ? CMDQ_OP_TLBI_EL2_ASID :
> +					  CMDQ_OP_TLBI_NH_ASID;
> +		break;
> +	case ARM_SMMU_DOMAIN_S2:
> +		cur->smmu = master->smmu;
> +		cur->type = INV_TYPE_S2_VMID;
> +		cur->id = smmu_domain->s2_cfg.vmid;
> +		cur->size_opcode = CMDQ_OP_TLBI_S2_IPA;
> +		cur->nsize_opcode = CMDQ_OP_TLBI_S12_VMALL;
> +		break;
> +	default:
> +		WARN_ON(true);
> +		return old_invs;

Return ERR_PTR, it makes the error flows possibly wrong or at least over
complex to return something that shouldn't be freed.

> +	}
> +
> +	/* Range-based invalidation requires the leaf pgsize for calculation */
> +	if (master->smmu->features & ARM_SMMU_FEAT_RANGE_INV)
> +		cur->pgsize = __ffs(smmu_domain->domain.pgsize_bitmap);
> +
> +	/* All the nested S1 ASIDs have to be flushed when S2 parent changes */
> +	if (nesting) {
> +		cur = &master->invs->inv[num_invs++];

Don't do both 'cur as an iterator' and 'num_invs as the
location'. Delete num_invs entirely and just use cur.

> +		cur->smmu = master->smmu;
> +		cur->type = INV_TYPE_S2_VMID_S1_CLEAR;
> +		cur->id = smmu_domain->s2_cfg.vmid;
> +		cur->size_opcode = CMDQ_OP_TLBI_NH_ALL;
> +		cur->nsize_opcode = CMDQ_OP_TLBI_NH_ALL;
> +	}
> +
> +	if (ats) {
> +		for (i = 0, cur++; i < master->num_streams; i++) {
> +			cur->smmu = master->smmu;
> +			/*
> +			 * If an S2 used as a nesting parent is changed we have
> +			 * no option but to completely flush the ATC.
> +			 */
> +			cur->type = nesting ? INV_TYPE_ATS_FULL : INV_TYPE_ATS;
> +			cur->id = master->streams[i].id;
> +			cur->ssid = ssid;
> +			cur->size_opcode = CMDQ_OP_ATC_INV;
> +			cur->nsize_opcode = CMDQ_OP_ATC_INV;
> +		}
> +		num_invs += master->num_streams;
> +	}
> +
> +	master->invs->num_invs = num_invs;

Like this:

	master->invs->num_invs = cur - master->invs->inv;

> +static int arm_smmu_attach_prepare_invs(struct arm_smmu_attach_state *state,
> +					struct arm_smmu_domain *new_smmu_domain)
> +{

How about a comment:

/*
 * During attachment the invalidation lists on the two domains are sequenced:
 *  1. old domain is invalidating master
 *  2. new and old domain are invalidating master
 *  3. new domain is invalidating master
 *
 * This uses two updated invalidation lists, one with master added to new domain
 * and one with master removed from old domain. Prepare these lists in advance
 * of changing anything. arm_smmu_asid_lock ensures that the invalidation list
 * in the domains doesn't change while we are sequencing to update it.
 */

> +	struct arm_smmu_domain *old_smmu_domain =
> +		to_smmu_domain_devices(state->old_domain);
> +	struct arm_smmu_master *master = state->master;
> +	bool blocking = false;
> +
> +	/* A re-attach case doesn't need to update invs array */
> +	if (new_smmu_domain == old_smmu_domain)
> +		return 0;
> +
> +	if (new_smmu_domain) {

This if wants a comment, it is tricky:

	/*
	 * At this point a NULL domain indicates the domain doesn't use the
	 * IOTLB, see to_smmu_domain_devices().
	 */

> +		state->new_domain_oinvs = rcu_dereference_protected(
> +			new_smmu_domain->invs,
> +			lockdep_is_held(&arm_smmu_asid_lock));
> +		state->new_domain_ninvs = arm_smmu_build_invs(
> +			state->new_domain_oinvs, new_smmu_domain, master,
> +			state->ats_enabled, state->ssid, arm_smmu_invs_add);
> +		if (IS_ERR(state->new_domain_ninvs))
> +			return PTR_ERR(state->new_domain_ninvs);
> +		state->new_domain_invs = &new_smmu_domain->invs;
> +		blocking = new_smmu_domain->domain.type == IOMMU_DOMAIN_BLOCKED;
> +	}
> +
> +	if (old_smmu_domain) {
> +		state->old_domain_oinvs = rcu_dereference_protected(
> +			old_smmu_domain->invs,
> +			lockdep_is_held(&arm_smmu_asid_lock));
> +		state->old_domain_ninvs = arm_smmu_build_invs(
> +			state->old_domain_oinvs, old_smmu_domain, master,
> +			master->ats_enabled, state->ssid, arm_smmu_invs_del);
> +		if (IS_ERR(state->old_domain_ninvs)) {

Then here, as per the last email, just get rid of invs_del and use
the scratch list master->invs for the next step. So all this goes away:

Jason