| Message ID | 20220718141123.136106-3-mlevitsk@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86: cpuid: improve support for broken CPUID configurations | expand |
On Mon, Jul 18, 2022 at 05:11:20PM +0300, Maxim Levitsky wrote: > Currently setup_clear_cpu_cap passes NULL 'struct cpuinfo_x86*' > to clear_cpu_cap to indicate that capability should be cleared from boot_cpu_data. > > Later that is used in clear_feature to do recursive call to > clear_cpu_cap together with clearing the feature bit from 'cpu_caps_cleared' > > Remove that code and just call the do_clear_cpu_cap on boot_cpu_data directly > from the setup_clear_cpu_cap. > > The only functional change this introduces is that now calling clear_cpu_cap > explicitly on boot_cpu_data also sets the bits in cpu_caps_cleared, > which is the only thing that makes sense anyway. > > All callers of both functions were checked for this and fixed. Change looks ok. What I can't grok is this sentence: what was checked and fixed where? What does need fixing and why? Thx.
On Fri, Oct 21, 2022 at 06:19:29PM +0200, Borislav Petkov wrote: > On Mon, Jul 18, 2022 at 05:11:20PM +0300, Maxim Levitsky wrote: > > Currently setup_clear_cpu_cap passes NULL 'struct cpuinfo_x86*' > > to clear_cpu_cap to indicate that capability should be cleared from boot_cpu_data. > > > > Later that is used in clear_feature to do recursive call to > > clear_cpu_cap together with clearing the feature bit from 'cpu_caps_cleared' > > > > Remove that code and just call the do_clear_cpu_cap on boot_cpu_data directly > > from the setup_clear_cpu_cap. > > > > The only functional change this introduces is that now calling clear_cpu_cap > > explicitly on boot_cpu_data also sets the bits in cpu_caps_cleared, > > which is the only thing that makes sense anyway. > > > > All callers of both functions were checked for this and fixed. > > Change looks ok. What I can't grok is this sentence: what was checked > and fixed where? Ok, I think I know what you mean. That: git grep -E "clear_cpu_cap.*boot" arch/x86/events/intel/lbr.c:1599: clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR); arch/x86/kernel/alternative.c:746: clear_cpu_cap(&boot_cpu_data, X86_FEATURE_UP); Right, so here's the difference: When you call setup_clear_cpu_cap(), it basically means, to disable the cap on *every* CPU. This is done with cpu_caps_cleared which gets ANDed in in apply_forced_caps(). clear_cpu_cap() clears the bit *only* in the first parameter supplied. Now, that first parameter can be boot_cpu_data too but then, strictly speaking, clear_cpu_cap() would really do what you want it to do - to clear it only in its first param. If you really want to enforce that bit cleared everywhere, you need to use the setup_* variant. So this patch is actually incorrect but I admit, the CPU caps handling are kinda subtle and probably need cleaning. Lemme document it so that it is at least clear. Who knows, we might end up improving it in the process. :-) Thx.
On Mon, Nov 07, 2022 at 08:10:42PM +0100, Borislav Petkov wrote: > Lemme document it so that it is at least clear. Who knows, we might end > up improving it in the process. IOW, something like this. It's a start at least... https://lore.kernel.org/r/20221107211505.8572-1-bp@alien8.de
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c index c881bcafba7d70..d6777d07ba3302 100644 --- a/arch/x86/kernel/cpu/cpuid-deps.c +++ b/arch/x86/kernel/cpu/cpuid-deps.c @@ -88,18 +88,16 @@ static inline void clear_feature(struct cpuinfo_x86 *c, unsigned int feature) * rest of the cpufeature code uses atomics as well, so keep it for * consistency. Cleanup all of it separately. */ - if (!c) { - clear_cpu_cap(&boot_cpu_data, feature); + clear_bit(feature, (unsigned long *)c->x86_capability); + + if (c == &boot_cpu_data) set_bit(feature, (unsigned long *)cpu_caps_cleared); - } else { - clear_bit(feature, (unsigned long *)c->x86_capability); - } } /* Take the capabilities and the BUG bits into account */ #define MAX_FEATURE_BITS ((NCAPINTS + NBUGINTS) * sizeof(u32) * 8) -static void do_clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature) +void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature) { DECLARE_BITMAP(disable, MAX_FEATURE_BITS); const struct cpuid_dep *d; @@ -129,12 +127,7 @@ static void do_clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature) } while (changed); } -void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature) -{ - do_clear_cpu_cap(c, feature); -} - void setup_clear_cpu_cap(unsigned int feature) { - do_clear_cpu_cap(NULL, feature); + clear_cpu_cap(&boot_cpu_data, feature); }
Currently setup_clear_cpu_cap passes NULL 'struct cpuinfo_x86*' to clear_cpu_cap to indicate that capability should be cleared from boot_cpu_data. Later that is used in clear_feature to do recursive call to clear_cpu_cap together with clearing the feature bit from 'cpu_caps_cleared' Remove that code and just call the do_clear_cpu_cap on boot_cpu_data directly from the setup_clear_cpu_cap. The only functional change this introduces is that now calling clear_cpu_cap explicitly on boot_cpu_data also sets the bits in cpu_caps_cleared, which is the only thing that makes sense anyway. All callers of both functions were checked for this and fixed. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- arch/x86/kernel/cpu/cpuid-deps.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)