[RFC PATCH 0/6] CPUID and FPU fixes

[RFC PATCH 0/6] CPUID and FPU fixes

[Andy Lutomirski]

Our CPUID-less FPU detection code was terminally broken and, as far
as I can tell, it's been broken for a long, long time.  This series
tries to improve the situation.

Thoughts?

Andy Lutomirski (5):
  x86/cpu: Factor out application of forced cpu caps
  x86/cpu: Re-apply forced caps every time cpu caps are re-read
  x86/fpu: Fix "x86/fpu: Legacy x87 FPU detected" message
  x86/fpu: Fix CPUID-less FPU detection
  x86/fpu: Fix the "Giving up, no FPU found" test

Borislav Petkov (1):
  x86/CPU: Add X86_FEATURE_CPUID

 arch/x86/include/asm/cpufeatures.h |  2 +-
 arch/x86/kernel/cpu/common.c       | 35 ++++++++++++++++++++++++-----------
 arch/x86/kernel/fpu/init.c         | 30 +++++++++++++++++-------------
 arch/x86/kernel/fpu/xstate.c       |  7 ++++++-
 4 files changed, 48 insertions(+), 26 deletions(-)

-- 
2.9.3

[RFC PATCH 1/6] x86/CPU: Add X86_FEATURE_CPUID

[Andy Lutomirski]

From: Borislav Petkov <bp@suse.de>

Add a synthetic CPUID flag denoting whether the CPU sports the CPUID
instruction or not. This will come useful later when accomodating
CPUID-less CPUs.

Signed-off-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/include/asm/cpufeatures.h | 2 +-
 arch/x86/kernel/cpu/common.c       | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index a4f9aee62217..e6be43b2679a 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -100,7 +100,7 @@
 #define X86_FEATURE_XTOPOLOGY	( 3*32+22) /* cpu topology enum extensions */
 #define X86_FEATURE_TSC_RELIABLE ( 3*32+23) /* TSC is known to be reliable */
 #define X86_FEATURE_NONSTOP_TSC	( 3*32+24) /* TSC does not stop in C states */
-/* free, was #define X86_FEATURE_CLFLUSH_MONITOR ( 3*32+25) * "" clflush reqd with monitor */
+#define X86_FEATURE_CPUID	( 3*32+25) /* CPU has CPUID instruction itself */
 #define X86_FEATURE_EXTD_APICID	( 3*32+26) /* has extended APICID (8 bits) */
 #define X86_FEATURE_AMD_DCM     ( 3*32+27) /* multi-node processor */
 #define X86_FEATURE_APERFMPERF	( 3*32+28) /* APERFMPERF */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index cc9e980c68ec..37f031f39e27 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -800,14 +800,12 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 	memset(&c->x86_capability, 0, sizeof c->x86_capability);
 	c->extended_cpuid_level = 0;
 
-	if (!have_cpuid_p())
-		identify_cpu_without_cpuid(c);
-
 	/* cyrix could have cpuid enabled via c_identify()*/
 	if (have_cpuid_p()) {
 		cpu_detect(c);
 		get_cpu_vendor(c);
 		get_cpu_cap(c);
+		setup_force_cpu_cap(X86_FEATURE_CPUID);
 
 		if (this_cpu->c_early_init)
 			this_cpu->c_early_init(c);
@@ -817,6 +815,9 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 
 		if (this_cpu->c_bsp_init)
 			this_cpu->c_bsp_init(c);
+	 } else {
+		identify_cpu_without_cpuid(c);
+		setup_clear_cpu_cap(X86_FEATURE_CPUID);
 	}
 
 	setup_force_cpu_cap(X86_FEATURE_ALWAYS);
-- 
2.9.3

[RFC PATCH 2/6] x86/cpu: Factor out application of forced cpu caps

[Andy Lutomirski]

There are multiple call sites that apply forced CPU caps.  Factor
them into a helper.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/cpu/common.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 37f031f39e27..347ae0a19380 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -655,6 +655,17 @@ void cpu_detect(struct cpuinfo_x86 *c)
 	}
 }
 
