[PATCH v2 1/3] x86/tsc: use CPUID.0x16 to calculate missing crystal frequency

[PATCH v2 1/3] x86/tsc: use CPUID.0x16 to calculate missing crystal frequency

[Daniel Drake]

native_calibrate_tsc() had a data mapping Intel CPU families
and crystal clock speed, but hardcoded tables are not ideal, and this
approach was already problematic at least in the Skylake X case, as
seen in commit b51120309348 ("x86/tsc: Fix erroneous TSC rate on Skylake
Xeon").

By examining CPUID data from http://instlatx64.atw.hu/ and units
in the lab, we have found that 3 different scenarios need to be dealt
with, and we can eliminate most of the hardcoded data using an approach a
little more advanced than before:

 1. ApolloLake, GeminiLake, CannonLake (and presumably all new chipsets
    from this point) report the crystal frequency directly via CPUID.0x15.
    That's definitive data that we can rely upon.

 2. Skylake, Kabylake and all variants of those two chipsets report a
    crystal frequency of zero, however we can calculate the crystal clock
    speed by condidering data from CPUID.0x16.

    This method correctly distinguishes between the two crystal clock
    frequencies present on different Skylake X variants that caused
    headaches before.

    As the calculations do not quite match the previously-hardcoded values
    in some cases (e.g. 23913043Hz instead of 24MHz), TSC refinement is
    enabled on all platforms where we had to calculate the crystal
    frequency in this way.

 3. Denverton (GOLDMONT_X) reports a crystal frequency of zero and does
    not support CPUID.0x16, so we leave this entry hardcoded.

Link: https://lkml.kernel.org/r/20190419083533.32388-1-drake@endlessm.com
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Daniel Drake <drake@endlessm.com>
---

Notes:
    v2:
     - Clarify the situation around Skylake X better.
     - Enable TSC refinement when we had to calculate the crystal clock,
       in case slight differences in the calculation result cause problems
       similar to those reported earlier on Skylake X.

 arch/x86/kernel/tsc.c | 47 +++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 15b5e98a86f9..6e6d933fb99c 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -631,31 +631,38 @@ unsigned long native_calibrate_tsc(void)
 
 	crystal_khz = ecx_hz / 1000;
 
-	if (crystal_khz == 0) {
-		switch (boot_cpu_data.x86_model) {
-		case INTEL_FAM6_SKYLAKE_MOBILE:
-		case INTEL_FAM6_SKYLAKE_DESKTOP:
-		case INTEL_FAM6_KABYLAKE_MOBILE:
-		case INTEL_FAM6_KABYLAKE_DESKTOP:
-			crystal_khz = 24000;	/* 24.0 MHz */
-			break;
-		case INTEL_FAM6_ATOM_GOLDMONT_X:
-			crystal_khz = 25000;	/* 25.0 MHz */
-			break;
-		case INTEL_FAM6_ATOM_GOLDMONT:
-			crystal_khz = 19200;	/* 19.2 MHz */
-			break;
-		}
-	}
+	/*
+	 * Denverton SoCs don't report crystal clock, and also don't support
+	 * CPUID.0x16 for the calculation below, so hardcode the 25MHz crystal
+	 * clock.
+	 */
+	if (crystal_khz == 0 &&
+			boot_cpu_data.x86_model == INTEL_FAM6_ATOM_GOLDMONT_X)
+		crystal_khz = 25000;
 
-	if (crystal_khz == 0)
-		return 0;
 	/*
-	 * TSC frequency determined by CPUID is a "hardware reported"
+	 * TSC frequency reported directly by CPUID is a "hardware reported"
 	 * frequency and is the most accurate one so far we have. This
 	 * is considered a known frequency.
 	 */
-	setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
+	if (crystal_khz != 0)
+		setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
+
+	/*
+	 * Some Intel SoCs like Skylake and Kabylake don't report the crystal
+	 * clock, but we can easily calculate it to a high degree of accuracy
+	 * by considering the crystal ratio and the CPU speed.
+	 */
+	if (crystal_khz == 0 && boot_cpu_data.cpuid_level >= 0x16) {
+		unsigned int eax_base_mhz, ebx, ecx, edx;
+
+		cpuid(0x16, &eax_base_mhz, &ebx, &ecx, &edx);
+		crystal_khz = eax_base_mhz * 1000 *
+			eax_denominator / ebx_numerator;
+	}
+
+	if (crystal_khz == 0)
+		return 0;
 
 	/*
 	 * For Atom SoCs TSC is the only reliable clocksource.
-- 
2.20.1

[PATCH v2 2/3] x86/apic: rename lapic_timer_frequency to lapic_timer_period

[Daniel Drake]

This variable is a period unit (number of clock cycles per jiffy),
not a frequency (which is number of cycles per second).

Give it a more appropriate name.

Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Daniel Drake <drake@endlessm.com>
---
 arch/x86/include/asm/apic.h    |  2 +-
 arch/x86/kernel/apic/apic.c    | 20 ++++++++++----------
 arch/x86/kernel/cpu/mshyperv.c |  4 ++--
 arch/x86/kernel/cpu/vmware.c   |  2 +-
 arch/x86/kernel/jailhouse.c    |  2 +-
 arch/x86/kernel/tsc_msr.c      |  4 ++--
 6 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 130e81e10fc7..fc505a84aa93 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -52,7 +52,7 @@ extern unsigned int apic_verbosity;
 extern int local_apic_timer_c2_ok;
 
 extern int disable_apic;
-extern unsigned int lapic_timer_frequency;
+extern unsigned int lapic_timer_period;
 
 extern enum apic_intr_mode_id apic_intr_mode;
 enum apic_intr_mode_id {
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index ab6af775f06c..93de7862eef8 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -194,7 +194,7 @@ static struct resource lapic_resource = {
 	.flags = IORESOURCE_MEM | IORESOURCE_BUSY,
 };
 
-unsigned int lapic_timer_frequency = 0;
+unsigned int lapic_timer_period = 0;
 
 static void apic_pm_activate(void);
 
@@ -500,7 +500,7 @@ lapic_timer_set_periodic_oneshot(struct clock_event_device *evt, bool oneshot)
 	if (evt->features & CLOCK_EVT_FEAT_DUMMY)
 		return 0;
 
-	__setup_APIC_LVTT(lapic_timer_frequency, oneshot, 1);
+	__setup_APIC_LVTT(lapic_timer_period, oneshot, 1);
 	return 0;
 }
 
@@ -804,11 +804,11 @@ calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
 
 static int __init lapic_init_clockevent(void)
 {
-	if (!lapic_timer_frequency)
+	if (!lapic_timer_period)
 		return -1;
 
 	/* Calculate the scaled math multiplication factor */
-	lapic_clockevent.mult = div_sc(lapic_timer_frequency/APIC_DIVISOR,
+	lapic_clockevent.mult = div_sc(lapic_timer_period/APIC_DIVISOR,
 					TICK_NSEC, lapic_clockevent.shift);
 	lapic_clockevent.max_delta_ns =
 		clockevent_delta2ns(0x7FFFFFFF, &lapic_clockevent);
@@ -838,7 +838,7 @@ static int __init calibrate_APIC_clock(void)
 	 */
 	if (!lapic_init_clockevent()) {
 		apic_printk(APIC_VERBOSE, "lapic timer already calibrated %d\n",
-			    lapic_timer_frequency);
+			    lapic_timer_period);
 		/*
 		 * Direct calibration methods must have an always running
 		 * local APIC timer, no need for broadcast timer.
@@ -883,13 +883,13 @@ static int __init calibrate_APIC_clock(void)
 	pm_referenced = !calibrate_by_pmtimer(lapic_cal_pm2 - lapic_cal_pm1,
 					&delta, &deltatsc);
 
-	lapic_timer_frequency = (delta * APIC_DIVISOR) / LAPIC_CAL_LOOPS;
+	lapic_timer_period = (delta * APIC_DIVISOR) / LAPIC_CAL_LOOPS;
 	lapic_init_clockevent();
 
 	apic_printk(APIC_VERBOSE, "..... delta %ld\n", delta);
 	apic_printk(APIC_VERBOSE, "..... mult: %u\n", lapic_clockevent.mult);
 	apic_printk(APIC_VERBOSE, "..... calibration result: %u\n",
-		    lapic_timer_frequency);
+		    lapic_timer_period);
 
 	if (boot_cpu_has(X86_FEATURE_TSC)) {
 		apic_printk(APIC_VERBOSE, "..... CPU clock speed is "
@@ -900,13 +900,13 @@ static int __init calibrate_APIC_clock(void)
 
 	apic_printk(APIC_VERBOSE, "..... host bus clock speed is "
 		    "%u.%04u MHz.\n",
-		    lapic_timer_frequency / (1000000 / HZ),
-		    lapic_timer_frequency % (1000000 / HZ));
+		    lapic_timer_period / (1000000 / HZ),
+		    lapic_timer_period % (1000000 / HZ));
 
 	/*
 	 * Do a sanity check on the APIC calibration result
 	 */
-	if (lapic_timer_frequency < (1000000 / HZ)) {
+	if (lapic_timer_period < (1000000 / HZ)) {
 		local_irq_enable();
 		pr_warning("APIC frequency too slow, disabling apic timer\n");
 		return -1;
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index 3fa238a137d2..faae6115ddef 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -270,9 +270,9 @@ static void __init ms_hyperv_init_platform(void)
 
 		rdmsrl(HV_X64_MSR_APIC_FREQUENCY, hv_lapic_frequency);
 		hv_lapic_frequency = div_u64(hv_lapic_frequency, HZ);
-		lapic_timer_frequency = hv_lapic_frequency;
+		lapic_timer_period = hv_lapic_frequency;
 		pr_info("Hyper-V: LAPIC Timer Frequency: %#x\n",
-			lapic_timer_frequency);
+			lapic_timer_period);
 	}
 
 	register_nmi_handler(NMI_UNKNOWN, hv_nmi_unknown, NMI_FLAG_FIRST,
diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
index 0eda91f8eeac..3c648476d4fb 100644
--- a/arch/x86/kernel/cpu/vmware.c
+++ b/arch/x86/kernel/cpu/vmware.c
@@ -157,7 +157,7 @@ static void __init vmware_platform_setup(void)
 
 #ifdef CONFIG_X86_LOCAL_APIC
 		/* Skip lapic calibration since we know the bus frequency. */
-		lapic_timer_frequency = ecx / HZ;
+		lapic_timer_period = ecx / HZ;
 		pr_info("Host bus clock speed read from hypervisor : %u Hz\n",
 			ecx);
 #endif
diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
index 1b2ee55a2dfb..ba95bc70460d 100644
--- a/arch/x86/kernel/jailhouse.c
+++ b/arch/x86/kernel/jailhouse.c
@@ -45,7 +45,7 @@ static void jailhouse_get_wallclock(struct timespec64 *now)
 
 static void __init jailhouse_timer_init(void)
 {
-	lapic_timer_frequency = setup_data.apic_khz * (1000 / HZ);
+	lapic_timer_period = setup_data.apic_khz * (1000 / HZ);
 }
 
 static unsigned long jailhouse_get_tsc(void)
diff --git a/arch/x86/kernel/tsc_msr.c b/arch/x86/kernel/tsc_msr.c
index 3d0e9aeea7c8..067858fe4db8 100644
--- a/arch/x86/kernel/tsc_msr.c
+++ b/arch/x86/kernel/tsc_msr.c
@@ -71,7 +71,7 @@ static const struct x86_cpu_id tsc_msr_cpu_ids[] = {
 /*
  * MSR-based CPU/TSC frequency discovery for certain CPUs.
  *
- * Set global "lapic_timer_frequency" to bus_clock_cycles/jiffy
+ * Set global "lapic_timer_period" to bus_clock_cycles/jiffy
  * Return processor base frequency in KHz, or 0 on failure.
  */
 unsigned long cpu_khz_from_msr(void)
@@ -104,7 +104,7 @@ unsigned long cpu_khz_from_msr(void)
 	res = freq * ratio;
 
 #ifdef CONFIG_X86_LOCAL_APIC
-	lapic_timer_frequency = (freq * 1000) / HZ;
+	lapic_timer_period = (freq * 1000) / HZ;
 #endif
 
 	/*
-- 
2.20.1

[tip:x86/apic] x86/apic: Rename 'lapic_timer_frequency' to 'lapic_timer_period'

[tip-bot for Daniel Drake]

Commit-ID:  52ae346bd26c7a8b17ea82e9a09671e98c5402b7
Gitweb:     https://git.kernel.org/tip/52ae346bd26c7a8b17ea82e9a09671e98c5402b7
Author:     Daniel Drake <drake@endlessm.com>
AuthorDate: Thu, 9 May 2019 13:54:16 +0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 9 May 2019 11:06:49 +0200

x86/apic: Rename 'lapic_timer_frequency' to 'lapic_timer_period'

This variable is a period unit (number of clock cycles per jiffy),
not a frequency (which is number of cycles per second).

Give it a more appropriate name.

Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Daniel Drake <drake@endlessm.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: len.brown@intel.com
Cc: linux@endlessm.com
Cc: rafael.j.wysocki@intel.com
Link: http://lkml.kernel.org/r/20190509055417.13152-2-drake@endlessm.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/apic.h    |  2 +-
 arch/x86/kernel/apic/apic.c    | 20 ++++++++++----------
 arch/x86/kernel/cpu/mshyperv.c |  4 ++--
 arch/x86/kernel/cpu/vmware.c   |  2 +-
 arch/x86/kernel/jailhouse.c    |  2 +-
 arch/x86/kernel/tsc_msr.c      |  4 ++--
 6 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 130e81e10fc7..fc505a84aa93 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -52,7 +52,7 @@ extern unsigned int apic_verbosity;
 extern int local_apic_timer_c2_ok;
 
 extern int disable_apic;
-extern unsigned int lapic_timer_frequency;
+extern unsigned int lapic_timer_period;
 
 extern enum apic_intr_mode_id apic_intr_mode;
 enum apic_intr_mode_id {
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index ab6af775f06c..93de7862eef8 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -194,7 +194,7 @@ static struct resource lapic_resource = {
 	.flags = IORESOURCE_MEM | IORESOURCE_BUSY,
 };
 
-unsigned int lapic_timer_frequency = 0;
+unsigned int lapic_timer_period = 0;
 
 static void apic_pm_activate(void);
 
@@ -500,7 +500,7 @@ lapic_timer_set_periodic_oneshot(struct clock_event_device *evt, bool oneshot)
 	if (evt->features & CLOCK_EVT_FEAT_DUMMY)
 		return 0;
 
-	__setup_APIC_LVTT(lapic_timer_frequency, oneshot, 1);
+	__setup_APIC_LVTT(lapic_timer_period, oneshot, 1);
 	return 0;
 }
 
@@ -804,11 +804,11 @@ calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc)
 
 static int __init lapic_init_clockevent(void)
 {
-	if (!lapic_timer_frequency)
+	if (!lapic_timer_period)
 		return -1;
 
 	/* Calculate the scaled math multiplication factor */
-	lapic_clockevent.mult = div_sc(lapic_timer_frequency/APIC_DIVISOR,
+	lapic_clockevent.mult = div_sc(lapic_timer_period/APIC_DIVISOR,
 					TICK_NSEC, lapic_clockevent.shift);
 	lapic_clockevent.max_delta_ns =
 		clockevent_delta2ns(0x7FFFFFFF, &lapic_clockevent);
@@ -838,7 +838,7 @@ static int __init calibrate_APIC_clock(void)
 	 */
 	if (!lapic_init_clockevent()) {
 		apic_printk(APIC_VERBOSE, "lapic timer already calibrated %d\n",
-			    lapic_timer_frequency);
+			    lapic_timer_period);
 		/*
 		 * Direct calibration methods must have an always running
 		 * local APIC timer, no need for broadcast timer.
@@ -883,13 +883,13 @@ static int __init calibrate_APIC_clock(void)
 	pm_referenced = !calibrate_by_pmtimer(lapic_cal_pm2 - lapic_cal_pm1,
 					&delta, &deltatsc);
 
-	lapic_timer_frequency = (delta * APIC_DIVISOR) / LAPIC_CAL_LOOPS;
+	lapic_timer_period = (delta * APIC_DIVISOR) / LAPIC_CAL_LOOPS;
 	lapic_init_clockevent();
 
 	apic_printk(APIC_VERBOSE, "..... delta %ld\n", delta);
 	apic_printk(APIC_VERBOSE, "..... mult: %u\n", lapic_clockevent.mult);
 	apic_printk(APIC_VERBOSE, "..... calibration result: %u\n",
-		    lapic_timer_frequency);
+		    lapic_timer_period);
 
 	if (boot_cpu_has(X86_FEATURE_TSC)) {
 		apic_printk(APIC_VERBOSE, "..... CPU clock speed is "
@@ -900,13 +900,13 @@ static int __init calibrate_APIC_clock(void)
 
 	apic_printk(APIC_VERBOSE, "..... host bus clock speed is "
 		    "%u.%04u MHz.\n",
-		    lapic_timer_frequency / (1000000 / HZ),
-		    lapic_timer_frequency % (1000000 / HZ));
+		    lapic_timer_period / (1000000 / HZ),
+		    lapic_timer_period % (1000000 / HZ));
 
 	/*
 	 * Do a sanity check on the APIC calibration result
 	 */
-	if (lapic_timer_frequency < (1000000 / HZ)) {
+	if (lapic_timer_period < (1000000 / HZ)) {
 		local_irq_enable();
 		pr_warning("APIC frequency too slow, disabling apic timer\n");
 		return -1;
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index 3fa238a137d2..faae6115ddef 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -270,9 +270,9 @@ static void __init ms_hyperv_init_platform(void)
 
 		rdmsrl(HV_X64_MSR_APIC_FREQUENCY, hv_lapic_frequency);
 		hv_lapic_frequency = div_u64(hv_lapic_frequency, HZ);
-		lapic_timer_frequency = hv_lapic_frequency;
+		lapic_timer_period = hv_lapic_frequency;
 		pr_info("Hyper-V: LAPIC Timer Frequency: %#x\n",
-			lapic_timer_frequency);
+			lapic_timer_period);
 	}
 
 	register_nmi_handler(NMI_UNKNOWN, hv_nmi_unknown, NMI_FLAG_FIRST,
diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
index 0eda91f8eeac..3c648476d4fb 100644
--- a/arch/x86/kernel/cpu/vmware.c
+++ b/arch/x86/kernel/cpu/vmware.c
@@ -157,7 +157,7 @@ static void __init vmware_platform_setup(void)
 
 #ifdef CONFIG_X86_LOCAL_APIC
 		/* Skip lapic calibration since we know the bus frequency. */
-		lapic_timer_frequency = ecx / HZ;
+		lapic_timer_period = ecx / HZ;
 		pr_info("Host bus clock speed read from hypervisor : %u Hz\n",
 			ecx);
 #endif
diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
index 1b2ee55a2dfb..ba95bc70460d 100644
--- a/arch/x86/kernel/jailhouse.c
+++ b/arch/x86/kernel/jailhouse.c
@@ -45,7 +45,7 @@ static void jailhouse_get_wallclock(struct timespec64 *now)
 
 static void __init jailhouse_timer_init(void)
 {
-	lapic_timer_frequency = setup_data.apic_khz * (1000 / HZ);
+	lapic_timer_period = setup_data.apic_khz * (1000 / HZ);
 }
 
 static unsigned long jailhouse_get_tsc(void)
diff --git a/arch/x86/kernel/tsc_msr.c b/arch/x86/kernel/tsc_msr.c
index 3d0e9aeea7c8..067858fe4db8 100644
--- a/arch/x86/kernel/tsc_msr.c
+++ b/arch/x86/kernel/tsc_msr.c
@@ -71,7 +71,7 @@ static const struct x86_cpu_id tsc_msr_cpu_ids[] = {
 /*
  * MSR-based CPU/TSC frequency discovery for certain CPUs.
  *
- * Set global "lapic_timer_frequency" to bus_clock_cycles/jiffy
+ * Set global "lapic_timer_period" to bus_clock_cycles/jiffy
  * Return processor base frequency in KHz, or 0 on failure.
  */
 unsigned long cpu_khz_from_msr(void)
@@ -104,7 +104,7 @@ unsigned long cpu_khz_from_msr(void)
 	res = freq * ratio;
 
 #ifdef CONFIG_X86_LOCAL_APIC
-	lapic_timer_frequency = (freq * 1000) / HZ;
+	lapic_timer_period = (freq * 1000) / HZ;
 #endif
 
 	/*

[PATCH v2 3/3] x86/tsc: set LAPIC timer period to crystal clock frequency

[Daniel Drake]

The APIC timer calibration (calibrate_APIC_timer()) can be skipped
in cases where we know the APIC timer frequency. On Intel SoCs,
we believe that the APIC is fed by the crystal clock; this would make
sense, and the crystal clock frequency has been verified against the
APIC timer calibration result on ApolloLake, GeminiLake, Kabylake,
CoffeeLake, WhiskeyLake and AmberLake.

Set lapic_timer_period based on the crystal clock frequency
accordingly.

APIC timer calibration would normally be skipped on modern CPUs
by nature of the TSC deadline timer being used instead,
however this change is still potentially useful, e.g. if the
TSC deadline timer has been disabled with a kernel parameter.
calibrate_APIC_timer() uses the legacy timer, but we are seeing
new platforms that omit such legacy functionality, so avoiding
such codepaths is becoming more important.

Link: https://lkml.kernel.org/r/20190419083533.32388-1-drake@endlessm.com
Link: https://lkml.kernel.org/r/alpine.DEB.2.21.1904031206440.1967@nanos.tec.linutronix.de
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Daniel Drake <drake@endlessm.com>
---

Notes:
    v2:
     - remove unnecessary braces
     - use newly renamed variable lapic_timer_period

 arch/x86/kernel/tsc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 6e6d933fb99c..8f47c4862c56 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -671,6 +671,16 @@ unsigned long native_calibrate_tsc(void)
 	if (boot_cpu_data.x86_model == INTEL_FAM6_ATOM_GOLDMONT)
 		setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
 
+#ifdef CONFIG_X86_LOCAL_APIC
+	/*
+	 * The local APIC appears to be fed by the core crystal clock
+	 * (which sounds entirely sensible). We can set the global
+	 * lapic_timer_period here to avoid having to calibrate the APIC
+	 * timer later.
+	 */
+	lapic_timer_period = crystal_khz * 1000 / HZ;
+#endif
+
 	return crystal_khz * ebx_numerator / eax_denominator;
 }
 
-- 
2.20.1

Re: [PATCH v2 1/3] x86/tsc: use CPUID.0x16 to calculate missing crystal frequency

[Thomas Gleixner]

On Thu, 9 May 2019, Daniel Drake wrote:

> native_calibrate_tsc() had a data mapping Intel CPU families
> and crystal clock speed, but hardcoded tables are not ideal, and this
> approach was already problematic at least in the Skylake X case, as
> seen in commit b51120309348 ("x86/tsc: Fix erroneous TSC rate on Skylake
> Xeon").

...

For the whole pile:

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH v2 1/3] x86/tsc: use CPUID.0x16 to calculate missing crystal frequency

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Thu, 9 May 2019, Daniel Drake wrote:
> 
> > native_calibrate_tsc() had a data mapping Intel CPU families
> > and crystal clock speed, but hardcoded tables are not ideal, and this
> > approach was already problematic at least in the Skylake X case, as
> > seen in commit b51120309348 ("x86/tsc: Fix erroneous TSC rate on Skylake
> > Xeon").
> 
> ...
> 
> For the whole pile:
> 
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Thanks - applied it to tip:x86/apic, will push it out after a bit of 
testing.

Thanks,

	Ingo

Detecting x86 LAPIC timer frequency from CPUID data

[Daniel Drake]

The CPUID.0x16 leaf provides "Bus (Reference) Frequency (in MHz)".

In the thread "No 8254 PIT & no HPET on new Intel N3350 platforms
causes kernel panic during early boot" we are exploring ways to have
the kernel avoid using the PIT/HPET IRQ0 timer in more cases, and
Thomas Gleixner suggested that we could use this CPUID data to set
lapic_timer_frequency, avoiding the need for calibrate_APIC_clock()
to measure the APIC clock against the IRQ0 timer.

I'm thinking of the the following code change, however I get
unexpected results on Intel i7-8565U (Whiskey Lake). When
booting without this change, and with apic=notscdeadline (so that
APIC clock gets calibrated and used), the bus speed is detected as
23MHz:

 ... lapic delta = 149994
 ... PM-Timer delta = 357939
 ... PM-Timer result ok
 ..... delta 149994
 ..... mult: 6442193
 ..... calibration result: 23999
 ..... CPU clock speed is 1991.0916 MHz.
 ..... host bus clock speed is 23.0999 MHz.

However the CPUID.0x16 ECX reports a 100MHz bus speed on this device,
so this code change would produce a significantly different calibration.

Am I doing anything obviously wrong?

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 3fae23834069..6c51ce842f86 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -679,6 +679,16 @@ static unsigned long cpu_khz_from_cpuid(void)
 
 	cpuid(0x16, &eax_base_mhz, &ebx_max_mhz, &ecx_bus_mhz, &edx);
 
+#ifdef CONFIG_X86_LOCAL_APIC
+	/*
+	 * If bus frequency is provided in CPUID data, set
+	 * global lapic_timer_frequency to bus_clock_cycles/jiffy.
+	 * This avoids having to calibrate the APIC timer later.
+	 */
+	if (ecx_bus_mhz)
+		lapic_timer_frequency = (ecx_bus_mhz * 1000000) / HZ;
+#endif
+
 	return eax_base_mhz * 1000;
 }
 
-- 
2.19.1

Re: Detecting x86 LAPIC timer frequency from CPUID data

[Thomas Gleixner]

On Wed, 17 Apr 2019, Daniel Drake wrote:

> The CPUID.0x16 leaf provides "Bus (Reference) Frequency (in MHz)".
> 
> In the thread "No 8254 PIT & no HPET on new Intel N3350 platforms
> causes kernel panic during early boot" we are exploring ways to have
> the kernel avoid using the PIT/HPET IRQ0 timer in more cases, and
> Thomas Gleixner suggested that we could use this CPUID data to set
> lapic_timer_frequency, avoiding the need for calibrate_APIC_clock()
> to measure the APIC clock against the IRQ0 timer.
> 
> I'm thinking of the the following code change, however I get
> unexpected results on Intel i7-8565U (Whiskey Lake). When
> booting without this change, and with apic=notscdeadline (so that
> APIC clock gets calibrated and used), the bus speed is detected as
> 23MHz:
> 
>  ... lapic delta = 149994
>  ... PM-Timer delta = 357939
>  ... PM-Timer result ok
>  ..... delta 149994
>  ..... mult: 6442193
>  ..... calibration result: 23999
>  ..... CPU clock speed is 1991.0916 MHz.
>  ..... host bus clock speed is 23.0999 MHz.
> 
> However the CPUID.0x16 ECX reports a 100MHz bus speed on this device,
> so this code change would produce a significantly different calibration.
> 
> Am I doing anything obviously wrong?

It's probably just my fault sending you down the wrong path. What's the
content of CPUUD.0x15 on that box?

Thanks,

	tglx

Re: Detecting x86 LAPIC timer frequency from CPUID data

[Thomas Gleixner]

On Thu, 18 Apr 2019, Thomas Gleixner wrote:
> On Wed, 17 Apr 2019, Daniel Drake wrote:
> 
> > The CPUID.0x16 leaf provides "Bus (Reference) Frequency (in MHz)".
> > 
> > In the thread "No 8254 PIT & no HPET on new Intel N3350 platforms
> > causes kernel panic during early boot" we are exploring ways to have
> > the kernel avoid using the PIT/HPET IRQ0 timer in more cases, and
> > Thomas Gleixner suggested that we could use this CPUID data to set
> > lapic_timer_frequency, avoiding the need for calibrate_APIC_clock()
> > to measure the APIC clock against the IRQ0 timer.
> > 
> > I'm thinking of the the following code change, however I get
> > unexpected results on Intel i7-8565U (Whiskey Lake). When
> > booting without this change, and with apic=notscdeadline (so that
> > APIC clock gets calibrated and used), the bus speed is detected as
> > 23MHz:
> > 
> >  ... lapic delta = 149994
> >  ... PM-Timer delta = 357939
> >  ... PM-Timer result ok
> >  ..... delta 149994
> >  ..... mult: 6442193
> >  ..... calibration result: 23999

That's 24MHZ which is the nominal clock for these machines.

> >  ..... CPU clock speed is 1991.0916 MHz.
> >  ..... host bus clock speed is 23.0999 MHz.

I think that printout is wrong in two aspects. First it should be
23.9999Mhz, second it should be 100MHz. This stuff comes from old systems
which had completely different clock setups.

> > However the CPUID.0x16 ECX reports a 100MHz bus speed on this device,
> > so this code change would produce a significantly different calibration.

Yes.

> > Am I doing anything obviously wrong?
> 
> It's probably just my fault sending you down the wrong path. What's the
> content of CPUUD.0x15 on that box?

I bet that CPUID.0x15 ECX says 24Mhz or it just says 0 like on my
machine. But then interestingly enough on that box I see:

   Time Stamp Counter/Core Crystal Clock Information (0x15):
      TSC/clock ratio = 168/2
      nominal core crystal clock = 0 Hz

   Processor Frequency Information (0x16):
      Core Base Frequency (MHz) = 0x834 (2100)
      Core Maximum Frequency (MHz) = 0xed8 (3800)
      Bus (Reference) Frequency (MHz) = 0x64 (100)

Assuming that TSC and local APIC timer run from the same frequency on these
modern machines.

       2100MHz * 2 / 168 = 25MHz

and disabling the tsc deadline timer tells me:

  ..... calibration result: 24999

Close enough. Thinking about it - that makes a lot of sense. With TSC
deadline timer available it would be pretty silly if there is yet another
clock feeding into local APIC.

And the 0x15 -> 0x16 correlation is clear as well. The TSC runs at the
nominal CPU frequency, in this case 2100MHz. So the nominator/denominator
pair from 0x15 allows to deduce the nominal core crystal clock which seems
to be exactly the clock which is fed into the local APIC timer.

If someone from Intel could confirm that, then we could make that work for
any modern system.

Thanks,

	tglx

Re: Detecting x86 LAPIC timer frequency from CPUID data

[Daniel Drake]

On Fri, Apr 19, 2019 at 6:30 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>    Time Stamp Counter/Core Crystal Clock Information (0x15):
>       TSC/clock ratio = 168/2
>       nominal core crystal clock = 0 Hz
>
>    Processor Frequency Information (0x16):
>       Core Base Frequency (MHz) = 0x834 (2100)
>       Core Maximum Frequency (MHz) = 0xed8 (3800)
>       Bus (Reference) Frequency (MHz) = 0x64 (100)
>
> Assuming that TSC and local APIC timer run from the same frequency on these
> modern machines.
>
>        2100MHz * 2 / 168 = 25MHz
>
> and disabling the tsc deadline timer tells me:
>
>   ..... calibration result: 24999
>
> Close enough.

I tested all the Intel SoC generations we have on hand. The assumption that
the core crystal clock feeds the APIC seems to be consistently true.

(Please note that all the following results are done with CONFIG_HZ=250,
 which is why the "calibration result" is 4x higher than HZ=1000 as used
 in previous mails)

In the easy case, the low cost platforms do not support CPUID.0x16 (so no
CPU frequency reporting), but they do tell us the core crystal clock, which
is consistent with the APIC calibration result:

N5000 (Gemini Lake)
[    0.122948] ... lapic delta = 119999
[    0.122948] ... PM-Timer delta = 357950
[    0.122948] ... PM-Timer result ok
[    0.122948] ..... delta 119999
[    0.122948] ..... mult: 5153917
[    0.122948] ..... calibration result: 76799
[    0.122948] ..... CPU clock speed is 1094.1542 MHz.
[    0.122948] ..... host bus clock speed is 19.0799 MHz.
   Time Stamp Counter/Core Crystal Clock Information (0x15):
      TSC/clock ratio = 171/3
      nominal core crystal clock = 19200000 Hz
   Processor Frequency Information (0x16):
      Core Base Frequency (MHz) = 0x0 (0)
      Core Maximum Frequency (MHz) = 0x0 (0)
      Bus (Reference) Frequency (MHz) = 0x0 (0)

N3350 (Apollo Lake)
[    0.248894] ... lapic delta = 119999
[    0.248894] ... PM-Timer delta = 357949
[    0.248894] ... PM-Timer result ok
[    0.248894] ..... delta 119999
[    0.248894] ..... mult: 5153917
[    0.248894] ..... calibration result: 76799
[    0.248894] ..... CPU clock speed is 1094.1540 MHz.
[    0.248894] ..... host bus clock speed is 19.0799 MHz.
   Time Stamp Counter/Core Crystal Clock Information (0x15):
      TSC/clock ratio = 171/3
      nominal core crystal clock = 19200000 Hz
(CPUID 0x16 not supported at all)

And the 4 higher-end SoCs that we have available for testing all report
crystal clock 0Hz from CPUID 0x15, but by combining the CPUID.0x16 base
frequency with the CPUID.0x15 TSC/clock ratio, the crystal frequency can
be calculated as you describe, and it consistently matches the APIC timer
calibration result.

i9-9980HK (Coffee Lake HR)
[    0.379421] ... lapic delta = 149998
[    0.379421] ... PM-Timer delta = 357950
[    0.379421] ... PM-Timer result ok
[    0.379421] ..... delta 149998
[    0.379421] ..... mult: 6442365
[    0.379421] ..... calibration result: 95998
[    0.379421] ..... CPU clock speed is 2399.3902 MHz.
[    0.379421] ..... host bus clock speed is 23.3998 MHz.
   Time Stamp Counter/Core Crystal Clock Information (0x15):
      TSC/clock ratio = 200/2
      nominal core crystal clock = 0 Hz
   Processor Frequency Information (0x16):
      Core Base Frequency (MHz) = 0x960 (2400)
      Core Maximum Frequency (MHz) = 0x1388 (5000)
      Bus (Reference) Frequency (MHz) = 0x64 (100)

i7-8565U (Whiskey Lake)
[    0.173776] ... lapic delta = 149998
[    0.173776] ... PM-Timer delta = 357950
[    0.173776] ... PM-Timer result ok
[    0.173776] ..... delta 149998
[    0.173776] ..... mult: 6442365
[    0.173776] ..... calibration result: 95998
[    0.173776] ..... CPU clock speed is 1991.3903 MHz.
[    0.173776] ..... host bus clock speed is 23.3998 MHz.
   Time Stamp Counter/Core Crystal Clock Information (0x15):
      TSC/clock ratio = 166/2
      nominal core crystal clock = 0 Hz
   Processor Frequency Information (0x16):
      Core Base Frequency (MHz) = 0x7d0 (2000)
      Core Maximum Frequency (MHz) = 0x11f8 (4600)
      Bus (Reference) Frequency (MHz) = 0x64 (100)

i5-7200U (Kabylake)
[    0.219142] ... lapic delta = 149998
[    0.219142] ... PM-Timer delta = 357951
[    0.219142] ... PM-Timer result ok
[    0.219142] ..... delta 149998
[    0.219142] ..... mult: 6442365
[    0.219142] ..... calibration result: 95998
[    0.219142] ..... CPU clock speed is 2711.3880 MHz.
[    0.219142] ..... host bus clock speed is 23.3998 MHz.
   Time Stamp Counter/Core Crystal Clock Information (0x15):
      TSC/clock ratio = 226/2
      nominal core crystal clock = 0 Hz
   Processor Frequency Information (0x16):
      Core Base Frequency (MHz) = 0xa8c (2700)
      Core Maximum Frequency (MHz) = 0xc1c (3100)
      Bus (Reference) Frequency (MHz) = 0x64 (100)

m3-8110Y (Amber Lake)
[    0.102289] ... lapic delta = 149998
[    0.102289] ... PM-Timer delta = 357951
[    0.102289] ... PM-Timer result ok
[    0.102289] ..... delta 149998
[    0.102289] ..... mult: 6442365
[    0.102289] ..... calibration result: 95998
[    0.102289] ..... CPU clock speed is 1607.3930 MHz.
[    0.102289] ..... host bus clock speed is 23.3998 MHz.
   Time Stamp Counter/Core Crystal Clock Information (0x15):
      TSC/clock ratio = 134/2
      nominal core crystal clock = 0 Hz
   Processor Frequency Information (0x16):
      Core Base Frequency (MHz) = 0x640 (1600)
      Core Maximum Frequency (MHz) = 0xd48 (3400)
      Bus (Reference) Frequency (MHz) = 0x64 (100)

Is this data convincing enough or should we additionally wait for some
comments from Intel?

I came up with the patch below. However, upon testing, I realised that, at
least for the platforms I have in hand, only the first hunk is really needed.
We don't need to use your magic calculation to find the crystal frequency
because Intel already told us! native_calibrate_tsc() already hardcodes the
crystal frequency for Kabylake, and Amber/Whiskey/Coffee also report the
0x8e/0x9e Kabylake model codes. Plus ApolloLake/GeminiLake do report the
crystal frequency in CPUID.0x15 so that is covered too.

While looking around this code I also spotted something curious.
In calibrate_APIC_clock() for the case where lapic_timer_frequency has been
externally provided, we have:
		lapic_clockevent.max_delta_ns =
			clockevent_delta2ns(0x7FFFFF, &lapic_clockevent);
		lapic_clockevent.max_delta_ticks = 0x7FFFFF;

But in the case where we calibrate, we have:
	lapic_clockevent.max_delta_ns =
		clockevent_delta2ns(0x7FFFFFFF, &lapic_clockevent);
	lapic_clockevent.max_delta_ticks = 0x7FFFFFFF;

0x7FFFFF vs 0x7FFFFFFF, is that intentional?

---
 arch/x86/kernel/tsc.c | 51 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 3fae23834069..8dcfd9a30b24 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -648,6 +648,23 @@ unsigned long native_calibrate_tsc(void)
 
 	if (crystal_khz == 0)
 		return 0;
+
+#ifdef CONFIG_X86_LOCAL_APIC
+	/* The local APIC appears to be fed by the core crystal clock
+	 * (which sounds entirely sensible). We can set the global
+	 * lapic_timer_frequency here to avoid having to calibrate the APIC
+	 * timer later.
+	 *
+	 * This has been empirically verified on ApolloLake and GeminiLake:
+	 * the APIC calibration otherwise determines the same bus frequency
+	 * as the CPUID.0x15 crystal frequency.
+	 * It has also been empirically verified on KabyLake, where the
+	 * APIC calibration otherwise determines the same bus frequency
+	 * as the one hardcoded above.
+	 */
+	lapic_timer_frequency = (crystal_khz * 1000) / HZ;
+#endif
+
 	/*
 	 * TSC frequency determined by CPUID is a "hardware reported"
 	 * frequency and is the most accurate one so far we have. This
@@ -665,6 +682,38 @@ unsigned long native_calibrate_tsc(void)
 	return crystal_khz * ebx_numerator / eax_denominator;
 }
 
+static void set_lapic_timer_freq_from_cpu_freq(unsigned int base_mhz)
+{
+#ifdef CONFIG_X86_LOCAL_APIC
+	unsigned int eax_denominator, ebx_numerator, ecx_hz, edx;
+
+	/*
+	 * Use CPUID.0x16 CPU base frequency to calculate the global
+	 * lapic_timer_frequency, which avoids having to calibrate the
+	 * APIC timer later.
+	 *
+	 * Like in native_calibrate_tsc(), we rely on the observation
+	 * that the local APIC is fed by the core crystal clock.
+	 * Experimenting with KabyLake, CoffeeLake, WhiskeyLake and
+	 * AmberLake, the core crystal clock is not reported in CPUID.0x15,
+	 * but we can calculate it by considering the CPU base frequency
+	 * and the TSC/clock ratio. The result has been verified to closely
+	 * match the result that would otherwise be obtained through
+	 * APIC clock calibration.
+	 */
+
+	if (!base_mhz)
+		return;
+
+	cpuid(0x15, &eax_denominator, &ebx_numerator, &ecx_hz, &edx);
+	if (!eax_denominator || !ebx_numerator)
+		return;
+
+	lapic_timer_frequency = (base_mhz * 1000000 * \
+				 eax_denominator / ebx_numerator) / HZ;
+#endif
+}
+
 static unsigned long cpu_khz_from_cpuid(void)
 {
 	unsigned int eax_base_mhz, ebx_max_mhz, ecx_bus_mhz, edx;
@@ -679,6 +728,8 @@ static unsigned long cpu_khz_from_cpuid(void)
 
 	cpuid(0x16, &eax_base_mhz, &ebx_max_mhz, &ecx_bus_mhz, &edx);
 
+	set_lapic_timer_freq_from_cpu_freq(eax_base_mhz);
+
 	return eax_base_mhz * 1000;
 }
 
-- 
2.19.1

Re: Detecting x86 LAPIC timer frequency from CPUID data

[Thomas Gleixner]

On Fri, 19 Apr 2019, Daniel Drake wrote:
> On Fri, Apr 19, 2019 at 6:30 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> >    Time Stamp Counter/Core Crystal Clock Information (0x15):
> >       TSC/clock ratio = 168/2
> >       nominal core crystal clock = 0 Hz
> >
> >    Processor Frequency Information (0x16):
> >       Core Base Frequency (MHz) = 0x834 (2100)
> >       Core Maximum Frequency (MHz) = 0xed8 (3800)
> >       Bus (Reference) Frequency (MHz) = 0x64 (100)
> >
> > Assuming that TSC and local APIC timer run from the same frequency on these
> > modern machines.
> >
> >        2100MHz * 2 / 168 = 25MHz
> >
> > and disabling the tsc deadline timer tells me:
> >
> >   ..... calibration result: 24999
> >
> > Close enough.
> 
> I tested all the Intel SoC generations we have on hand. The assumption that
> the core crystal clock feeds the APIC seems to be consistently true.
> 
> (Please note that all the following results are done with CONFIG_HZ=250,
>  which is why the "calibration result" is 4x higher than HZ=1000 as used
>  in previous mails)
> 
> In the easy case, the low cost platforms do not support CPUID.0x16 (so no
> CPU frequency reporting), but they do tell us the core crystal clock, which
> is consistent with the APIC calibration result:

...

> And the 4 higher-end SoCs that we have available for testing all report
> crystal clock 0Hz from CPUID 0x15, but by combining the CPUID.0x16 base
> frequency with the CPUID.0x15 TSC/clock ratio, the crystal frequency can
> be calculated as you describe, and it consistently matches the APIC timer
> calibration result.

...

> Is this data convincing enough or should we additionally wait for some
> comments from Intel?

For me it's pretty convincing, but having some confirmation from Intel
wouldn't be a bad thing.
 
> I came up with the patch below. However, upon testing, I realised that, at
> least for the platforms I have in hand, only the first hunk is really needed.
> We don't need to use your magic calculation to find the crystal frequency
> because Intel already told us! native_calibrate_tsc() already hardcodes the
> crystal frequency for Kabylake, and Amber/Whiskey/Coffee also report the
> 0x8e/0x9e Kabylake model codes.

I'd rather replace these model checks with math. These tables are horrible
to maintain.

> Plus ApolloLake/GeminiLake do report the crystal frequency in CPUID.0x15
> so that is covered too.

> While looking around this code I also spotted something curious.
> In calibrate_APIC_clock() for the case where lapic_timer_frequency has been
> externally provided, we have:
> 		lapic_clockevent.max_delta_ns =
> 			clockevent_delta2ns(0x7FFFFF, &lapic_clockevent);
> 		lapic_clockevent.max_delta_ticks = 0x7FFFFF;
> 
> But in the case where we calibrate, we have:
> 	lapic_clockevent.max_delta_ns =
> 		clockevent_delta2ns(0x7FFFFFFF, &lapic_clockevent);
> 	lapic_clockevent.max_delta_ticks = 0x7FFFFFFF;
> 
> 0x7FFFFF vs 0x7FFFFFFF, is that intentional?

I don't think so. Looks like a failed copy and paste. Cc'ed Jacob, he might
know.

Thanks,

	tglx

Re: Detecting x86 LAPIC timer frequency from CPUID data

[Jacob Pan]

On Fri, 19 Apr 2019 10:57:10 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Fri, 19 Apr 2019, Daniel Drake wrote:
> > On Fri, Apr 19, 2019 at 6:30 AM Thomas Gleixner
> > <tglx@linutronix.de> wrote:  
> > >    Time Stamp Counter/Core Crystal Clock Information (0x15):
> > >       TSC/clock ratio = 168/2
> > >       nominal core crystal clock = 0 Hz
> > >
> > >    Processor Frequency Information (0x16):
> > >       Core Base Frequency (MHz) = 0x834 (2100)
> > >       Core Maximum Frequency (MHz) = 0xed8 (3800)
> > >       Bus (Reference) Frequency (MHz) = 0x64 (100)
> > >
> > > Assuming that TSC and local APIC timer run from the same
> > > frequency on these modern machines.
> > >
> > >        2100MHz * 2 / 168 = 25MHz
> > >
> > > and disabling the tsc deadline timer tells me:
> > >
> > >   ..... calibration result: 24999
> > >
> > > Close enough.  
> > 
> > I tested all the Intel SoC generations we have on hand. The
> > assumption that the core crystal clock feeds the APIC seems to be
> > consistently true.
> > 
> > (Please note that all the following results are done with
> > CONFIG_HZ=250, which is why the "calibration result" is 4x higher
> > than HZ=1000 as used in previous mails)
> > 
> > In the easy case, the low cost platforms do not support CPUID.0x16
> > (so no CPU frequency reporting), but they do tell us the core
> > crystal clock, which is consistent with the APIC calibration
> > result:  
> 
> ...
> 
> > And the 4 higher-end SoCs that we have available for testing all
> > report crystal clock 0Hz from CPUID 0x15, but by combining the
> > CPUID.0x16 base frequency with the CPUID.0x15 TSC/clock ratio, the
> > crystal frequency can be calculated as you describe, and it
> > consistently matches the APIC timer calibration result.  
> 
> ...
> 
> > Is this data convincing enough or should we additionally wait for
> > some comments from Intel?  
> 
> For me it's pretty convincing, but having some confirmation from Intel
> wouldn't be a bad thing.
>  
> > I came up with the patch below. However, upon testing, I realised
> > that, at least for the platforms I have in hand, only the first
> > hunk is really needed. We don't need to use your magic calculation
> > to find the crystal frequency because Intel already told us!
> > native_calibrate_tsc() already hardcodes the crystal frequency for
> > Kabylake, and Amber/Whiskey/Coffee also report the 0x8e/0x9e
> > Kabylake model codes.  
> 
> I'd rather replace these model checks with math. These tables are
> horrible to maintain.
> 
> > Plus ApolloLake/GeminiLake do report the crystal frequency in
> > CPUID.0x15 so that is covered too.  
> 
> > While looking around this code I also spotted something curious.
> > In calibrate_APIC_clock() for the case where lapic_timer_frequency
> > has been externally provided, we have:
> > 		lapic_clockevent.max_delta_ns =
> > 			clockevent_delta2ns(0x7FFFFF,
> > &lapic_clockevent); lapic_clockevent.max_delta_ticks = 0x7FFFFF;
> > 
> > But in the case where we calibrate, we have:
> > 	lapic_clockevent.max_delta_ns =
> > 		clockevent_delta2ns(0x7FFFFFFF, &lapic_clockevent);
> > 	lapic_clockevent.max_delta_ticks = 0x7FFFFFFF;
> > 
> > 0x7FFFFF vs 0x7FFFFFFF, is that intentional?  
> 
> I don't think so. Looks like a failed copy and paste. Cc'ed Jacob, he
> might know.
> 
At the time of v2.6.35 both places use 0x7FFFFF. But later this patch
increased the latter to 0x7FFFFFFF but forgot the first part. So I
guess it is not exactly a failed copy and paste.

commit 4aed89d6b515b9185351706ca95cd712c9d8d6a3
Author: Pierre Tardy <pierre.tardy@intel.com>
Date:   Thu Jan 6 16:23:29 2011 +0100

    x86, lapic-timer: Increase the max_delta to 31 bits
    

> Thanks,
> 
> 	tglx

[Jacob Pan]

Re: Detecting x86 LAPIC timer frequency from CPUID data

[Thomas Gleixner]

On Fri, 19 Apr 2019, Jacob Pan wrote:
> On Fri, 19 Apr 2019 10:57:10 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Fri, 19 Apr 2019, Daniel Drake wrote:
> > > 0x7FFFFF vs 0x7FFFFFFF, is that intentional?  
> > 
> > I don't think so. Looks like a failed copy and paste. Cc'ed Jacob, he
> > might know.
> > 
> At the time of v2.6.35 both places use 0x7FFFFF. But later this patch
> increased the latter to 0x7FFFFFFF but forgot the first part. So I
> guess it is not exactly a failed copy and paste.
> 
> commit 4aed89d6b515b9185351706ca95cd712c9d8d6a3
> Author: Pierre Tardy <pierre.tardy@intel.com>
> Date:   Thu Jan 6 16:23:29 2011 +0100
> 
>     x86, lapic-timer: Increase the max_delta to 31 bits

Indeed. Thanks for digging that up!

	tglx

Re: Detecting x86 LAPIC timer frequency from CPUID data

[Jacob Pan]

On Fri, 19 Apr 2019 22:52:01 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Fri, 19 Apr 2019, Jacob Pan wrote:
> > On Fri, 19 Apr 2019 10:57:10 +0200 (CEST)
> > Thomas Gleixner <tglx@linutronix.de> wrote:  
> > > On Fri, 19 Apr 2019, Daniel Drake wrote:  
> > > > 0x7FFFFF vs 0x7FFFFFFF, is that intentional?    
> > > 
> > > I don't think so. Looks like a failed copy and paste. Cc'ed
> > > Jacob, he might know.
> > >   
> > At the time of v2.6.35 both places use 0x7FFFFF. But later this
> > patch increased the latter to 0x7FFFFFFF but forgot the first part.
> > So I guess it is not exactly a failed copy and paste.
> > 
> > commit 4aed89d6b515b9185351706ca95cd712c9d8d6a3
> > Author: Pierre Tardy <pierre.tardy@intel.com>
> > Date:   Thu Jan 6 16:23:29 2011 +0100
> > 
> >     x86, lapic-timer: Increase the max_delta to 31 bits  
> 
> Indeed. Thanks for digging that up!
> 
> 	tglx

How about a fix like this? I should have taken your advice 9 years ago
to avoid duplicated code :)
https://lkml.org/lkml/2010/5/11/499

From 18450bb67e09f5b472f1ed313d7f87a983cb0ac1 Mon Sep 17 00:00:00 2001
From: Jacob Pan <jacob.jun.pan@linux.intel.com>
Date: Fri, 19 Apr 2019 15:56:06 -0700
Subject: [PATCH] x86/apic: Fix duplicated lapic timer calculation

Local APIC timer clockevent parameters can be calculated based on
platform specific methods. However the code is mostly duplicated
with the interrupt based calibration. This causes further updates
prone to mistakes.

Fixes: 4aed89d ("x86, lapic-timer: Increase the max_delta to 31 bits")

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 arch/x86/kernel/apic/apic.c | 46
++++++++++++++++++++++++++------------------- 1 file changed, 27
insertions(+), 19 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index b7bcdd7..b2ef91c 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -802,6 +802,24 @@ calibrate_by_pmtimer(long deltapm, long *delta,
long *deltatsc) return 0;
 }
 
+static int __init calculate_lapic_clockevent(void)
+{
+	if (!lapic_timer_frequency)
+		return -1;
+
+	/* Calculate the scaled math multiplication factor */
+	lapic_clockevent.mult =
div_sc(lapic_timer_frequency/APIC_DIVISOR,
+					TICK_NSEC,
lapic_clockevent.shift);
+	lapic_clockevent.max_delta_ns =
+		clockevent_delta2ns(0x7FFFFFFF, &lapic_clockevent);
+	lapic_clockevent.max_delta_ticks = 0x7FFFFFFF;
+	lapic_clockevent.min_delta_ns =
+		clockevent_delta2ns(0xF, &lapic_clockevent);
+	lapic_clockevent.min_delta_ticks = 0xF;
+
+	return 0;
+}
+
 static int __init calibrate_APIC_clock(void)
 {
 	struct clock_event_device *levt = this_cpu_ptr(&lapic_events);
@@ -818,18 +836,17 @@ static int __init calibrate_APIC_clock(void)
 
 	if (boot_cpu_has(X86_FEATURE_TSC_DEADLINE_TIMER)) {
 		return 0;
-	} else if (lapic_timer_frequency) {
+	}
+
+	if (!calculate_lapic_clockevent()) {
 		apic_printk(APIC_VERBOSE, "lapic timer already
calibrated %d\n", lapic_timer_frequency);
-		lapic_clockevent.mult =
div_sc(lapic_timer_frequency/APIC_DIVISOR,
-					TICK_NSEC,
lapic_clockevent.shift);
-		lapic_clockevent.max_delta_ns =
-			clockevent_delta2ns(0x7FFFFF,
&lapic_clockevent);
-		lapic_clockevent.max_delta_ticks = 0x7FFFFF;
-		lapic_clockevent.min_delta_ns =
-			clockevent_delta2ns(0xF, &lapic_clockevent);
-		lapic_clockevent.min_delta_ticks = 0xF;
+		/*
+		 * Newer platforms provide early calibration methods
must have always
+		 * running local APIC timers, no need for boradcast
timer.
+		 */
 		lapic_clockevent.features &= ~CLOCK_EVT_FEAT_DUMMY;
+
 		return 0;
 	}
 
@@ -869,17 +886,8 @@ static int __init calibrate_APIC_clock(void)
 	pm_referenced = !calibrate_by_pmtimer(lapic_cal_pm2 -
lapic_cal_pm1, &delta, &deltatsc);
 
-	/* Calculate the scaled math multiplication factor */
-	lapic_clockevent.mult = div_sc(delta, TICK_NSEC *
LAPIC_CAL_LOOPS,
-				       lapic_clockevent.shift);
-	lapic_clockevent.max_delta_ns =
-		clockevent_delta2ns(0x7FFFFFFF, &lapic_clockevent);
-	lapic_clockevent.max_delta_ticks = 0x7FFFFFFF;
-	lapic_clockevent.min_delta_ns =
-		clockevent_delta2ns(0xF, &lapic_clockevent);
-	lapic_clockevent.min_delta_ticks = 0xF;
-
 	lapic_timer_frequency = (delta * APIC_DIVISOR) /
LAPIC_CAL_LOOPS;
+	calculate_lapic_clockevent();
 
 	apic_printk(APIC_VERBOSE, "..... delta %ld\n", delta);
 	apic_printk(APIC_VERBOSE, "..... mult: %u\n",
lapic_clockevent.mult);
-- 
2.7.4

[tip:x86/apic] x86/tsc: Use CPUID.0x16 to calculate missing crystal frequency

[tip-bot for Daniel Drake]

Commit-ID:  604dc9170f2435d27da5039a3efd757dceadc684
Gitweb:     https://git.kernel.org/tip/604dc9170f2435d27da5039a3efd757dceadc684
Author:     Daniel Drake <drake@endlessm.com>
AuthorDate: Thu, 9 May 2019 13:54:15 +0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 9 May 2019 11:06:48 +0200

x86/tsc: Use CPUID.0x16 to calculate missing crystal frequency

native_calibrate_tsc() had a data mapping Intel CPU families
and crystal clock speed, but hardcoded tables are not ideal, and this
approach was already problematic at least in the Skylake X case, as
seen in commit:

  b51120309348 ("x86/tsc: Fix erroneous TSC rate on Skylake Xeon")

By examining CPUID data from http://instlatx64.atw.hu/ and units
in the lab, we have found that 3 different scenarios need to be dealt
with, and we can eliminate most of the hardcoded data using an approach a
little more advanced than before:

 1. ApolloLake, GeminiLake, CannonLake (and presumably all new chipsets
    from this point) report the crystal frequency directly via CPUID.0x15.
    That's definitive data that we can rely upon.

 2. Skylake, Kabylake and all variants of those two chipsets report a
    crystal frequency of zero, however we can calculate the crystal clock
    speed by condidering data from CPUID.0x16.

    This method correctly distinguishes between the two crystal clock
    frequencies present on different Skylake X variants that caused
    headaches before.

    As the calculations do not quite match the previously-hardcoded values
    in some cases (e.g. 23913043Hz instead of 24MHz), TSC refinement is
    enabled on all platforms where we had to calculate the crystal
    frequency in this way.

 3. Denverton (GOLDMONT_X) reports a crystal frequency of zero and does
    not support CPUID.0x16, so we leave this entry hardcoded.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Daniel Drake <drake@endlessm.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: len.brown@intel.com
Cc: linux@endlessm.com
Cc: rafael.j.wysocki@intel.com
Link: http://lkml.kernel.org/r/20190509055417.13152-1-drake@endlessm.com
Link: https://lkml.kernel.org/r/20190419083533.32388-1-drake@endlessm.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/tsc.c | 47 +++++++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 15b5e98a86f9..6e6d933fb99c 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -631,31 +631,38 @@ unsigned long native_calibrate_tsc(void)
 
 	crystal_khz = ecx_hz / 1000;
 
-	if (crystal_khz == 0) {
-		switch (boot_cpu_data.x86_model) {
-		case INTEL_FAM6_SKYLAKE_MOBILE:
-		case INTEL_FAM6_SKYLAKE_DESKTOP:
-		case INTEL_FAM6_KABYLAKE_MOBILE:
-		case INTEL_FAM6_KABYLAKE_DESKTOP:
-			crystal_khz = 24000;	/* 24.0 MHz */
-			break;
-		case INTEL_FAM6_ATOM_GOLDMONT_X:
-			crystal_khz = 25000;	/* 25.0 MHz */
-			break;
-		case INTEL_FAM6_ATOM_GOLDMONT:
-			crystal_khz = 19200;	/* 19.2 MHz */
-			break;
-		}
-	}
+	/*
+	 * Denverton SoCs don't report crystal clock, and also don't support
+	 * CPUID.0x16 for the calculation below, so hardcode the 25MHz crystal
+	 * clock.
+	 */
+	if (crystal_khz == 0 &&
+			boot_cpu_data.x86_model == INTEL_FAM6_ATOM_GOLDMONT_X)
+		crystal_khz = 25000;
 
-	if (crystal_khz == 0)
-		return 0;
 	/*
-	 * TSC frequency determined by CPUID is a "hardware reported"
+	 * TSC frequency reported directly by CPUID is a "hardware reported"
 	 * frequency and is the most accurate one so far we have. This
 	 * is considered a known frequency.
 	 */
-	setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
+	if (crystal_khz != 0)
+		setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
+
+	/*
+	 * Some Intel SoCs like Skylake and Kabylake don't report the crystal
+	 * clock, but we can easily calculate it to a high degree of accuracy
+	 * by considering the crystal ratio and the CPU speed.
+	 */
+	if (crystal_khz == 0 && boot_cpu_data.cpuid_level >= 0x16) {
+		unsigned int eax_base_mhz, ebx, ecx, edx;
+
+		cpuid(0x16, &eax_base_mhz, &ebx, &ecx, &edx);
+		crystal_khz = eax_base_mhz * 1000 *
+			eax_denominator / ebx_numerator;
+	}
+
+	if (crystal_khz == 0)
+		return 0;
 
 	/*
 	 * For Atom SoCs TSC is the only reliable clocksource.

No 8254 PIT & no HPET on new Intel N3350 platforms causes kernel panic during early boot

[Daniel Drake]

Hi,

I already wrote about this problem in the thread "APIC timer checked
before it is set up, boot fails on Connex L1430"
https://lkml.org/lkml/2018/12/28/10
However my initial diagnosis was misguided, and I have some new
findings to share now, so I'm starting over in this new thread.

Also CCing Hans, who also often attracts this class of problem on low
cost hardware!

The problem is that on affected platforms, all Linux distros (and all
known kernel versions) fail to boot, hanging on a black screen. EFI
earlyprintk can be used to see the panic:

APIC: switch to symmetric I/O mode setup
x2apic: IRQ remapping doesn't support X2APIC mode
..TIMER: vector=0x30 apic1=0 pin1=2 apic2=-1 pin2=-1
..MP-BIOS bug: 8254 timer not connected to IO-APIC
...tryign to set up timer (IRQ0) through the 8259A ...
..... (found apic 0 pin 2) ...
....... failed.
...trying to set up timer as Virtual Wire IRQ...
..... failed.
...trying to set up timer as ExtINT IRQ...
do_IRQ: 0.55 No irq handler for vector
..... failed :(.
Kernel panic - not syncing: IO-APIC + timer doesn't work! Boot with
apic=debug and send a report.

After encountering this on Connex L1430 last time, we have now
encountered another affected product, from a different vendor (SCOPE
SN116PYA). They both have Intel Apollo Lake N3350 and AMI BIOS.

The code in question is making sure that the IRQ0 timer works, by
waiting for an interrupt. In this case there is no interrupt.

The x86 platform code in hpet_time_init() tries to enable the HPET
timer for this, however that is not available on these affected
platforms (no HPET ACPI table). So it then falls back on the 8253/8254
legacy PIT. The i8253.c driver is invoked to program the PIT
accordingly, however in this case it does not result in any IRQ0
interrupts being generated --> panic.

I found a relevant setting in the BIOS: Chipset -> South Cluster
Configuration -> Miscellaneous Configuration -> 8254 Clock Gating
This option is set to Enabled by default. Setting it to Disabled makes
the PIT tick and Linux boot finally works.

It's nice to have a workaround but I would hope we could do better -
especially because it seems like this problem is spreading. In
addition to the two products we found here, searching around finds
several other product manuals and discussions that tell you to go into
the BIOS and change this option if you want Linux to boot, some
examples:
https://blog.csdn.net/qhtsm/article/details/88600316
https://www.manualslib.com/manual/1316475/Ecs-Ed20pa2.html?page=23
https://tools.exone.de/live/shop/img/produkte/fs_112124_2.pdf page 11

As another data point, Windows 10 boots fine in this no-PIT no-HPET
configuation.

Going deeper, I found the clock_gate_8254 option in the coreboot
source code. This pointed me to the ITSSPRC register, which is
documented on page 1694 of
https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/300-series-chipset-pch-datasheet-vol-2.pdf

"8254 Static Clock Gating Enable (CGE8254): When set, the 8254 timer
is disabled statically. This bit shall be set by BIOS if the 8254
feature is not needed in the system or before BIOS hands off the
system that supports C11. Normal operation of 8254 requires this bit
to 0."

(what's C11?)

I verified that the BIOS setting controls this specific bit value, and
I also created and verified a workaround that unsets this bit - now
Linux boots fine regardless of the BIOS setting:

#define INTEL_APL_PSR_BASE        0xd0000000
#define INTEL_APL_PID_ITSS        0xd0
#define INTEL_PCR_PORTID_SHIFT    16
#define INTEL_APL_PCR_ITSSPRC    0x3300
static void quirk_intel_apl_8254(void)
{
    u32 addr = INTEL_APL_PSR_BASE | \
        (INTEL_APL_PID_ITSS << INTEL_PCR_PORTID_SHIFT) | \
        INTEL_APL_PCR_ITSSPRC;
    u32 value;
    void __iomem *itssprc = ioremap_nocache(addr, 4);

    if (!itssprc)
        return;

    value = readl(itssprc);
    if (value & 4) {
        value &= ~4;
        writel(value, itssprc);
    }
    iounmap(itssprc);
}

I was hoping I could send a workaround patch here, but I'm not sure of
an appropriate way to detect that we are on an Intel Apollo Lake
platform. This timer stuff happens during early boot, the early quirks
in pci/quirks.c run too late for this. Suggestions appreciated.

Poking at other angles, I tried taking the HPET ACPI table from
another (working) Intel N3350 system and putting it in the initrd as
an override. This makes the HPET work fine, at which point Linux boots
OK without having to touch the (BIOS-crippled) PIT.

I also spotted that GRUB was previously affected by this BIOS-level
behaviour change.
http://git.savannah.gnu.org/cgit/grub.git/commit/?id=446794de8da4329ea532cbee4ca877bcafd0e534
Apparently GRUB used to rely on the 8254 PIT too, but it now uses the
pmtimer for TSC calibration instead. I guess the originally-affected
platforms only ran into GRUB freezing here (as opposed to both GRUB
and Linux freezing) because those platforms had a working HPET,
meaning that Linux was unaware/unaffected by the newly-gated PIT.

I'm at the limit of my current knowledge here, but there's an open
question of whether Linux could be made to work without a working PIT
and no HPET, in the same way that grub and Windows seem to manage.
Even though it is currently essential for boot, the PIT (or HPET) is
usually only needed to tick a few times before being replaced with the
APIC timer as a clocksource (when setup_APIC_timer() happens, the
clocksource layer disables the previous timer source). However, Thomas
Gleixner gave some hints at the importance of the PIT/HPET here:

> Well, [avoiding the PIT/HPET ticking requirement] would be trivial if we
> could rely on the APIC timer being functional on all CPUs and if we could
> figure out the APIC timer frequency without calibrating it against the
> PIT/HPET on older CPUs. Plus a gazillion of other issues (e.g. APIC stops
> in C states ....)
> [...]
> Under certain conditions we actually might avoid touching PIT/HPET and
> solely rely on the CPUID/MSR calibration values. Needs quite some thought
> though.

I'm not sure what is the best way forward on this issue, but hopefully
this investigation is useful somehow, and I'd be happy to act on any
suggestions.

Thanks
Daniel

Re: No 8254 PIT & no HPET on new Intel N3350 platforms causes kernel panic during early boot

[Thomas Gleixner]

Daniel,

On Wed, 3 Apr 2019, Daniel Drake wrote:

> After encountering this on Connex L1430 last time, we have now
> encountered another affected product, from a different vendor (SCOPE
> SN116PYA). They both have Intel Apollo Lake N3350 and AMI BIOS.
> 
> The code in question is making sure that the IRQ0 timer works, by
> waiting for an interrupt. In this case there is no interrupt.

Right.

> The x86 platform code in hpet_time_init() tries to enable the HPET
> timer for this, however that is not available on these affected
> platforms (no HPET ACPI table). So it then falls back on the 8253/8254
> legacy PIT. The i8253.c driver is invoked to program the PIT
> accordingly, however in this case it does not result in any IRQ0
> interrupts being generated --> panic.

Correct.

> I found a relevant setting in the BIOS: Chipset -> South Cluster
> Configuration -> Miscellaneous Configuration -> 8254 Clock Gating
> This option is set to Enabled by default. Setting it to Disabled makes
> the PIT tick and Linux boot finally works.

Well, your BIOS at least has this switch ...

> As another data point, Windows 10 boots fine in this no-PIT no-HPET
> configuation.

We have support for HPET/PIT less systems already. We just need to figure
out how to switch to that mode automagically at early boot.

ACPI obviously does not switch to it with the ACPI_FADT_HW_REDUCED flag.

> Going deeper, I found the clock_gate_8254 option in the coreboot
> source code. This pointed me to the ITSSPRC register, which is
> documented on page 1694 of
> https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/300-series-chipset-pch-datasheet-vol-2.pdf
> 
> "8254 Static Clock Gating Enable (CGE8254): When set, the 8254 timer
> is disabled statically. This bit shall be set by BIOS if the 8254
> feature is not needed in the system or before BIOS hands off the
> system that supports C11. Normal operation of 8254 requires this bit
> to 0."
> 
> (what's C11?)

Don't know. Some magic new C-State perhaps? Rafael?

Btw, one of those links you provided

  https://www.manualslib.com/manual/1316475/Ecs-Ed20pa2.html?page=23

claims that you have to disable MWAIT as well. No idea why. Is MWAIT
disabled on your platform?

> I verified that the BIOS setting controls this specific bit value, and
> I also created and verified a workaround that unsets this bit - now
> Linux boots fine regardless of the BIOS setting:
> 
> #define INTEL_APL_PSR_BASE        0xd0000000
> #define INTEL_APL_PID_ITSS        0xd0
> #define INTEL_PCR_PORTID_SHIFT    16
> #define INTEL_APL_PCR_ITSSPRC    0x3300
> static void quirk_intel_apl_8254(void)
> {
>     u32 addr = INTEL_APL_PSR_BASE | \
>         (INTEL_APL_PID_ITSS << INTEL_PCR_PORTID_SHIFT) | \
>         INTEL_APL_PCR_ITSSPRC;
>     u32 value;
>     void __iomem *itssprc = ioremap_nocache(addr, 4);
> 
>     if (!itssprc)
>         return;
> 
>     value = readl(itssprc);
>     if (value & 4) {
>         value &= ~4;
>         writel(value, itssprc);
>     }
>     iounmap(itssprc);
> }
>
> I was hoping I could send a workaround patch here, but I'm not sure of
> an appropriate way to detect that we are on an Intel Apollo Lake
> platform. This timer stuff happens during early boot, the early quirks
> in pci/quirks.c run too late for this. Suggestions appreciated.

We have early-quirks.c in arch/x86/kernel/ for that.

> Poking at other angles, I tried taking the HPET ACPI table from
> another (working) Intel N3350 system and putting it in the initrd as
> an override. This makes the HPET work fine, at which point Linux boots
> OK without having to touch the (BIOS-crippled) PIT.

We already have quirks for force enabling HPET, so that could be added.

> I'm at the limit of my current knowledge here, but there's an open
> question of whether Linux could be made to work without a working PIT
> and no HPET, in the same way that grub and Windows seem to manage.
> Even though it is currently essential for boot, the PIT (or HPET) is
> usually only needed to tick a few times before being replaced with the
> APIC timer as a clocksource (when setup_APIC_timer() happens, the
> clocksource layer disables the previous timer source). However, Thomas
> Gleixner gave some hints at the importance of the PIT/HPET here:
> 
> > Well, [avoiding the PIT/HPET ticking requirement] would be trivial if we
> > could rely on the APIC timer being functional on all CPUs and if we could
> > figure out the APIC timer frequency without calibrating it against the
> > PIT/HPET on older CPUs. Plus a gazillion of other issues (e.g. APIC stops
> > in C states ....)
> > [...]
> > Under certain conditions we actually might avoid touching PIT/HPET and
> > solely rely on the CPUID/MSR calibration values. Needs quite some thought
> > though.

For newer CPUs we might assume that:

 1) The TSC and APIC timer are actually usable

 2) The frequencies can be retrieved from CPUID or MSRs

If #1 and #2 are reliable we can avoid the whole calibration and interrupt
delivery mess.

That means we need the following decision logic:

  1) If HPET is available in ACPI, boot normal.

  2) If HPET is not available, verify that the PIT actually counts. If it
     does, boot normal.

     If it does not either:

     2A) Verify that this is a PCH 300/C240 and fiddle with that ISST bit.

     	 But that means that we need to chase PCH ids forever...

     2B) Shrug and just avoid the whole PIT/HPET magic all over the place:

     	 - Avoid the interrupt delivery check in the IOAPIC code as it's
           uninteresting in that case. Trivial to do.
	 
	 - Prevent the TSC calibration code from touching PIT/HPET. It
           should do that already when the TSC frequency can be retrieved
           via CPUID or MSR. Should work, emphasis on should ...

    	   See the mess in: native_calibrate_tsc() and the magic tables in
	   tsc_msr.c how well that stuff works.

	   The cpu_khz_from_cpuid() case at seems to not have these
	   issues. Knock on wood!

         - Prevent the APIC calibration code from touching PIT/HPET. That's
           only happening right now when the TSC frequency comes from
	   the MSRs. No idea why the CPUID method does not provide that.

	   CPUID leaf 0x16 provides the bus frequency, so we can deduce the
	   APIC timer frequency from there and spare the whole APIC timer
	   calibration mess:

  	       ECX Bits 15 - 00: Bus (Reference) Frequency (in MHz).

	   It's usually not required on these newer CPUs because they
	   support TSC deadline timer, but you can disable that on the
	   kernel command line and some implementations of that were
	   broken. With that we are back to square one.

	   So we need to make sure that these things work under all
	   circumstances.

   Rafael?

Thanks,

	tglx

Re: No 8254 PIT & no HPET on new Intel N3350 platforms causes kernel panic during early boot

[Thomas Gleixner]

On Wed, 3 Apr 2019, Thomas Gleixner wrote:
> For newer CPUs we might assume that:
> 
>  1) The TSC and APIC timer are actually usable
> 
>  2) The frequencies can be retrieved from CPUID or MSRs
> 
> If #1 and #2 are reliable we can avoid the whole calibration and interrupt
> delivery mess.
> 
> That means we need the following decision logic:
> 
>   1) If HPET is available in ACPI, boot normal.
> 
>   2) If HPET is not available, verify that the PIT actually counts. If it
>      does, boot normal.
> 
>      If it does not either:
> 
>      2A) Verify that this is a PCH 300/C240 and fiddle with that ISST bit.
> 
>      	 But that means that we need to chase PCH ids forever...
> 
>      2B) Shrug and just avoid the whole PIT/HPET magic all over the place:
> 
>      	 - Avoid the interrupt delivery check in the IOAPIC code as it's
>            uninteresting in that case. Trivial to do.
> 	 
> 	 - Prevent the TSC calibration code from touching PIT/HPET. It
>            should do that already when the TSC frequency can be retrieved
>            via CPUID or MSR. Should work, emphasis on should ...
> 
>     	   See the mess in: native_calibrate_tsc() and the magic tables in
> 	   tsc_msr.c how well that stuff works.
> 
> 	   The cpu_khz_from_cpuid() case at seems to not have these
> 	   issues. Knock on wood!
> 
>          - Prevent the APIC calibration code from touching PIT/HPET. That's
>            only happening right now when the TSC frequency comes from
> 	   the MSRs. No idea why the CPUID method does not provide that.
> 
> 	   CPUID leaf 0x16 provides the bus frequency, so we can deduce the
> 	   APIC timer frequency from there and spare the whole APIC timer
> 	   calibration mess:
> 
>   	       ECX Bits 15 - 00: Bus (Reference) Frequency (in MHz).
> 
> 	   It's usually not required on these newer CPUs because they
> 	   support TSC deadline timer, but you can disable that on the
> 	   kernel command line and some implementations of that were
> 	   broken. With that we are back to square one.
> 
> 	   So we need to make sure that these things work under all
> 	   circumstances.
> 
>    Rafael?

And we have to think hard about how we are going to provide that for
backporting in a digestable form. People are supposed to run recent stable
kernels (I'm not talking about dead kernels ...).

Thanks,

	tglx

Re: No 8254 PIT & no HPET on new Intel N3350 platforms causes kernel panic during early boot

[Daniel Drake]

On Wed, Apr 3, 2019 at 7:21 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> Btw, one of those links you provided
>
>   https://www.manualslib.com/manual/1316475/Ecs-Ed20pa2.html?page=23
>
> claims that you have to disable MWAIT as well. No idea why. Is MWAIT
> disabled on your platform?

I don't have that option in the BIOS. However there's no mention of
"mwait" nor "mwaitx" in /proc/cpuinfo. Checking our more general
database of 202 x86_64 consumer products released over the last few
years, only 19 of them have mwait/mwaitx listed there and they tend to
be older platforms.

> We have early-quirks.c in arch/x86/kernel/ for that.

Nice, we should be able to work around the issue there, but I hope we
can find something better...

> For newer CPUs we might assume that:
>
>  1) The TSC and APIC timer are actually usable
>
>  2) The frequencies can be retrieved from CPUID or MSRs
>
> If #1 and #2 are reliable we can avoid the whole calibration and interrupt
> delivery mess.

Let's take a step back and re-examine the wider sequence of events
here (which I've now done thanks to your pointers).

1. In very early boot, we face the TSC calibration challenge, arriving
at determine_cpu_tsc_frequencies(). This function calculates CPU
frequency and TSC frequency separately.

For the CPU frequency, native_calibrate_cpu_early() tries to do it via
cpu_khz_from_cpuid() with CPUID leaf 0x16, but this is not available
on the platforms in question, which have max cpuid level 0x15.
cpu_khz_from_msr() is then tried, but that doesn't support this
platform either (looks like it only supports older SoC generations).
So now we arrive in quick_pit_calibrate(), which directly programs the
PIT and measures the TSC rate against the PIT ticks.

When the 8254 is ungated in the BIOS, this function fails early because:
    if (pit_expect_msb(0xff, &tsc, &d1)) {
/* returned at count=13, d1 is now 32118 */
        for (i = 1; i <= MAX_QUICK_PIT_ITERATIONS; i++) {
            if (!pit_expect_msb(0xff-i, &delta, &d2))
/* returned at count=13, d2 is now 48595 */
                break;

            delta -= tsc;
/* delta is now 246741 */

            /*
             * Extrapolate the error and fail fast if the error will
             * never be below 500 ppm.
             */
            if (i == 1 &&
                d1 + d2 >= (delta * MAX_QUICK_PIT_ITERATIONS) >> 11)
                return 0;
/* this return statement is hit, the calculation is:
32118 + 48595 >= (246741 * 233) >> 11
*/

so the error was too high (I'm not sure why) and
determine_cpu_tsc_frequencies() records the CPU frequency as 0.

Then, comparing to when the 8254 is gated via the BIOS option, the
behaviour is surprising too.
In that case, the quick_calibrate_pit() loop runs through up to i=128,
at which point the error level is low enough to be accepted,
calculating the CPU frequency as 4448MHz (4x higher than reality).
During each loop iteration, pit_expect_msb() returns when the value
changes at count=63 (compared to 13 in the PIT-ungated case). Does
this suggest the PIT is not actually fully gated, it's just ticking a
lot slower than otherwise?

Anyway, back in determine_cpu_tsc_frequencies() with the CPU frequency
calibration done, we now do TSC calibration. This one succeeds in all
cases via native_calibrate_tsc()  using CPUID leaf 0x15 to read the
correct value. The TSC is 1094MHz.

Then, in both cases (8254 gated or not), the CPU frequency calculation
is discarded here, because it's wildly different from the TSC rate:
    else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
        cpu_khz = tsc_khz;

So it seems that this code already behaves along the lines you
describe: it gives more trust to the TSC value read in the modern way,
and does not get upset if the CPU frequency calibration against the
PIT didn't produce a meaningful result.


2. Significantly later during boot, x86_late_time_init() calls
hpet_time_init() which sets up either the PIT or HPET. However, as far
as I can see, there is no checking that the selected clock is actually
ticking. In the case of these affected platforms with the 8254 gated,
we sail right pass this point without a working clock source.

3. x86_late_time_init() then calls apic_intr_mode_init() ->
apic_bsp_setup() -> setup_IO_APIC() and at this point we reach
check_timer(), which attempts to verify/fixup IRQ0 delivery via the
IO-APIC. At this point we check that jiffies increments, and if not,
panic.

4. Some time later, naive_smp_prepare_cpus() calls
setup_boot_APIC_clock() -> setup_APIC_timer() which registers the
local APIC clocksource, replacing the previous PIT/HPET clocksource.
There's no check to make sure that the new clocksource is ticking, as
far as I can see.

5. Some time later, start_secondary() calls
start_secondary_APIC_clock() -> setup_APIC_timer() registering the
APIC clocksource (again? or just for another CPU core?).


Hopefully that analysis helps refine/elaborate the plan a bit more...

> That means we need the following decision logic:
>
>   1) If HPET is available in ACPI, boot normal.
>
>   2) If HPET is not available, verify that the PIT actually counts. If it
>      does, boot normal.
>
>      If it does not either:
>
>      2A) Verify that this is a PCH 300/C240 and fiddle with that ISST bit.
>
>          But that means that we need to chase PCH ids forever...

(I found the ISST bit in the coreboot source code, which shows that
the register is shared over multiple Intel SoC generations. I then
searched for the register name online and found it documented in the
320/C240 public documentation, which I linked to. However that's not
actually the platform in question. In this case we are working with
Intel Apollo Lake N3350.)

Anyway, I agree that doing it with PCI IDs would be painful.

>      2B) Shrug and just avoid the whole PIT/HPET magic all over the place:
>
>          - Avoid the interrupt delivery check in the IOAPIC code as it's
>            uninteresting in that case. Trivial to do.

What do you mean by "in that case"? In the case of having an IOAPIC?

From my analysis above, this interrupt delivery check feels misplaced.
Other parts of the clock setup code (e.g. where PIC, HPET and APIC
timer are enabled) do not seem to check that the timers being set up
actually work. If I were to try a kernel with no APIC/LAPIC support
then Linux would boot with a broken PIT as the clock source without
checking it. So why do we check it here specifically in the IOAPIC
code? I see it does some tricks which are presumably needed on
historical platforms, but maybe it could let boot continue even if it
can't find a working IRQ0 setup? Or it could at least skip the check
if IRQ0 was not working before the IOAPIC gets set up?

If there is desire for some "check that the clocksource is actually
ticking" panic logic, maybe this could be done after the local APIC
timer is setup (which is ultimately the clock source selected and
used), maybe it should even be done in arch-independent code?

>          - Prevent the TSC calibration code from touching PIT/HPET. It
>            should do that already when the TSC frequency can be retrieved
>            via CPUID or MSR. Should work, emphasis on should ...

From above, this seems to be working acceptably already. It does touch
the PIT, but ultimately ignores the information that it provided.

>          - Prevent the APIC calibration code from touching PIT/HPET. That's
>            only happening right now when the TSC frequency comes from
>            the MSRs. No idea why the CPUID method does not provide that.

Where's the APIC calibration code?

>            CPUID leaf 0x16 provides the bus frequency, so we can deduce the
>            APIC timer frequency from there and spare the whole APIC timer
>            calibration mess:
>
>                ECX Bits 15 - 00: Bus (Reference) Frequency (in MHz).

That's not available on this platform, plus
https://software.intel.com/sites/default/files/managed/c5/15/architecture-instruction-set-extensions-programming-reference.pdf
page 1-21 says that the data returned is actually marketing stuff, and
shouldn't be treated as real. I think you mean CPUID leaf 0x15
instead.

Thanks for your input!

Daniel

Re: No 8254 PIT & no HPET on new Intel N3350 platforms causes kernel panic during early boot

[Thomas Gleixner]

On Tue, 9 Apr 2019, Daniel Drake wrote:
> On Wed, Apr 3, 2019 at 7:21 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> > That means we need the following decision logic:
> >
> >   1) If HPET is available in ACPI, boot normal.
> >
> >   2) If HPET is not available, verify that the PIT actually counts. If it
> >      does, boot normal.
> >
> >      If it does not either:
> >
> >      2A) Verify that this is a PCH 300/C240 and fiddle with that ISST bit.
> >
> >          But that means that we need to chase PCH ids forever...
> 
> (I found the ISST bit in the coreboot source code, which shows that
> the register is shared over multiple Intel SoC generations. I then
> searched for the register name online and found it documented in the
> 320/C240 public documentation, which I linked to. However that's not
> actually the platform in question. In this case we are working with
> Intel Apollo Lake N3350.)
> 
> Anyway, I agree that doing it with PCI IDs would be painful.
> 
> >      2B) Shrug and just avoid the whole PIT/HPET magic all over the place:
> >
> >          - Avoid the interrupt delivery check in the IOAPIC code as it's
> >            uninteresting in that case. Trivial to do.
> 
> What do you mean by "in that case"? In the case of having an IOAPIC?

