RFC: Bogus setting of CONSTANT TSC ?
[RFC because I'm not sure about this. Can any of the Intel people confirm/deny that there are recent CPUs without constant TSC ? I was under the belief that all current ones had this, but either this isn't the case, or cpuid is lying to me.
also: That hardcoded model check looks suspect to me. Shouldn't that be checking for extended models ? ]
I've got a core2 laptop that I noticed reports all zeros as a result of cpuid(0x80000007), indicating no constant TSC. The flags field of /proc/cpuinfo seems to think otherwise however. This looks to be because of this code in early_init_intel()
if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
(c->x86 == 0x6 && c->x86_model >= 0x0e))
set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);We explicitly set X86_FEATURE_CONSTANT_TSC on certain steppings when we enter early_init_intel, and then later we check if cpuid reports this feature, and set it on any other CPUs that were missed.
I think there are two problems here:
1. The code doesn't validate that x86_power was set to something sane before we trust it (it's only valid if cpuid_level is >= 0x80000007) On cpu's that don't support this level, it /should/ return zeros, but I'm not sure that's defined behaviour.
2. If x86_power was valid, but the CONSTANT_TSC bit was 0, we don't clear it if it was mistakenly set by the hard-coded stepping check.
We need to add the first check, otherwise the fix for the second part would always clear it on the CPUs that were hardcoded that don't support cpuid 0x80000007
As a follow-up, perhaps we should move the hardcoding code to an } else arm here where we don't support that cpuid feature to keep all the relevant code that frobs this stuff in one place.
arch/x86/kernel/cpu/intel.c +9 -5
@@ -88,11 +88,15 @@ static void __cpuinit early_init_intel(struct cpuinfo_x86 *c)* It is also reliable across cores and sockets. (but not across * cabinets - we turn it off in that case explicitly.) */ -if (c->x86_power & (1 <<8 )) { -set_cpu_cap (c,X86_FEATURE_CONSTANT_TSC ); -set_cpu_cap (c,X86_FEATURE_NONSTOP_TSC ); -if (!check_tsc_unstable ()) - sched_clock_stable =1 ; +if (c->extended_cpuid_level >=0x80000007 ) { +if (c->x86_power & (1 <<8 )) { +set_cpu_cap (c,X86_FEATURE_CONSTANT_TSC ); +set_cpu_cap (c,X86_FEATURE_NONSTOP_TSC ); +if (!check_tsc_unstable ()) + sched_clock_stable =1 ; + }else { +clear_cpu_cap (c,X86_FEATURE_CONSTANT_TSC ); + } }/*
Quoted 10 lines · Dave Jones
[RFC because I'm not sure about this. Can any of the Intel people confirm/deny that there are recent CPUs without constant TSC ? I was under the belief that all current ones had this, but either this isn't the case, or cpuid is lying to me.
also: That hardcoded model check looks suspect to me. Shouldn't that be checking for extended models ? ]
Quoted 8 lines
I've got a core2 laptop that I noticed reports all zeros as a result of cpuid(0x80000007), indicating no constant TSC. The flags field of
/proc/cpuinfoseems to think otherwise however. This looks to be because of this code inearly_init_intel()if ((c->x86 == 0xf && c->x86_model >= 0x03) || (c->x86 == 0x6 && c->x86_model >= 0x0e)) set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
This was initially introduced with commit 39b3a7 ([PATCH] i386/x86-64:
Generalize X86_FEATURE_CONSTANT_TSC flag), though the changelog is utterly useless. Andi ??
Quoted 47 lines
We explicitly set
X86_FEATURE_CONSTANT_TSCon certain steppings when we enterearly_init_intel, and then later we check if cpuid reports this feature, and set it on any other CPUs that were missed.I think there are two problems here:
1. The code doesn't validate thatx86_powerwas set to something sane before we trust it (it's only valid ifcpuid_levelis >=0x80000007) On cpu's that don't support this level, it /should/ return zeros, but I'm not sure that's defined behaviour.
2. Ifx86_powerwas valid, but theCONSTANT_TSCbit was 0, we don't clear it if it was mistakenly set by the hard-coded stepping check.We need to add the first check, otherwise the fix for the second part would always clear it on the CPUs that were hardcoded that don't support cpuid
0x80000007As a follow-up, perhaps we should move the hardcoding code to an } else arm here where we don't support that cpuid feature to keep all the relevant code that frobs this stuff in one place.
Signed-off-by Dave Jones <[email protected]>arch/x86/kernel/cpu/intel.c +9
-5@@ -88,11 +88,15 @@ static void __cpuinit early_init_intel(struct cpuinfo_x86 *c)* It is also reliable across cores and sockets. (but not across * cabinets - we turn it off in that case explicitly.) */ -if (c->x86_power & (1 <<8 )) { -set_cpu_cap (c,X86_FEATURE_CONSTANT_TSC ); -set_cpu_cap (c,X86_FEATURE_NONSTOP_TSC ); -if (!check_tsc_unstable ()) - sched_clock_stable =1 ; +if (c->extended_cpuid_level >=0x80000007 ) { +if (c->x86_power & (1 <<8 )) { +set_cpu_cap (c,X86_FEATURE_CONSTANT_TSC ); +set_cpu_cap (c,X86_FEATURE_NONSTOP_TSC ); +if (!check_tsc_unstable ()) + sched_clock_stable =1 ; + }else { +clear_cpu_cap (c,X86_FEATURE_CONSTANT_TSC ); + } }/*
Quoted 3 lines
This was initially introduced with commit 39b3a7 ([PATCH] i386/x86-64:
GeneralizeX86_FEATURE_CONSTANT_TSCflag), though the changelog is utterly useless. Andi ??
Only Nehalem and beyond set the 8000_0007 flag, but Intel has had a constant TSC since Yonah. That is what this logic implements.
However on Nehalem and beyond if the 8000_0007 is clear the BIOS wants to tell us that it knows the system doesn't have a synchronized TSC. This is currently still broken, but the timer watchdog usually catches it later. It only affects a few relatively obscure systems.
-Andi