[PATCH v2 0/3] Enable FRED earlier

[PATCH v2 0/3] Enable FRED earlier

[Xin Li (Intel)]

Wenlong Hou from Ant group reported two problems during the FRED
initialization:
https://lore.kernel.org/lkml/cover.1718972598.git.houwenlong.hwl@antgroup.com/

The first problem is that spurious_interrupt() gets called on the
HYPERVISOR_CALLBACK_VECTOR vector.  Because kvm_guest_init(), being
executed way before trap_init() in which it is decided that whether
FRED will be enabled or not, calls sysvec_install() to install
HYPERVISOR_CALLBACK_VECTOR's interrupt handler into FRED system
vector dispatch table or IDT depending on whether FRED is enabled.

The other problem is that the #PF handler gets a wrong faulting
address from the stack instead of CR2 before FRED is enabled.
Because the #PF handler fetches its faulting addresss from the
stack or CR2 based on whether FRED is available rather than active.

This patchset fixes the 2 problems with suggestions from tglx:

  1) Parse cmdline param "fred=" in cpu_parse_early_param() to
     minimize the gap mentioned above, before kvm_guest_init().

  2) Enable FRED right after init_mem_mapping() to switch to FRED
     from early IDT ASAP, avoid intermediately using the IDT #PF
     handler.


Link to v1:
https://lore.kernel.org/lkml/20240703085426.274801-1-xin@zytor.com/

Changes since v1:
* Drop the patch that changes wrmsrl() to wrmsrns().
* Use strncmp() instead of strcmp().


Xin Li (Intel) (3):
  x86/fred: Parse cmdline param "fred=" in cpu_parse_early_param()
  x86/fred: Split FRED RSP initialization into a separate function
  x86/fred: Enable FRED right after init_mem_mapping()

 arch/x86/include/asm/fred.h  |  2 ++
 arch/x86/kernel/cpu/common.c |  9 ++++++---
 arch/x86/kernel/fred.c       | 28 +++++++++++++++++++---------
 arch/x86/kernel/setup.c      | 11 ++++++++++-
 arch/x86/kernel/smpboot.c    |  6 ++++++
 arch/x86/kernel/traps.c      | 30 ++++--------------------------
 6 files changed, 47 insertions(+), 39 deletions(-)


base-commit: aa9d8caba6e40b0b02a6f2b5f2bd9177cd76cacf
-- 
2.45.2

[PATCH v2 1/3] x86/fred: Parse cmdline param "fred=" in cpu_parse_early_param()

[Xin Li (Intel)]

Depending on whether FRED will be used, sysvec_install() installs
a system interrupt handler into FRED system vector dispatch table
or IDT.  However FRED can be disabled later in trap_init(), after
sysvec_install() is called.  E.g., the HYPERVISOR_CALLBACK_VECTOR
handler is registered with sysvec_install() in kvm_guest_init(),
which is called in setup_arch() but way before trap_init().  IOW,
there is a gap between FRED is available and available but disabled.
As a result, when FRED is available but disabled, its IDT handler
is not installed thus spurious_interrupt() will be invoked.

Fix it by parsing cmdline param "fred=" in cpu_parse_early_param()
to minimize the gap between FRED is available and available but
disabled.

Fixes: 3810da12710a ("x86/fred: Add a fred= cmdline param")
Reported-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---

Change since v1:
* Use strncmp() instead of strcmp().
---
 arch/x86/kernel/cpu/common.c |  5 +++++
 arch/x86/kernel/traps.c      | 26 --------------------------
 2 files changed, 5 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index d4e539d4e158..10a5402d8297 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1510,6 +1510,11 @@ static void __init cpu_parse_early_param(void)
 	if (cmdline_find_option_bool(boot_command_line, "nousershstk"))
 		setup_clear_cpu_cap(X86_FEATURE_USER_SHSTK);
 
+	/* Minimize the gap between FRED is available and available but disabled. */
+	arglen = cmdline_find_option(boot_command_line, "fred", arg, sizeof(arg));
+	if (arglen != 2 || strncmp(arg, "on", 2))
+		setup_clear_cpu_cap(X86_FEATURE_FRED);
+
 	arglen = cmdline_find_option(boot_command_line, "clearcpuid", arg, sizeof(arg));
 	if (arglen <= 0)
 		return;
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 4fa0b17e5043..6afb41e6cbbb 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -1402,34 +1402,8 @@ DEFINE_IDTENTRY_SW(iret_error)
 }
 #endif
 