In the case that neither HPET nor PIT are available or required.

> From my analysis above, this interrupt delivery check feels misplaced.

Not really.

> Other parts of the clock setup code (e.g. where PIC, HPET and APIC
> timer are enabled) do not seem to check that the timers being set up
> actually work. If I were to try a kernel with no APIC/LAPIC support
> then Linux would boot with a broken PIT as the clock source without
> checking it. So why do we check it here specifically in the IOAPIC
> code? I see it does some tricks which are presumably needed on
> historical platforms, but maybe it could let boot continue even if it
> can't find a working IRQ0 setup? Or it could at least skip the check
> if IRQ0 was not working before the IOAPIC gets set up?

It's not only historical. The irq0 'wiring' is still screwed up on newer
systems and we cannot rely on the ACPI/BIOS information.
 
> If there is desire for some "check that the clocksource is actually
> ticking" panic logic, maybe this could be done after the local APIC
> timer is setup (which is ultimately the clock source selected and
> used), maybe it should even be done in arch-independent code?

Well. Most systems have working timers. Just x86 is an utter trainwreck in
that regard.

PIT/HPET used to work just fine forever (except for the actual interrupt
delivery) so this is new terretory.
 
> >          - Prevent the TSC calibration code from touching PIT/HPET. It
> >            should do that already when the TSC frequency can be retrieved
> >            via CPUID or MSR. Should work, emphasis on should ...
> 
> >From above, this seems to be working acceptably already. It does touch
> the PIT, but ultimately ignores the information that it provided.

