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

Nicolin Chen
2023-08-22T17:03:47Z

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@nvidia.com>
> > ---
> > 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);
> > }
> >