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

Nicolin Chen
2023-08-29T23:04:42Z

On Tue, Aug 29, 2023 at 10:54:00PM +0100, Robin Murphy wrote:
> On 2023-08-22 18:03, Nicolin Chen wrote:
> > 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?
>
> Oh, this is becoming quite the lesson in not reviewing patches in a hurry :(
>
> Apparently I managed to misread the diff and the horribly subtle
> difference between "arm_smmu_tlb_inv_domain" and
> "arm_smmu_atc_inv_domain", and think that arm_smmu_tlb_inv_context() was
> already just dealing with the TLBI command and you were moving the
> entire body into the new helper. Sorry about that.
>
> Still, the part about the comment remains true, and I think it goes to
> show what a thoroughly horrible naming scheme it is to have "tlb_inv"
> denote a function responsible for TLBI commands and "atc_inv" denote a
> function responsible for ATC commands and "tlb_inv" denote a function
> responsible for both TLBI and ATC commands...

Well, "atc_inv" is quite clear I think. But the"tlb_inv" might
not be, as you pointed out.

So, we have:
tlb_inv_range_asid: tlbi only (NH_VA/EL2_VA) // used by SVA too
tlb_inv_range_domain:
if (S1)
tlb_inv_range_asid(); // NH_VA/EL2_VA
else
tlbi only (S2_IPA);
atc();
tlb_inv_asid: tlbi (NH_ASID) // only used by tlb_inv_context()
tlb_inv_context:
if (S1)
tlb_inv_asid(); // NH_ASID
else
tlbi only (S2_VMALL);
atc();

Then, what this patch wants another non-atc:
tlb_inv_asid: tlbi (NH_ASID) // only used by tlb_inv_domain()
tlb_inv_domain: // new
if (S1)
tlb_inv_asid(); // NH_ASID
else
tlbi only (S2_VMALL);
tlb_inv_context:
tlb_inv_domain();
atc();

The problem of this is that it conflicts with the naming used in
other tlb_inv_range_domain() that does an atc().

Perhaps, we could rename to the following patterns?
tlb_inv_range_asid: // used by SVA too
tlb_inv_range_domain:
if (S1)
return tlb_inv_range_asid();
else
tlbi only (S2_IPA)
tlb_inv_range_domain_with_atc:
tlb_inv_range_domain();
atc();

# remove tlb_inv_asid() since it doesn't help much
tlb_inv_domain:
if (S1)
tlbi only (NH_ASID)
else
tlbi only (S2_VMALL)
tlb_inv_domain_with_atc:
tlb_inv_domain();
atc();

tlb_inv_context:
struct arm_smmu_domain *smmu_domain =
(struct arm_smmu_domain *cookie);
tlb_inv_domain_with_atc(smmu_domain);

Thanks
Nicolin