x86/tsc: Set X86_FEATURE_TSC_RELIABLE to skip refined calibration

x86/tsc: Set X86_FEATURE_TSC_RELIABLE to skip refined calibration

[Bin Gao]

Unlike PIT based calibration which counts TSC cycles against another timer,
MSR or CPUID method has no calibration - it simply multiplies the known
frequency of a timer by a ratio. So TSC frequency computed by MSR or CPUID
is the final frequency and doesn't need the refined calibration process.
We used to use set_cpu_cap(&boot_cpu_data, X86_FEATURE_TSC_RELIABLE) but
it actually doesn't skip refined calibration because the flag is cleared
later in identify_cpu(). A cpu caps flag is not cleared only if it's set
by setup_force_cpu_cap(). This patch sets the flag in tsc_msr.c and
replaces set_cpu_cap() with setup_force_cpu_cap() in other files.

Signed-off-by: Bin Gao <bin.gao@intel.com>
---
 arch/x86/kernel/tsc_msr.c          | 2 ++
 arch/x86/platform/intel-mid/mfld.c | 2 +-
 arch/x86/platform/intel-mid/mrfl.c | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/tsc_msr.c b/arch/x86/kernel/tsc_msr.c
index 9911a06..52223aa 100644
--- a/arch/x86/kernel/tsc_msr.c
+++ b/arch/x86/kernel/tsc_msr.c
@@ -122,6 +122,8 @@ unsigned long try_msr_calibrate_tsc(void)
 	lapic_timer_frequency = (freq * 1000) / HZ;
 	pr_info("lapic_timer_frequency = %d\n", lapic_timer_frequency);
 #endif
+
+	setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
 	return res;
 
 fail:
diff --git a/arch/x86/platform/intel-mid/mfld.c b/arch/x86/platform/intel-mid/mfld.c
index 1eb47b6..c75e7a4 100644
--- a/arch/x86/platform/intel-mid/mfld.c
+++ b/arch/x86/platform/intel-mid/mfld.c
@@ -50,7 +50,7 @@ static unsigned long __init mfld_calibrate_tsc(void)
 	pr_debug("read penwell tsc %lu khz\n", fast_calibrate);
 	lapic_timer_frequency = fsb * 1000 / HZ;
 	/* mark tsc clocksource as reliable */
-	set_cpu_cap(&boot_cpu_data, X86_FEATURE_TSC_RELIABLE);
+	setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
 
 	return fast_calibrate;
 }
diff --git a/arch/x86/platform/intel-mid/mrfl.c b/arch/x86/platform/intel-mid/mrfl.c
index bd1adc6..aa49531 100644
--- a/arch/x86/platform/intel-mid/mrfl.c
+++ b/arch/x86/platform/intel-mid/mrfl.c
@@ -79,7 +79,7 @@ static unsigned long __init tangier_calibrate_tsc(void)
 			lapic_timer_frequency);
 
 	/* mark tsc clocksource as reliable */
-	set_cpu_cap(&boot_cpu_data, X86_FEATURE_TSC_RELIABLE);
+	setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
 
 	return fast_calibrate;
 }
-- 
1.9.1

Re: x86/tsc: Set X86_FEATURE_TSC_RELIABLE to skip refined calibration

[Thomas Gleixner]

On Thu, 16 Jun 2016, Bin Gao wrote:

> Unlike PIT based calibration which counts TSC cycles against another timer,
> MSR or CPUID method has no calibration - it simply multiplies the known
> frequency of a timer by a ratio. So TSC frequency computed by MSR or CPUID
> is the final frequency and doesn't need the refined calibration process.
> We used to use set_cpu_cap(&boot_cpu_data, X86_FEATURE_TSC_RELIABLE) but
> it actually doesn't skip refined calibration because the flag is cleared
> later in identify_cpu(). A cpu caps flag is not cleared only if it's set
> by setup_force_cpu_cap(). This patch sets the flag in tsc_msr.c and
> replaces set_cpu_cap() with setup_force_cpu_cap() in other files.

I'm not entirely sure that this is correct. At least I want to know John
Stultz's opinion on that.
 
