Re: [PATCH 6/9] iommu/arm-smmu-v3: Directly encode simple commands

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, May 08, 2026 at 11:33:32AM +0000, Pranjal Shrivastava wrote:

> > -static int __arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,
> > -				     struct arm_smmu_cmd *cmd,
> > -				     bool sync)
> > +static int arm_smmu_cmdq_issue_cmd_p(struct arm_smmu_device *smmu,
> > +				     struct arm_smmu_cmd *cmd, bool sync)
> 
> Nit: I'm not sure why we need to rename this? We can still define the 
> rest of the helpers like:

I made it have the same naming system as this:

> > +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)
> 
> Nit: Same here, why not  __arm_smmu_cmdq_batch_add_cmd? I understand
> that _p just means we'll aceept ptr.. but the name's kinda wonky.

Which becomes a fairly widly used public entry point, so I didn't want
to have the __

Though there is no external user of arm_smmu_cmdq_issue_cmd_p()

> >  static irqreturn_t arm_smmu_priq_thread(int irq, void *dev)
> > @@ -3464,7 +3405,7 @@ static void arm_smmu_inv_flush_iotlb_tag(struct arm_smmu_inv *inv)
> >  
> >  	cmd.opcode = inv->nsize_opcode;
> >  	arm_smmu_cmdq_build_cmd(&hw_cmd, &cmd);
> > -	arm_smmu_cmdq_issue_cmd_with_sync(inv->smmu, &hw_cmd);
> > +	arm_smmu_cmdq_issue_cmd_with_sync(inv->smmu, hw_cmd);
> 
> Nit: are we passing it by value here? This would be a 16-byte stack
> copy? As with the macro expansion this looks like:
> 
> {
> 	struct arm_smmu_cmd __cmd = hw_cmd; // <-- Redundant 16-byte copy
> 	arm_smmu_cmdq_issue_cmd_p(inv->smmu, &__cmd, true);
> }
> 
> Why not use arm_smmu_cmdq_issue_cmd_p(inv->smmu, &hw_cmd, true) ?
> Although, I see this is eventually cleaned up in Patch 9.

Because it is eventually cleaned up in patch 9 :) The point was not to
change this logic in this patch.

> > +static inline struct arm_smmu_cmd arm_smmu_make_cmd_cfgi_all(void)
> > +{
> > +	struct arm_smmu_cmd cmd = arm_smmu_make_cmd_op(CMDQ_OP_CFGI_ALL);
> > +
> > +	cmd.data[1] |= FIELD_PREP(CMDQ_CFGI_1_RANGE, 31);
> 
> Maybe this is a good opportunity to define "31"? We already have a 
> similar definition for TLBI: #define CMDQ_TLBI_RANGE_NUM_MAX 31

I went with how the spec was written. The CMD_CFGI_ALL has its own section
with a direct encoding of 31 in that position, no field name.

While CMD_CFGI_STE_RANGE has the same op code and names that spot
"range" and it would be a NUM_MAX, we don't use STE_RANGE..

I'm inclined to leave it for someone who adds STE_RANGE..

Jason