Yes, but we might actually be smarter than that.

> >          - Prevent the APIC calibration code from touching PIT/HPET. That's
> >            only happening right now when the TSC frequency comes from
> >            the MSRs. No idea why the CPUID method does not provide that.
> 
> Where's the APIC calibration code?

calibrate_APIC_clock()
 
> >            CPUID leaf 0x16 provides the bus frequency, so we can deduce the
> >            APIC timer frequency from there and spare the whole APIC timer
> >            calibration mess:
> >
> >                ECX Bits 15 - 00: Bus (Reference) Frequency (in MHz).
> 
> That's not available on this platform, plus
> architecture-instruction-set-extensions-programming-reference.pdf

Please use the SDM as reference, but yes it tells the same.

> page 1-21 says that the data returned is actually marketing stuff, and
> shouldn't be treated as real. I think you mean CPUID leaf 0x15
> instead.

No, it's not marketing. It's the specification and there must be a reason
why Intel decided to actually use it in the kernel:

    Author: Len Brown <len.brown@intel.com>
    Date:   Fri Jun 17 01:22:51 2016 -0400

    x86/tsc: Enumerate SKL cpu_khz and tsc_khz via CPUID
    
    Skylake CPU base-frequency and TSC frequency may differ
    by up to 2%.
    
    Enumerate CPU and TSC frequencies separately, allowing
    cpu_khz and tsc_khz to differ.
    
    The existing CPU frequency calibration mechanism is unchanged.
    However, CPUID extensions are preferred, when available.
    
    CPUID.0x16 is preferred over MSR and timer calibration
    for CPU frequency discovery.
    
    CPUID.0x15 takes precedence over CPU-frequency
    for TSC frequency discovery.


