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

[Nicolin Chen]

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