Re: [PATCH v11 9/9] iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF

[Nicolin Chen <nicolinc@nvidia.com>]

On Fri, Aug 16, 2024 at 11:15:31AM -0700, Nicolin Chen wrote:
> On Fri, Aug 16, 2024 at 10:34:24AM -0700, Nicolin Chen wrote:
> > On Fri, Aug 16, 2024 at 02:21:03PM +0100, Will Deacon wrote:
> > 
> > > >  static void arm_smmu_cmdq_batch_init(struct arm_smmu_device *smmu,
> > > > -                                  struct arm_smmu_cmdq_batch *cmds)
> > > > +                                  struct arm_smmu_cmdq_batch *cmds,
> > > > +                                  u8 opcode)
> > > >  {
> > > > +     WARN_ON_ONCE(!opcode);
> > > 
> > > This seems like a fairly arbitrary warning. Remove it?
> > 
> > OK.
> > 
> > > > +
> > > >       cmds->num = 0;
> > > > -     cmds->cmdq = arm_smmu_get_cmdq(smmu);
> > > > +     cmds->cmdq = arm_smmu_get_cmdq(smmu, opcode);
> > > 
> > > If we stashed the opcode here, we could actually just enforce that all
> > > commands in the batch are the same type in arm_smmu_cmdq_batch_add().
> > > 
> > > Would that work better for you or not?
> > 
> > A guested-owned queue is okay to mix different command types:
> > 	CMDQ_OP_TLBI_NH_ASID
> > 	CMDQ_OP_TLBI_NH_VA
> > 	CMDQ_OP_ATC_INV
> > 
> > So, limiting a batch to one single opcode isn't ideal. Instead,
> > if we really have to apply an enforcement to every batch_add(),
> > I think the cmdq structure would need a scan function pointer:
> > 
> > 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 d0d7c75c030a..1a83ad5ebadc 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -918,2 +918,10 @@ static void arm_smmu_cmdq_batch_init(struct arm_smmu_device *smmu,
> >  
> > +static bool arm_smmu_cmdq_supports_cmd(struct arm_smmu_cmdq *cmdq,
> > +				       struct arm_smmu_cmdq_ent *ent)
> > +{
> > +	if (!cmdq->supports_cmd)
> > +		return true;
> > +	return cmdq->supports_cmd(ent);
> > +}
> > +
> >  static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu,
> > @@ -924,4 +932,5 @@ static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu,
> >  
> > -	if (cmds->num == CMDQ_BATCH_ENTRIES - 1 &&
> > -	    (smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC)) {
> > +	if ((cmds->num == CMDQ_BATCH_ENTRIES - 1 &&
> > +	     (smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC)) ||
> > +	    !arm_smmu_cmdq_supports_cmd(cmds->cmdq, cmd)) {
> >  		arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmdq, cmds->cmds,
> 
> We'd need re-init the batch after this too..
> 
> Nicolin
> 
> > 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 e131d8170b90..c4872af6232c 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > @@ -616,2 +616,3 @@ struct arm_smmu_cmdq {
> >  	atomic_t			lock;
> > +	bool                            (*supports_cmd)(struct arm_smmu_cmdq_ent *ent);
> >  };
> > 
> > That being said, the whole thing doesn't seem to have a lot value
> > at this moment, since the SMMU driver doesn't mix commands?

OK. I have added a patch for this. Let's just make things a bit
perfect at once.

Here is a v13 branch that addressed most of your remarks here:
https://github.com/nicolinc/iommufd/commits/vcmdq_in_kernel-v13

Would you please let me know if you are okay with this?

Thank you
Nicolin