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

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

[Benjamin Gray]

Converts the BUILD_BUG to a WARN to allow building with a low/unoptimised
compiler.

The original expectation was that a compiler would see that the only
usage of this function was in a function that is only called behind
radix-only guards. And it worked this way on GCC. It seems Clang does
not optimise away this call however, so thinks the function may be
invoked and triggers the build bug as reported by the kernel test robot.

https://lore.kernel.org/llvm/202301212348.eDkowvfF-lkp@intel.com

This fix converts the build bug to a warning to allow builds without
relying on particular compiler optimisation behaviours. The warning is
not rate limited because this implementation should still never be called
as-is, and anyone who might invoke it might appreciate it being very
obvious that it's not behaving as expected.

Fixes: 274d842fa1ef ("powerpc/tlb: Add local flush for page given mm_struct and psize")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
 arch/powerpc/include/asm/book3s/32/tlbflush.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/book3s/32/tlbflush.h b/arch/powerpc/include/asm/book3s/32/tlbflush.h
index 4be572908124..675196884640 100644
--- a/arch/powerpc/include/asm/book3s/32/tlbflush.h
+++ b/arch/powerpc/include/asm/book3s/32/tlbflush.h
@@ -2,7 +2,7 @@
 #ifndef _ASM_POWERPC_BOOK3S_32_TLBFLUSH_H
 #define _ASM_POWERPC_BOOK3S_32_TLBFLUSH_H
 
-#include <linux/build_bug.h>
+#include <linux/bug.h>
 
 #define MMU_NO_CONTEXT      (0)
 /*
@@ -80,7 +80,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();
+	WARN(1, "Unimplemented local TLB flush with psize");
 }
 
 static inline void local_flush_tlb_mm(struct mm_struct *mm)

base-commit: 53ab112a95086d10dc353ea4f979abb01644bbb6
-- 
2.39.1

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

[Christophe Leroy]

Le 24/01/2023 à 22:54, Benjamin Gray a écrit :
> Converts the BUILD_BUG to a WARN to allow building with a low/unoptimised
> compiler.

No no no no. Please don't do that.

That approach is used everywhere in the kernel, why should it be a 
problem only for local_flush_tlb_page_psize() and not everywhere else ?

Really if it doesn't work it means there is a deep problem in your 
compiler. We really don't want the overhead of WARN over BUILD_BUG in 
the normal case.

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 ?

Christophe

> 
> The original expectation was that a compiler would see that the only
> usage of this function was in a function that is only called behind
> radix-only guards. And it worked this way on GCC. It seems Clang does
> not optimise away this call however, so thinks the function may be
> invoked and triggers the build bug as reported by the kernel test robot.
> 
> https://lore.kernel.org/llvm/202301212348.eDkowvfF-lkp@intel.com
> 
> This fix converts the build bug to a warning to allow builds without
> relying on particular compiler optimisation behaviours. The warning is
> not rate limited because this implementation should still never be called
> as-is, and anyone who might invoke it might appreciate it being very
> obvious that it's not behaving as expected.
> 
> Fixes: 274d842fa1ef ("powerpc/tlb: Add local flush for page given mm_struct and psize")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/book3s/32/tlbflush.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/book3s/32/tlbflush.h b/arch/powerpc/include/asm/book3s/32/tlbflush.h
> index 4be572908124..675196884640 100644
> --- a/arch/powerpc/include/asm/book3s/32/tlbflush.h
> +++ b/arch/powerpc/include/asm/book3s/32/tlbflush.h
> @@ -2,7 +2,7 @@
>   #ifndef _ASM_POWERPC_BOOK3S_32_TLBFLUSH_H
>   #define _ASM_POWERPC_BOOK3S_32_TLBFLUSH_H
>   
> -#include <linux/build_bug.h>
> +#include <linux/bug.h>
>   
>   #define MMU_NO_CONTEXT      (0)
>   /*
> @@ -80,7 +80,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();
> +	WARN(1, "Unimplemented local TLB flush with psize");
>   }
>   
>   static inline void local_flush_tlb_mm(struct mm_struct *mm)
> 
> base-commit: 53ab112a95086d10dc353ea4f979abb01644bbb6

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

[Benjamin Gray]

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).

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)

Re: [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)

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

[Michael Ellerman]

Benjamin Gray <bgray@linux.ibm.com> writes:
> Converts the BUILD_BUG to a WARN to allow building with a low/unoptimised
> compiler.
>
> The original expectation was that a compiler would see that the only
> usage of this function was in a function that is only called behind
> radix-only guards. And it worked this way on GCC. It seems Clang does
> not optimise away this call however, so thinks the function may be
> invoked and triggers the build bug as reported by the kernel test robot.
>
> https://lore.kernel.org/llvm/202301212348.eDkowvfF-lkp@intel.com
>
> This fix converts the build bug to a warning to allow builds without
> relying on particular compiler optimisation behaviours. The warning is
> not rate limited because this implementation should still never be called
> as-is, and anyone who might invoke it might appreciate it being very
> obvious that it's not behaving as expected.
>
> Fixes: 274d842fa1ef ("powerpc/tlb: Add local flush for page given mm_struct and psize")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/book3s/32/tlbflush.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/32/tlbflush.h b/arch/powerpc/include/asm/book3s/32/tlbflush.h
> index 4be572908124..675196884640 100644
> --- a/arch/powerpc/include/asm/book3s/32/tlbflush.h
> +++ b/arch/powerpc/include/asm/book3s/32/tlbflush.h
> @@ -2,7 +2,7 @@
>  #ifndef _ASM_POWERPC_BOOK3S_32_TLBFLUSH_H
>  #define _ASM_POWERPC_BOOK3S_32_TLBFLUSH_H
>  
> -#include <linux/build_bug.h>
> +#include <linux/bug.h>
>  
>  #define MMU_NO_CONTEXT      (0)
>  /*
> @@ -80,7 +80,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();
> +	WARN(1, "Unimplemented local TLB flush with psize");

Can't we just fall back to flush_tlb_page(vma, vmaddr)?

I'd guess those CPUs can't flush based on page size anyway.

cheers

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

[Benjamin Gray]

On Wed, 2023-01-25 at 22:35 +1100, Michael Ellerman wrote:
> Can't we just fall back to flush_tlb_page(vma, vmaddr)?
> 
> I'd guess those CPUs can't flush based on page size anyway.
> 
> cheers

Probably. Do they have a fixed page size? It's not a BUILD_BUG/WARN
because it _should_ be unimplemented, just that I don't have an idea of
how that target works.

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

[Michael Ellerman]

Benjamin Gray <bgray@linux.ibm.com> writes:
> On Wed, 2023-01-25 at 22:35 +1100, Michael Ellerman wrote:
>> Can't we just fall back to flush_tlb_page(vma, vmaddr)?
>> 
>> I'd guess those CPUs can't flush based on page size anyway.
>> 
>> cheers
>
> Probably. Do they have a fixed page size?

AFAIK yes.

cheers