Re: [PATCH v3 3/3] iommu/arm-smmu-v3: Issue CFGI/TLBI twice on Tegra264

[Ashish Mhetre]

On 6/3/2026 1:52 AM, Will Deacon wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, Jun 01, 2026 at 10:48:45AM +0000, Ashish Mhetre wrote:
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > index 1e9f7d2de344..78c96a2b652b 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > @@ -350,6 +350,26 @@ static int arm_vsmmu_convert_user_cmd(struct arm_vsmmu *vsmmu,
> >        return 0;
> >   }
> >
> > +/*
> > + * On Tegra264, arm_smmu_cmdq_issue_cmdlist() doubles every CFGI/TLBI
> > + * submission (see ARM_SMMU_OPT_TLBI_TWICE). The doubling decision is
> > + * taken once per cmdlist based on the first command, so a single
> > + * batch must not mix commands that need doubling with commands that
> > + * do not. Split the iommufd batch whenever the next user command
> > + * crosses that boundary.
> > + */
> Again, I wouldn't bother with this comment. You probably _should_ update
> Documentation/arch/arm64/silicon-errata.rst, however.

Ack, I'll remove this comment and add in Documentation.

> > +static bool arm_vsmmu_can_batch_cmd(struct arm_smmu_device *smmu,
> > +                                 struct arm_vsmmu_invalidation_cmd *last,
> > +                                 struct arm_vsmmu_invalidation_cmd *next)
> > +{
> > +     struct arm_smmu_cmd next_cmd = {
> > +             .data[0] = le64_to_cpu(next->ucmd.cmd[0]),
> > +     };
> > +
> > +     return arm_smmu_cmd_needs_tlbi_twice(smmu, &last->cmd) ==
> > +            arm_smmu_cmd_needs_tlbi_twice(smmu, &next_cmd);
> > +}
> > +
> >   int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu,
> >                               struct iommu_user_data_array *array)
> >   {
> > @@ -382,7 +402,8 @@ int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu,
> >
> >                /* FIXME work in blocks of CMDQ_BATCH_ENTRIES and copy each block? */
> >                cur++;
> > -             if (cur != end && (cur - last) != CMDQ_BATCH_ENTRIES - 1)
> > +             if (cur != end && (cur - last) != CMDQ_BATCH_ENTRIES - 1 &&
> > +                 arm_vsmmu_can_batch_cmd(smmu, last, cur))
> >                        continue;
> FYI: Sashiko is unhappy with the existing code here, so somebody should
> check that out:
>
> https://sashiko.dev/#/patchset/20260601104845.995005-2-amhetre@xxxxxxxxxx
>
> >                /* FIXME always uses the main cmdq rather than trying to group by type */
> > 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 08684bd40a6d..f38c21b56f28 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -698,10 +698,10 @@ static void arm_smmu_cmdq_write_entries(struct arm_smmu_cmdq *cmdq,
> >    *   insert their own list of commands then all of the commands from one
> >    *   CPU will appear before any of the commands from the other CPU.
> >    */
> > -int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> > -                             struct arm_smmu_cmdq *cmdq,
> > -                             struct arm_smmu_cmd *cmds, int n,
> > -                             bool sync)
> > +static int __arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> > +                                      struct arm_smmu_cmdq *cmdq,
> > +                                      struct arm_smmu_cmd *cmds, int n,
> > +                                      bool sync)
> >   {
> >        struct arm_smmu_cmd cmd_sync;
> >        u32 prod;
> > @@ -820,6 +820,26 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> >        return ret;
> >   }
> >
> > +int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> > +                             struct arm_smmu_cmdq *cmdq,
> > +                             struct arm_smmu_cmd *cmds, int n,
> > +                             bool sync)
> > +{
> > +     int ret = __arm_smmu_cmdq_issue_cmdlist(smmu, cmdq, cmds, n, sync);
> > +
> > +     /*
> > +      * On Tegra264 (see ARM_SMMU_OPT_TLBI_TWICE) re-issue the same
> > +      * cmdlist with another CMD_SYNC to satisfy the erratum.
> > +      * Callers must ensure the batch carries a uniform opcode class
> > +      * so that checking the first command is enough; the iommufd
> > +      * VSMMU path enforces this with arm_vsmmu_can_batch_cmd().
> > +      */
> > +     if (!ret && sync && arm_smmu_cmd_needs_tlbi_twice(smmu, &cmds[0]))
> Can you move the arm_smmu_cmd_... part to the start of the conjunction,
> please? If you make it a static key as I mentioned previously, then
> hopefully that should mean everything else is moved out of line.

Okay, I'll fix this in V4.

> > +             ret = __arm_smmu_cmdq_issue_cmdlist(smmu, cmdq, cmds, n, sync);
> Sashiko is also unhappy here if n == 0 because we probably shouldn't
> be inspecting the command array in that case. Generally, it's a pity
> that we can't handle this all a bit further up in the stack when we know
> exactly what operationg we're trying to perform, but I suppose with all
> the different users of the invalidation commands that's hard to catch in
> one place?
>
> > +
> > +     return ret;
> > +}
> > +
> >   static int arm_smmu_cmdq_issue_cmd_p(struct arm_smmu_device *smmu,
> >                                     struct arm_smmu_cmd *cmd, bool sync)
> >   {
> > @@ -863,6 +883,14 @@ static bool arm_smmu_cmdq_batch_force_sync(struct arm_smmu_device *smmu,
> >            (smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC))
> >                return true;
> >
> > +     /*
> > +      * Tegra264 erratum (see ARM_SMMU_OPT_TLBI_TWICE). The batch holds
> > +      * a uniform opcode class, so checking the first command is enough.
> > +      */
> Again, please drop the Tegra264 mention and just refer to the option.

Ack.

> Will