powerpc/mm: Enable clang context analysis
Ritesh Harjani (IBM)
reviewercover powerpc/mm: Enable clang context analysis
Lock acquire and release checking used to be done with Sparse. That support was removed [1] and replaced with Clang context analysis [2].
Note that enabling this caught a real locking rcu bug in arch/powerpc/mm (Patch-1). Note, more invasive checkers to check what each lock protects is deferred for later enablement.
This is based on top of previous powerpc/powernv,pseries Clang context analysis enablement series [3].
The series was build tested with LLVM/Clang 23.x
[1] 5b63d0ae94cc ("compiler-context-analysis: Remove Sparse support")
[2] 3269701cb256 ("compiler-context-analysis: Add infrastructure for Context Analysis with Clang") [3]: https://lore.kernel.org/linuxppc-dev/[email protected]/
Ritesh Harjani (IBM) (6):
powerpc/mm/book3s64: unlock RCU before returning from mm_iommu_is_devmem() powerpc/mm/book3s64: Rework tlbie() to remove conditional locking powerpc/mm/book3s64: Rework native_flush_hash_range() to remove conditional locking powerpc/mm/book3s64: Annotate the subpage-protection PTE lock powerpc/mm: Rework nohash TLB broadcast locking
powerpc/mm: Enable context analysis
arch/powerpc/mm/Makefile | 2 + arch/powerpc/mm/book3s32/Makefile | 2 + arch/powerpc/mm/book3s64/Makefile | 2 + arch/powerpc/mm/book3s64/hash_native.c | 122 +++++++++++++++--------- arch/powerpc/mm/book3s64/iommu_api.c | 6 +- arch/powerpc/mm/book3s64/subpage_prot.c | 2 + arch/powerpc/mm/nohash/Makefile | 2 + arch/powerpc/mm/nohash/mmu_context.c | 1 + arch/powerpc/mm/nohash/tlb.c | 8 +- 9 files changed, 98 insertions(+), 49 deletions(-)
--
2.39.5
Ritesh Harjani (IBM)
reviewerRFC 1/6 powerpc/mm/book3s64: unlock RCU before returning from mm_iommu_is_devmem()
Clang's context analysis reports:
iommu_api.c:380:1: error: __ctx_lock_RCU 'RCU' is not held on every path through here [-Werror,-Wthread-safety-analysis]
mm_iommu_is_devmem() takes the RCU read lock to walk the preregistered device-memory list. On a match it returned true from inside the loop and skipped rcu_read_unlock(). Hence fix it.
--- arch/powerpc/mm/book3s64/iommu_api.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
arch/powerpc/mm/book3s64/iommu_api.c +4 -2
@@ -356,6 +356,7 @@ bool mm_iommu_is_devmem(struct mm_struct *mm, unsigned long hpa, {struct mm_iommu_table_group_mem_t *mem;unsigned long end; +bool ret = false;rcu_read_lock ();list_for_each_entry_rcu (mem, &mm->context.iommu_group_mem_list, next) {
@@ -371,12 +372,13 @@ bool mm_iommu_is_devmem(struct mm_struct *mm, unsigned long hpa,* and the caller needs to distinguish this situation. */ *size =min (1UL << pageshift, end - hpa); -return true; + ret = true; +break ; } }rcu_read_unlock (); -return false; +return ret; }EXPORT_SYMBOL_GPL (mm_iommu_is_devmem);
Signature · 1 line
2.39.5
Ritesh Harjani (IBM)
reviewerRFC 2/6 powerpc/mm/book3s64: Rework tlbie() to remove conditional locking
In reply to Ritesh Harjani (IBM)Clang's context analysis reports:
hash_native.c:235:2: error: raw_spinlock 'native_tlbie_lock' is not held on every path through here [-Werror,-Wthread-safety-analysis] hash_native.c:245:3: error: releasing raw_spinlock 'native_tlbie_lock' that was not held [-Werror,-Wthread-safety-analysis]
This patch reworks the tlbie() function to remove the conditional locking. It fixes the clang context analysis reported errors. However, this also simplifies the code around tlbie[l] - which is also a win.
No functional change intended.
--- arch/powerpc/mm/book3s64/hash_native.c | 34 ++++++++++++++++++-------- 1 file changed, 24 insertions(+), 10 deletions(-)
arch/powerpc/mm/book3s64/hash_native.c +24 -10
@@ -222,6 +222,23 @@ static inline void __tlbiel(unsigned long vpn, int psize, int apsize, int ssize) } +static inline void __tlbie_sequence (unsigned long vpn,int psize,int apsize, +int ssize) +{ + asmvolatile ("ptesync" : : :"memory" ); +__tlbie (vpn, psize, apsize, ssize); +fixup_tlbie_vpn (vpn, psize, apsize, ssize); + asmvolatile ("eieio; tlbsync; ptesync" : : :"memory" ); +} + +static inline void __tlbiel_sequence (unsigned long vpn,int psize,int apsize, +int ssize) +{ + asmvolatile ("ptesync" : : :"memory" ); +__tlbiel (vpn, psize, apsize, ssize); +ppc_after_tlbiel_barrier (); +} +static inline void tlbie (unsigned long vpn,int psize,int apsize,int ssize,int local) {
@@ -230,19 +247,16 @@ static inline void tlbie(unsigned long vpn, int psize, int apsize,if (use_local) use_local = mmu_psize_defs[psize].tlbiel; -if (lock_tlbie && !use_local) -raw_spin_lock (&native_tlbie_lock); - asmvolatile ("ptesync" : : :"memory" ); +if (use_local) { -__tlbiel (vpn, psize, apsize, ssize); -ppc_after_tlbiel_barrier (); +__tlbiel_sequence (vpn, psize, apsize, ssize); + }else if (lock_tlbie) { +raw_spin_lock (&native_tlbie_lock); +__tlbie_sequence (vpn, psize, apsize, ssize); +raw_spin_unlock (&native_tlbie_lock); }else { -__tlbie (vpn, psize, apsize, ssize); -fixup_tlbie_vpn (vpn, psize, apsize, ssize); - asmvolatile ("eieio; tlbsync; ptesync" : : :"memory" ); +__tlbie_sequence (vpn, psize, apsize, ssize); } -if (lock_tlbie && !use_local) -raw_spin_unlock (&native_tlbie_lock); }static inline void native_lock_hpte (struct hash_pte *hptep)
Signature · 1 line
2.39.5
Ritesh Harjani (IBM)
reviewerRFC 3/6 powerpc/mm/book3s64: Rework native_flush_hash_range() to remove conditional locking
In reply to Ritesh Harjani (IBM)Clang's context analysis reports:
hash_native.c:851:2: error: raw_spinlock 'native_tlbie_lock' is not held on every path through here [-Werror,-Wthread-safety-analysis] hash_native.c:868:3: error: releasing raw_spinlock 'native_tlbie_lock' that was not held [-Werror,-Wthread-safety-analysis]
This patch reworks the native_flush_hash_range() function to remove the conditional locking. It fixes the clang context analysis reported errors. However, this also simplifies the code in native_flush_hash_range() due to various if conditions.
No functional change intended.
--- arch/powerpc/mm/book3s64/hash_native.c | 88 ++++++++++++++++---------- 1 file changed, 54 insertions(+), 34 deletions(-)
arch/powerpc/mm/book3s64/hash_native.c +54 -34
@@ -792,6 +792,54 @@ static notrace void native_hpte_clear(void) asmvolatile ("eieio; tlbsync; ptesync" :::"memory" ); } +static void native_flush_hash_range_tlbiel (struct ppc64_tlb_batch *batch, +unsigned long number) +{ +unsigned long vpn =0 , index, shift; +real_pte_t pte; +unsigned long psize = batch->psize; +int ssize = batch->ssize; +unsigned long i; + + asmvolatile ("ptesync" :::"memory" ); +for (i =0 ; i < number; i++) { + vpn = batch->vpn[i]; + pte = batch->pte[i]; + +pte_iterate_hashed_subpages (pte, psize, + vpn, index, shift) { +__tlbiel (vpn, psize, psize, ssize); + }pte_iterate_hashed_end (); + } +ppc_after_tlbiel_barrier (); +} + +static void native_flush_hash_range_tlbie (struct ppc64_tlb_batch *batch, +unsigned long number) +{ +unsigned long vpn =0 , index, shift; +real_pte_t pte; +unsigned long psize = batch->psize; +int ssize = batch->ssize; +unsigned long i; + + asmvolatile ("ptesync" :::"memory" ); +for (i =0 ; i < number; i++) { + vpn = batch->vpn[i]; + pte = batch->pte[i]; + +pte_iterate_hashed_subpages (pte, psize, + vpn, index, shift) { +__tlbie (vpn, psize, psize, ssize); + }pte_iterate_hashed_end (); + } +/* +* Just do one more with the last used values. +*/ +fixup_tlbie_vpn (vpn, psize, psize, ssize); + asmvolatile ("eieio; tlbsync; ptesync" :::"memory" ); +} +/* * Batched hash table flush, we batch the tlbie's to avoid taking/releasing * the lock all the time
@@ -845,41 +893,13 @@ static void native_flush_hash_range(unsigned long number, int local)if (mmu_has_feature (MMU_FTR_TLBIEL ) && mmu_psize_defs[psize].tlbiel && local) { - asmvolatile ("ptesync" :::"memory" ); -for (i =0 ; i < number; i++) { - vpn = batch->vpn[i]; - pte = batch->pte[i]; - -pte_iterate_hashed_subpages (pte, psize, - vpn, index, shift) { -__tlbiel (vpn, psize, psize, ssize); - }pte_iterate_hashed_end (); - } -ppc_after_tlbiel_barrier (); +native_flush_hash_range_tlbiel (batch, number); + }else if (!mmu_has_feature (MMU_FTR_LOCKLESS_TLBIE )) { +raw_spin_lock (&native_tlbie_lock); +native_flush_hash_range_tlbie (batch, number); +raw_spin_unlock (&native_tlbie_lock); }else { -int lock_tlbie = !mmu_has_feature (MMU_FTR_LOCKLESS_TLBIE ); - -if (lock_tlbie) -raw_spin_lock (&native_tlbie_lock); - - asmvolatile ("ptesync" :::"memory" ); -for (i =0 ; i < number; i++) { - vpn = batch->vpn[i]; - pte = batch->pte[i]; - -pte_iterate_hashed_subpages (pte, psize, - vpn, index, shift) { -__tlbie (vpn, psize, psize, ssize); - }pte_iterate_hashed_end (); - } -/* -* Just do one more with the last used values. -*/ -fixup_tlbie_vpn (vpn, psize, psize, ssize); - asmvolatile ("eieio; tlbsync; ptesync" :::"memory" ); - -if (lock_tlbie) -raw_spin_unlock (&native_tlbie_lock); +native_flush_hash_range_tlbie (batch, number); }local_irq_restore (flags);
Signature · 1 line
2.39.5
Ritesh Harjani (IBM)
reviewerRFC 4/6 powerpc/mm/book3s64: Annotate the subpage-protection PTE lock
In reply to Ritesh Harjani (IBM)Clang's context analysis reports:
subpage_prot.c:83:2: error: releasing spinlock 'ptl' that was not held [-Werror,-Wthread-safety-analysis]
pte_offset_map_lock() holds ptl when it returns a PTE and pte_unmap_unlock() drops it. However since the lock is taken inside the helper, which is not annotated, so the unlock in hpte_flush_range() looks unheld.
This patch fixes it in hpte_flush_range() by adding the needed annotation here. IMO, should ideally be added to pte_offset_map_lock(), once that happens (since it might need larger enablement), we can remove it from here.
--- arch/powerpc/mm/book3s64/subpage_prot.c | 2 ++ 1 file changed, 2 insertions(+)
arch/powerpc/mm/book3s64/subpage_prot.c +2 -0
@@ -73,6 +73,8 @@ static void hpte_flush_range(struct mm_struct *mm, unsigned long addr, pte =pte_offset_map_lock (mm, pmd, addr, &ptl);if (!pte)return ; +/* pte_offset_map_lock on success returns with ptl lock held */ +__acquire (ptl);lazy_mmu_mode_enable ();for (; npages >0 ; --npages) {pte_update (mm, addr, pte,0 ,0 ,0 );
--
2.39.5
Ritesh Harjani (IBM)
reviewerRFC 5/6 powerpc/mm: Rework nohash TLB broadcast locking
In reply to Ritesh Harjani (IBM)Clang's context analysis reports:
mmu_context.c:141:2: error: releasing raw_spinlock 'context_lock' that was not held [-Werror,-Wthread-safety-analysis] mmu_context.c:147:1: error: raw_spinlock 'context_lock' is not held on every path through here [-Werror,-Wthread-safety-analysis]
tlb.c:257:4: error: raw_spinlock 'tlbivax_lock' is not held on every path through here [-Werror,-Wthread-safety-analysis] tlb.c:259:5: error: releasing raw_spinlock 'tlbivax_lock' that was not held [-Werror,-Wthread-safety-analysis]
This reworks the locking in __flush_tlb_page() a bit to satisfy clang context analyzer requirement.
Also steal_context_smp() drops the context_lock, so it needs to be annotated with __must_hold to inform the clang analyzer that this function holds the context_lock.
--- arch/powerpc/mm/nohash/mmu_context.c | 1 + arch/powerpc/mm/nohash/tlb.c | 8 +++++--- 2 files changed, 6 insertions(+), 3 deletions(-)
arch/powerpc/mm/nohash/mmu_context.c +1 -0
@@ -95,6 +95,7 @@ static DEFINE_RAW_SPINLOCK(context_lock);* -- benh */ static unsigned int steal_context_smp (unsigned int id) + __must_hold(&context_lock) {struct mm_struct *mm;unsigned int cpu, max, i;
arch/powerpc/mm/nohash/tlb.c +5 -3
@@ -252,11 +252,13 @@ void __flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr,/* If broadcast tlbivax is supported, use it */ if (mmu_has_feature (MMU_FTR_USE_TLBIVAX_BCAST )) {int lock =mmu_has_feature (MMU_FTR_LOCK_BCAST_INVAL ); -if (lock) +if (lock) {raw_spin_lock (&tlbivax_lock); -_tlbivax_bcast (vmaddr, pid, tsize, ind); -if (lock) +_tlbivax_bcast (vmaddr, pid, tsize, ind);raw_spin_unlock (&tlbivax_lock); + }else { +_tlbivax_bcast (vmaddr, pid, tsize, ind); + } goto bail; }else {struct tlb_flush_param p = {
Signature · 1 line
2.39.5
Ritesh Harjani (IBM)
reviewerRFC 6/6 powerpc/mm: Enable context analysis
In reply to Ritesh Harjani (IBM)powerpc/mm/ objects now build with Clang context analysis enabled. Turn it on for book3s32, book3s64 and nohash mmu.
--- arch/powerpc/mm/Makefile | 2 ++ arch/powerpc/mm/book3s32/Makefile | 2 ++ arch/powerpc/mm/book3s64/Makefile | 2 ++ arch/powerpc/mm/nohash/Makefile | 2 ++ 4 files changed, 8 insertions(+)
arch/powerpc/mm/Makefile +2 -0
@@ -3,6 +3,8 @@ # Makefile for the linux ppc-specific parts of the memory manager. # +CONTEXT_ANALYSIS := y + obj-y := fault.o mem.o pgtable.o maccess.o pageattr.o \ init_$(BITS).o pgtable_$(BITS).o \ pgtable-frag.o ioremap.o ioremap_$(BITS).o \
arch/powerpc/mm/book3s32/Makefile +2 -0
@@ -1,5 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 +CONTEXT_ANALYSIS := y + KASAN_SANITIZE_mmu.o := n ifdef CONFIG_KASAN
arch/powerpc/mm/book3s64/Makefile +2 -0
@@ -1,5 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 +CONTEXT_ANALYSIS := y + obj-y += mmu_context.o pgtable.o trace.o ifdef CONFIG_PPC_64S_HASH_MMU CFLAGS_REMOVE_slb.o = $(CC_FLAGS_FTRACE)
arch/powerpc/mm/nohash/Makefile +2 -0
@@ -1,5 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 +CONTEXT_ANALYSIS := y + obj-y += mmu_context.o tlb.o tlb_low.o kup.o obj-$(CONFIG_PPC_BOOK3E_64) += tlb_64e.o tlb_low_64e.o book3e_pgtable.o obj-$(CONFIG_44x) += 44x.o
--
2.39.5