+static void apply_forced_caps(struct cpuinfo_x86 *c)
+{
+	int i;
+
+	for (i = 0; i < NCAPINTS; i++) {
+		c->x86_capability[i] &= ~cpu_caps_cleared[i];
+		c->x86_capability[i] |= cpu_caps_set[i];
+	}
+
+}
+
 void get_cpu_cap(struct cpuinfo_x86 *c)
 {
 	u32 eax, ebx, ecx, edx;
@@ -1042,10 +1053,7 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 		this_cpu->c_identify(c);
 
 	/* Clear/Set all flags overridden by options, after probe */
-	for (i = 0; i < NCAPINTS; i++) {
-		c->x86_capability[i] &= ~cpu_caps_cleared[i];
-		c->x86_capability[i] |= cpu_caps_set[i];
-	}
+	apply_forced_caps(c);
 
 #ifdef CONFIG_X86_64
 	c->apicid = apic->phys_pkg_id(c->initial_apicid, 0);
@@ -1104,10 +1112,7 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 	 * Clear/Set all flags overridden by options, need do it
 	 * before following smp all cpus cap AND.
 	 */
-	for (i = 0; i < NCAPINTS; i++) {
-		c->x86_capability[i] &= ~cpu_caps_cleared[i];
-		c->x86_capability[i] |= cpu_caps_set[i];
-	}
+	apply_forced_caps(c);
 
 	/*
 	 * On SMP, boot_cpu_data holds the common feature set between
-- 
2.9.3

[RFC PATCH 3/6] x86/cpu: Re-apply forced caps every time cpu caps are re-read

[Andy Lutomirski]

Calling get_cpu_cap() will reset a bunch of CPU features.  This will
cause the system to lose track of force-set and force-cleared
featueres in the words that are reset until the end of CPU
initialization.  This can cause X86_FEATURE_FPU, for example, to
change back and forth during boot and potentially confuse CPU setup.

To minimize the chance of confusion, re-apply forced caps every time
get_cpu_cap() is called.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/cpu/common.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 347ae0a19380..24e1e4004d42 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -758,6 +758,13 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 		c->x86_capability[CPUID_8000_000A_EDX] = cpuid_edx(0x8000000a);
 
 	init_scattered_cpuid_features(c);
+
+	/*
+	 * Clear/Set all flags overridden by options, after probe.
+	 * This needs to happen each time we re-probe, which may happen
+	 * several times during CPU initialization.
+	 */
+	apply_forced_caps(c);
 }
 
 static void identify_cpu_without_cpuid(struct cpuinfo_x86 *c)
-- 
2.9.3

[RFC PATCH 4/6] x86/fpu: Fix "x86/fpu: Legacy x87 FPU detected" message

[Andy Lutomirski]

That message isn't at all clear -- what does "Legacy x87" even
mean?

Clarify it.  If there's no FPU, say "x86/fpu: No FPU detected".  If
there's an FPU that doesn't have XSAVE, say "x86/fpu: Pre-XSAVE x87
FPU detected".

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/fpu/xstate.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 1d7770447b3e..2d592b1c75e4 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -705,8 +705,13 @@ void __init fpu__init_system_xstate(void)
 	WARN_ON_FPU(!on_boot_cpu);
 	on_boot_cpu = 0;
 
+	if (!boot_cpu_has(X86_FEATURE_FPU)) {
+		pr_info("x86/fpu: No FPU detected.\n");
+		return;
+	}
+
 	if (!boot_cpu_has(X86_FEATURE_XSAVE)) {
-		pr_info("x86/fpu: Legacy x87 FPU detected.\n");
+		pr_info("x86/fpu: Pre-XSAVE x87 FPU detected.\n");
 		return;
 	}
 
-- 
2.9.3

[RFC PATCH 5/6] x86/fpu: Fix CPUID-less FPU detection

[Andy Lutomirski]

