[PATCH] powerpc/tlb: Implement book3s/32/tlbflush.h local_flush_tlb_page_psize

[Christophe Leroy]

Le 31/01/2023 à 03:58, Benjamin Gray a écrit :
> The commit introducing this function implemented it as a build bug on this
> platform to make the compiler happy, as the only use in the code is guarded
> behind a radix_enabled() conditional.
> 
> GCC recognises that cpu_has_feature(MMU_FTR_TYPE_RADIX) returns false on this
> platform and eliminates the build bug as dead code. However, when
> CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG is enabled, the assertion and possible
> call to early_cpu_... prevents Clang from eliminating any code that's
> conditional on the return value. So Clang triggers the build bug as reported
> by the kernel test robot:

I still think it is not the correct fix. You are putting the problem 
under the carpet instead of fixing it. There are many other places where 
radix_enabled() or other mmu_has_feature() are used with the expectation 
that one leg will be eliminated at build time.

As written in previous thread, have you considered reworking 
mmu_has_feature() to move the CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG 
after the below block:

	if (MMU_FTRS_ALWAYS & feature)
		return true;

	if (!(MMU_FTRS_POSSIBLE & feature))
		return false;


And as this looks like a Clang bug or limitation, can you provide us 
with a link to the Clang issue you have opened for it ?


Looking into it in more details, I'm even more puzzled. As far as I can 
see, local_flush_tlb_page_psize() is used only at one place, that is 
function __do_patch_instruction_mm(). So if Clang fails to identify it 
as a dead leg, it is the full __do_patch_instruction_mm() which is kept 
for no reason.

On the other hand, I see that local_flush_tlb_page_psize() implemented 
for nohash/32, so yes we can also implement it for book3s/32. But then 
the commit log should explain things differently.

By the way, I also see that local_flush_tlb_page_psize() for book3s/64 
does just nothing at all for non Radix. Is that correct ?

Thanks
Christophe


> 
> https://lore.kernel.org/llvm/202301212348.eDkowvfF-lkp@intel.com
> 
> Fixes: 274d842fa1ef ("powerpc/tlb: Add local flush for page given mm_struct and psize")
> Reported-by: kernel test robot <lkp at intel.com>
> Signed-off-by: Benjamin Gray <bgray at linux.ibm.com>
> ---
> 
> Supersedes https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20230124215424.9068-1-bgray@linux.ibm.com/
> 
> ---
>   arch/powerpc/include/asm/book3s/32/tlbflush.h | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/book3s/32/tlbflush.h b/arch/powerpc/include/asm/book3s/32/tlbflush.h
> index 4be572908124..cde3b6f5d563 100644
> --- a/arch/powerpc/include/asm/book3s/32/tlbflush.h
> +++ b/arch/powerpc/include/asm/book3s/32/tlbflush.h
> @@ -2,8 +2,6 @@
>   #ifndef _ASM_POWERPC_BOOK3S_32_TLBFLUSH_H
>   #define _ASM_POWERPC_BOOK3S_32_TLBFLUSH_H
> 
> -#include <linux/build_bug.h>
> -
>   #define MMU_NO_CONTEXT      (0)
>   /*
>    * TLB flushing for "classic" hash-MMU 32-bit CPUs, 6xx, 7xx, 7xxx
> @@ -80,7 +78,7 @@ static inline void local_flush_tlb_page(struct vm_area_struct *vma,
>   static inline void local_flush_tlb_page_psize(struct mm_struct *mm,
>   					      unsigned long vmaddr, int psize)
>   {
> -	BUILD_BUG();
> +	flush_range(mm, vmaddr, vmaddr + PAGE_SIZE);
>   }
> 
>   static inline void local_flush_tlb_mm(struct mm_struct *mm)
> 
> base-commit: ca272751ba18ca8f137af631cbc9f3f987fab6e3
> --
> 2.39.1