Thanks,

	tglx

Re: x86/tsc: Set X86_FEATURE_TSC_RELIABLE to skip refined calibration

[John Stultz]

On Fri, Jun 17, 2016 at 12:48 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 16 Jun 2016, Bin Gao wrote:
>
>> Unlike PIT based calibration which counts TSC cycles against another timer,
>> MSR or CPUID method has no calibration - it simply multiplies the known
>> frequency of a timer by a ratio. So TSC frequency computed by MSR or CPUID
>> is the final frequency and doesn't need the refined calibration process.
>> We used to use set_cpu_cap(&boot_cpu_data, X86_FEATURE_TSC_RELIABLE) but
>> it actually doesn't skip refined calibration because the flag is cleared
>> later in identify_cpu(). A cpu caps flag is not cleared only if it's set
>> by setup_force_cpu_cap(). This patch sets the flag in tsc_msr.c and
>> replaces set_cpu_cap() with setup_force_cpu_cap() in other files.
>
> I'm not entirely sure that this is correct. At least I want to know John
> Stultz's opinion on that.

So, I'm worried my context here is a bit too stale to be of much use.
Generally, yea, if we can get the TSC freq from the hardware
registers, that would be ideal, as there's too many cases where the
hardware we're calibrating off of has problems.

But I feel like there were some early edge cases where the MSR didn't
report the right values on some cpus? I may be remembering this wrong,
as its been a few years.  That's my main concern, if we start skipping
the calibration completely, but I'd trust the intel folks have a
better sense of the edge cases here then my poor memory.

thanks
-john

Re: x86/tsc: Set X86_FEATURE_TSC_RELIABLE to skip refined calibration

[Bin Gao]

On Mon, Jun 20, 2016 at 04:20:26PM -0700, John Stultz wrote:
> On Fri, Jun 17, 2016 at 12:48 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Thu, 16 Jun 2016, Bin Gao wrote:
> >
> >> Unlike PIT based calibration which counts TSC cycles against another timer,
> >> MSR or CPUID method has no calibration - it simply multiplies the known
> >> frequency of a timer by a ratio. So TSC frequency computed by MSR or CPUID
> >> is the final frequency and doesn't need the refined calibration process.
> >> We used to use set_cpu_cap(&boot_cpu_data, X86_FEATURE_TSC_RELIABLE) but
> >> it actually doesn't skip refined calibration because the flag is cleared
> >> later in identify_cpu(). A cpu caps flag is not cleared only if it's set
> >> by setup_force_cpu_cap(). This patch sets the flag in tsc_msr.c and
> >> replaces set_cpu_cap() with setup_force_cpu_cap() in other files.
> >
> > I'm not entirely sure that this is correct. At least I want to know John
> > Stultz's opinion on that.
> 
> So, I'm worried my context here is a bit too stale to be of much use.
> Generally, yea, if we can get the TSC freq from the hardware
> registers, that would be ideal, as there's too many cases where the
> hardware we're calibrating off of has problems.
> 
> But I feel like there were some early edge cases where the MSR didn't
> report the right values on some cpus? I may be remembering this wrong,
> as its been a few years.  That's my main concern, if we start skipping
> the calibration completely, but I'd trust the intel folks have a
> better sense of the edge cases here then my poor memory.
> 
> thanks
> -john

A CPU ID is added to the tsc_msr match table only after we(Intel) verified
it on the real silicon so if a CPU supports MSR method the resulting TSC
frequency must be correct and trustable. The edge cases John mentioned might
be caused be a wrong or missing FSB frequency for a specific CPU. But this is
a software bug, not a hardware problem so it can be found and fixed easily.

To completly skip the delayed refined calibration is inspired by an example
happened recently. One of our customers requires the time drift to be less than
1 second in 24 hours on Intel CherryTrail platform. When the refined calibration
is in place the MSR based result is ignored, so the 24 hours time drift is
3.6 seconds compared to 0.6 second when refined calibration is skipped(so MSR
based result is used by the kernel). This result makes me believe we have to
stick to MSR result whenver it's available.