Subject: Re: [PATCH v2 2/2] iommu/arm-smmu-v3: Issue CFGI/TLBI twice on Tegra264

From: Ashish Mhetre <[email-masked]>
Date: Mon, 1 Jun 2026 14:50:10 +0530

On 5/30/2026 2:40 AM, Nicolin Chen wrote:
> On Fri, May 29, 2026 at 02:08:30PM +0000, Ashish Mhetre wrote:
>> +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);
>> +
>> +	/*
>> +	 * The driver's batch invariants keep a single submission's
>> +	 * opcode class uniform, so checking the first command is enough.
>> +	 */
>> +	if (!ret && sync && (smmu->options & ARM_SMMU_OPT_TLBI_TWICE) &&
>> +	    arm_smmu_cmd_needs_tlbi_twice(FIELD_GET(CMDQ_0_OP,
>> +						    cmds[0].data[0])))
>> +		ret = __arm_smmu_cmdq_issue_cmdlist(smmu, cmdq, cmds, n, sync);
> https://sashiko.dev/#/patchset/20260529140830.629738-1-amhetre%40nvidia.com
> Sashiko pointed out that the iommufd path might mix commands when
> calling arm_smmu_cmdq_issue_cmdlist(), which is valid I think.

Okay, I'll update the batching for iommufd path as suggested by you in
next version.

