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
