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?
>
> Thanks
> Nicolin