The old code didn't work at all because it adjusted the current caps
instead of the forced caps.  Anything it did would be undone later
during cpu identification.  Fix that and, while we're at it, improve
the logging and don't bother running it if CPUID is available.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/fpu/init.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 60dece392b3a..75e1bf3b0319 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -50,29 +50,33 @@ void fpu__init_cpu(void)
 
 /*
  * The earliest FPU detection code.
- *
- * Set the X86_FEATURE_FPU CPU-capability bit based on
- * trying to execute an actual sequence of FPU instructions:
  */
 static void fpu__init_system_early_generic(struct cpuinfo_x86 *c)
 {
-	unsigned long cr0;
-	u16 fsw, fcw;
+	if (!boot_cpu_has(X86_FEATURE_CPUID) &&
+	    !test_bit(X86_FEATURE_FPU, (unsigned long *)cpu_caps_cleared)) {
+		/*
+		 * Set the X86_FEATURE_FPU CPU-capability bit based on
+		 * trying to execute an actual sequence of FPU instructions:
+		 */
 
-	fsw = fcw = 0xffff;
+		unsigned long cr0;
+		u16 fsw, fcw;
 
-	cr0 = read_cr0();
-	cr0 &= ~(X86_CR0_TS | X86_CR0_EM);
-	write_cr0(cr0);
+		fsw = fcw = 0xffff;
+
+		cr0 = read_cr0();
+		cr0 &= ~(X86_CR0_TS | X86_CR0_EM);
+		write_cr0(cr0);
 
-	if (!test_bit(X86_FEATURE_FPU, (unsigned long *)cpu_caps_cleared)) {
 		asm volatile("fninit ; fnstsw %0 ; fnstcw %1"
 			     : "+m" (fsw), "+m" (fcw));
+		pr_info("x86/fpu: FSW=0x%04hx FCW=0x%04hx\n", fsw, fcw);
 
 		if (fsw == 0 && (fcw & 0x103f) == 0x003f)
-			set_cpu_cap(c, X86_FEATURE_FPU);
+			setup_force_cpu_cap(X86_FEATURE_FPU);
 		else
-			clear_cpu_cap(c, X86_FEATURE_FPU);
+			setup_clear_cpu_cap(X86_FEATURE_FPU);
 	}
 
 #ifndef CONFIG_MATH_EMULATION
-- 
2.9.3

[RFC PATCH 6/6] x86/fpu: Fix the "Giving up, no FPU found" test

[Andy Lutomirski]

We would never print "Giving up, no FPU found" because
X86_FEATURE_FPU was in REQUIRED_MASK on non-FPU-emulating builds, so
the boot_cpu_has() test didn't do anything.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/fpu/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 75e1bf3b0319..d41924a72576 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -80,7 +80,7 @@ static void fpu__init_system_early_generic(struct cpuinfo_x86 *c)
 	}
 
 #ifndef CONFIG_MATH_EMULATION
-	if (!boot_cpu_has(X86_FEATURE_FPU)) {
+	if (!test_cpu_cap(&boot_cpu_data, X86_FEATURE_FPU)) {
 		pr_emerg("x86/fpu: Giving up, no FPU found and no math emulation present\n");
 		for (;;)
 			asm volatile("hlt");
-- 
2.9.3

Re: [RFC PATCH 1/6] x86/CPU: Add X86_FEATURE_CPUID

[Borislav Petkov]

On Mon, Dec 05, 2016 at 05:01:10PM -0800, Andy Lutomirski wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> Add a synthetic CPUID flag denoting whether the CPU sports the CPUID
> instruction or not. This will come useful later when accomodating
> CPUID-less CPUs.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/include/asm/cpufeatures.h | 2 +-
>  arch/x86/kernel/cpu/common.c       | 7 ++++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index a4f9aee62217..e6be43b2679a 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -100,7 +100,7 @@
>  #define X86_FEATURE_XTOPOLOGY	( 3*32+22) /* cpu topology enum extensions */
>  #define X86_FEATURE_TSC_RELIABLE ( 3*32+23) /* TSC is known to be reliable */
>  #define X86_FEATURE_NONSTOP_TSC	( 3*32+24) /* TSC does not stop in C states */
> -/* free, was #define X86_FEATURE_CLFLUSH_MONITOR ( 3*32+25) * "" clflush reqd with monitor */
> +#define X86_FEATURE_CPUID	( 3*32+25) /* CPU has CPUID instruction itself */
>  #define X86_FEATURE_EXTD_APICID	( 3*32+26) /* has extended APICID (8 bits) */
>  #define X86_FEATURE_AMD_DCM     ( 3*32+27) /* multi-node processor */
>  #define X86_FEATURE_APERFMPERF	( 3*32+28) /* APERFMPERF */
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index cc9e980c68ec..37f031f39e27 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -800,14 +800,12 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
>  	memset(&c->x86_capability, 0, sizeof c->x86_capability);
>  	c->extended_cpuid_level = 0;
>  
> -	if (!have_cpuid_p())
> -		identify_cpu_without_cpuid(c);
> -
>  	/* cyrix could have cpuid enabled via c_identify()*/
>  	if (have_cpuid_p()) {
>  		cpu_detect(c);
>  		get_cpu_vendor(c);
>  		get_cpu_cap(c);
> +		setup_force_cpu_cap(X86_FEATURE_CPUID);
>  
>  		if (this_cpu->c_early_init)
>  			this_cpu->c_early_init(c);
> @@ -817,6 +815,9 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
>  
>  		if (this_cpu->c_bsp_init)
>  			this_cpu->c_bsp_init(c);
> +	 } else {
> +		identify_cpu_without_cpuid(c);
> +		setup_clear_cpu_cap(X86_FEATURE_CPUID);

checkpatch complains here:

WARNING: Statements should start on a tabstop
#68: FILE: arch/x86/kernel/cpu/common.c:818:
+        } else {


Please reflow tabs.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [RFC PATCH 2/6] x86/cpu: Factor out application of forced cpu caps

[Borislav Petkov]

On Mon, Dec 05, 2016 at 05:01:11PM -0800, Andy Lutomirski wrote:
> There are multiple call sites that apply forced CPU caps.  Factor
> them into a helper.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/kernel/cpu/common.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 37f031f39e27..347ae0a19380 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -655,6 +655,17 @@ void cpu_detect(struct cpuinfo_x86 *c)
>  	}
>  }
>  
> +static void apply_forced_caps(struct cpuinfo_x86 *c)
> +{
> +	int i;
> +
> +	for (i = 0; i < NCAPINTS; i++) {
> +		c->x86_capability[i] &= ~cpu_caps_cleared[i];
> +		c->x86_capability[i] |= cpu_caps_set[i];
> +	}
> +

Superfluous newline.

> +}

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [RFC PATCH 3/6] x86/cpu: Re-apply forced caps every time cpu caps are re-read

[Borislav Petkov]

On Mon, Dec 05, 2016 at 05:01:12PM -0800, Andy Lutomirski wrote:
> Calling get_cpu_cap() will reset a bunch of CPU features.  This will
> cause the system to lose track of force-set and force-cleared
> featueres in the words that are reset until the end of CPU
> initialization.  This can cause X86_FEATURE_FPU, for example, to
> change back and forth during boot and potentially confuse CPU setup.
> 
> To minimize the chance of confusion, re-apply forced caps every time
> get_cpu_cap() is called.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/kernel/cpu/common.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 347ae0a19380..24e1e4004d42 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -758,6 +758,13 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
>  		c->x86_capability[CPUID_8000_000A_EDX] = cpuid_edx(0x8000000a);
>  
>  	init_scattered_cpuid_features(c);
> +
> +	/*
> +	 * Clear/Set all flags overridden by options, after probe.
> +	 * This needs to happen each time we re-probe, which may happen
> +	 * several times during CPU initialization.
> +	 */
> +	apply_forced_caps(c);

I guess...

Although I have to say all that early capabilities detection has grown
a lot of cruft during the years and is nuts. And calling get_cpu_cap()
multiple times is simply plain wrong.

What we should do is read CPUID *once*, filter out caps and set our
internal representation bits and be done with it.

Stuff like setup_pku() which *sets* CPUID bits will then have to run
*before* we do the detection and that's it.

But I guess that's future work.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [RFC PATCH 4/6] x86/fpu: Fix "x86/fpu: Legacy x87 FPU detected" message

[Borislav Petkov]

On Mon, Dec 05, 2016 at 05:01:13PM -0800, Andy Lutomirski wrote:
> That message isn't at all clear -- what does "Legacy x87" even
> mean?
> 
> Clarify it.  If there's no FPU, say "x86/fpu: No FPU detected".  If
> there's an FPU that doesn't have XSAVE, say "x86/fpu: Pre-XSAVE x87
> FPU detected".
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/kernel/fpu/xstate.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 1d7770447b3e..2d592b1c75e4 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -705,8 +705,13 @@ void __init fpu__init_system_xstate(void)
>  	WARN_ON_FPU(!on_boot_cpu);
>  	on_boot_cpu = 0;
>  
> +	if (!boot_cpu_has(X86_FEATURE_FPU)) {
> +		pr_info("x86/fpu: No FPU detected.\n");
> +		return;
> +	}
> +
>  	if (!boot_cpu_has(X86_FEATURE_XSAVE)) {
> -		pr_info("x86/fpu: Legacy x87 FPU detected.\n");
> +		pr_info("x86/fpu: Pre-XSAVE x87 FPU detected.\n");

Well, there's also FXSAVE and FSAVE. The legacy thing kinda made sense
to me here.

Maybe say something like this:

	pr_info("x86/fpu: FPU detected: FSAVE|FXSAVE|XSAVE support\n", ...

and use X86_FEATURE_XSAVE and X86_FEATURE_FXSR to print the respective
*SAVE string...? FSAVE we issue by default.

Hmm.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [RFC PATCH 5/6] x86/fpu: Fix CPUID-less FPU detection

[Borislav Petkov]

On Mon, Dec 05, 2016 at 05:01:14PM -0800, Andy Lutomirski wrote:
> The old code didn't work at all because it adjusted the current caps
> instead of the forced caps.  Anything it did would be undone later
> during cpu identification.  Fix that and, while we're at it, improve
> the logging and don't bother running it if CPUID is available.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/kernel/fpu/init.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
> index 60dece392b3a..75e1bf3b0319 100644
> --- a/arch/x86/kernel/fpu/init.c
> +++ b/arch/x86/kernel/fpu/init.c
> @@ -50,29 +50,33 @@ void fpu__init_cpu(void)
>  
>  /*
>   * The earliest FPU detection code.
> - *
> - * Set the X86_FEATURE_FPU CPU-capability bit based on
> - * trying to execute an actual sequence of FPU instructions:
>   */
>  static void fpu__init_system_early_generic(struct cpuinfo_x86 *c)
>  {
> -	unsigned long cr0;
> -	u16 fsw, fcw;
> +	if (!boot_cpu_has(X86_FEATURE_CPUID) &&
> +	    !test_bit(X86_FEATURE_FPU, (unsigned long *)cpu_caps_cleared)) {

Flip test and save an indentation level.

> +		/*
> +		 * Set the X86_FEATURE_FPU CPU-capability bit based on
> +		 * trying to execute an actual sequence of FPU instructions:
> +		 */
>  
> -	fsw = fcw = 0xffff;
> +		unsigned long cr0;
> +		u16 fsw, fcw;
>  
> -	cr0 = read_cr0();
> -	cr0 &= ~(X86_CR0_TS | X86_CR0_EM);
> -	write_cr0(cr0);
> +		fsw = fcw = 0xffff;
> +
> +		cr0 = read_cr0();
> +		cr0 &= ~(X86_CR0_TS | X86_CR0_EM);
> +		write_cr0(cr0);
>  
> -	if (!test_bit(X86_FEATURE_FPU, (unsigned long *)cpu_caps_cleared)) {
>  		asm volatile("fninit ; fnstsw %0 ; fnstcw %1"
>  			     : "+m" (fsw), "+m" (fcw));
> +		pr_info("x86/fpu: FSW=0x%04hx FCW=0x%04hx\n", fsw, fcw);

Why do we want those in dmesg? Maybe KERN_DEBUG?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [RFC PATCH 5/6] x86/fpu: Fix CPUID-less FPU detection

[Andy Lutomirski]

On Tue, Dec 6, 2016 at 1:40 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Dec 05, 2016 at 05:01:14PM -0800, Andy Lutomirski wrote:
>> The old code didn't work at all because it adjusted the current caps
>> instead of the forced caps.  Anything it did would be undone later
>> during cpu identification.  Fix that and, while we're at it, improve
>> the logging and don't bother running it if CPUID is available.
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  arch/x86/kernel/fpu/init.c | 28 ++++++++++++++++------------
>>  1 file changed, 16 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
>> index 60dece392b3a..75e1bf3b0319 100644
>> --- a/arch/x86/kernel/fpu/init.c
>> +++ b/arch/x86/kernel/fpu/init.c
>> @@ -50,29 +50,33 @@ void fpu__init_cpu(void)
>>
>>  /*
>>   * The earliest FPU detection code.
>> - *
>> - * Set the X86_FEATURE_FPU CPU-capability bit based on
>> - * trying to execute an actual sequence of FPU instructions:
>>   */
>>  static void fpu__init_system_early_generic(struct cpuinfo_x86 *c)
>>  {
>> -     unsigned long cr0;
>> -     u16 fsw, fcw;
>> +     if (!boot_cpu_has(X86_FEATURE_CPUID) &&
>> +         !test_bit(X86_FEATURE_FPU, (unsigned long *)cpu_caps_cleared)) {
>
> Flip test and save an indentation level.

How?  There's that bit at the bottom to worry about.

>
>> +             /*
>> +              * Set the X86_FEATURE_FPU CPU-capability bit based on
>> +              * trying to execute an actual sequence of FPU instructions:
>> +              */
>>
>> -     fsw = fcw = 0xffff;
>> +             unsigned long cr0;
>> +             u16 fsw, fcw;
>>
>> -     cr0 = read_cr0();
>> -     cr0 &= ~(X86_CR0_TS | X86_CR0_EM);
>> -     write_cr0(cr0);
>> +             fsw = fcw = 0xffff;
>> +
>> +             cr0 = read_cr0();
>> +             cr0 &= ~(X86_CR0_TS | X86_CR0_EM);
>> +             write_cr0(cr0);
>>
>> -     if (!test_bit(X86_FEATURE_FPU, (unsigned long *)cpu_caps_cleared)) {
>>               asm volatile("fninit ; fnstsw %0 ; fnstcw %1"
>>                            : "+m" (fsw), "+m" (fcw));
>> +             pr_info("x86/fpu: FSW=0x%04hx FCW=0x%04hx\n", fsw, fcw);
>
> Why do we want those in dmesg? Maybe KERN_DEBUG?

For debugging, since this code appears busted in every version of
Linux I looked at.  It's certainly been busted for quite a few years.
And this line won't print on any CPU with CPUID, so it's not going to
cause widespread spam.  I kind of line the idea of being able to ask
users of these ancient CPUs to just send in their logs.

--Andy

Re: [RFC PATCH 4/6] x86/fpu: Fix "x86/fpu: Legacy x87 FPU detected" message

[Andy Lutomirski]

On Tue, Dec 6, 2016 at 1:24 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Dec 05, 2016 at 05:01:13PM -0800, Andy Lutomirski wrote:
>> That message isn't at all clear -- what does "Legacy x87" even
>> mean?
>>
>> Clarify it.  If there's no FPU, say "x86/fpu: No FPU detected".  If
>> there's an FPU that doesn't have XSAVE, say "x86/fpu: Pre-XSAVE x87
>> FPU detected".
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  arch/x86/kernel/fpu/xstate.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
>> index 1d7770447b3e..2d592b1c75e4 100644
>> --- a/arch/x86/kernel/fpu/xstate.c
>> +++ b/arch/x86/kernel/fpu/xstate.c
>> @@ -705,8 +705,13 @@ void __init fpu__init_system_xstate(void)
>>       WARN_ON_FPU(!on_boot_cpu);
>>       on_boot_cpu = 0;
>>
>> +     if (!boot_cpu_has(X86_FEATURE_FPU)) {
>> +             pr_info("x86/fpu: No FPU detected.\n");
>> +             return;
>> +     }
>> +
>>       if (!boot_cpu_has(X86_FEATURE_XSAVE)) {
>> -             pr_info("x86/fpu: Legacy x87 FPU detected.\n");
>> +             pr_info("x86/fpu: Pre-XSAVE x87 FPU detected.\n");
>
> Well, there's also FXSAVE and FSAVE. The legacy thing kinda made sense
> to me here.
>
> Maybe say something like this:
>
>         pr_info("x86/fpu: FPU detected: FSAVE|FXSAVE|XSAVE support\n", ...
>
> and use X86_FEATURE_XSAVE and X86_FEATURE_FXSR to print the respective
> *SAVE string...? FSAVE we issue by default.

I did something like this for FSAVE vs FXSAVE.  XSAVE has its own pile
of printouts and I don't feel like I need to add another.

>
> Hmm.
>
> --
> Regards/Gruss,
>     Boris.
>
> Good mailing practices for 400: avoid top-posting and trim the reply.



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [RFC PATCH 4/6] x86/fpu: Fix "x86/fpu: Legacy x87 FPU detected" message

[Borislav Petkov]

On Tue, Dec 06, 2016 at 09:59:56AM -0800, Andy Lutomirski wrote:
> I did something like this for FSAVE vs FXSAVE.  XSAVE has its own pile
> of printouts and I don't feel like I need to add another.

So frankly, replacing "legacy" with "pre-XSAVE" is not making it any
clearer - it is just calling it differently. IMO.

Dumping those three makes it exact.

But I don't care too much about it - it is in /proc/cpuinfo anyway.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [RFC PATCH 5/6] x86/fpu: Fix CPUID-less FPU detection

[Borislav Petkov]

On Tue, Dec 06, 2016 at 09:52:11AM -0800, Andy Lutomirski wrote:
> How?  There's that bit at the bottom to worry about.

Even better: carve it out into a separate function. It was begging for
it already. Diff ontop of yours:

---
Index: b/arch/x86/kernel/fpu/init.c
===================================================================
--- a/arch/x86/kernel/fpu/init.c	2016-12-07 10:39:57.643083996 +0100
+++ b/arch/x86/kernel/fpu/init.c	2016-12-07 10:39:45.807083690 +0100
@@ -49,31 +49,37 @@ void fpu__init_cpu(void)
 }
 
 /*
- * The earliest FPU detection code.
+ * Try to execute an actual sequence of FPU instructions. Our last resort for
+ * FPU detection in case the respective CPUID bit is not set or we don't
+ * support CPUID at all.
  */
-static void fpu__init_system_early_generic(struct cpuinfo_x86 *c)
+static bool __fpu__init_poke_fpu(void)
 {
-	if (!boot_cpu_has(X86_FEATURE_CPUID) &&
-	    !test_bit(X86_FEATURE_FPU, (unsigned long *)cpu_caps_cleared)) {
-		/*
-		 * Set the X86_FEATURE_FPU CPU-capability bit based on
-		 * trying to execute an actual sequence of FPU instructions:
-		 */
+	unsigned long cr0;
+	u16 fsw, fcw;
 
-		unsigned long cr0;
-		u16 fsw, fcw;
+	fsw = fcw = 0xffff;
 
-		fsw = fcw = 0xffff;
+	cr0 = read_cr0();
+	cr0 &= ~(X86_CR0_TS | X86_CR0_EM);
+	write_cr0(cr0);
 
-		cr0 = read_cr0();
-		cr0 &= ~(X86_CR0_TS | X86_CR0_EM);
-		write_cr0(cr0);
+	asm volatile("fninit ; fnstsw %0 ; fnstcw %1"
+		     : "+m" (fsw), "+m" (fcw));
 
-		asm volatile("fninit ; fnstsw %0 ; fnstcw %1"
-			     : "+m" (fsw), "+m" (fcw));
-		pr_info("x86/fpu: FSW=0x%04hx FCW=0x%04hx\n", fsw, fcw);
+	pr_info("x86/fpu: FSW=0x%04hx FCW=0x%04hx\n", fsw, fcw);
 
-		if (fsw == 0 && (fcw & 0x103f) == 0x003f)
+	return fsw == 0 && (fcw & 0x103f) == 0x003f;
+}
+
+/*
+ * The earliest FPU detection code.
+ */
+static void fpu__init_system_early_generic(struct cpuinfo_x86 *c)
+{
+	if (!boot_cpu_has(X86_FEATURE_CPUID) &&
+	    !test_bit(X86_FEATURE_FPU, (unsigned long *)cpu_caps_cleared)) {
+		if (__fpu__init_poke_fpu())
 			setup_force_cpu_cap(X86_FEATURE_FPU);
 		else
 			setup_clear_cpu_cap(X86_FEATURE_FPU);

> I kind of line the idea of being able to ask users of these ancient
> CPUs to just send in their logs.

Prudent :)

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.