-/* Do not enable FRED by default yet. */
-static bool enable_fred __ro_after_init = false;
-
-#ifdef CONFIG_X86_FRED
-static int __init fred_setup(char *str)
-{
-	if (!str)
-		return -EINVAL;
-
-	if (!cpu_feature_enabled(X86_FEATURE_FRED))
-		return 0;
-
-	if (!strcmp(str, "on"))
-		enable_fred = true;
-	else if (!strcmp(str, "off"))
-		enable_fred = false;
-	else
-		pr_warn("invalid FRED option: 'fred=%s'\n", str);
-	return 0;
-}
-early_param("fred", fred_setup);
-#endif
-
 void __init trap_init(void)
 {
-	if (cpu_feature_enabled(X86_FEATURE_FRED) && !enable_fred)
-		setup_clear_cpu_cap(X86_FEATURE_FRED);
-
 	/* Init cpu_entry_area before IST entries are set up */
 	setup_cpu_entry_areas();
 
-- 
2.45.2

Re: [PATCH v2 1/3] x86/fred: Parse cmdline param "fred=" in cpu_parse_early_param()

[Nikolay Borisov]

On 9.07.24 г. 18:40 ч., Xin Li (Intel) wrote:
> Depending on whether FRED will be used, sysvec_install() installs
> a system interrupt handler into FRED system vector dispatch table
> or IDT.  However FRED can be disabled later in trap_init(), after
> sysvec_install() is called.  E.g., the HYPERVISOR_CALLBACK_VECTOR
> handler is registered with sysvec_install() in kvm_guest_init(),
> which is called in setup_arch() but way before trap_init().  IOW,
> there is a gap between FRED is available and available but disabled.
> As a result, when FRED is available but disabled, its IDT handler
> is not installed thus spurious_interrupt() will be invoked.
> 
> Fix it by parsing cmdline param "fred=" in cpu_parse_early_param()
> to minimize the gap between FRED is available and available but
> disabled.
> 
> Fixes: 3810da12710a ("x86/fred: Add a fred= cmdline param")
> Reported-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> ---
> 
> Change since v1:
> * Use strncmp() instead of strcmp().
> ---
>   arch/x86/kernel/cpu/common.c |  5 +++++
>   arch/x86/kernel/traps.c      | 26 --------------------------
>   2 files changed, 5 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index d4e539d4e158..10a5402d8297 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1510,6 +1510,11 @@ static void __init cpu_parse_early_param(void)
>   	if (cmdline_find_option_bool(boot_command_line, "nousershstk"))
>   		setup_clear_cpu_cap(X86_FEATURE_USER_SHSTK);
>   
> +	/* Minimize the gap between FRED is available and available but disabled. */
> +	arglen = cmdline_find_option(boot_command_line, "fred", arg, sizeof(arg));
> +	if (arglen != 2 || strncmp(arg, "on", 2))

I'm confused why you keep perverting the calling convention of 
cmdline_find_option. The doc clearly states:

     * Returns the position of that @option (starts counting with 1) 

     * or 0 on not found.  @option will only be found if it is found 

     * as an entire word in @cmdline.  For instance, if @option="car" 

     * then a cmdline which contains "cart" will not match.

You should only care if arglen is non 0, which if it is you check if its 
value equal 'on', why bother with its starting position?

> +		setup_clear_cpu_cap(X86_FEATURE_FRED);
> +
>   	arglen = cmdline_find_option(boot_command_line, "clearcpuid", arg, sizeof(arg));
>   	if (arglen <= 0)
>   		return;
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 4fa0b17e5043..6afb41e6cbbb 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -1402,34 +1402,8 @@ DEFINE_IDTENTRY_SW(iret_error)
>   }
>   #endif
>   
> -/* Do not enable FRED by default yet. */
> -static bool enable_fred __ro_after_init = false;
> -
> -#ifdef CONFIG_X86_FRED
> -static int __init fred_setup(char *str)
> -{
> -	if (!str)
> -		return -EINVAL;
> -
> -	if (!cpu_feature_enabled(X86_FEATURE_FRED))
> -		return 0;
> -
> -	if (!strcmp(str, "on"))
> -		enable_fred = true;
> -	else if (!strcmp(str, "off"))
> -		enable_fred = false;
> -	else
> -		pr_warn("invalid FRED option: 'fred=%s'\n", str);
> -	return 0;
> -}
> -early_param("fred", fred_setup);
> -#endif
> -
>   void __init trap_init(void)
>   {
> -	if (cpu_feature_enabled(X86_FEATURE_FRED) && !enable_fred)
> -		setup_clear_cpu_cap(X86_FEATURE_FRED);
> -
>   	/* Init cpu_entry_area before IST entries are set up */
>   	setup_cpu_entry_areas();
>   

Re: [PATCH v2 1/3] x86/fred: Parse cmdline param "fred=" in cpu_parse_early_param()

[Xin Li]

On 7/10/2024 11:53 AM, Nikolay Borisov wrote:
> On 9.07.24 г. 18:40 ч., Xin Li (Intel) wrote:

>> @@ -1510,6 +1510,11 @@ static void __init cpu_parse_early_param(void)
>>       if (cmdline_find_option_bool(boot_command_line, "nousershstk"))
>>           setup_clear_cpu_cap(X86_FEATURE_USER_SHSTK);
>> +    /* Minimize the gap between FRED is available and available but 
>> disabled. */
>> +    arglen = cmdline_find_option(boot_command_line, "fred", arg, 
>> sizeof(arg));
>> +    if (arglen != 2 || strncmp(arg, "on", 2))
> 
> I'm confused why you keep perverting the calling convention of 
> cmdline_find_option. The doc clearly states:
> 
>      * Returns the position of that @option (starts counting with 1)
>      * or 0 on not found.  @option will only be found if it is found
>      * as an entire word in @cmdline.  For instance, if @option="car"
>      * then a cmdline which contains "cart" will not match.
> 
> You should only care if arglen is non 0, which if it is you check if its 
> value equal 'on', why bother with its starting position?
> 

Well, just look at how it is used in match_option() in
arch/x86/kernel/cpu/bugs.c and arch/x86/kernel/cpu/intel.c.

This is a short version and it will be expanded once we have more
option strings well defined (match_option() should be a common lib
function then).

Re: [PATCH v2 1/3] x86/fred: Parse cmdline param "fred=" in cpu_parse_early_param()

[Nikolay Borisov]

On 12.07.24 г. 20:40 ч., Xin Li wrote:
> On 7/10/2024 11:53 AM, Nikolay Borisov wrote:
>> On 9.07.24 г. 18:40 ч., Xin Li (Intel) wrote:
> 
>>> @@ -1510,6 +1510,11 @@ static void __init cpu_parse_early_param(void)
>>>       if (cmdline_find_option_bool(boot_command_line, "nousershstk"))
>>>           setup_clear_cpu_cap(X86_FEATURE_USER_SHSTK);
>>> +    /* Minimize the gap between FRED is available and available but 
>>> disabled. */
>>> +    arglen = cmdline_find_option(boot_command_line, "fred", arg, 
>>> sizeof(arg));
>>> +    if (arglen != 2 || strncmp(arg, "on", 2))
>>
>> I'm confused why you keep perverting the calling convention of 
>> cmdline_find_option. The doc clearly states:
>>
>>      * Returns the position of that @option (starts counting with 1)
>>      * or 0 on not found.  @option will only be found if it is found
>>      * as an entire word in @cmdline.  For instance, if @option="car"
>>      * then a cmdline which contains "cart" will not match.
>>
>> You should only care if arglen is non 0, which if it is you check if 
>> its value equal 'on', why bother with its starting position?
>>


Actually, I have quoted the wrong doc, the correct one is:

"
Returns the length of the argument (regardless of if it was
truncated to fit in the buffer), or -1 on not found.
"

> 
> Well, just look at how it is used in match_option() in
> arch/x86/kernel/cpu/bugs.c and arch/x86/kernel/cpu/intel.c.

Exactly, in bugs.c it's used as I've suggested:

In spectre_v2_parse_user_cmdline it checks if spectre_v2_user is present 
(if a negative value is returned) and if not it returns some default.

In spectre_v2_parse_cmdline it's used exactly the same way - return some 
default if that function returns a negative value (spectre_v2 check) or 
return some specific value if it found nospectre_v2.

And in sld_state_setup the code just checks for a non-negative value i.e 
the argument has been found.

Otoh, I see what you are trying to say if I look at the usage of this 
function in arch/x86/boot/compressed/acpi.c


Still I find this convention a bit counter-intuitive, but given it's not 
a precedent I'm fine with leaving it as is.




> 
> This is a short version and it will be expanded once we have more
> option strings well defined (match_option() should be a common lib
> function then).
> 

[tip: x86/fred] x86/fred: Parse cmdline param "fred=" in cpu_parse_early_param()

[tip-bot2 for Xin Li (Intel)]

The following commit has been merged into the x86/fred branch of tip:

Commit-ID:     989b5cfaa7b6054f4e1bde914470ee091c23e6a5
Gitweb:        https://git.kernel.org/tip/989b5cfaa7b6054f4e1bde914470ee091c23e6a5
Author:        Xin Li (Intel) <xin@zytor.com>
AuthorDate:    Tue, 09 Jul 2024 08:40:46 -07:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 13 Aug 2024 21:59:21 +02:00

x86/fred: Parse cmdline param "fred=" in cpu_parse_early_param()

Depending on whether FRED is enabled, sysvec_install() installs a system
interrupt handler into either into FRED's system vector dispatch table or
into the IDT.

However FRED can be disabled later in trap_init(), after sysvec_install()
has been invoked already; e.g., the HYPERVISOR_CALLBACK_VECTOR handler is
registered with sysvec_install() in kvm_guest_init(), which is called in
setup_arch() but way before trap_init().

IOW, there is a gap between FRED is available and available but disabled.
As a result, when FRED is available but disabled, early sysvec_install()
invocations fail to install the IDT handler resulting in spurious
interrupts.

Fix it by parsing cmdline param "fred=" in cpu_parse_early_param() to
ensure that FRED is disabled before the first sysvec_install() incovations.

Fixes: 3810da12710a ("x86/fred: Add a fred= cmdline param")
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-2-xin@zytor.com

---
 arch/x86/kernel/cpu/common.c |  5 +++++
 arch/x86/kernel/traps.c      | 26 --------------------------
 2 files changed, 5 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index d4e539d..10a5402 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1510,6 +1510,11 @@ static void __init cpu_parse_early_param(void)
 	if (cmdline_find_option_bool(boot_command_line, "nousershstk"))
 		setup_clear_cpu_cap(X86_FEATURE_USER_SHSTK);
 
+	/* Minimize the gap between FRED is available and available but disabled. */
+	arglen = cmdline_find_option(boot_command_line, "fred", arg, sizeof(arg));
+	if (arglen != 2 || strncmp(arg, "on", 2))
+		setup_clear_cpu_cap(X86_FEATURE_FRED);
+
 	arglen = cmdline_find_option(boot_command_line, "clearcpuid", arg, sizeof(arg));
 	if (arglen <= 0)
 		return;
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 4fa0b17..6afb41e 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -1402,34 +1402,8 @@ DEFINE_IDTENTRY_SW(iret_error)
 }
 #endif
 
