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_TRESHOLD5

@@ -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 
Sent: Thursday, October 25, 2018 12:13 PM
To: Prarit Bhargava; Brown, Len; linux-kernel@vger.kernel.org
Cc: mi...@kernel.org; Du, Alek; ar...@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 q

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_TRESHOLD5

@@ -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 
Sent: Thursday, October 25, 2018 12:13 PM
To: Prarit Bhargava; Brown, Len; linux-kernel@vger.kernel.org
Cc: mi...@kernel.org; Du, Alek; ar...@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 q

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_TRESHOLD5
 
@@ -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

[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_TRESHOLD5
 
@@ -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

[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

[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]

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

[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

[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]

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]

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]

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]

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]

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]

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]

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

[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

[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]

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

[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?

[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_TRESHOLD5
 
@@ -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

---

[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_TRESHOLD5
 
@@ -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

[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...