>>   static int arm_smmu_cmdq_issue_cmd_p(struct arm_smmu_device *smmu,
>>   				     struct arm_smmu_cmd *cmd, bool sync)
>>   {
>> @@ -863,8 +909,18 @@ static void arm_smmu_cmdq_batch_add_cmd_p(struct arm_smmu_device *smmu,
>>   	}
>>   
>>   	if (cmds->num == CMDQ_BATCH_ENTRIES) {
>> +		/*
>> +		 * Force a SYNC only when the batch carries commands that
>> +		 * have to be doubled (see ARM_SMMU_OPT_TLBI_TWICE).
>> +		 * The batch holds a uniform opcode class, so checking
>> +		 * the first command is sufficient.
>> +		 */
>> +		bool need_sync = (smmu->options & ARM_SMMU_OPT_TLBI_TWICE) &&
>> +				 arm_smmu_cmd_needs_tlbi_twice(FIELD_GET(CMDQ_0_OP,
>> +									 cmds->cmds[0].data[0]));
>> +
> Also, given that this does "force a sync", I think it might be nicer
> to go to the force_sync path. One of my ongoing series also needs to
> add another force_sync condition, so I think it would be cleaner to
> have a helper function.
>
> Maybe try the following diff. That arm_smmu_cmdq_batch_force_sync()
> might be added with a preparatory patch, but it's up to you.
>
> --------------------------------------------------------------------
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> index 1e9f7d2de3441..4c9ce974d31a8 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> @@ -350,6 +350,18 @@ static int arm_vsmmu_convert_user_cmd(struct arm_vsmmu *vsmmu,
>   	return 0;
>   }
>   
> +static bool arm_vsmmu_can_batch_cmd(struct arm_smmu_device *smmu,
> +				    struct arm_vsmmu_invalidation_cmd *last,
> +				    struct arm_vsmmu_invalidation_cmd *next)
> +{
> +	struct arm_smmu_cmd next_cmd = {
> +		.data[0] = le64_to_cpu(next->ucmd.cmd[0]),
> +	};
> +
> +	return arm_smmu_cmd_needs_tlbi_twice(smmu, &last->cmd) ==
> +	       arm_smmu_cmd_needs_tlbi_twice(smmu, &next_cmd);
> +}
> +
>   int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu,
>   			       struct iommu_user_data_array *array)
>   {
> @@ -382,7 +394,8 @@ int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu,
>   
>   		/* FIXME work in blocks of CMDQ_BATCH_ENTRIES and copy each block? */
>   		cur++;
> -		if (cur != end && (cur - last) != CMDQ_BATCH_ENTRIES - 1)
> +		if (cur != end && (cur - last) != CMDQ_BATCH_ENTRIES - 1 &&
> +		    arm_vsmmu_can_batch_cmd(smmu, last, cur))
>   			continue;
>   
>   		/* FIXME always uses the main cmdq rather than trying to group by type */
> 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 a63155e9e7f28..9b150e3145034 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -820,33 +820,6 @@ static int __arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
>   	return ret;
>   }
>   
> -/*
> - * Returns true if @opcode is a CFGI_* or TLBI_* command, i.e. one of the
> - * invalidations covered by Tegra264 erratum (see ARM_SMMU_OPT_TLBI_TWICE).
> - */
> -static bool arm_smmu_cmd_needs_tlbi_twice(u8 opcode)
> -{
> -	switch (opcode) {
> -	case CMDQ_OP_CFGI_STE:
> -	case CMDQ_OP_CFGI_ALL:
> -	case CMDQ_OP_CFGI_CD:
> -	case CMDQ_OP_CFGI_CD_ALL:
> -	case CMDQ_OP_TLBI_NH_ALL:
> -	case CMDQ_OP_TLBI_NH_ASID:
> -	case CMDQ_OP_TLBI_NH_VA:
> -	case CMDQ_OP_TLBI_NH_VAA:
> -	case CMDQ_OP_TLBI_EL2_ALL:
> -	case CMDQ_OP_TLBI_EL2_ASID:
> -	case CMDQ_OP_TLBI_EL2_VA:
> -	case CMDQ_OP_TLBI_S12_VMALL:
> -	case CMDQ_OP_TLBI_S2_IPA:
> -	case CMDQ_OP_TLBI_NSNH_ALL:
> -		return true;
> -	default:
> -		return false;
> -	}
> -}
> -
>   int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
>   				struct arm_smmu_cmdq *cmdq,
>   				struct arm_smmu_cmd *cmds, int n,
> @@ -858,9 +831,7 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
>   	 * The driver's batch invariants keep a single submission's
>   	 * opcode class uniform, so checking the first command is enough.
>   	 */
> -	if (!ret && sync && (smmu->options & ARM_SMMU_OPT_TLBI_TWICE) &&
> -	    arm_smmu_cmd_needs_tlbi_twice(FIELD_GET(CMDQ_0_OP,
> -						    cmds[0].data[0])))
> +	if (!ret && sync && arm_smmu_cmd_needs_tlbi_twice(smmu, &cmds[0]))
>   		ret = __arm_smmu_cmdq_issue_cmdlist(smmu, cmdq, cmds, n, sync);
>   
>   	return ret;
> @@ -893,34 +864,48 @@ static void arm_smmu_cmdq_batch_init_cmd(struct arm_smmu_device *smmu,
>   	cmds->cmdq = arm_smmu_get_cmdq(smmu, cmd);
>   }
>   
> +static bool arm_smmu_cmdq_batch_force_sync(struct arm_smmu_device *smmu,
> +					   struct arm_smmu_cmdq_batch *cmds,
> +					   struct arm_smmu_cmd *cmd)
> +{
> +	if (!cmds->num)
> +		return false;
> +
> +	/* The batch's pre-assigned cmdq doesn't support the new command */
> +	if (!arm_smmu_cmdq_supports_cmd(cmds->cmdq, cmd))
> +		return true;
> +
> +	/* Arm erratum 2812531 */
> +	if (cmds->num == CMDQ_BATCH_ENTRIES - 1 &&
> +	    (smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC))
> +		return true;
> +
> +	/*
> +	 * Tegra264 erratum (see ARM_SMMU_OPT_TLBI_TWICE). The batch holds a
> +	 * uniform opcode class, so checking the first command is enough.
> +	 */
> +	if ((cmds->num == CMDQ_BATCH_ENTRIES) &&
> +	    arm_smmu_cmd_needs_tlbi_twice(smmu, &cmds->cmds[0]))
> +		return true;
> +
> +	return false;
> +}
> +
>   static void arm_smmu_cmdq_batch_add_cmd_p(struct arm_smmu_device *smmu,
>   					  struct arm_smmu_cmdq_batch *cmds,
>   					  struct arm_smmu_cmd *cmd)
>   {
> -	bool force_sync = (cmds->num == CMDQ_BATCH_ENTRIES - 1) &&
> -			  (smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC);
> -	bool unsupported_cmd;
> +	bool force_sync = arm_smmu_cmdq_batch_force_sync(smmu, cmds, cmd);
>   
> -	unsupported_cmd = !arm_smmu_cmdq_supports_cmd(cmds->cmdq, cmd);
> -	if (force_sync || unsupported_cmd) {
> +	if (force_sync) {
>   		arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmdq, cmds->cmds,
>   					    cmds->num, true);
>   		arm_smmu_cmdq_batch_init_cmd(smmu, cmds, cmd);
>   	}
>   
>   	if (cmds->num == CMDQ_BATCH_ENTRIES) {
> -		/*
> -		 * Force a SYNC only when the batch carries commands that
> -		 * have to be doubled (see ARM_SMMU_OPT_TLBI_TWICE).
> -		 * The batch holds a uniform opcode class, so checking
> -		 * the first command is sufficient.
> -		 */
> -		bool need_sync = (smmu->options & ARM_SMMU_OPT_TLBI_TWICE) &&
> -				 arm_smmu_cmd_needs_tlbi_twice(FIELD_GET(CMDQ_0_OP,
> -									 cmds->cmds[0].data[0]));
> -
>   		arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmdq, cmds->cmds,
> -					    cmds->num, need_sync);
> +					    cmds->num, false);
>   		arm_smmu_cmdq_batch_init_cmd(smmu, cmds, cmd);
>   	}
>   
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 08d1abaf31ae2..e6afc832c0078 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -1219,6 +1219,37 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
>   				struct arm_smmu_cmd *cmds, int n,
>   				bool sync);
>   
> +/*
> + * Returns true if @cmd opcode is a CFGI_* or TLBI_* command, i.e. one of the
> + * invalidations covered by Tegra264 erratum (see ARM_SMMU_OPT_TLBI_TWICE).
> + */
> +static inline bool arm_smmu_cmd_needs_tlbi_twice(struct arm_smmu_device *smmu,
> +						 struct arm_smmu_cmd *cmd)
> +{
> +	if (!(smmu->options & ARM_SMMU_OPT_TLBI_TWICE))
> +		return false;
> +
> +	switch (FIELD_GET(CMDQ_0_OP, cmd->data[0])) {
> +	case CMDQ_OP_CFGI_STE:
> +	case CMDQ_OP_CFGI_ALL:
> +	case CMDQ_OP_CFGI_CD:
> +	case CMDQ_OP_CFGI_CD_ALL:
> +	case CMDQ_OP_TLBI_NH_ALL:
> +	case CMDQ_OP_TLBI_NH_ASID:
> +	case CMDQ_OP_TLBI_NH_VA:
> +	case CMDQ_OP_TLBI_NH_VAA:
> +	case CMDQ_OP_TLBI_EL2_ALL:
> +	case CMDQ_OP_TLBI_EL2_ASID:
> +	case CMDQ_OP_TLBI_EL2_VA:
> +	case CMDQ_OP_TLBI_S12_VMALL:
> +	case CMDQ_OP_TLBI_S2_IPA:
> +	case CMDQ_OP_TLBI_NSNH_ALL:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>   #ifdef CONFIG_ARM_SMMU_V3_SVA
>   bool arm_smmu_sva_supported(struct arm_smmu_device *smmu);
>   void arm_smmu_sva_notifier_synchronize(void);
> --------------------------------------------------------------------
>
> Nicolinc

Ack, I'll add force_sync patch by you as a preparatory patch and rebase my
changes on top of it and send it in V3.

Thanks,
Ashish Mhetre