| Message ID | 20240509121248.270878-3-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | [1/3] powerpc: Drop clang workaround for builtin constant checks | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
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
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
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
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) {
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(+)