diff mbox series

[v2,2/5] x86/cpuid: refactor setup_clear_cpu_cap()/clear_cpu_cap()

Message ID 20220718141123.136106-3-mlevitsk@redhat.com (mailing list archive)
State New, archived
Headers show
Series x86: cpuid: improve support for broken CPUID configurations | expand

Commit Message

Maxim Levitsky July 18, 2022, 2:11 p.m. UTC
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(-)

Comments

Borislav Petkov Oct. 21, 2022, 4:19 p.m. UTC | #1
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.
Borislav Petkov Nov. 7, 2022, 7:10 p.m. UTC | #2
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.
Borislav Petkov Nov. 7, 2022, 9:57 p.m. UTC | #3
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 mbox series

Patch

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);
 }