-/* Do not enable FRED by default yet. */
-static bool enable_fred __ro_after_init = false;
-
-#ifdef CONFIG_X86_FRED
-static int __init fred_setup(char *str)
-{
-	if (!str)
-		return -EINVAL;
-
-	if (!cpu_feature_enabled(X86_FEATURE_FRED))
-		return 0;
-
-	if (!strcmp(str, "on"))
-		enable_fred = true;
-	else if (!strcmp(str, "off"))
-		enable_fred = false;
-	else
-		pr_warn("invalid FRED option: 'fred=%s'\n", str);
-	return 0;
-}
-early_param("fred", fred_setup);
-#endif
-
 void __init trap_init(void)
 {
-	if (cpu_feature_enabled(X86_FEATURE_FRED) && !enable_fred)
-		setup_clear_cpu_cap(X86_FEATURE_FRED);
-
 	/* Init cpu_entry_area before IST entries are set up */
 	setup_cpu_entry_areas();
 

[PATCH v2 2/3] x86/fred: Split FRED RSP initialization into a separate function

[Xin Li (Intel)]

To enable FRED earlier, split FRED RSP initialization into a separate
function, as they are initialized with memory from CPU entry areas,
thus their initialization has to be kept after setup_cpu_entry_areas().

No functional change intended.

Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---
 arch/x86/include/asm/fred.h  |  2 ++
 arch/x86/kernel/cpu/common.c |  6 ++++--
 arch/x86/kernel/fred.c       | 28 +++++++++++++++++++---------
 3 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/fred.h b/arch/x86/include/asm/fred.h
index e86c7ba32435..66d7dbe2d314 100644
--- a/arch/x86/include/asm/fred.h
+++ b/arch/x86/include/asm/fred.h
@@ -84,11 +84,13 @@ static __always_inline void fred_entry_from_kvm(unsigned int type, unsigned int
 }
 
 void cpu_init_fred_exceptions(void);
+void cpu_init_fred_rsps(void);
 void fred_complete_exception_setup(void);
 
 #else /* CONFIG_X86_FRED */
 static __always_inline unsigned long fred_event_data(struct pt_regs *regs) { return 0; }
 static inline void cpu_init_fred_exceptions(void) { }
+static inline void cpu_init_fred_rsps(void) { }
 static inline void fred_complete_exception_setup(void) { }
 static __always_inline void fred_entry_from_kvm(unsigned int type, unsigned int vector) { }
 #endif /* CONFIG_X86_FRED */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 10a5402d8297..6de12b3c1b04 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2195,10 +2195,12 @@ void cpu_init_exception_handling(void)
 	/* GHCB needs to be setup to handle #VC. */
 	setup_ghcb();
 
-	if (cpu_feature_enabled(X86_FEATURE_FRED))
+	if (cpu_feature_enabled(X86_FEATURE_FRED)) {
 		cpu_init_fred_exceptions();
-	else
+		cpu_init_fred_rsps();
+	} else {
 		load_current_idt();
+	}
 }
 
 /*
diff --git a/arch/x86/kernel/fred.c b/arch/x86/kernel/fred.c
index 4bcd8791ad96..99a134fcd5bf 100644
--- a/arch/x86/kernel/fred.c
+++ b/arch/x86/kernel/fred.c
@@ -32,6 +32,25 @@ void cpu_init_fred_exceptions(void)
 	       FRED_CONFIG_INT_STKLVL(0) |
 	       FRED_CONFIG_ENTRYPOINT(asm_fred_entrypoint_user));
 
+	wrmsrl(MSR_IA32_FRED_STKLVLS, 0);
+	wrmsrl(MSR_IA32_FRED_RSP0, 0);
+	wrmsrl(MSR_IA32_FRED_RSP1, 0);
+	wrmsrl(MSR_IA32_FRED_RSP2, 0);
+	wrmsrl(MSR_IA32_FRED_RSP3, 0);
+
+	/* Enable FRED */
+	cr4_set_bits(X86_CR4_FRED);
+	/* Any further IDT use is a bug */
+	idt_invalidate();
+
+	/* Use int $0x80 for 32-bit system calls in FRED mode */
+	setup_clear_cpu_cap(X86_FEATURE_SYSENTER32);
+	setup_clear_cpu_cap(X86_FEATURE_SYSCALL32);
+}
+
+/* Must be called after setup_cpu_entry_areas() */
+void cpu_init_fred_rsps(void)
+{
 	/*
 	 * The purpose of separate stacks for NMI, #DB and #MC *in the kernel*
 	 * (remember that user space faults are always taken on stack level 0)
@@ -47,13 +66,4 @@ void cpu_init_fred_exceptions(void)
 	wrmsrl(MSR_IA32_FRED_RSP1, __this_cpu_ist_top_va(DB));
 	wrmsrl(MSR_IA32_FRED_RSP2, __this_cpu_ist_top_va(NMI));
 	wrmsrl(MSR_IA32_FRED_RSP3, __this_cpu_ist_top_va(DF));
-
-	/* Enable FRED */
-	cr4_set_bits(X86_CR4_FRED);
-	/* Any further IDT use is a bug */
-	idt_invalidate();
-
-	/* Use int $0x80 for 32-bit system calls in FRED mode */
-	setup_clear_cpu_cap(X86_FEATURE_SYSENTER32);
-	setup_clear_cpu_cap(X86_FEATURE_SYSCALL32);
 }
