Re: [PATCH v1] x86/smp: Set up exception handling before cr4_init()

[Xin Li <xin@zytor.com>]

>> + /*
>> +  * Ensure bits set in cr4_pinned_bits are set in CR4.
>> +  *
>> +  * cr4_pinned_bits is a subset of cr4_pinned_mask, which includes
>> +  * the following bits:
>> +  *         X86_CR4_SMEP
>> +  *         X86_CR4_SMAP
>> +  *         X86_CR4_UMIP
>> +  *         X86_CR4_FSGSBASE
>> +  *         X86_CR4_CET
>> +  *         X86_CR4_FRED
>> +  */
>> + cr4_init();
> 
> I'm not as big of a fan of this comment. The next pinned bit that gets
> added will make this stale. Could we try to make this more timeless, please?

Right, it’s still a potential problem.

> 
> I'm also not sure I like the asymmetry of this between the boot and
> secondary CPUs. On a boot CPU, CR4.SMEP will get set via
> identify_boot_cpu() and eventually setup_smep(). On a secondary CPU,
> it'll get set in cr4_init() and *not* in setup_smep().
> 
> This asymmetry is (I think) part of what the root of the problem is here
> and how this bug came to be.

Are you proposing a single CPU initialization function for both BSP and
APs?

It reminds me that tglx proposed the following change when I was fixing
a FRED boot order bug.

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 6de12b3c1b04..a4735d9b5a1d 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2176,7 +2176,7 @@ static inline void tss_setup_io_bitmap(struct tss_struct *tss)
  * Setup everything needed to handle exceptions from the IDT, including the IST
  * exceptions which use paranoid_entry().
  */
-void cpu_init_exception_handling(void)
+void cpu_init_exception_handling(bool boot_cpu)
 {
 	struct tss_struct *tss = this_cpu_ptr(&cpu_tss_rw);
 	int cpu = raw_smp_processor_id();


So something like

	void cpu_init(bool boot_cpu)

and it gets called on both BSP and APs.

> 
> I really think the current pinning behavior is too invasive. It has zero
> benefit to be pinning CR bits this early in bringup. It's only causing pain.


Maybe someone can explain why it’s added like this before I find time to
dig into it.

I’m curious why cr4_init() is not part of the following cpu_init()? IOW,
why does it need to be called so early in the existing code?


> 
> I _really_ think we need a defined per-cpu point where pinning comes
> into effect. Marking the CPU online is one idea.
> 
> Thoughts?

It seems a good fit.  Just that {on,off}line() are not called on BSP (not
a real problem).

Question is that who would work on it ;) ?