Leaf 0x15 does not tell the bus/reference clock which is what we need for
using the local apic timer (not the TSC deadline timer).

Thanks,

	tglx

Re: No 8254 PIT & no HPET on new Intel N3350 platforms causes kernel panic during early boot

[Daniel Drake]

On Wed, Apr 10, 2019 at 8:54 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> On Tue, 9 Apr 2019, Daniel Drake wrote:
> > On Wed, Apr 3, 2019 at 7:21 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> > >          - Prevent the TSC calibration code from touching PIT/HPET. It
> > >            should do that already when the TSC frequency can be retrieved
> > >            via CPUID or MSR. Should work, emphasis on should ...
> >
> > >From above, this seems to be working acceptably already. It does touch
> > the PIT, but ultimately ignores the information that it provided.
>
> Yes, but we might actually be smarter than that.

Do you have anything specific in mind?

You originally laid out this idea in the context of doing this if the
PIT/HPET is not working. However, I can't immediately see how to judge
that because:
- According to the analysis in my last mail, the PIT is actually
ticking even when it is gated in the BIOS. The BIOS setting just seems
to make it tick 4 times slower and not generate any IRQ0 interrupts.
- TSC calibration code runs really early during boot. To make it
detect this situation we could make it check if IRQ0 is working,
however setup_default_timer_irq() only happens a lot later, so I'm not
sure how this could be checked at such an early stage.

