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

[Pranjal Shrivastava <praan@google.com>]

On Fri, May 08, 2026 at 02:37:36PM -0300, Jason Gunthorpe wrote:
> 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:
> 

I know lol, I just meant why are we prefering  "_p" names. No strong 
feelings here though.

> > > +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()
> 

It's just that we're calling "arm_smmu_cmdq_batch_add_cmd_p" at one
place and using `arm_smmu_make_cmd_<cmd_name>` at the other. It makes
one think what's "_p" in issue_cmd, only to realize "_p: pointer variant

I guess I didn't like the new _p ones but I guess it's fine. Happy to
leave it at your discretion.

> > >  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.

Yea, just trying to call it out for a situation if this series gets
merged in 2 parts. Not suggesting this would happen but helps to know
for bisection etc too..

But I understand the point on keeping the intermediate diff clean.
Always learning :)

> 
> > > +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..
> 

Alright. We'll wait for someone to add it with STE_RANGE.

Praan