Re: [PATCH 2/9] iommu/arm-smmu-v3: Use the HW arm_smmu_cmd in cmdq selection functions

[Mostafa Saleh <smostafa@google.com>]

On Fri, May 01, 2026 at 11:29:11AM -0300, Jason Gunthorpe wrote:
> Start removing the use of struct arm_smmu_cmdq_ent, starting at the
> lower levels of the call chain. Change the functions that determine what
> cmdq to issue the batch to into using struct arm_smmu_cmd directly.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 47 ++++++++++++-------
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  8 ++--
>  .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    |  8 ++--
>  3 files changed, 38 insertions(+), 25 deletions(-)
> 
> 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 e8d7dbe495f030..5cdeaec890592f 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -371,12 +371,12 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
>  }
>  
>  static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu,
> -					       struct arm_smmu_cmdq_ent *ent)
> +					       struct arm_smmu_cmd *cmd)
>  {
>  	struct arm_smmu_cmdq *cmdq = NULL;
>  
>  	if (smmu->impl_ops && smmu->impl_ops->get_secondary_cmdq)
> -		cmdq = smmu->impl_ops->get_secondary_cmdq(smmu, ent);
> +		cmdq = smmu->impl_ops->get_secondary_cmdq(smmu, cmd);
>  
>  	return cmdq ?: &smmu->cmdq;
>  }
> @@ -924,16 +924,16 @@ static int __arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,
>  				     struct arm_smmu_cmdq_ent *ent,
>  				     bool sync)
>  {
> -	u64 cmd[CMDQ_ENT_DWORDS];
> +	struct arm_smmu_cmd cmd;
>  
> -	if (unlikely(arm_smmu_cmdq_build_cmd(cmd, ent))) {
> +	if (unlikely(arm_smmu_cmdq_build_cmd(cmd.data, ent))) {
>  		dev_warn(smmu->dev, "ignoring unknown CMDQ opcode 0x%x\n",
>  			 ent->opcode);
>  		return -EINVAL;
>  	}
>  
>  	return arm_smmu_cmdq_issue_cmdlist(
> -		smmu, arm_smmu_get_cmdq(smmu, ent), cmd, 1, sync);
> +		smmu, arm_smmu_get_cmdq(smmu, &cmd), cmd.data, 1, sync);
>  }
>  
>  static int arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,
> @@ -948,42 +948,55 @@ static int arm_smmu_cmdq_issue_cmd_with_sync(struct arm_smmu_device *smmu,
>  	return __arm_smmu_cmdq_issue_cmd(smmu, ent, true);
>  }
>  
> +static void arm_smmu_cmdq_batch_init_cmd(struct arm_smmu_device *smmu,
> +					 struct arm_smmu_cmdq_batch *cmds,
> +					 struct arm_smmu_cmd *cmd)
> +{
> +	cmds->num = 0;
> +	cmds->cmdq = arm_smmu_get_cmdq(smmu, cmd);
> +}
> +
>  static void arm_smmu_cmdq_batch_init(struct arm_smmu_device *smmu,
>  				     struct arm_smmu_cmdq_batch *cmds,
>  				     struct arm_smmu_cmdq_ent *ent)
>  {
> -	cmds->num = 0;
> -	cmds->cmdq = arm_smmu_get_cmdq(smmu, ent);
> +	struct arm_smmu_cmd cmd;
> +
> +	arm_smmu_cmdq_build_cmd(cmd.data, ent);
> +	arm_smmu_cmdq_batch_init_cmd(smmu, cmds, &cmd);
>  }
>  
>  static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu,
>  				    struct arm_smmu_cmdq_batch *cmds,
> -				    struct arm_smmu_cmdq_ent *cmd)
> +				    struct arm_smmu_cmdq_ent *ent)
>  {
> -	bool unsupported_cmd = !arm_smmu_cmdq_supports_cmd(cmds->cmdq, cmd);
>  	bool force_sync = (cmds->num == CMDQ_BATCH_ENTRIES - 1) &&
>  			  (smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC);
> +	struct arm_smmu_cmd cmd;
> +	bool unsupported_cmd;
>  	int index;
>  
> +	if (unlikely(arm_smmu_cmdq_build_cmd(cmd.data, ent))) {
> +		dev_warn(smmu->dev, "ignoring unknown CMDQ opcode 0x%x\n",
> +			 ent->opcode);
> +		return;
> +	}
> +
> +	unsupported_cmd = !arm_smmu_cmdq_supports_cmd(cmds->cmdq, &cmd);
>  	if (force_sync || unsupported_cmd) {
>  		arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmdq, cmds->cmds,
>  					    cmds->num, true);
> -		arm_smmu_cmdq_batch_init(smmu, cmds, cmd);
> +		arm_smmu_cmdq_batch_init_cmd(smmu, cmds, &cmd);
>  	}
>  
>  	if (cmds->num == CMDQ_BATCH_ENTRIES) {
>  		arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmdq, cmds->cmds,
>  					    cmds->num, false);
> -		arm_smmu_cmdq_batch_init(smmu, cmds, cmd);
> +		arm_smmu_cmdq_batch_init_cmd(smmu, cmds, &cmd);
>  	}
>  
>  	index = cmds->num * CMDQ_ENT_DWORDS;
> -	if (unlikely(arm_smmu_cmdq_build_cmd(&cmds->cmds[index], cmd))) {
> -		dev_warn(smmu->dev, "ignoring unknown CMDQ opcode 0x%x\n",
> -			 cmd->opcode);
> -		return;
> -	}
> -
> +	memcpy(&cmds->cmds[index], cmd.data, sizeof(cmd.data));