-- 
2.45.2

[tip: x86/fred] x86/fred: Move FRED RSP initialization into a separate function

[tip-bot2 for Xin Li (Intel)]

The following commit has been merged into the x86/fred branch of tip:

Commit-ID:     73270c1f2369fb37683121ebe097cd37172602b6
Gitweb:        https://git.kernel.org/tip/73270c1f2369fb37683121ebe097cd37172602b6
Author:        Xin Li (Intel) <xin@zytor.com>
AuthorDate:    Tue, 09 Jul 2024 08:40:47 -07:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 13 Aug 2024 21:59:21 +02:00

x86/fred: Move FRED RSP initialization into a separate function

To enable FRED earlier, move the RSP initialization out of
cpu_init_fred_exceptions() into cpu_init_fred_rsps().

This is required as the FRED RSP initialization depends on the availability
of the CPU entry areas which are set up late in trap_init(),

No functional change intended. Marked with Fixes as it's a depedency for
the real fix.

Fixes: 14619d912b65 ("x86/fred: FRED entry/exit and dispatch code")
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-3-xin@zytor.com
---
 arch/x86/include/asm/fred.h  |  2 ++
 arch/x86/kernel/cpu/common.c |  6 ++++--
 arch/x86/kernel/fred.c       | 28 +++++++++++++++++++---------
 3 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/fred.h b/arch/x86/include/asm/fred.h
