[PATCH 1/3] powerpc: Drop clang workaround for builtin constant checks

[PATCH 1/3] powerpc: Drop clang workaround for builtin constant checks

[Michael Ellerman]

The CPU/MMU feature code has build-time checks that the feature value is
a builtin constant.

Back when the code was added clang wasn't able to compile the
checks, so an ifdef was added to avoid the checks for clang builds.
See b5fa0f7f88ed ("powerpc: Fix build failure with clang due to
BUILD_BUG_ON()")

These days clang 13 and later are able to build the checks successfully,
so drop the workaround.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/cpu_has_feature.h | 2 --
 arch/powerpc/include/asm/mmu.h             | 2 --
 2 files changed, 4 deletions(-)

diff --git a/arch/powerpc/include/asm/cpu_has_feature.h b/arch/powerpc/include/asm/cpu_has_feature.h
index 0efabccd820c..92e24e979954 100644
--- a/arch/powerpc/include/asm/cpu_has_feature.h
+++ b/arch/powerpc/include/asm/cpu_has_feature.h
@@ -24,9 +24,7 @@ static __always_inline bool cpu_has_feature(unsigned long feature)
 {
 	int i;
 
-#ifndef __clang__ /* clang can't cope with this */
 	BUILD_BUG_ON(!__builtin_constant_p(feature));
-#endif
 
 #ifdef CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG
 	if (!static_key_feature_checks_initialized) {
diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
index 24f830cf9bb4..4ab9a630d943 100644
--- a/arch/powerpc/include/asm/mmu.h
+++ b/arch/powerpc/include/asm/mmu.h
@@ -246,9 +246,7 @@ static __always_inline bool mmu_has_feature(unsigned long feature)
 {
 	int i;
 
-#ifndef __clang__ /* clang can't cope with this */
 	BUILD_BUG_ON(!__builtin_constant_p(feature));
-#endif
 
 #ifdef CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG
 	if (!static_key_feature_checks_initialized) {
-- 
2.45.0

[PATCH 2/3] powerpc/xmon: Fix disassembly CPU feature checks

[Michael Ellerman]

In the xmon disassembly code there are several CPU feature checks to
determine what dialects should be passed to the disassembler. The
dialect controls which instructions the disassembler will recognise.

Unfortunately the checks are incorrect, because instead of passing a
single CPU feature they are passing a mask of feature bits.

For example the code:

  if (cpu_has_feature(CPU_FTRS_POWER5))
      dialect |= PPC_OPCODE_POWER5;

Is trying to check if the system is running on a Power5 CPU. But
CPU_FTRS_POWER5 is a mask of *all* the feature bits that are enabled on
a Power5.

In practice the test will always return true for any 64-bit CPU, because
at least one bit in the mask will be present in the CPU_FTRS_ALWAYS
mask.

Similarly for all the other checks against CPU_FTRS_xx masks.

Rather than trying to match the disassembly behaviour exactly to the
current CPU, just differentiate between 32-bit and 64-bit, and Altivec,
VSX and HTM.

That will cause some instructions to be shown in disassembly even
on a CPU that doesn't support them, but that's OK, objdump -d output
has the same behaviour, and if anything it's less confusing than some
instructions not being disassembled.

Fixes: 897f112bb42e ("[POWERPC] Import updated version of ppc disassembly code for xmon")
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/xmon/ppc-dis.c | 33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/arch/powerpc/xmon/ppc-dis.c b/arch/powerpc/xmon/ppc-dis.c
index 75fa98221d48..af105e1bc3fc 100644
--- a/arch/powerpc/xmon/ppc-dis.c
+++ b/arch/powerpc/xmon/ppc-dis.c
@@ -122,32 +122,21 @@ int print_insn_powerpc (unsigned long insn, unsigned long memaddr)
   bool insn_is_short;
   ppc_cpu_t dialect;
 
-  dialect = PPC_OPCODE_PPC | PPC_OPCODE_COMMON
-            | PPC_OPCODE_64 | PPC_OPCODE_POWER4 | PPC_OPCODE_ALTIVEC;
+  dialect = PPC_OPCODE_PPC | PPC_OPCODE_COMMON;
 
-  if (cpu_has_feature(CPU_FTRS_POWER5))
-    dialect |= PPC_OPCODE_POWER5;
+  if (IS_ENABLED(CONFIG_PPC64))
+    dialect |= PPC_OPCODE_64 | PPC_OPCODE_POWER4 | PPC_OPCODE_CELL |
+	PPC_OPCODE_POWER5 | PPC_OPCODE_POWER6 | PPC_OPCODE_POWER7 | PPC_OPCODE_POWER8 |
+	PPC_OPCODE_POWER9;
 
-  if (cpu_has_feature(CPU_FTRS_CELL))
-    dialect |= (PPC_OPCODE_CELL | PPC_OPCODE_ALTIVEC);
+  if (cpu_has_feature(CPU_FTR_TM))
+    dialect |= PPC_OPCODE_HTM;
 
-  if (cpu_has_feature(CPU_FTRS_POWER6))
-    dialect |= (PPC_OPCODE_POWER5 | PPC_OPCODE_POWER6 | PPC_OPCODE_ALTIVEC);
+  if (cpu_has_feature(CPU_FTR_ALTIVEC))
+    dialect |= PPC_OPCODE_ALTIVEC | PPC_OPCODE_ALTIVEC2;
 
-  if (cpu_has_feature(CPU_FTRS_POWER7))
-    dialect |= (PPC_OPCODE_POWER5 | PPC_OPCODE_POWER6 | PPC_OPCODE_POWER7
-                | PPC_OPCODE_ALTIVEC | PPC_OPCODE_VSX);
-
-  if (cpu_has_feature(CPU_FTRS_POWER8))
-    dialect |= (PPC_OPCODE_POWER5 | PPC_OPCODE_POWER6 | PPC_OPCODE_POWER7
-		| PPC_OPCODE_POWER8 | PPC_OPCODE_HTM
-		| PPC_OPCODE_ALTIVEC | PPC_OPCODE_ALTIVEC2 | PPC_OPCODE_VSX);
-
-  if (cpu_has_feature(CPU_FTRS_POWER9))
-    dialect |= (PPC_OPCODE_POWER5 | PPC_OPCODE_POWER6 | PPC_OPCODE_POWER7
-		| PPC_OPCODE_POWER8 | PPC_OPCODE_POWER9 | PPC_OPCODE_HTM
-		| PPC_OPCODE_ALTIVEC | PPC_OPCODE_ALTIVEC2
-		| PPC_OPCODE_VSX | PPC_OPCODE_VSX3);
+  if (cpu_has_feature(CPU_FTR_VSX))
+    dialect |= PPC_OPCODE_VSX | PPC_OPCODE_VSX3;
 
   /* Get the major opcode of the insn.  */
   opcode = NULL;
-- 
2.45.0

[PATCH 3/3] powerpc: Check only single values are passed to CPU/MMU feature checks

[Michael Ellerman]

cpu_has_feature()/mmu_has_feature() are only able to check a single
feature at a time, but there is no enforcement of that.

In fact, as fixed in the previous commit, there was code that was
passing multiple values to cpu_has_feature().

So add a check that only a single feature is passed using popcount.

Note that the test allows 0 or 1 bits to be set, because some code
relies on cpu_has_feature(0) being false, the check with
CPU_FTRS_POSSIBLE ensures that. See for example CPU_FTR_PPC_LE.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/cpu_has_feature.h | 1 +
 arch/powerpc/include/asm/mmu.h             | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/powerpc/include/asm/cpu_has_feature.h b/arch/powerpc/include/asm/cpu_has_feature.h
index 92e24e979954..bf8a228229fa 100644
--- a/arch/powerpc/include/asm/cpu_has_feature.h
+++ b/arch/powerpc/include/asm/cpu_has_feature.h
@@ -25,6 +25,7 @@ static __always_inline bool cpu_has_feature(unsigned long feature)
 	int i;
 
 	BUILD_BUG_ON(!__builtin_constant_p(feature));
+	BUILD_BUG_ON(__builtin_popcountl(feature) > 1);
 
 #ifdef CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG
 	if (!static_key_feature_checks_initialized) {
diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
index 4ab9a630d943..eb3065692055 100644
--- a/arch/powerpc/include/asm/mmu.h
+++ b/arch/powerpc/include/asm/mmu.h
@@ -247,6 +247,7 @@ static __always_inline bool mmu_has_feature(unsigned long feature)
 	int i;
 
 	BUILD_BUG_ON(!__builtin_constant_p(feature));
+	BUILD_BUG_ON(__builtin_popcountl(feature) > 1);
 
 #ifdef CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG
 	if (!static_key_feature_checks_initialized) {
-- 
2.45.0

Re: [PATCH 3/3] powerpc: Check only single values are passed to CPU/MMU feature checks

[Segher Boessenkool]

On Thu, May 09, 2024 at 10:12:48PM +1000, Michael Ellerman wrote:
> cpu_has_feature()/mmu_has_feature() are only able to check a single
> feature at a time, but there is no enforcement of that.
> 
> In fact, as fixed in the previous commit, there was code that was
> passing multiple values to cpu_has_feature().
> 
> So add a check that only a single feature is passed using popcount.
> 
> Note that the test allows 0 or 1 bits to be set, because some code
> relies on cpu_has_feature(0) being false, the check with
> CPU_FTRS_POSSIBLE ensures that. See for example CPU_FTR_PPC_LE.

This btw is exactly

	BUILD_BUG_ON(feature & (feature - 1));

but the popcount is more readable :-)


Segher

Re: [PATCH 3/3] powerpc: Check only single values are passed to CPU/MMU feature checks

[Michael Ellerman]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Thu, May 09, 2024 at 10:12:48PM +1000, Michael Ellerman wrote:
>> cpu_has_feature()/mmu_has_feature() are only able to check a single
>> feature at a time, but there is no enforcement of that.
>> 
>> In fact, as fixed in the previous commit, there was code that was
>> passing multiple values to cpu_has_feature().
>> 
>> So add a check that only a single feature is passed using popcount.
>> 
>> Note that the test allows 0 or 1 bits to be set, because some code
>> relies on cpu_has_feature(0) being false, the check with
>> CPU_FTRS_POSSIBLE ensures that. See for example CPU_FTR_PPC_LE.
>
> This btw is exactly
>
> 	BUILD_BUG_ON(feature & (feature - 1));
>
> but the popcount is more readable :-)

Yeah for those of us who don't see bits cascading in our sleep I think
the popcount is easier to understand ;)

cheers

Re: [PATCH 3/3] powerpc: Check only single values are passed to CPU/MMU feature checks

[Segher Boessenkool]

On Fri, May 10, 2024 at 04:45:37PM +1000, Michael Ellerman wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Thu, May 09, 2024 at 10:12:48PM +1000, Michael Ellerman wrote:
> >> cpu_has_feature()/mmu_has_feature() are only able to check a single
> >> feature at a time, but there is no enforcement of that.
> >> 
> >> In fact, as fixed in the previous commit, there was code that was
> >> passing multiple values to cpu_has_feature().
> >> 
> >> So add a check that only a single feature is passed using popcount.
> >> 
> >> Note that the test allows 0 or 1 bits to be set, because some code
> >> relies on cpu_has_feature(0) being false, the check with
> >> CPU_FTRS_POSSIBLE ensures that. See for example CPU_FTR_PPC_LE.
> >
> > This btw is exactly
> >
> > 	BUILD_BUG_ON(feature & (feature - 1));
> >
> > but the popcount is more readable :-)
> 
> Yeah for those of us who don't see bits cascading in our sleep I think
> the popcount is easier to understand ;)

Absolutely :-)  This is just one of the most well-known bittricks, for
seeing if x is a power of two you write

  x && x & (x-1)

but here you do not need to test for x not being zero.  Hardly ever get
to use that simpler thing, so it jumped out at me here :-)

