Messages in this thread | | | Date | Tue, 29 Aug 2023 16:03:43 -0700 | From | Nicolin Chen <> | Subject | Re: [PATCH 2/3] iommu/arm-smmu-v3: Add an arm_smmu_tlb_inv_domain helper |
| |
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
| |