I think I'm now fairly clear on the other suggestions you have made so
I'll see if I can come up with some patches.

Thanks!
Daniel

[tip:x86/apic] x86/tsc: Set LAPIC timer period to crystal clock frequency

[tip-bot for Daniel Drake]

Commit-ID:  2420a0b1798d7a78d1f9b395f09f3c80d92cc588
Gitweb:     https://git.kernel.org/tip/2420a0b1798d7a78d1f9b395f09f3c80d92cc588
Author:     Daniel Drake <drake@endlessm.com>
AuthorDate: Thu, 9 May 2019 13:54:17 +0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 9 May 2019 11:06:49 +0200

x86/tsc: Set LAPIC timer period to crystal clock frequency

The APIC timer calibration (calibrate_APIC_timer()) can be skipped
in cases where we know the APIC timer frequency. On Intel SoCs,
we believe that the APIC is fed by the crystal clock; this would make
sense, and the crystal clock frequency has been verified against the
APIC timer calibration result on ApolloLake, GeminiLake, Kabylake,
CoffeeLake, WhiskeyLake and AmberLake.

Set lapic_timer_period based on the crystal clock frequency
accordingly.

APIC timer calibration would normally be skipped on modern CPUs
by nature of the TSC deadline timer being used instead,
however this change is still potentially useful, e.g. if the
TSC deadline timer has been disabled with a kernel parameter.
calibrate_APIC_timer() uses the legacy timer, but we are seeing
new platforms that omit such legacy functionality, so avoiding
such codepaths is becoming more important.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Daniel Drake <drake@endlessm.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: len.brown@intel.com
Cc: linux@endlessm.com
Cc: rafael.j.wysocki@intel.com
Link: http://lkml.kernel.org/r/20190509055417.13152-3-drake@endlessm.com
Link: https://lkml.kernel.org/r/20190419083533.32388-1-drake@endlessm.com
Link: https://lkml.kernel.org/r/alpine.DEB.2.21.1904031206440.1967@nanos.tec.linutronix.de
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/tsc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 6e6d933fb99c..8f47c4862c56 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -671,6 +671,16 @@ unsigned long native_calibrate_tsc(void)
 	if (boot_cpu_data.x86_model == INTEL_FAM6_ATOM_GOLDMONT)
 		setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
 
