[PATCH] powerpc/tlb: Remove BUILD_BUG for book3s/32/tlbflush.h local_flush_tlb_page_psize

[Christophe Leroy]

Le 26/01/2023 à 23:30, Benjamin Gray a écrit :
> On Wed, 2023-01-25 at 09:43 +0000, Christophe Leroy wrote:
> 
>> By the way, are you should the problem is really BUILD_BUG() ?
>> Looking
>> at your patch I would think that the problem is because it is "static
>> inline". Have you tried 'static __always_inline' instead ?
> 
> I did not try it, so I just did but it doesn't make a difference.
> 
> Looking further, the failing config also enabled
> CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG, which causes the
> mmu_has_feature(MMU_FTR_TYPE_RADIX) call of radix_enabled() to be non-
> trivial. It must check static_key_initialized, and falls back to
> early_mmu_has_feature if it triggers. Clang apparently can't see that
> early_mmu_has_feature will also always return false for Radix, so
> doesn't see that everything guarded by radix_enabled() is dead code. I
> suppose it's complicated by the fact it still has to run
> mmu_has_feature for the assertion side effect despite the return value
> being knowable at compile time.
> 
> So because of this weird interaction, the following should (and does)
> also prevent the compilation error by making the radix_enabled() return
> value more obvious to the compiler in the case where Radix is not
> implemented. (I've put the constant second here so the early usage
> assertion still runs).

But then, that's probably not the only place where we may get an issue 
with radix_enabled() or any other use of mmu_has_feature() by the way.

We are in a trivial situation where the feature check is either always 
true or always false. Is it worth checking for jump label init in that 
case ?

I think the best solution should be to move the following trivial checks 
above the static_key_initialised check:

	if (MMU_FTRS_ALWAYS & feature)
		return true;

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

Christophe

> 
> diff --git a/arch/powerpc/include/asm/mmu.h
> b/arch/powerpc/include/asm/mmu.h
> index 94b981152667..3592ff9a522b 100644
> --- a/arch/powerpc/include/asm/mmu.h
> +++ b/arch/powerpc/include/asm/mmu.h
> @@ -327,7 +327,7 @@ static inline void assert_pte_locked(struct
> mm_struct *mm, unsigned long addr)
>   
>   static __always_inline bool radix_enabled(void)
>   {
> -       return mmu_has_feature(MMU_FTR_TYPE_RADIX);
> +       return mmu_has_feature(MMU_FTR_TYPE_RADIX) &&
> IS_ENABLED(CONFIG_PPC_RADIX_MMU);
>   }
>   
>   static __always_inline bool early_radix_enabled(void)