index e86c7ba..66d7dbe 100644
--- a/arch/x86/include/asm/fred.h
+++ b/arch/x86/include/asm/fred.h
@@ -84,11 +84,13 @@ static __always_inline void fred_entry_from_kvm(unsigned int type, unsigned int 
 }
 
 void cpu_init_fred_exceptions(void);
+void cpu_init_fred_rsps(void);
 void fred_complete_exception_setup(void);
 
 #else /* CONFIG_X86_FRED */
 static __always_inline unsigned long fred_event_data(struct pt_regs *regs) { return 0; }
 static inline void cpu_init_fred_exceptions(void) { }
+static inline void cpu_init_fred_rsps(void) { }
 static inline void fred_complete_exception_setup(void) { }
 static __always_inline void fred_entry_from_kvm(unsigned int type, unsigned int vector) { }
 #endif /* CONFIG_X86_FRED */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 10a5402..6de12b3 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2195,10 +2195,12 @@ void cpu_init_exception_handling(void)
 	/* GHCB needs to be setup to handle #VC. */
 	setup_ghcb();
 
-	if (cpu_feature_enabled(X86_FEATURE_FRED))
+	if (cpu_feature_enabled(X86_FEATURE_FRED)) {
 		cpu_init_fred_exceptions();
-	else
+		cpu_init_fred_rsps();
+	} else {
 		load_current_idt();
+	}
 }
 
 /*
diff --git a/arch/x86/kernel/fred.c b/arch/x86/kernel/fred.c
index 4bcd879..99a134f 100644
--- a/arch/x86/kernel/fred.c
+++ b/arch/x86/kernel/fred.c
@@ -32,6 +32,25 @@ void cpu_init_fred_exceptions(void)
 	       FRED_CONFIG_INT_STKLVL(0) |
 	       FRED_CONFIG_ENTRYPOINT(asm_fred_entrypoint_user));
 
+	wrmsrl(MSR_IA32_FRED_STKLVLS, 0);
+	wrmsrl(MSR_IA32_FRED_RSP0, 0);
+	wrmsrl(MSR_IA32_FRED_RSP1, 0);
+	wrmsrl(MSR_IA32_FRED_RSP2, 0);
+	wrmsrl(MSR_IA32_FRED_RSP3, 0);
+
+	/* Enable FRED */
+	cr4_set_bits(X86_CR4_FRED);
+	/* Any further IDT use is a bug */
+	idt_invalidate();
+
+	/* Use int $0x80 for 32-bit system calls in FRED mode */
+	setup_clear_cpu_cap(X86_FEATURE_SYSENTER32);
+	setup_clear_cpu_cap(X86_FEATURE_SYSCALL32);
+}
+
+/* Must be called after setup_cpu_entry_areas() */
+void cpu_init_fred_rsps(void)
+{
 	/*
 	 * The purpose of separate stacks for NMI, #DB and #MC *in the kernel*
 	 * (remember that user space faults are always taken on stack level 0)
@@ -47,13 +66,4 @@ void cpu_init_fred_exceptions(void)
 	wrmsrl(MSR_IA32_FRED_RSP1, __this_cpu_ist_top_va(DB));
 	wrmsrl(MSR_IA32_FRED_RSP2, __this_cpu_ist_top_va(NMI));
 	wrmsrl(MSR_IA32_FRED_RSP3, __this_cpu_ist_top_va(DF));
-
-	/* Enable FRED */
-	cr4_set_bits(X86_CR4_FRED);
-	/* Any further IDT use is a bug */
-	idt_invalidate();
-
-	/* Use int $0x80 for 32-bit system calls in FRED mode */
-	setup_clear_cpu_cap(X86_FEATURE_SYSENTER32);
-	setup_clear_cpu_cap(X86_FEATURE_SYSCALL32);
 }

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