+#ifdef CONFIG_X86_LOCAL_APIC
+	/*
+	 * The local APIC appears to be fed by the core crystal clock
+	 * (which sounds entirely sensible). We can set the global
+	 * lapic_timer_period here to avoid having to calibrate the APIC
+	 * timer later.
+	 */
+	lapic_timer_period = crystal_khz * 1000 / HZ;
+#endif
+
 	return crystal_khz * ebx_numerator / eax_denominator;
 }
 

Re: No 8254 PIT & no HPET on new Intel N3350 platforms causes kernel panic during early boot

[Daniel Drake]

Hi Thomas,

Picking up this issue again after a break!

We made some progress last time on reducing PIT usage in the TSC
calibration code, but we still have the bigger issue to resolve:
IO-APIC code panicing when the PIT isn't ticking.

On Wed, Apr 3, 2019 at 7:21 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> For newer CPUs we might assume that:
>
>  1) The TSC and APIC timer are actually usable
>
>  2) The frequencies can be retrieved from CPUID or MSRs
>
> If #1 and #2 are reliable we can avoid the whole calibration and interrupt
> delivery mess.
>
> That means we need the following decision logic:
>
>   1) If HPET is available in ACPI, boot normal.
>
>   2) If HPET is not available, verify that the PIT actually counts. If it
>      does, boot normal.
>
>      If it does not either:
>
>      2A) Verify that this is a PCH 300/C240 and fiddle with that ISST bit.
>
>          But that means that we need to chase PCH ids forever...
>
>      2B) Shrug and just avoid the whole PIT/HPET magic all over the place:
>
>          - Avoid the interrupt delivery check in the IOAPIC code as it's
>            uninteresting in that case. Trivial to do.

