arm64: errata: Fix handling of 1418040 with late CPU onlining
Will Deacon
maintainerPATCH 1/1 arm64: errata: Fix handling of 1418040 with late CPU onlining
In a surprising turn of events, it transpires that CPU capabilities configured as ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE are never set as the result of late-onlining. Therefore our handling of erratum 1418040 does not get activated if it is not required by any of the boot CPUs, even though we allow late-onlining of an affected CPU.
In order to get things working again, replace the cpus_have_const_cap() invocation with an explicit check for the current CPU using this_cpu_has_cap().
---
Found by code inspection and compile-tested only, so I would really appreciate a second look.
arch/arm64/include/asm/cpufeature.h | 2 ++ arch/arm64/kernel/process.c | 5 ++--- 2 files changed, 4 insertions(+), 3 deletions(-)
arch/arm64/include/asm/cpufeature.h +2 -0
@@ -268,6 +268,8 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;/* * CPU feature detected at boot time based on feature of one or more CPUs. * All possible conflicts for a late CPU are ignored. +* NOTE: this means that a late CPU with the feature will *not* cause the +* capability to be advertised by cpus_have_*cap()! */ #define ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE \ (ARM64_CPUCAP_SCOPE_LOCAL_CPU | \
arch/arm64/kernel/process.c +2 -3
@@ -522,14 +522,13 @@ static void erratum_1418040_thread_switch(struct task_struct *prev,bool prev32, next32;u64 val; -if (!(IS_ENABLED (CONFIG_ARM64_ERRATUM_1418040 ) && -cpus_have_const_cap (ARM64_WORKAROUND_1418040 ))) +if (!IS_ENABLED (CONFIG_ARM64_ERRATUM_1418040 ))return ; prev32 =is_compat_thread (task_thread_info (prev)); next32 =is_compat_thread (task_thread_info (next)); -if (prev32 == next32) +if (prev32 == next32 || !this_cpu_has_cap (ARM64_WORKAROUND_1418040 ))return ; val =read_sysreg (cntkctl_el1);
Signature · 7 lines
2.29.1.341.ge80a0c044ae-goog _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 52 lines · Will Deacon
In a surprising turn of events, it transpires that CPU capabilities configured as
ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATUREare never set as the result of late-onlining. Therefore our handling of erratum 1418040 does not get activated if it is not required by any of the boot CPUs, even though we allow late-onlining of an affected CPU.In order to get things working again, replace the
cpus_have_const_cap()invocation with an explicit check for the current CPU usingthis_cpu_has_cap().Cc Marc Zyngier <[email protected]>Cc Sai Prakash Ranjan <[email protected]>Cc Stephen Boyd <[email protected]>Cc Catalin Marinas <[email protected]>Cc Mark Rutland <[email protected]>Cc Suzuki Poulose <[email protected]>Signed-off-by Will Deacon <[email protected]>---
Found by code inspection and compile-tested only, so I would really appreciate a second look.
arch/arm64/include/asm/cpufeature.h | 2 ++ arch/arm64/kernel/process.c | 5 ++--- 2 files changed, 4 insertions(+), 3 deletions(-)arch/arm64/include/asm/cpufeature.h +0
-0@@ -268,6 +268,8 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;/* * CPU feature detected at boot time based on feature of one or more CPUs.
* All possible conflicts for a late CPU are ignored.
+ * NOTE: this means that a late CPU with the feature will *not* cause the
+ * capability to be advertised by cpus_have_*cap()!
*/
#defineARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE\arch/arm64/include/asm/cpufeature.h +0
-0(ARM64_CPUCAP_SCOPE_LOCAL_CPU | \arch/arm64/kernel/process.c +0
-1@@ -522,14 +522,13 @@ static void erratum_1418040_thread_switch(structtask_struct *prev, bool prev32, next32;u64 val; - if (!(IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040) &&
arch/arm64/kernel/process.c static void erratum_1418040_thread_switch(struct · line 533
Quoted 10 lines
arch/arm64/kernel/process.c +2
-2- cpus_have_const_cap(ARM64_WORKAROUND_1418040))) +if (!IS_ENABLED (CONFIG_ARM64_ERRATUM_1418040 ))return ; prev32 =is_compat_thread (task_thread_info (prev)); next32 =is_compat_thread (task_thread_info (next)); -if (prev32 == next32) +if (prev32 == next32 || !this_cpu_has_cap (ARM64_WORKAROUND_1418040 ))return ;
It is not going to be cheap to switch between 32 and 64bit. Hopefully it doesn't happen too often... :-(.
M.
Signature · 6 lines
Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Will,
Quoted 5 lines · Will Deacon
In a surprising turn of events, it transpires that CPU capabilities configured as
ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATUREare never set as the result of late-onlining. Therefore our handling of erratum 1418040 does not get activated if it is not required by any of the boot CPUs, even though we allow late-onlining of an affected CPU.
The capability state is not altered after the SMP boot for all types of caps. The weak caps are there to allow a late CPU to turn online without getting "banned". This may be something we could relax with a new flag in the scope.
Quoted 26 lines
In order to get things working again, replace the
cpus_have_const_cap()invocation with an explicit check for the current CPU usingthis_cpu_has_cap().Cc Marc Zyngier <[email protected]>Cc Sai Prakash Ranjan <[email protected]>Cc Stephen Boyd <[email protected]>Cc Catalin Marinas <[email protected]>Cc Mark Rutland <[email protected]>Cc Suzuki Poulose <[email protected]>Signed-off-by Will Deacon <[email protected]>---
Found by code inspection and compile-tested only, so I would really appreciate a second look.
arch/arm64/include/asm/cpufeature.h | 2 ++ arch/arm64/kernel/process.c | 5 ++--- 2 files changed, 4 insertions(+), 3 deletions(-)arch/arm64/include/asm/cpufeature.h +0
-0
arch/arm64/include/asm/cpufeature.h extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0; · line 272
Quoted 5 lines
arch/arm64/include/asm/cpufeature.h +2
-0@@ -268,6 +268,8 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;/* * CPU feature detected at boot time based on feature of one or more CPUs. * All possible conflicts for a late CPU are ignored. +* NOTE: this means that a late CPU with the feature will *not* cause the +* capability to be advertised by cpus_have_*cap()!
This comment applies to all the types, so it may be confusing. And the comment already mentions that the feature is detected at boot time.
Quoted 15 lines
arch/arm64/include/asm/cpufeature.h +0
-0*/ #define ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE \ (ARM64_CPUCAP_SCOPE_LOCAL_CPU | \arch/arm64/kernel/process.c +1
-2@@ -522,14 +522,13 @@ static void erratum_1418040_thread_switch(struct task_struct *prev,bool prev32, next32;u64 val; -if (!(IS_ENABLED (CONFIG_ARM64_ERRATUM_1418040 ) && -cpus_have_const_cap (ARM64_WORKAROUND_1418040 ))) +if (!IS_ENABLED (CONFIG_ARM64_ERRATUM_1418040 ))
arch/arm64/kernel/process.c static void erratum_1418040_thread_switch(struct task_struct *prev, · line 534
Quoted 10 lines
arch/arm64/kernel/process.c +1
-1return ; prev32 =is_compat_thread (task_thread_info (prev)); next32 =is_compat_thread (task_thread_info (next)); -if (prev32 == next32) +if (prev32 == next32 || !this_cpu_has_cap (ARM64_WORKAROUND_1418040 ))return ; val =read_sysreg (cntkctl_el1);
This change as such looks good to me.
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Will Deacon
maintainerQuoted 11 lines · Suzuki K Poulose
Quoted 5 lines · Will Deacon
In a surprising turn of events, it transpires that CPU capabilities configured as
ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATUREare never set as the result of late-onlining. Therefore our handling of erratum 1418040 does not get activated if it is not required by any of the boot CPUs, even though we allow late-onlining of an affected CPU.The capability state is not altered after the SMP boot for all types of caps. The weak caps are there to allow a late CPU to turn online without getting "banned". This may be something we could relax with a new flag in the scope.
I did look briefly into that, but it's really difficult to enable the static key as this operation can block so you end up having to defer it past the point of signalling the completion back to the CPU doing cpu_up().
So I figured I'd focus on fixing the current problem for now.
Quoted 20 lines
Quoted 20 lines
In order to get things working again, replace the
cpus_have_const_cap()invocation with an explicit check for the current CPU usingthis_cpu_has_cap().Cc Marc Zyngier <[email protected]>Cc Sai Prakash Ranjan <[email protected]>Cc Stephen Boyd <[email protected]>Cc Catalin Marinas <[email protected]>Cc Mark Rutland <[email protected]>Cc Suzuki Poulose <[email protected]>Signed-off-by Will Deacon <[email protected]>---
Found by code inspection and compile-tested only, so I would really appreciate a second look.
arch/arm64/include/asm/cpufeature.h | 2 ++ arch/arm64/kernel/process.c | 5 ++--- 2 files changed, 4 insertions(+), 3 deletions(-)
Quoted 13 lines
arch/arm64/include/asm/cpufeature.h extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0; · line 272
Quoted 10 lines
arch/arm64/include/asm/cpufeature.h +2
-0@@ -268,6 +268,8 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;/* * CPU feature detected at boot time based on feature of one or more CPUs. * All possible conflicts for a late CPU are ignored. +* NOTE: this means that a late CPU with the feature will *not* cause the +* capability to be advertised by cpus_have_*cap()! This comment applies to all the types, so it may be confusing. And the comment already mentions that the feature is detected at boot time.
The other types don't allow a late CPU to come online with the feature though, so I don't think it's quite the same. Given that we made this mistake for this erratum workaround and previously for SSBS, I wanted to add something to the comment to try to avoid others falling into this trap.
Ideally, we'd warn when calling cpus_have_*cap() for one of these things, but that adds runtime cost that we'd probably have to gate behind a DEBUG option.
Quoted 25 lines
Quoted 11 lines
arch/arm64/include/asm/cpufeature.h +0
-0*/ #define ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE \ (ARM64_CPUCAP_SCOPE_LOCAL_CPU | \arch/arm64/kernel/process.c +0
-1@@ -522,14 +522,13 @@ static void erratum_1418040_thread_switch(struct task_struct *prev,bool prev32, next32;u64 val; - if (!(IS_ENABLED(CONFIG_ARM64_ERRATUM_1418040) &&arch/arm64/kernel/process.c static void erratum_1418040_thread_switch(struct task_struct *prev, · line 531
Quoted 10 lines
arch/arm64/kernel/process.c +2
-2- cpus_have_const_cap(ARM64_WORKAROUND_1418040))) +if (!IS_ENABLED (CONFIG_ARM64_ERRATUM_1418040 ))return ; prev32 =is_compat_thread (task_thread_info (prev)); next32 =is_compat_thread (task_thread_info (next)); -if (prev32 == next32) +if (prev32 == next32 || !this_cpu_has_cap (ARM64_WORKAROUND_1418040 ))return ; val =read_sysreg (cntkctl_el1);This change as such looks good to me.
Quoted 1 line
Reviewed-by Suzuki K Poulose <[email protected]>
Thanks!
Will
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Catalin Marinas
maintainerQuoted 11 lines · Suzuki K Poulose
Quoted 5 lines · Will Deacon
In a surprising turn of events, it transpires that CPU capabilities configured as
ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATUREare never set as the result of late-onlining. Therefore our handling of erratum 1418040 does not get activated if it is not required by any of the boot CPUs, even though we allow late-onlining of an affected CPU.The capability state is not altered after the SMP boot for all types of caps. The weak caps are there to allow a late CPU to turn online without getting "banned". This may be something we could relax with a new flag in the scope.
Like this? Of course, it needs some testing.
arch/arm64/include/asm/cpufeature.h +12 -0
@@ -246,6 +246,8 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;#define ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU ((u16)BIT(5))/* Panic when a conflict is detected */ #define ARM64_CPUCAP_PANIC_ON_CONFLICT ((u16)BIT(6)) +/* Together with PERMITTED_FOR_LATE_CPU, set the corresponding cpu_hwcaps bit */ +#define ARM64_CPUCAP_SET_FOR_LATE_CPU ((u16)BIT(7))/* * CPU errata workarounds that need to be enabled at boot time if one or
@@ -481,6 +483,16 @@ static __always_inline bool cpus_have_const_cap(int num)return cpus_have_cap (num); } +/* +* Test for a capability with a runtime check. This is an alias for +* cpus_have_cap() but with the name chosen to emphasize the applicability to +* late capability setting. +*/ +static __always_inline bool cpus_have_late_cap(int num) +{ +return cpus_have_cap (num); +} +static inline void cpus_set_cap (unsigned int num) {if (num >=ARM64_NCAPS ) {
arch/arm64/kernel/cpu_errata.c +2 -1
@@ -481,7 +481,8 @@ const struct arm64_cpu_capabilities arm64_errata[] = {* also need the non-affected CPUs to be able to come * in at any point in time. Wonderful. */ - .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, + .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE | + ARM64_CPUCAP_SET_FOR_LATE_CPU, },#endif #ifdef CONFIG_ARM64_WORKAROUND_SPECULATIVE_AT
arch/arm64/kernel/cpufeature.c +11 -0
@@ -1720,6 +1720,12 @@ cpucap_late_cpu_permitted(const struct arm64_cpu_capabilities *cap)return !!(cap->type &ARM64_CPUCAP_PERMITTED_FOR_LATE_CPU ); } +static bool +cpucap_set_for_late_cpu (const struct arm64_cpu_capabilities *cap) +{ +return !!(cap->type &ARM64_CPUCAP_SET_FOR_LATE_CPU ); +} +static bool cpucap_panic_on_conflict (const struct arm64_cpu_capabilities *cap) {
@@ -2489,6 +2495,11 @@ static void verify_local_cpu_caps(u16 scope_mask)*/ if (cpu_has_cap && !cpucap_late_cpu_permitted (caps))break ; +/* +* Set the capability bit if it allows late setting. +*/ +if (cpucap_set_for_late_cpu (caps)) +cpus_set_cap (caps->capability); } }
arch/arm64/kernel/process.c +1 -1
@@ -523,7 +523,7 @@ static void erratum_1418040_thread_switch(struct task_struct *prev,u64 val;if (!(IS_ENABLED (CONFIG_ARM64_ERRATUM_1418040 ) && -cpus_have_const_cap (ARM64_WORKAROUND_1418040 ))) +cpus_have_late_cap (ARM64_WORKAROUND_1418040 )))return ; prev32 =is_compat_thread (task_thread_info (prev));
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Catalin
Quoted 8 lines · Catalin Marinas
Quoted 7 lines · Suzuki K Poulose
Quoted 5 lines · Will Deacon
In a surprising turn of events, it transpires that CPU capabilities configured as
ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATUREare never set as the result of late-onlining. Therefore our handling of erratum 1418040 does not get activated if it is not required by any of the boot CPUs, even though we allow late-onlining of an affected CPU.
Quoted 6 lines
Quoted 4 lines
The capability state is not altered after the SMP boot for all types of caps. The weak caps are there to allow a late CPU to turn online without getting "banned". This may be something we could relax with a new flag in the scope.
Like this? Of course, it needs some testing.
Yes, this is exactly what I had in mind. However, there is some concern over how we expose it.
Quoted 18 lines
arch/arm64/include/asm/cpufeature.h +2
-0@@ -246,6 +246,8 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;#define ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU ((u16)BIT(5))/* Panic when a conflict is detected */ #define ARM64_CPUCAP_PANIC_ON_CONFLICT ((u16)BIT(6)) +/* Together with PERMITTED_FOR_LATE_CPU, set the corresponding cpu_hwcaps bit */ +#define ARM64_CPUCAP_SET_FOR_LATE_CPU ((u16)BIT(7))/* * CPU errata workarounds that need to be enabled at boot time if one or @@ -481,6 +483,16 @@ static __always_inline bool cpus_have_const_cap(int num)return cpus_have_cap (num); }
arch/arm64/include/asm/cpufeature.h static __always_inline bool cpus_have_const_cap(int num) · line 495
Quoted 10 lines
arch/arm64/include/asm/cpufeature.h +10
-0+/* +* Test for a capability with a runtime check. This is an alias for +* cpus_have_cap() but with the name chosen to emphasize the applicability to +* late capability setting. +*/ +static __always_inline bool cpus_have_late_cap(int num) +{ +return cpus_have_cap (num); +} +
It is quite easy for people to misuse the ubiquitous cpus_have_const_cap(). So, unless we make sure that we fix that to handle these "caps" we might see subtle bugs. How about the following fix on top of your patch, to add another (sigh!) set of keys to detect if we can use the const caps or not.
Untested...
---8>---
arch/arm64/include/asm/cpufeature.h +11 -2
@@ -379,6 +379,7 @@ cpucap_multi_entry_cap_matches(const struct arm64_cpu_capabilities *entry, extern DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);extern struct static_key_false cpu_hwcap_keys[ARM64_NCAPS ]; +extern struct static_key_false cpu_hwcap_const_key[ARM64_NCAPS ];extern struct static_key_false arm64_const_caps_ready;/* ARM64 CAPS + alternative_cb */
@@ -414,6 +415,14 @@ static inline bool cpus_have_cap(unsigned int num)return test_bit (num, cpu_hwcaps); } +static __always_inline bool cpucap_has_const_key(int num) +{ +if (num >=ARM64_NCAPS ) +return false; + +return static_branch_unlikely (&cpu_hwcap_const_key[num]); +} +/* * Test for a capability without a runtime check. *
@@ -424,7 +433,7 @@ static inline bool cpus_have_cap(unsigned int num)*/ static __always_inline bool __cpus_have_const_cap(int num) { -if (num >=ARM64_NCAPS ) +if (num >=ARM64_NCAPS &&WARN_ON (!cpucap_has_const_key (num)))return false;return static_branch_unlikely (&cpu_hwcap_keys[num]); }
@@ -439,7 +448,7 @@ static __always_inline bool __cpus_have_const_cap(int num)*/ static __always_inline bool cpus_have_const_cap(int num) { -if (system_capabilities_finalized ()) +if (cpucap_has_const_key (num) &&system_capabilities_finalized ())return __cpus_have_const_cap (num);else return cpus_have_cap (num);
arch/arm64/kernel/cpufeature.c +6 -1
@@ -127,7 +127,9 @@ void dump_cpu_features(void) }DEFINE_STATIC_KEY_ARRAY_FALSE (cpu_hwcap_keys,ARM64_NCAPS ); +DEFINE_STATIC_KEY_ARRAY_FALSE (cpu_hwcap_const_key,ARM64_NCAPS );EXPORT_SYMBOL (cpu_hwcap_keys); +EXPORT_SYMBOL (cpu_hwcap_const_key);#define __ARM64_FTR_BITS (SIGNED ,VISIBLE ,STRICT ,TYPE ,SHIFT ,WIDTH ,SAFE_VAL ) \ { \
@@ -2426,7 +2428,10 @@ static void __init enable_cpu_capabilities(u16 scope_mask)continue ;/* Ensure cpus_have_const_cap(num) works */ -static_branch_enable (&cpu_hwcap_keys[num]); +if (!cpucap_set_for_late_cpu (caps)) { +static_branch_enable (&cpu_hwcap_const_key[num]); +static_branch_enable (&cpu_hwcap_keys[num]); + }if (boot_scope && caps->cpu_enable)/*
arch/arm64/kernel/image-vars.h +1 -0
@@ -90,6 +90,7 @@ KVM_NVHE_ALIAS(__icache_flags);/* Kernel symbols needed for cpus_have_final/const_caps checks. */ KVM_NVHE_ALIAS (arm64_const_caps_ready);KVM_NVHE_ALIAS (cpu_hwcap_keys); +KVM_NVHE_ALIAS (cpu_hwcap_const_key);KVM_NVHE_ALIAS (cpu_hwcaps);/* Static keys which are set if a vGIC trap should be handled in hyp. */
--
Quoted 51 lines
arch/arm64/include/asm/cpufeature.h +0
-0static inline void cpus_set_cap (unsigned int num) {if (num >=ARM64_NCAPS ) {arch/arm64/kernel/cpu_errata.c +2
-1@@ -481,7 +481,8 @@ const struct arm64_cpu_capabilities arm64_errata[] = {* also need the non-affected CPUs to be able to come * in at any point in time. Wonderful. */ - .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, + .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE | + ARM64_CPUCAP_SET_FOR_LATE_CPU, },#endif #ifdef CONFIG_ARM64_WORKAROUND_SPECULATIVE_AT arch/arm64/kernel/cpufeature.c +11
-0@@ -1720,6 +1720,12 @@ cpucap_late_cpu_permitted(const struct arm64_cpu_capabilities *cap)return !!(cap->type &ARM64_CPUCAP_PERMITTED_FOR_LATE_CPU ); } +static bool +cpucap_set_for_late_cpu (const struct arm64_cpu_capabilities *cap) +{ +return !!(cap->type &ARM64_CPUCAP_SET_FOR_LATE_CPU ); +} +static bool cpucap_panic_on_conflict (const struct arm64_cpu_capabilities *cap) {@@ -2489,6 +2495,11 @@ static void verify_local_cpu_caps(u16 scope_mask)*/ if (cpu_has_cap && !cpucap_late_cpu_permitted (caps))break ; +/* +* Set the capability bit if it allows late setting. +*/ +if (cpucap_set_for_late_cpu (caps)) +cpus_set_cap (caps->capability); } }arch/arm64/kernel/process.c +0
-0
arch/arm64/kernel/process.c static void erratum_1418040_thread_switch(struct task_struct *prev, · line 529
Quoted 8 lines
arch/arm64/kernel/process.c +1
-1@@ -523,7 +523,7 @@ static void erratum_1418040_thread_switch(struct task_struct *prev,u64 val;if (!(IS_ENABLED (CONFIG_ARM64_ERRATUM_1418040 ) && -cpus_have_const_cap (ARM64_WORKAROUND_1418040 ))) +cpus_have_late_cap (ARM64_WORKAROUND_1418040 )))return ; prev32 =is_compat_thread (task_thread_info (prev));
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Catalin Marinas
maintainerQuoted 5 lines · Suzuki K Poulose
Catalin
Quoted 2 lines · Catalin Marinas
Quoted 1 line · Suzuki K Poulose
On 11/6/20 11:49 AM, Will Deacon wrote:
Quoted 16 lines
Review
Quoted 10 lines
(quoted diff) +10
-0+/* + * Test for a capability with a runtime check. This is an alias for + * cpus_have_cap() but with the name chosen to emphasize the applicability to + * late capability setting. + */ +static __always_inline bool cpus_have_late_cap(int num) +{ + return cpus_have_cap(num); +} +It is quite easy for people to misuse the ubiquitous
cpus_have_const_cap(). So, unless we make sure that we fix that to handle these "caps" we might see subtle bugs. How about the following fix on top of your patch, to add another (sigh!) set of keys to detect if we can use the const caps or not.
I thought about this as well but decided it's not worth the additional code.
Quoted 41 lines
Untested...
---8>---
arch/arm64/include/asm/cpufeature.h +10
-1@@ -379,6 +379,7 @@ cpucap_multi_entry_cap_matches(const struct arm64_cpu_capabilities *entry, extern DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);extern struct static_key_false cpu_hwcap_keys[ARM64_NCAPS ]; +extern struct static_key_false cpu_hwcap_const_key[ARM64_NCAPS ];extern struct static_key_false arm64_const_caps_ready;/* ARM64 CAPS + alternative_cb */ @@ -414,6 +415,14 @@ static inline bool cpus_have_cap(unsigned int num)return test_bit (num, cpu_hwcaps); } +static __always_inline bool cpucap_has_const_key(int num) +{ +if (num >=ARM64_NCAPS ) +return false; + +return static_branch_unlikely (&cpu_hwcap_const_key[num]); +} +/* * Test for a capability without a runtime check. * @@ -424,7 +433,7 @@ static inline bool cpus_have_cap(unsigned int num)*/ static __always_inline bool __cpus_have_const_cap(int num) { -if (num >=ARM64_NCAPS ) +if (num >=ARM64_NCAPS &&WARN_ON (!cpucap_has_const_key (num)))return false;return static_branch_unlikely (&cpu_hwcap_keys[num]); }
arch/arm64/include/asm/cpufeature.h static __always_inline bool __cpus_have_const_cap(int num) · line 454
Quoted 8 lines
arch/arm64/include/asm/cpufeature.h +1
-1@@ -439,7 +448,7 @@ static __always_inline bool __cpus_have_const_cap(int num)*/ static __always_inline bool cpus_have_const_cap(int num) { -if (system_capabilities_finalized ()) +if (cpucap_has_const_key (num) &&system_capabilities_finalized ())return __cpus_have_const_cap (num);else return cpus_have_cap (num);
I don't particularly like that we end up with three static key checks here but, OTOH, this has the advantage that we don't need a new cpus_have_late_cap().
Going back to the original problem, we want to allow late cpucap setting after the system caps have been finalised and, in a addition, we want to detect API misuse. We solve the former by setting the corresponding cpu_hwcaps bit late, based on a new capability flag.
For the API, we can use your approach with another static key and that's pretty foolproof, it gives us warnings. Alternatively (not as foolproof), we could skip the static key initialisation entirely for a late feature even if we detect it early and that would allow one to (hopefully) figure out the wrong API (cpus_have_const_cap() never true):
arch/arm64/kernel/cpufeature.c +2 -1
@@ -2436,7 +2436,8 @@ static void __init enable_cpu_capabilities(u16 scope_mask)continue ;/* Ensure cpus_have_const_cap(num) works */ -static_branch_enable (&cpu_hwcap_keys[num]); +if (!cpucap_set_for_late_cpu (caps)) +static_branch_enable (&cpu_hwcap_keys[num]);if (boot_scope && caps->cpu_enable)/*
Anyway, either option is fine by me.
Signature · 6 lines
Catalin _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 96 lines · Catalin Marinas
Quoted 5 lines · Suzuki K Poulose
Catalin
Quoted 2 lines · Catalin Marinas
Quoted 1 line · Suzuki K Poulose
On 11/6/20 11:49 AM, Will Deacon wrote:
Quoted 16 lines
Review
Quoted 10 lines
(quoted diff) +10
-0+/* + * Test for a capability with a runtime check. This is an alias for + * cpus_have_cap() but with the name chosen to emphasize the applicability to + * late capability setting. + */ +static __always_inline bool cpus_have_late_cap(int num) +{ + return cpus_have_cap(num); +} +It is quite easy for people to misuse the ubiquitous
cpus_have_const_cap(). So, unless we make sure that we fix that to handle these "caps" we might see subtle bugs. How about the following fix on top of your patch, to add another (sigh!) set of keys to detect if we can use the const caps or not.I thought about this as well but decided it's not worth the additional code.
Quoted 41 lines
Untested...
---8>---
arch/arm64/include/asm/cpufeature.h +10
-1@@ -379,6 +379,7 @@ cpucap_multi_entry_cap_matches(const struct arm64_cpu_capabilities *entry, extern DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);extern struct static_key_false cpu_hwcap_keys[ARM64_NCAPS ]; +extern struct static_key_false cpu_hwcap_const_key[ARM64_NCAPS ];extern struct static_key_false arm64_const_caps_ready;/* ARM64 CAPS + alternative_cb */ @@ -414,6 +415,14 @@ static inline bool cpus_have_cap(unsigned int num)return test_bit (num, cpu_hwcaps); } +static __always_inline bool cpucap_has_const_key(int num) +{ +if (num >=ARM64_NCAPS ) +return false; + +return static_branch_unlikely (&cpu_hwcap_const_key[num]); +} +/* * Test for a capability without a runtime check. * @@ -424,7 +433,7 @@ static inline bool cpus_have_cap(unsigned int num)*/ static __always_inline bool __cpus_have_const_cap(int num) { -if (num >=ARM64_NCAPS ) +if (num >=ARM64_NCAPS &&WARN_ON (!cpucap_has_const_key (num)))return false;return static_branch_unlikely (&cpu_hwcap_keys[num]); }arch/arm64/include/asm/cpufeature.h static __always_inline bool __cpus_have_const_cap(int num) · line 454
Quoted 8 lines
arch/arm64/include/asm/cpufeature.h +1
-1@@ -439,7 +448,7 @@ static __always_inline bool __cpus_have_const_cap(int num)*/ static __always_inline bool cpus_have_const_cap(int num) { -if (system_capabilities_finalized ()) +if (cpucap_has_const_key (num) &&system_capabilities_finalized ())return __cpus_have_const_cap (num);else return cpus_have_cap (num);I don't particularly like that we end up with three static key checks here but, OTOH, this has the advantage that we don't need a new
cpus_have_late_cap().Going back to the original problem, we want to allow late cpucap setting after the system caps have been finalised and, in a addition, we want to detect API misuse. We solve the former by setting the corresponding
cpu_hwcapsbit late, based on a new capability flag.For the API, we can use your approach with another static key and that's pretty foolproof, it gives us warnings. Alternatively (not as foolproof), we could skip the static key initialisation entirely for a late feature even if we detect it early and that would allow one to (hopefully) figure out the wrong API (
cpus_have_const_cap()never true):arch/arm64/kernel/cpufeature.c +0
-0
arch/arm64/kernel/cpufeature.c static void __init enable_cpu_capabilities(u16 scope_mask) · line 2440
Quoted 6 lines
arch/arm64/kernel/cpufeature.c +2
-1@@ -2436,7 +2436,8 @@ static void __init enable_cpu_capabilities(u16 scope_mask)continue ;/* Ensure cpus_have_const_cap(num) works */ -static_branch_enable (&cpu_hwcap_keys[num]); +if (!cpucap_set_for_late_cpu (caps)) +static_branch_enable (&cpu_hwcap_keys[num]);
True, I think this would do the trick, provided proper testing is
performed ;-). The issue is, we would advertise the "errata work around" (in dmesg) and may not really apply it where it matters. So, if the testing doesn't really verify this, but instead looks for the message,
then we are back to the problem the original patch was trying to solve.
Cheers
Suzuki
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel