| Message ID | 20240327045911.64543-1-nicholas@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add static_key_feature_checks_initialized flag | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
Le 27/03/2024 à 05:59, Nicholas Miehlbradt a écrit : > JUMP_LABEL_FEATURE_CHECK_DEBUG used static_key_initialized to determine > whether {cpu,mmu}_has_feature() was used before static keys were > initialized. However, {cpu,mmu}_has_feature() should not be used before > setup_feature_keys() is called. As static_key_initalized is set much > earlier during boot there is a window in which JUMP_LABEL_FEATURE_CHECK_DEBUG > will not report errors. Add a flag specifically to indicate when > {cpu,mmu}_has_feature() is safe to use. What do you mean by "much earlier" ? As far as I can see, static_key_initialized is set by jump_label_init() as cpu_feature_keys_init() and mmu_feature_keys_init() are call immediately after. I don't think it is possible to do anything inbetween. Or maybe you mean the problem is the call to jump_label_init() in early_init_devtree() ? You should make it explicit in the message, and see if it wouldn't be better to call cpu_feature_keys_init() and mmu_feature_keys_init() as well in early_init_devtree() in that case ? Christophe
On 28/3/2024 2:20 am, Christophe Leroy wrote: > > > Le 27/03/2024 à 05:59, Nicholas Miehlbradt a écrit : >> JUMP_LABEL_FEATURE_CHECK_DEBUG used static_key_initialized to determine >> whether {cpu,mmu}_has_feature() was used before static keys were >> initialized. However, {cpu,mmu}_has_feature() should not be used before >> setup_feature_keys() is called. As static_key_initalized is set much >> earlier during boot there is a window in which JUMP_LABEL_FEATURE_CHECK_DEBUG >> will not report errors. Add a flag specifically to indicate when >> {cpu,mmu}_has_feature() is safe to use. > > What do you mean by "much earlier" ? > > As far as I can see, static_key_initialized is set by jump_label_init() > as cpu_feature_keys_init() and mmu_feature_keys_init() are call > immediately after. I don't think it is possible to do anything inbetween. > > Or maybe you mean the problem is the call to jump_label_init() in > early_init_devtree() ? You should make it explicit in the message, and > see if it wouldn't be better to call cpu_feature_keys_init() and > mmu_feature_keys_init() as well in early_init_devtree() in that case ? > The jump_label_init() call in early_init_devtree() is exactly the issue. I don't think it's possible to move the call to mmu_feature_keys_init() earlier without significant refactoring since mmu features are being set as late as setup_kup(). I'll still sent a v2 with a better worded commit message. Nicholas > Christophe
On 28/3/2024 2:20 am, Christophe Leroy wrote: > > > Le 27/03/2024 à 05:59, Nicholas Miehlbradt a écrit : >> JUMP_LABEL_FEATURE_CHECK_DEBUG used static_key_initialized to determine >> whether {cpu,mmu}_has_feature() was used before static keys were >> initialized. However, {cpu,mmu}_has_feature() should not be used before >> setup_feature_keys() is called. As static_key_initalized is set much >> earlier during boot there is a window in which JUMP_LABEL_FEATURE_CHECK_DEBUG >> will not report errors. Add a flag specifically to indicate when >> {cpu,mmu}_has_feature() is safe to use. > > What do you mean by "much earlier" ? > > As far as I can see, static_key_initialized is set by jump_label_init() > as cpu_feature_keys_init() and mmu_feature_keys_init() are call > immediately after. I don't think it is possible to do anything inbetween. > > Or maybe you mean the problem is the call to jump_label_init() in > early_init_devtree() ? You should make it explicit in the message, and > see if it wouldn't be better to call cpu_feature_keys_init() and > mmu_feature_keys_init() as well in early_init_devtree() in that case ? > The jump_label_init() call in early_init_devtree() is exactly the issue. I don't think it's possible to move the call to mmu_feature_keys_init() earlier without significant refactoring since mmu features are being set as late as setup_kup(). I'll still sent a v2 with a better worded commit message. Nicholas > Christophe
diff --git a/arch/powerpc/include/asm/cpu_has_feature.h b/arch/powerpc/include/asm/cpu_has_feature.h index 727d4b321937..0efabccd820c 100644 --- a/arch/powerpc/include/asm/cpu_has_feature.h +++ b/arch/powerpc/include/asm/cpu_has_feature.h @@ -29,7 +29,7 @@ static __always_inline bool cpu_has_feature(unsigned long feature) #endif #ifdef CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG - if (!static_key_initialized) { + if (!static_key_feature_checks_initialized) { printk("Warning! cpu_has_feature() used prior to jump label init!\n"); dump_stack(); return early_cpu_has_feature(feature); diff --git a/arch/powerpc/include/asm/feature-fixups.h b/arch/powerpc/include/asm/feature-fixups.h index 77824bd289a3..17d168dd8b49 100644 --- a/arch/powerpc/include/asm/feature-fixups.h +++ b/arch/powerpc/include/asm/feature-fixups.h @@ -291,6 +291,8 @@ extern long __start___rfi_flush_fixup, __stop___rfi_flush_fixup; extern long __start___barrier_nospec_fixup, __stop___barrier_nospec_fixup; extern long __start__btb_flush_fixup, __stop__btb_flush_fixup; +extern bool static_key_feature_checks_initialized; + void apply_feature_fixups(void); void update_mmu_feature_fixups(unsigned long mask); void setup_feature_keys(void); diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h index 3b72c7ed24cf..24f830cf9bb4 100644 --- a/arch/powerpc/include/asm/mmu.h +++ b/arch/powerpc/include/asm/mmu.h @@ -251,7 +251,7 @@ static __always_inline bool mmu_has_feature(unsigned long feature) #endif #ifdef CONFIG_JUMP_LABEL_FEATURE_CHECK_DEBUG - if (!static_key_initialized) { + if (!static_key_feature_checks_initialized) { printk("Warning! mmu_has_feature() used prior to jump label init!\n"); dump_stack(); return early_mmu_has_feature(feature); diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c index 4f82581ca203..b7201ba50b2e 100644 --- a/arch/powerpc/lib/feature-fixups.c +++ b/arch/powerpc/lib/feature-fixups.c @@ -25,6 +25,13 @@ #include <asm/firmware.h> #include <asm/inst.h> +/* + * Used to generate warnings if mmu or cpu feature check functions that + * use static keys before they are initialized. + */ +bool static_key_feature_checks_initialized __read_mostly; +EXPORT_SYMBOL_GPL(static_key_feature_checks_initialized); + struct fixup_entry { unsigned long mask; unsigned long value; @@ -679,6 +686,7 @@ void __init setup_feature_keys(void) jump_label_init(); cpu_feature_keys_init(); mmu_feature_keys_init(); + static_key_feature_checks_initialized = true; } static int __init check_features(void)
JUMP_LABEL_FEATURE_CHECK_DEBUG used static_key_initialized to determine whether {cpu,mmu}_has_feature() was used before static keys were initialized. However, {cpu,mmu}_has_feature() should not be used before setup_feature_keys() is called. As static_key_initalized is set much earlier during boot there is a window in which JUMP_LABEL_FEATURE_CHECK_DEBUG will not report errors. Add a flag specifically to indicate when {cpu,mmu}_has_feature() is safe to use. Signed-off-by: Nicholas Miehlbradt <nicholas@linux.ibm.com> --- arch/powerpc/include/asm/cpu_has_feature.h | 2 +- arch/powerpc/include/asm/feature-fixups.h | 2 ++ arch/powerpc/include/asm/mmu.h | 2 +- arch/powerpc/lib/feature-fixups.c | 8 ++++++++ 4 files changed, 12 insertions(+), 2 deletions(-)