[ For understanding the x & (x-1) thing, it is perhaps easiest if you
first consider an x with more than one bit set: x-1 will have the same
topmost set bit. ]


Segher

Re: [PATCH 1/3] powerpc: Drop clang workaround for builtin constant checks

[Michael Ellerman]

On Thu, 09 May 2024 22:12:46 +1000, Michael Ellerman wrote:
> The CPU/MMU feature code has build-time checks that the feature value is
> a builtin constant.
> 
> Back when the code was added clang wasn't able to compile the
> checks, so an ifdef was added to avoid the checks for clang builds.
> See b5fa0f7f88ed ("powerpc: Fix build failure with clang due to
> BUILD_BUG_ON()")
> 
> [...]

Applied to powerpc/next.

[1/3] powerpc: Drop clang workaround for builtin constant checks
      https://git.kernel.org/powerpc/c/489116d784bebec7e441f400715fbfe6edbce66c
[2/3] powerpc/xmon: Fix disassembly CPU feature checks
      https://git.kernel.org/powerpc/c/14196e47c5ffe32af7ed5a51c9e421c5ea5bccce
[3/3] powerpc: Check only single values are passed to CPU/MMU feature checks
      https://git.kernel.org/powerpc/c/db25a9625dbc3aa6613c0347f574689c248a3d0b

cheers