[v2] Enable FRED earlier

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

Posted by Xin Li (Intel) 1 year, 5 months ago

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

Posted by Nikolay Borisov 1 year, 5 months ago


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

Posted by Xin Li 1 year, 5 months ago

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

Posted by Nikolay Borisov 1 year, 5 months ago

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

Posted by tip-bot2 for Xin Li (Intel) 1 year, 4 months ago

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