[RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences

[RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences

[patrickg]

This RFC patch is intended to allow bypass CPUID, MSR and QuickPIT calibration methods should the user desire to.

The current ordering in ML x86 tsc is to calibrate in the order listed above; returning whenever there's a successful calibration.  However there are certain BIOS/HW Designs for overclocking that cause the TSC to change along with the max core clock; and simple 'trusting' calibration methodologies will lead to the TSC running 'faster' and eventually, TSC instability.

I only know that there's a use-case for me to want to be able to skip CPUID calibration, however I included args for skipping all the rest just so that all functionality is covered in the long run instead of just one use-case.

I included some noise; in the end it's probably not too necessary to have, but it could be useful from a debugging standpoint to see if someone is utilizing the flags.

---
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 74392d9..5a07d12 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -47,6 +47,13 @@ static DEFINE_STATIC_KEY_FALSE(__use_tsc);
 
 int tsc_clocksource_reliable;
 
+/*
+ * TSC calibration sequence disablement
+ */
+int calibrate_cpuid_khz_disabled = 0;
+int calibrate_msr_disabled = 0;
+int calibrate_quick_disabled = 0;
+
 static u32 art_to_tsc_numerator;
 static u32 art_to_tsc_denominator;
 static u64 art_to_tsc_offset;
@@ -281,6 +288,32 @@ static int __init tsc_setup(char *str)
 
 __setup("tsc=", tsc_setup);
 
+static int __init setup_tsc_calibration_order(char *str)
+{
+	if (!str)
+		return -EINVAL;
+
+	while (*str) {
+		if (!strncmp(str, "nocpuid", 7)) {
+			calibrate_cpuid_khz_disabled = 1;
+			pr_info("TSC CPUID khz calibrate disabled\n");
+		} else if (!strncmp(str, "nomsr", 5)) {
+			calibrate_msr_disabled = 1;
+			pr_info("TSC msr calibrate disabled\n");
+		} else if (!strncmp(str, "noquick", 7)) {
+			calibrate_quick_disabled = 1;
+			pr_info("TSC quick calibrate disabled\n");
+		}
+
+		str += strcspn(str, ",");
+		while (*str == ',')
+			str++;
+	}
+	return 1;
+}
+
+__setup("tsc_calibrate=", setup_tsc_calibration_order);
+
 #define MAX_RETRIES     5
 #define SMI_TRESHOLD    50000
 
@@ -675,19 +708,25 @@ unsigned long native_calibrate_cpu(void)
 	unsigned long flags, latch, ms, fast_calibrate;
 	int hpet = is_hpet_enabled(), i, loopmin;
 
-	fast_calibrate = cpu_khz_from_cpuid();
-	if (fast_calibrate)
-		return fast_calibrate;
+	if (!calibrate_cpuid_khz_disabled) {
+		fast_calibrate = cpu_khz_from_cpuid();
+		if (fast_calibrate)
+			return fast_calibrate;
+	}
 
-	fast_calibrate = cpu_khz_from_msr();
-	if (fast_calibrate)
-		return fast_calibrate;
+	if (!calibrate_msr_disabled) {
+		fast_calibrate = cpu_khz_from_msr();
+		if (fast_calibrate)
+			return fast_calibrate;
+	}
 
-	local_irq_save(flags);
-	fast_calibrate = quick_pit_calibrate();
-	local_irq_restore(flags);
-	if (fast_calibrate)
-		return fast_calibrate;
+	if (!calibrate_quick_disabled) {
+		local_irq_save(flags);
+		fast_calibrate = quick_pit_calibrate();
+		local_irq_restore(flags);
+		if (fast_calibrate)
+			return fast_calibrate;
+	}
 
 	/*
 	 * Run 5 calibration loops to get the lowest frequency value

---

Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences

[Arjan van de Ven]

On 7/13/2018 12:19 PM, patrickg wrote:
> This RFC patch is intended to allow bypass CPUID, MSR and QuickPIT calibration methods should the user desire to.
> 
> The current ordering in ML x86 tsc is to calibrate in the order listed above; returning whenever there's a successful calibration.  However there are certain BIOS/HW Designs for overclocking that cause the TSC to change along with the max core clock; and simple 'trusting' calibration methodologies will lead to the TSC running 'faster' and eventually, TSC instability.
> 


that would be a real violation of the contract between cpu and OS: tsc is not supposed to change for the duration of the boot

> I only know that there's a use-case for me to want to be able to skip CPUID calibration, however I included args for skipping all the rest just so that all functionality is covered in the long run instead of just one use-case.

wouldn't it be better to start the detailed calibration with the value from CPUID instead; that way we also properly calibrate spread spectrum etc...

I thought we switched to that recently to be honest...

Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences

[patrickg]

On 07/13/2018 12:40 PM, Arjan van de Ven wrote:
> On 7/13/2018 12:19 PM, patrickg wrote:
>> This RFC patch is intended to allow bypass CPUID, MSR and QuickPIT calibration methods should the user desire to.
>>
>> The current ordering in ML x86 tsc is to calibrate in the order listed above; returning whenever there's a successful calibration.  However there are certain BIOS/HW Designs for overclocking that cause the TSC to change along with the max core clock; and simple 'trusting' calibration methodologies will lead to the TSC running 'faster' and eventually, TSC instability.
>>
> 
> 
> that would be a real violation of the contract between cpu and OS: tsc is not supposed to change for the duration of the boot
With the methodology used; the TSC is still invariant; it's just running faster than the CPUID math calculates.

> 
>> I only know that there's a use-case for me to want to be able to skip CPUID calibration, however I included args for skipping all the rest just so that all functionality is covered in the long run instead of just one use-case.
> 
> wouldn't it be better to start the detailed calibration with the value from CPUID instead; that way we also properly calibrate spread spectrum etc...
> 
> I thought we switched to that recently to be honest...
Are you referencing:

1bf8915ae5156dff439d2c65314bd8fdde1b83bf - x86/tsc: Enumerate SKL cpu_khz and tsc_khz via CPUID

However since it's returning at CPUID calibration during native_calibrate_cpu(); it's not compared after-the-fact, leading to the TSC to use the 'slower' number returned by CPUID.

Now keep in mind; I dunno if there was any reason to explicitly not want to utilize the PIT calib sequences on SKL.  That'd be a factor for this.

Would comparing the number after the fact; then if there's a significant difference between PIT and MSR/CPUID, defaulting to the 'faster' value be a better solution?

Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences

[patrickg]

On 07/13/2018 12:40 PM, Arjan van de Ven wrote:
> On 7/13/2018 12:19 PM, patrickg wrote:
>> This RFC patch is intended to allow bypass CPUID, MSR and QuickPIT calibration methods should the user desire to.
>>
>> The current ordering in ML x86 tsc is to calibrate in the order listed above; returning whenever there's a successful calibration.  However there are certain BIOS/HW Designs for overclocking that cause the TSC to change along with the max core clock; and simple 'trusting' calibration methodologies will lead to the TSC running 'faster' and eventually, TSC instability.
>>
> 
> 
> that would be a real violation of the contract between cpu and OS: tsc is not supposed to change for the duration of the boot
With the methodology used; the TSC is still invariant; it's just running faster than the CPUID math calculates.

> 
>> I only know that there's a use-case for me to want to be able to skip CPUID calibration, however I included args for skipping all the rest just so that all functionality is covered in the long run instead of just one use-case.
> 
> wouldn't it be better to start the detailed calibration with the value from CPUID instead; that way we also properly calibrate spread spectrum etc...
> 
> I thought we switched to that recently to be honest...
Are you referencing:

1bf8915ae5156dff439d2c65314bd8fdde1b83bf - x86/tsc: Enumerate SKL cpu_khz and tsc_khz via CPUID

However since it's returning at CPUID calibration during native_calibrate_cpu(); it's not compared after-the-fact, leading to the TSC to use the 'slower' number returned by CPUID.

Now keep in mind; I dunno if there was any reason to explicitly not want to utilize the PIT calib sequences on SKL.  That'd be a factor for this.

Would comparing the number after the fact; then if there's a significant difference between PIT and MSR/CPUID, defaulting to the 'faster' value be a better solution?

RE: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences

[Brown, Len]

We disabled CPUID-based TSC calibration on SKX in December for several reasons.
If you still have it enabled, you need this patch:

commit b511203093489eb1829cb4de86e8214752205ac6
    x86/tsc: Fix erroneous TSC rate on Skylake Xeon

If you are referring to another platform that has CPUID-TSC calibration...
it should still work on an over-clocked system.  Over-clocked platforms should
use exactly the same reference crystal as non-overclocked platforms, but should
modify the crystal/core multiplier.   If you are changing the reference
crystal, then I believe you are using an un-supported hardware configuration,
and my ability to support you is limited.

-Len

Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences

[patrickg]

Sorry for the delay.  Expect another large delay if you have any questions.  I'm pretty heavily context switching.

I wanted to double check to make sure that I wasn't mis-documenting and mis-remembering things.

On 07/13/2018 07:40 PM, Brown, Len wrote:
> We disabled CPUID-based TSC calibration on SKX in December for several reasons.
> If you still have it enabled, you need this patch:
> 
> commit b511203093489eb1829cb4de86e8214752205ac6
>     x86/tsc: Fix erroneous TSC rate on Skylake Xeon
So, yeah.  I tested against mainline RHEL-alike elrepo builds before and I still saw the TSC running faster.

I've also tested against 3.10.0-862.6.3 which has those patches backported.

> 
> If you are referring to another platform that has CPUID-TSC calibration...
> it should still work on an over-clocked system.  Over-clocked platforms should
> use exactly the same reference crystal as non-overclocked platforms, but should
> modify the crystal/core multiplier.   If you are changing the reference
> crystal, then I believe you are using an un-supported hardware configuration,
> and my ability to support you is limited.
FYI for reference this is SKX Server.  Specifically the `gold` series procs.

Now; I'm not sure if we happen to be doing something strange in regards to changing the ref crystal.  I'll need to poke at them to figure that out.

I'm working on building something up with a lot of verbosity so that I can see if perhaps something is happening or not happening in an expected way.

Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences

[patrickg]

K, did significant poking.

native_calibrate_cpu is getting precidence no matter what because on SKL server, native_calibrate_tsc is always returning zero (Note that there is a caveat 2 lines down).

In native_calibrate_tsc, I'm seeing it always return zero after the `switch (boot_cpu_data.x86_model)`.  crystal_khz is zero so it rolls through that, never assigns it.

Now I'm apparently not testing on production CPU's.  I've requested that they drop em' in so I can ensure `CPUID 15H TSC/Crystal ratio` doesn't differ between the ES and Prod silicon, since that will effect the math 

Anyways, Then after calibrate is found to be zero, it's utilizing the CPUID calculation from native_calibrate_cpu for tsc_khz since that's returning a 'proper' value when it does the reassignment:

---
    if (tsc_khz == 0) /* Hits here and reassigns tsc_khz to the cpuid calculation. */
        tsc_khz = cpu_khz;
    else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
        cpu_khz = tsc_khz;
---

I'll post another update when I've checked with Prod CPU's, /probably/ tomorrow.

Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences

[patrickg]

tsc_khz still zero with production CPU's, so it's reassigning tsc_khz to the cpuid-acquired cpu_khz value.

Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences

[patrickg]

So it's been pretty quiet here... anything?  Even just to call me nuts or explicitly state that the hardware is doing something totally wrong?

Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences

[patrickg]

Resurrecting w/ +prarit who was handling mentions in a RHEL bug report regarding this.

Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences

[Prarit Bhargava]

On 10/25/2018 01:12 PM, patrickg wrote:
> Resurrecting w/ +prarit who was handling mentions in a RHEL bug report regarding this.
> 

Patrick can you reply back with the entire patch?

Thanks,

P.

Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences

[patrickg]

Sorry for the delay; lkml folder sorting gone wrong.

On 10/25/18 11:01 AM, Prarit Bhargava wrote:
> Patrick can you reply back with the entire patch

Yes; watch the editor bork it even more than it originally did, though.


---Copypasta of first RFC patch---

---
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 74392d9..5a07d12 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -47,6 +47,13 @@ static DEFINE_STATIC_KEY_FALSE(__use_tsc);
 
 int tsc_clocksource_reliable;
 
+/*
+ * TSC calibration sequence disablement
+ */
+int calibrate_cpuid_khz_disabled = 0;
+int calibrate_msr_disabled = 0;
+int calibrate_quick_disabled = 0;
+
 static u32 art_to_tsc_numerator;
 static u32 art_to_tsc_denominator;
 static u64 art_to_tsc_offset;
@@ -281,6 +288,32 @@ static int __init tsc_setup(char *str)
 
 __setup("tsc=", tsc_setup);
 
+static int __init setup_tsc_calibration_order(char *str)
+{
+        if (!str)
+                return -EINVAL;
+
+        while (*str) {
+                if (!strncmp(str, "nocpuid", 7)) {
+                        calibrate_cpuid_khz_disabled = 1;
+                        pr_info("TSC CPUID khz calibrate disabled\n");
+                } else if (!strncmp(str, "nomsr", 5)) {
+                        calibrate_msr_disabled = 1;
+                        pr_info("TSC msr calibrate disabled\n");
+                } else if (!strncmp(str, "noquick", 7)) {
+                        calibrate_quick_disabled = 1;
+                        pr_info("TSC quick calibrate disabled\n");
+                }
+
+                str += strcspn(str, ",");
+                while (*str == ',')
+                        str++;
+        }
+        return 1;
+}
+
+__setup("tsc_calibrate=", setup_tsc_calibration_order);
+
 #define MAX_RETRIES     5
 #define SMI_TRESHOLD    50000
 
@@ -675,19 +708,25 @@ unsigned long native_calibrate_cpu(void)
         unsigned long flags, latch, ms, fast_calibrate;
         int hpet = is_hpet_enabled(), i, loopmin;
 
-        fast_calibrate = cpu_khz_from_cpuid();
-        if (fast_calibrate)
-                return fast_calibrate;
+        if (!calibrate_cpuid_khz_disabled) {
+                fast_calibrate = cpu_khz_from_cpuid();
+                if (fast_calibrate)
+                        return fast_calibrate;
+        }
 
-        fast_calibrate = cpu_khz_from_msr();
-        if (fast_calibrate)
-                return fast_calibrate;
+        if (!calibrate_msr_disabled) {
+                fast_calibrate = cpu_khz_from_msr();
+                if (fast_calibrate)
+                        return fast_calibrate;
+        }
 
-        local_irq_save(flags);
-        fast_calibrate = quick_pit_calibrate();
-        local_irq_restore(flags);
-        if (fast_calibrate)
-                return fast_calibrate;
+        if (!calibrate_quick_disabled) {
+                local_irq_save(flags);
+                fast_calibrate = quick_pit_calibrate();
+                local_irq_restore(flags);
+                if (fast_calibrate)
+                        return fast_calibrate;
+        }
 
         /*
          * Run 5 calibration loops to get the lowest frequency value
---

Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences

[Patrick Geary]

I'm sorry; I'm OoO and this is all I have for a mailer.


---Copypasta of first RFC patch---

---
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 74392d9..5a07d12 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -47,6 +47,13 @@ static DEFINE_STATIC_KEY_FALSE(__use_tsc);

 int tsc_clocksource_reliable;

+/*
+ * TSC calibration sequence disablement
+ */
+int calibrate_cpuid_khz_disabled = 0;
+int calibrate_msr_disabled = 0;
+int calibrate_quick_disabled = 0;
+
 static u32 art_to_tsc_numerator;
 static u32 art_to_tsc_denominator;
 static u64 art_to_tsc_offset;
@@ -281,6 +288,32 @@ static int __init tsc_setup(char *str)

 __setup("tsc=", tsc_setup);

+static int __init setup_tsc_calibration_order(char *str)
+{
+        if (!str)
+                return -EINVAL;
+
+        while (*str) {
+                if (!strncmp(str, "nocpuid", 7)) {
+                        calibrate_cpuid_khz_disabled = 1;
+                        pr_info("TSC CPUID khz calibrate disabled\n");
+                } else if (!strncmp(str, "nomsr", 5)) {
+                        calibrate_msr_disabled = 1;
+                        pr_info("TSC msr calibrate disabled\n");
+                } else if (!strncmp(str, "noquick", 7)) {
+                        calibrate_quick_disabled = 1;
+                        pr_info("TSC quick calibrate disabled\n");
+                }
+
+                str += strcspn(str, ",");
+                while (*str == ',')
+                        str++;
+        }
+        return 1;
+}
+
+__setup("tsc_calibrate=", setup_tsc_calibration_order);
+
 #define MAX_RETRIES     5
 #define SMI_TRESHOLD    50000

@@ -675,19 +708,25 @@ unsigned long native_calibrate_cpu(void)
         unsigned long flags, latch, ms, fast_calibrate;
         int hpet = is_hpet_enabled(), i, loopmin;

-        fast_calibrate = cpu_khz_from_cpuid();
-        if (fast_calibrate)
-                return fast_calibrate;
+        if (!calibrate_cpuid_khz_disabled) {
+                fast_calibrate = cpu_khz_from_cpuid();
+                if (fast_calibrate)
+                        return fast_calibrate;
+        }

-        fast_calibrate = cpu_khz_from_msr();
-        if (fast_calibrate)
-                return fast_calibrate;
+        if (!calibrate_msr_disabled) {
+                fast_calibrate = cpu_khz_from_msr();
+                if (fast_calibrate)
+                        return fast_calibrate;
+        }

-        local_irq_save(flags);
-        fast_calibrate = quick_pit_calibrate();
-        local_irq_restore(flags);
-        if (fast_calibrate)
-                return fast_calibrate;
+        if (!calibrate_quick_disabled) {
+                local_irq_save(flags);
+                fast_calibrate = quick_pit_calibrate();
+                local_irq_restore(flags);
+                if (fast_calibrate)
+                        return fast_calibrate;
+        }

         /*
          * Run 5 calibration loops to get the lowest frequency value
---

________________________________________
From: patrickg <patrickg@supermicro.com>
Sent: Thursday, October 25, 2018 12:13 PM
To: Prarit Bhargava; Brown, Len; linux-kernel@vger.kernel.org
Cc: mingo@kernel.org; Du, Alek; arjan@linux.intel.com; Tang, Feng
Subject: Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences

Sorry for the delay; lkml folder sorting gone wrong.

On 10/25/18 11:01 AM, Prarit Bhargava wrote:
> Patrick can you reply back with the entire patch

Yes; watch the editor bork it even more than it originally did, though.


---Copypasta of first RFC patch---

---
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 74392d9..5a07d12 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -47,6 +47,13 @@ static DEFINE_STATIC_KEY_FALSE(__use_tsc);

 int tsc_clocksource_reliable;

+/*
+ * TSC calibration sequence disablement
+ */
+int calibrate_cpuid_khz_disabled = 0;
+int calibrate_msr_disabled = 0;
+int calibrate_quick_disabled = 0;
+
 static u32 art_to_tsc_numerator;
 static u32 art_to_tsc_denominator;
 static u64 art_to_tsc_offset;
@@ -281,6 +288,32 @@ static int __init tsc_setup(char *str)

 __setup("tsc=", tsc_setup);

+static int __init setup_tsc_calibration_order(char *str)
+{
+        if (!str)
+                return -EINVAL;
+
+        while (*str) {
+                if (!strncmp(str, "nocpuid", 7)) {
+                        calibrate_cpuid_khz_disabled = 1;
+                        pr_info("TSC CPUID khz calibrate disabled\n");
+                } else if (!strncmp(str, "nomsr", 5)) {
+                        calibrate_msr_disabled = 1;
+                        pr_info("TSC msr calibrate disabled\n");
+                } else if (!strncmp(str, "noquick", 7)) {
+                        calibrate_quick_disabled = 1;
+                        pr_info("TSC quick calibrate disabled\n");
+                }
+
+                str += strcspn(str, ",");
+                while (*str == ',')
+                        str++;
+        }
+        return 1;
+}
+
+__setup("tsc_calibrate=", setup_tsc_calibration_order);
+
 #define MAX_RETRIES     5
 #define SMI_TRESHOLD    50000

@@ -675,19 +708,25 @@ unsigned long native_calibrate_cpu(void)
         unsigned long flags, latch, ms, fast_calibrate;
         int hpet = is_hpet_enabled(), i, loopmin;

-        fast_calibrate = cpu_khz_from_cpuid();
-        if (fast_calibrate)
-                return fast_calibrate;
+        if (!calibrate_cpuid_khz_disabled) {
+                fast_calibrate = cpu_khz_from_cpuid();
+                if (fast_calibrate)
+                        return fast_calibrate;
+        }

-        fast_calibrate = cpu_khz_from_msr();
-        if (fast_calibrate)
-                return fast_calibrate;
+        if (!calibrate_msr_disabled) {
+                fast_calibrate = cpu_khz_from_msr();
+                if (fast_calibrate)
+                        return fast_calibrate;
+        }

-        local_irq_save(flags);
-        fast_calibrate = quick_pit_calibrate();
-        local_irq_restore(flags);
-        if (fast_calibrate)
-                return fast_calibrate;
+        if (!calibrate_quick_disabled) {
+                local_irq_save(flags);
+                fast_calibrate = quick_pit_calibrate();
+                local_irq_restore(flags);
+                if (fast_calibrate)
+                        return fast_calibrate;
+        }

         /*
          * Run 5 calibration loops to get the lowest frequency value
---