I tried to explore this idea here:
https://lore.kernel.org/patchwork/patch/1064972/
https://lore.kernel.org/patchwork/patch/1064971/

But I can't say I really knew what I was doing there, and you
pointed out some problems.

Any new ideas that I can experiment with?

Being more conservative, how about something like this?
---
 arch/x86/kernel/apic/io_apic.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 53aa234a6803..36b1e7d5b657 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2073,7 +2073,7 @@ static int mp_alloc_timer_irq(int ioapic, int pin)
  *
  * FIXME: really need to revamp this for all platforms.
  */
-static inline void __init check_timer(void)
+static inline void __init check_timer(int timer_was_working)
 {
 	struct irq_data *irq_data = irq_get_irq_data(0);
 	struct mp_chip_data *data = irq_data->chip_data;
@@ -2216,8 +2216,15 @@ static inline void __init check_timer(void)
 		apic_printk(APIC_QUIET, KERN_INFO
 			    "Perhaps problem with the pre-enabled x2apic mode\n"
 			    "Try booting with x2apic and interrupt-remapping disabled in the bios.\n");
-	panic("IO-APIC + timer doesn't work!  Boot with apic=debug and send a "
-		"report.  Then try booting with the 'noapic' option.\n");
+
+	if (timer_was_working)
+		panic("IO-APIC + timer doesn't work!  Boot with apic=debug and send a "
+			"report.  Then try booting with the 'noapic' option.\n");
+	else
+		apic_printk(APIC_QUIET, KERN_INFO
+			    "Continuing anyway with no 8254 timer, as it was not working even before IO-APIC setup was attempted.\n"
+			    "Boot will fail unless another working clocksource is found.\n");
+
 out:
 	local_irq_restore(flags);
 }
