Re: [PATCH v2 3/3] x86/fred: Enable FRED right after init_mem_mapping()

[Xin Li <xin@zytor.com>]

On 8/13/2024 5:45 AM, Thomas Gleixner wrote:
> On Tue, Jul 09 2024 at 08:40, Xin Li wrote:
> 
> I'm really unhappy about sprinkling all these FRED conditionals all over
> the place:

Sigh, for the reason of a bad taste...


>>   	init_mem_mapping();
>>   
>> -	idt_setup_early_pf();
>> +	/*
>> +	 * init_mem_mapping() uses early IDT to setup memory mappings, thus FRED
>> +	 * can't be enabled earlier than that, unless FRED adds support to setup
>> +	 * memory mappings.
>> +	 */
>> +	if (cpu_feature_enabled(X86_FEATURE_FRED))
>> +		cpu_init_fred_exceptions();
>> +	else
>> +		idt_setup_early_pf();
>    
>> @@ -248,6 +249,11 @@ static void notrace start_secondary(void *unused)
>>   
>>   	cpu_init_exception_handling();
>>   
>> +	if (cpu_feature_enabled(X86_FEATURE_FRED)) {
>> +		cpu_init_fred_exceptions();
>> +		cpu_init_fred_rsps();
>> +	}
> 
>>   	/* Init cpu_entry_area before IST entries are set up */
>>   	setup_cpu_entry_areas();
>>   
>> +	/* FRED RSPs pointing to memory from CPU entry areas */
>> +	if (cpu_feature_enabled(X86_FEATURE_FRED))
>> +		cpu_init_fred_rsps();
>> +
>>   	/* Init GHCB memory pages when running as an SEV-ES guest */
>>   	sev_es_init_vc_handling();
> 
> This really can be encapsulated and kept in places which need to know
> about FRED already. See below. Can you please validate?

I reviewed your patch, and there is no reason it won't work; my tests
show no problem with it.

Thanks!
     Xin


> ---
> From: "Xin Li (Intel)" <xin@zytor.com>
> Subject: x86/fred: Enable FRED right after init_mem_mapping()
> Date: Tue, 09 Jul 2024 08:40:48 -0700
> 
> From: "Xin Li (Intel)" <xin@zytor.com>
> 
> On 64-bit init_mem_mapping() relies on the minimal page fault handler
> provided by the early IDT mechanism. The real page fault handler is
> installed right afterwards into the IDT.
> 
> This is problematic on CPUs which have X86_FEATURE_FRED set because the
> real page fault handler retrieves the faulting address from the FRED
> exception stack frame and not from CR2, but that does obviously not work
> when FRED is not yet enabled in the CPU.
> 
> To prevent this enable FRED right after init_mem_mapping() without
> interrupt stacks. Those are enabled later in trap_init() after the CPU
> entry area is set up.
> 
> [ tglx: Encapsulate the FRED details ]
> 
> Fixes: 14619d912b65 ("x86/fred: FRED entry/exit and dispatch code")
> Reported-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Link: https://lore.kernel.org/all/20240709154048.3543361-4-xin@zytor.com
> ---
>   arch/x86/include/asm/processor.h |    3 ++-
>   arch/x86/kernel/cpu/common.c     |   15 +++++++++++++--
>   arch/x86/kernel/setup.c          |    7 ++++++-
>   arch/x86/kernel/smpboot.c        |    2 +-
>   arch/x86/kernel/traps.c          |    2 +-
>   5 files changed, 23 insertions(+), 6 deletions(-)
> 
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -582,7 +582,8 @@ extern void switch_gdt_and_percpu_base(i
>   extern void load_direct_gdt(int);
>   extern void load_fixmap_gdt(int);
>   extern void cpu_init(void);
> -extern void cpu_init_exception_handling(void);
> +extern void cpu_init_exception_handling(bool boot_cpu);
> +extern void cpu_init_replace_early_idt(void);
>   extern void cr4_init(void);
>   
>   extern void set_task_blockstep(struct task_struct *task, bool on);
> --- 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(s
>    * 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();
> @@ -2196,13 +2196,24 @@ void cpu_init_exception_handling(void)
>   	setup_ghcb();
>   
>   	if (cpu_feature_enabled(X86_FEATURE_FRED)) {
> -		cpu_init_fred_exceptions();
> +		/* The boot CPU has enabled FRED during early boot */
> +		if (!boot_cpu)
> +			cpu_init_fred_exceptions();
> +
>   		cpu_init_fred_rsps();
>   	} else {
>   		load_current_idt();
>   	}
>   }
>   
> +void __init cpu_init_replace_early_idt(void)
> +{
> +	if (cpu_feature_enabled(X86_FEATURE_FRED))
> +		cpu_init_fred_exceptions();
> +	else
> +		idt_setup_early_pf();
> +}
> +
>   /*
>    * cpu_init() initializes state that is per-CPU. Some data is already
>    * initialized (naturally) in the bootstrap process, such as the GDT.  We
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -1039,7 +1039,12 @@ void __init setup_arch(char **cmdline_p)
>   
>   	init_mem_mapping();
>   
> -	idt_setup_early_pf();
> +	/*
> +	 * init_mem_mapping() relies on the early IDT page fault handling.
> +	 * Now either enable FRED or install the real page fault handler
> +	 * for 64-bit in the IDT.
> +	 */
> +	cpu_init_replace_early_idt();
>   
>   	/*
>   	 * Update mmu_cr4_features (and, indirectly, trampoline_cr4_features)
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -246,7 +246,7 @@ static void notrace start_secondary(void
>   		__flush_tlb_all();
>   	}
>   
> -	cpu_init_exception_handling();
> +	cpu_init_exception_handling(false);
>   
>   	/*
>   	 * Load the microcode before reaching the AP alive synchronization
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -1411,7 +1411,7 @@ void __init trap_init(void)
>   	sev_es_init_vc_handling();
>   
>   	/* Initialize TSS before setting up traps so ISTs work */
> -	cpu_init_exception_handling();
> +	cpu_init_exception_handling(true);
>   
>   	/* Setup traps as cpu_init() might #GP */
>   	if (!cpu_feature_enabled(X86_FEATURE_FRED))
>