[Xin Li (Intel)]

Enable FRED right after init_mem_mapping() to avoid #PF handler,
exc_page_fault(), fetching its faulting address from the stack
before FRED is enabled.

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>
---
 arch/x86/kernel/cpu/common.c |  6 +-----
 arch/x86/kernel/setup.c      | 11 ++++++++++-
 arch/x86/kernel/smpboot.c    |  6 ++++++
 arch/x86/kernel/traps.c      |  4 ++++
 4 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 6de12b3c1b04..42d4136ed6ac 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2195,12 +2195,8 @@ void cpu_init_exception_handling(void)
 	/* GHCB needs to be setup to handle #VC. */
 	setup_ghcb();
 
-	if (cpu_feature_enabled(X86_FEATURE_FRED)) {
-		cpu_init_fred_exceptions();
-		cpu_init_fred_rsps();
-	} else {
+	if (!cpu_feature_enabled(X86_FEATURE_FRED))
 		load_current_idt();
-	}
 }
 
 /*
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 728927e4ba51..36403b901eb2 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -39,6 +39,7 @@
 #include <asm/coco.h>
 #include <asm/cpu.h>
 #include <asm/efi.h>
+#include <asm/fred.h>
 #include <asm/gart.h>
 #include <asm/hypervisor.h>
 #include <asm/io_apic.h>
@@ -1040,7 +1041,15 @@ void __init setup_arch(char **cmdline_p)
 
 	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();
 
 	/*
 	 * Update mmu_cr4_features (and, indirectly, trampoline_cr4_features)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 0c35207320cb..0d83377f9dcd 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -64,6 +64,7 @@
 #include <asm/acpi.h>
 #include <asm/cacheinfo.h>
 #include <asm/desc.h>
+#include <asm/fred.h>
 #include <asm/nmi.h>
 #include <asm/irq.h>
 #include <asm/realmode.h>
@@ -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();
+	}
+
 	/*
 	 * Load the microcode before reaching the AP alive synchronization
 	 * point below so it is not part of the full per CPU serialized
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 6afb41e6cbbb..81648bd07576 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -1407,6 +1407,10 @@ void __init trap_init(void)
 	/* 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();
 
-- 
2.45.2

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

[Thomas Gleixner]

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:

>  	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?

Thanks,

        tglx
---
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))

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

[Xin Li]

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

[tip: x86/fred] x86/fred: Enable FRED right after init_mem_mapping()

[tip-bot2 for Xin Li (Intel)]

The following commit has been merged into the x86/fred branch of tip:

Commit-ID:     a97756cbec448032f84b5bbfe4e101478d1e01e0
Gitweb:        https://git.kernel.org/tip/a97756cbec448032f84b5bbfe4e101478d1e01e0
Author:        Xin Li (Intel) <xin@zytor.com>
AuthorDate:    Tue, 09 Jul 2024 08:40:48 -07:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 13 Aug 2024 21:59:21 +02:00

x86/fred: Enable FRED right after init_mem_mapping()

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(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index a75a07f..399f7d1 100644
--- 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(int);
 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);
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 6de12b3..a4735d9 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();
@@ -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
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 6129dc2..f1fea50 100644
--- 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)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 0c35207..dc4fff8 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -246,7 +246,7 @@ static void notrace start_secondary(void *unused)
 		__flush_tlb_all();
 	}
 
-	cpu_init_exception_handling();
+	cpu_init_exception_handling(false);
 
 	/*
 	 * Load the microcode before reaching the AP alive synchronization
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 6afb41e..197d588 100644
--- 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))

Re: [PATCH v2 0/3] Enable FRED earlier

[Thomas Gleixner]

On Tue, Jul 09 2024 at 08:40, Xin Li wrote:
> Wenlong Hou from Ant group reported two problems during the FRED
> initialization:
> https://lore.kernel.org/lkml/cover.1718972598.git.houwenlong.hwl@antgroup.com/
>
> The first problem is that spurious_interrupt() gets called on the
> HYPERVISOR_CALLBACK_VECTOR vector.  Because kvm_guest_init(), being
> executed way before trap_init() in which it is decided that whether
> FRED will be enabled or not, calls sysvec_install() to install
> HYPERVISOR_CALLBACK_VECTOR's interrupt handler into FRED system
> vector dispatch table or IDT depending on whether FRED is enabled.
>
> The other problem is that the #PF handler gets a wrong faulting
> address from the stack instead of CR2 before FRED is enabled.
> Because the #PF handler fetches its faulting addresss from the
> stack or CR2 based on whether FRED is available rather than active.
>
> This patchset fixes the 2 problems with suggestions from tglx:
>
>   1) Parse cmdline param "fred=" in cpu_parse_early_param() to
>      minimize the gap mentioned above, before kvm_guest_init().
>
>   2) Enable FRED right after init_mem_mapping() to switch to FRED
>      from early IDT ASAP, avoid intermediately using the IDT #PF
>      handler.

I just noticed that there is another leftover regarding FRED:

arch/x86/kernel/cpu/cpuid-deps.c:86: { X86_FEATURE_FRED, X86_FEATURE_WRMSRNS },

We removed the dependency on X86_FEATURE_WRMSRNS, right? So this is
stale and should be removed too.

Thanks,

        tglx

Re: [PATCH v2 0/3] Enable FRED earlier

[Xin Li]

> I just noticed that there is another leftover regarding FRED:
> 
> arch/x86/kernel/cpu/cpuid-deps.c:86: { X86_FEATURE_FRED, X86_FEATURE_WRMSRNS },
> 
> We removed the dependency on X86_FEATURE_WRMSRNS, right? So this is
> stale and should be removed too.

Right, I did plan to include such a change per your ask to remove the
comment in arch/x86/include/asm/switch_to.h:

https://lore.kernel.org/lkml/87ttfw18jy.ffs@tglx/

So it will be in the next iteration of that patch set.

Thanks!
    Xin

Re: [PATCH v2 0/3] Enable FRED earlier

[Xin Li]

On 8/13/2024 8:58 AM, Xin Li wrote:
>> I just noticed that there is another leftover regarding FRED:
>>
>> arch/x86/kernel/cpu/cpuid-deps.c:86: { X86_FEATURE_FRED, 
>> X86_FEATURE_WRMSRNS },
>>
>> We removed the dependency on X86_FEATURE_WRMSRNS, right? So this is
>> stale and should be removed too.

Sorry, this dependency is not removed yet, but will be as mentioned in
my previous reply.

Thanks!
     Xin


> Right, I did plan to include such a change per your ask to remove the
> comment in arch/x86/include/asm/switch_to.h:
> 
> https://lore.kernel.org/lkml/87ttfw18jy.ffs@tglx/
> 
> So it will be in the next iteration of that patch set.