@@ -2304,12 +2311,20 @@ static void ioapic_destroy_irqdomain(int idx)
 void __init setup_IO_APIC(void)
 {
 	int ioapic;
+	int timer_was_working = 0;
 
 	if (skip_ioapic_setup || !nr_ioapics)
 		return;
 
 	io_apic_irqs = nr_legacy_irqs() ? ~PIC_IRQS : ~0UL;
 
+	/*
+	 * Record if the timer was in working state before we do any
+	 * IO-APIC setup.
+	 */
+	if (nr_legacy_irqs())
+		timer_was_working = timer_irq_works();
+
 	apic_printk(APIC_VERBOSE, "ENABLING IO-APIC IRQs\n");
 	for_each_ioapic(ioapic)
 		BUG_ON(mp_irqdomain_create(ioapic));
@@ -2323,7 +2338,7 @@ void __init setup_IO_APIC(void)
 	setup_IO_APIC_irqs();
 	init_IO_APIC_traps();
 	if (nr_legacy_irqs())
-		check_timer();
+		check_timer(timer_was_working);
 
 	ioapic_initialized = 1;
 }
-- 
2.20.1

Re: No 8254 PIT & no HPET on new Intel N3350 platforms causes kernel panic during early boot

[Thomas Gleixner]

Daniel,

On Thu, 27 Jun 2019, Daniel Drake wrote:
> Picking up this issue again after a break!
> 
> We made some progress last time on reducing PIT usage in the TSC
> calibration code, but we still have the bigger issue to resolve:
> IO-APIC code panicing when the PIT isn't ticking.

Yeah. I was busy with other stuff and simply forgot.

> Being more conservative, how about something like this?
>  
> +	/*
> +	 * Record if the timer was in working state before we do any
> +	 * IO-APIC setup.
> +	 */
> +	if (nr_legacy_irqs())
> +		timer_was_working = timer_irq_works();

Nah. That extra timer works thing is just another bandaid.

What I had in mind is something like the below. That's on top of

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/apic

Be warned. It's neither compiled nor tested, so keep a fire extinguisher
handy. If it explodes you own the pieces.

/me goes off to find icecream 

Thanks,

	tglx

8<-----------------
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -173,6 +173,7 @@ extern void lapic_assign_system_vectors(
 extern void lapic_assign_legacy_vector(unsigned int isairq, bool replace);
 extern void lapic_online(void);
 extern void lapic_offline(void);
+extern bool apic_needs_pit(void);
 
 #else /* !CONFIG_X86_LOCAL_APIC */
 static inline void lapic_shutdown(void) { }
@@ -186,6 +187,7 @@ static inline void init_bsp_APIC(void) {
 static inline void apic_intr_mode_init(void) { }
 static inline void lapic_assign_system_vectors(void) { }
 static inline void lapic_assign_legacy_vector(unsigned int i, bool r) { }
+static inline bool apic_needs_pit(void) { return true; }
 #endif /* !CONFIG_X86_LOCAL_APIC */
 
 #ifdef CONFIG_X86_X2APIC
--- a/arch/x86/include/asm/time.h
+++ b/arch/x86/include/asm/time.h
@@ -7,6 +7,7 @@
 
 extern void hpet_time_init(void);
 extern void time_init(void);
+extern bool pit_timer_init(void);
 
 extern struct clock_event_device *global_clock_event;
 
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -820,6 +820,33 @@ static int __init lapic_init_clockevent(
 	return 0;
 }
 
+bool __init apic_and_tsc_needs_pit(void)
+{
+	/*
+	 * If the frequencies are not known, PIT is required for both TSC
+	 * and apic timer calibration.
+	 */
+	if (!tsc_khz || !cpu_khz)
+		return true;
+
+	/* Is there an APIC at all? */
+	if (!boot_cpu_has(X86_FEATURE_APIC))
+		return true;
+
+	/* Deadline timer is based on TSC so no further PIT action required */
+	if (boot_cpu_has(X86_FEATURE_TSC_DEADLINE_TIMER))
+		return false;
+
+	/* APIC timer disabled? */
+	if (disable_apic_timer)
+		return true;
+	/*
+	 * The APIC timer frequency is known already, no PIT calibration
+	 * required. If unknown, let the PIT be initialized.
+	 */
+	return lapic_timer_period == 0;
+}
+
 static int __init calibrate_APIC_clock(void)
 {
 	struct clock_event_device *levt = this_cpu_ptr(&lapic_events);
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -58,6 +58,7 @@
 #include <asm/acpi.h>
 #include <asm/dma.h>
 #include <asm/timer.h>
+#include <asm/time.h>
 #include <asm/i8259.h>
 #include <asm/setup.h>
 #include <asm/irq_remapping.h>
@@ -2083,6 +2084,9 @@ static inline void __init check_timer(vo
 	unsigned long flags;
 	int no_pin1 = 0;
 
+	if (!global_clock_event)
+		return;
+
 	local_irq_save(flags);
 
 	/*
--- a/arch/x86/kernel/i8253.c
+++ b/arch/x86/kernel/i8253.c
@@ -8,6 +8,7 @@
 #include <linux/timex.h>
 #include <linux/i8253.h>
 
+#include <asm/apic.h>
 #include <asm/hpet.h>
 #include <asm/time.h>
 #include <asm/smp.h>
@@ -18,10 +19,32 @@
  */
 struct clock_event_device *global_clock_event;
 
-void __init setup_pit_timer(void)
+/*
+ * Modern chipsets can disable the PIT clock which makes it unusable. It
+ * would be possible to enable the clock but the registers are chipset
+ * specific and not discoverable. Avoid the whack a mole game.
+ *
+ * These platforms have discoverable TSC/CPU frequencies but this also
+ * requires to know the local APIC timer frequency as it normally is
+ * calibrated against the PIT interrupt.
+ */
+static bool __init use_pit(void)
 {
+	if (!IS_ENABLED(CONFIG_X86_TSC) || !boot_cpu_has(X86_FEATURE_TSC))
+		return true;
+
+	/* This also returns true when APIC is disabled */
+	return apic_needs_pit();
+}
+
+bool __init pit_timer_init(void)
+{
+	if (!use_pit())
+		return false;
+
 	clockevent_i8253_init(true);
 	global_clock_event = &i8253_clockevent;
+	return true;
 }
 
 #ifndef CONFIG_X86_64
--- a/arch/x86/kernel/time.c
+++ b/arch/x86/kernel/time.c
@@ -82,8 +82,11 @@ static void __init setup_default_timer_i
 /* Default timer init function */
 void __init hpet_time_init(void)
 {
-	if (!hpet_enable())
-		setup_pit_timer();
+	if (!hpet_enable()) {
+		if (!pit_timer_init())
+			return;
+	}
+
 	setup_default_timer_irq();
 }
 

Re: No 8254 PIT & no HPET on new Intel N3350 platforms causes kernel panic during early boot

[Daniel Drake]

On Thu, Jun 27, 2019 at 10:07 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> Nah. That extra timer works thing is just another bandaid.
>
> What I had in mind is something like the below. That's on top of
>
>    git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/apic
>
> Be warned. It's neither compiled nor tested, so keep a fire extinguisher
> handy. If it explodes you own the pieces.

Thanks, it works, and makes sense!

I'll add a commit message and send it as a patch, just one quick
function naming question... did you mean apic_and_tsc_needs_pit() or
apic_needs_pit()? That's the only compile fix needed.

Daniel

Re: No 8254 PIT & no HPET on new Intel N3350 platforms causes kernel panic during early boot

[Thomas Gleixner]

On Fri, 28 Jun 2019, Daniel Drake wrote:
> On Thu, Jun 27, 2019 at 10:07 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> > Nah. That extra timer works thing is just another bandaid.
> >
> > What I had in mind is something like the below. That's on top of
> >
> >    git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/apic
> >
> > Be warned. It's neither compiled nor tested, so keep a fire extinguisher
> > handy. If it explodes you own the pieces.
> 
> Thanks, it works, and makes sense!
> 
> I'll add a commit message and send it as a patch, just one quick
> function naming question... did you mean apic_and_tsc_needs_pit() or
> apic_needs_pit()? That's the only compile fix needed.

I'd say apic_needs_pit(). Less places to change :)

Thanks,

	tglx