Re: [PATCH v5 2/3] iommu/arm-smmu-v3: Introduce CFGI/TLBI-repeat workaround infrastructure

[Nicolin Chen]

On Thu, Jul 09, 2026 at 09:56:08AM +0000, Ashish Mhetre wrote:
> No callers enable the static key yet, so this patch introduces no
> functional change. A subsequent change hooks the DT probe to enable
> the key on affected instances.

Nit: once a patch is merged, it's no longer a "patch" but "commit".

Instead,

No callers enable the static key yet, so there is no functional change. A
subsequent change will enable the key on affected instances.

> Suggested-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> Signed-off-by: Ashish Mhetre <amhetre@xxxxxxxxxx>

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 76efe479e80f..15b9d0170520 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -41,6 +41,7 @@ MODULE_PARM_DESC(disable_msipolling,
>  
>  static const struct iommu_ops arm_smmu_ops;
>  static struct iommu_dirty_ops arm_smmu_dirty_ops;
> +DEFINE_STATIC_KEY_FALSE(arm_smmu_erratum_repeat_tlbi_cfgi_key);

It's an erratum; it should have an inline description, like the
one you had at ARM_SMMU_OPT_TLBI_TWICE previously.

> +int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> +				struct arm_smmu_cmdq *cmdq,
> +				struct arm_smmu_cmd *cmds, int n,
> +				bool sync)
> +{
> +	int ret = __arm_smmu_cmdq_issue_cmdlist(smmu, cmdq, cmds, n, sync);
> +
> +	/*
> +	 * arm_smmu_cmdq_batch_add_cmd_p() can flush its current batch
> +	 * with sync=true and n=0 (bare SYNC) when the next command is
> +	 * not supported by the batch's pre-selected cmdq, so the

After a second thought, this "supported" case is not reachable.

When a batch is init-ed, it's given an opcode (same as cmd[0]'s).

If an opcode is not supported, it's init-ed with smmu->cmdq that
must support all commands.

So, adding the !n is more like a defensive play. But the comments
here would be misleading.

> +	 * repeat path must not inspect cmds[0].
> +	 */
> +	if (!n || ret || !sync)
> +		return ret;
> +
> +	if (arm_smmu_erratum_cmd_needs_repeating(smmu, &cmds[0]))
> +		ret = __arm_smmu_cmdq_issue_cmdlist(smmu, cmdq, cmds, n, sync);
> +
> +	return ret;
> +}
> +
>  static int arm_smmu_cmdq_issue_cmd_p(struct arm_smmu_device *smmu,
>  				     struct arm_smmu_cmd *cmd, bool sync)
>  {
> @@ -860,6 +883,11 @@ static bool arm_smmu_cmdq_batch_force_sync(struct arm_smmu_device *smmu,
>  	    (smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC))
>  		return true;
>  
> +	/* See arm_smmu_erratum_cmd_needs_repeating() */
> +	if (cmds->num == CMDQ_BATCH_ENTRIES &&
> +	    arm_smmu_erratum_cmd_needs_repeating(smmu, &cmds->cmds[0]))

The comment above is useless; arm_smmu_erratum_cmd_needs_repeating
doesn't give any useful explanation either.

Like I suggested, add the missing piece of war description to the
arm_smmu_erratum_repeat_tlbi_cfgi_key at the top. Then,

	/* See the description at arm_smmu_erratum_repeat_tlbi_cfgi_key */

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -1211,6 +1212,20 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
>  				struct arm_smmu_cmdq *cmdq,
>  				struct arm_smmu_cmd *cmds, int n,
>  				bool sync);
> +DECLARE_STATIC_KEY_FALSE(arm_smmu_erratum_repeat_tlbi_cfgi_key);
> +
> +static inline bool
> +arm_smmu_erratum_cmd_needs_repeating(struct arm_smmu_device *smmu,
> +				     struct arm_smmu_cmd *cmd)

smmu is unused.

> +{
> +	u8 opcode;
> +
> +	if (!static_branch_unlikely(&arm_smmu_erratum_repeat_tlbi_cfgi_key))
> +		return false;
> +
> +	opcode = FIELD_GET(CMDQ_0_OP, cmd->data[0]);
> +	return opcode >= CMDQ_OP_CFGI_STE && opcode < CMDQ_OP_ATC_INV;
> +}

Maybe move this to the first caller in arm-smmu-v3.c? Instead, add
its stub here, so we can drop that DECLARE_STATIC_KEY_FALSE. Then,
DEFINE_STATIC_KEY_FALSE will need a "static".

Nicolin