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

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette