Re: [PATCH 2/3] iommu/arm-smmu-v3: Add an arm_smmu_tlb_inv_domain helper

[Nicolin Chen]

On Tue, Aug 22, 2023 at 10:40:18AM +0100, Robin Murphy wrote:
 
> On 2023-08-22 09:45, Nicolin Chen wrote:
> > Move the part of per-asid or per-vmid invalidation command issuing into a
> > new helper function, which will be used in the following change.
> 
> Why? This achieves nothing except make the code harder to follow and
> disconnect the rather important comment even further from the code it is

We need the same if-else routine to issue a per-asid or per-vmid
TLBI command. If making a copy of this same routine feels better
to you, yea, I can change that.

> significant to. It's not like we need a specific prototype to take a
> function pointer from, it's just another internal call - see
> arm_smmu_flush_iotlb_all() for instance. We know the cookie is an
> arm_smmu_domain pointer because we put it there, and converting it back
> from a void pointer is exactly the same *at* the function call boundary
> as immediately afterwards.

Hmm, I am not quite following this. What do you suggest here?

Thanks
Nic

> > Signed-off-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> > ---
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 24 +++++++++++++--------
> >   1 file changed, 15 insertions(+), 9 deletions(-)
> > 
> > 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 9b0dc3505601..d6c647e1eb01 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -1854,12 +1854,24 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,
> >       return arm_smmu_cmdq_batch_submit(smmu_domain->smmu, &cmds);
> >   }
> > 
> > +static void arm_smmu_tlb_inv_domain(struct arm_smmu_domain *smmu_domain)
> > +{
> > +     struct arm_smmu_device *smmu = smmu_domain->smmu;
> > +     struct arm_smmu_cmdq_ent cmd;
> > +
> > +     if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
> > +             arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg.cd.asid);
> > +     } else {
> > +             cmd.opcode      = CMDQ_OP_TLBI_S12_VMALL;
> > +             cmd.tlbi.vmid   = smmu_domain->s2_cfg.vmid;
> > +             arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
> > +     }
> > +}
> > +
> >   /* IO_PGTABLE API */
> >   static void arm_smmu_tlb_inv_context(void *cookie)
> >   {
> >       struct arm_smmu_domain *smmu_domain = cookie;
> > -     struct arm_smmu_device *smmu = smmu_domain->smmu;
> > -     struct arm_smmu_cmdq_ent cmd;
> > 
> >       /*
> >        * NOTE: when io-pgtable is in non-strict mode, we may get here with
> > @@ -1868,13 +1880,7 @@ static void arm_smmu_tlb_inv_context(void *cookie)
> >        * insertion to guarantee those are observed before the TLBI. Do be
> >        * careful, 007.
> >        */
> > -     if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
> > -             arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg.cd.asid);
> > -     } else {
> > -             cmd.opcode      = CMDQ_OP_TLBI_S12_VMALL;
> > -             cmd.tlbi.vmid   = smmu_domain->s2_cfg.vmid;
> > -             arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
> > -     }
> > +     arm_smmu_tlb_inv_domain(smmu_domain);
> >       arm_smmu_atc_inv_domain(smmu_domain, 0, 0, 0);
> >   }
> >