Maybe this would be better squashed with other arm_smmu_cmdq_batch
patch to avoid this memcpy, but no strong opinion.

Reviewed-by: Mostafa Saleh <smostafa@google.com>

Thanks,
Mostafa
>  	cmds->num++;
>  }
>  
> 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 092179f689e9f1..6d73f6b63e64a9 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -641,13 +641,13 @@ struct arm_smmu_cmdq {
>  	atomic_long_t			*valid_map;
>  	atomic_t			owner_prod;
>  	atomic_t			lock;
> -	bool				(*supports_cmd)(struct arm_smmu_cmdq_ent *ent);
> +	bool				(*supports_cmd)(struct arm_smmu_cmd *cmd);
>  };
>  
>  static inline bool arm_smmu_cmdq_supports_cmd(struct arm_smmu_cmdq *cmdq,
> -					      struct arm_smmu_cmdq_ent *ent)
> +					      struct arm_smmu_cmd *cmd)
>  {
> -	return cmdq->supports_cmd ? cmdq->supports_cmd(ent) : true;
> +	return cmdq->supports_cmd ? cmdq->supports_cmd(cmd) : true;
>  }
>  
>  struct arm_smmu_cmdq_batch {
> @@ -815,7 +815,7 @@ struct arm_smmu_impl_ops {
>  	void (*device_remove)(struct arm_smmu_device *smmu);
>  	int (*init_structures)(struct arm_smmu_device *smmu);
>  	struct arm_smmu_cmdq *(*get_secondary_cmdq)(
> -		struct arm_smmu_device *smmu, struct arm_smmu_cmdq_ent *ent);
> +		struct arm_smmu_device *smmu, struct arm_smmu_cmd *cmd);
>  	/*
>  	 * An implementation should define its own type other than the default
>  	 * IOMMU_HW_INFO_TYPE_ARM_SMMUV3. And it must validate the input @type
> diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> index 83f6e9f6c51d6b..b4d8c1f2fd3878 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> @@ -367,9 +367,9 @@ static irqreturn_t tegra241_cmdqv_isr(int irq, void *devid)
>  
>  /* Command Queue Function */
>  
> -static bool tegra241_guest_vcmdq_supports_cmd(struct arm_smmu_cmdq_ent *ent)
> +static bool tegra241_guest_vcmdq_supports_cmd(struct arm_smmu_cmd *cmd)
>  {
> -	switch (ent->opcode) {
> +	switch (FIELD_GET(CMDQ_0_OP, cmd->data[0])) {
>  	case CMDQ_OP_TLBI_NH_ASID:
>  	case CMDQ_OP_TLBI_NH_VA:
>  	case CMDQ_OP_ATC_INV:
> @@ -381,7 +381,7 @@ static bool tegra241_guest_vcmdq_supports_cmd(struct arm_smmu_cmdq_ent *ent)
>  
>  static struct arm_smmu_cmdq *
>  tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu,
> -			struct arm_smmu_cmdq_ent *ent)
> +			struct arm_smmu_cmd *cmd)
>  {
>  	struct tegra241_cmdqv *cmdqv =
>  		container_of(smmu, struct tegra241_cmdqv, smmu);
> @@ -409,7 +409,7 @@ tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu,
>  		return NULL;
>  
>  	/* Unsupported CMD goes for smmu->cmdq pathway */
> -	if (!arm_smmu_cmdq_supports_cmd(&vcmdq->cmdq, ent))
> +	if (!arm_smmu_cmdq_supports_cmd(&vcmdq->cmdq, cmd))
>  		return NULL;
>  	return &vcmdq->cmdq;
>  }
> -- 
> 2.43.0
>