Re: [PATCH v15 19/26] x86/tsc: calibrate tsc only once

Re: [PATCH v15 19/26] x86/tsc: calibrate tsc only once

[Chuan Hua, Lei]

> static unsigned long __init get_loops_per_jiffy(void)
> {
> 	unsigned long lpj = tsc_khz * KHZ;
> 
> 	do_div(lpj, HZ);
> 	return lpj;
> }
Just tried this with 4.19-rc2 on x86(32bit). lpj return as zero which is not expected
After disassembling the code,
	0xc1239a9e <+199>:   imul   $0x3e8,0xc12296e4,%edx
	0xc1239aa8 <+209>:   xor    %ecx,%ecx
	0xc1239aaa <+211>:   test   %edx,%edx
	0xc1239aac <+213>:   mov    %eax,%ebx
	0xc1239aae <+215>:   je     0xc1239abd <tsc_init+230>
	0xc1239ab0 <+217>:   mov    $0x64,%ecx
	0xc1239ab5 <+222>:   mov    %edx,%eax
	0xc1239ab7 <+224>:   xor    %edx,%edx
	0xc1239ab9 <+226>:   div    %ecx
	0xc1239abb <+228>:   mov    %eax,%ecx
	0xc1239abd <+230>:   mov    %ebx,%eax
	0xc1239abf <+232>:   mov    $0x64,%ebx
	0xc1239ac4 <+237>:   div    %ebx
	0xc1239ac6 <+239>:   mov    %ecx,%edx
imul will load the result into %edx, %edx supposed to be high 32 bit which is not zero,
It should be zero in this case. both lpj and tsc_khz should be u64 to work properly.

Re: [PATCH v15 19/26] x86/tsc: calibrate tsc only once

[Thomas Gleixner]

On Wed, 5 Sep 2018, Chuan Hua, Lei wrote:

> > static unsigned long __init get_loops_per_jiffy(void)
> > {
> > 	unsigned long lpj = tsc_khz * KHZ;
> > 
> > 	do_div(lpj, HZ);
> > 	return lpj;
> > }
> Just tried this with 4.19-rc2 on x86(32bit). lpj return as zero which is not
> expected
> After disassembling the code,
> 	0xc1239a9e <+199>:   imul   $0x3e8,0xc12296e4,%edx
> 	0xc1239aa8 <+209>:   xor    %ecx,%ecx
> 	0xc1239aaa <+211>:   test   %edx,%edx
> 	0xc1239aac <+213>:   mov    %eax,%ebx
> 	0xc1239aae <+215>:   je     0xc1239abd <tsc_init+230>
> 	0xc1239ab0 <+217>:   mov    $0x64,%ecx
> 	0xc1239ab5 <+222>:   mov    %edx,%eax
> 	0xc1239ab7 <+224>:   xor    %edx,%edx
> 	0xc1239ab9 <+226>:   div    %ecx
> 	0xc1239abb <+228>:   mov    %eax,%ecx
> 	0xc1239abd <+230>:   mov    %ebx,%eax
> 	0xc1239abf <+232>:   mov    $0x64,%ebx
> 	0xc1239ac4 <+237>:   div    %ebx
> 	0xc1239ac6 <+239>:   mov    %ecx,%edx
> imul will load the result into %edx, %edx supposed to be high 32 bit which is
> not zero,
> It should be zero in this case. both lpj and tsc_khz should be u64 to work
> properly.

Good catch! Care to send a patch?

Thanks,

	tglx

[PATCH v15 19/26] x86/tsc: calibrate tsc only once

[Pavel Tatashin]

During boot tsc is calibrated twice: once in tsc_early_delay_calibrate(),
and the second time in tsc_init().

Rename tsc_early_delay_calibrate() to tsc_early_init(), and rework it so
the calibration is done only early, and make tsc_init() to use the values
already determined in tsc_early_init().

Sometimes it is not possible to determine tsc early, as the subsystem that
is required is not yet initialized, in such case try again later in
tsc_init().

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Pavel Tatashin <pasha.tatashin@oracle.com>
---
 arch/x86/include/asm/tsc.h |  2 +-
 arch/x86/kernel/setup.c    |  2 +-
 arch/x86/kernel/tsc.c      | 87 ++++++++++++++++++++------------------
 3 files changed, 49 insertions(+), 42 deletions(-)

diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
index 2701d221583a..c4368ff73652 100644
--- a/arch/x86/include/asm/tsc.h
+++ b/arch/x86/include/asm/tsc.h
@@ -33,7 +33,7 @@ static inline cycles_t get_cycles(void)
 extern struct system_counterval_t convert_art_to_tsc(u64 art);
 extern struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns);
 
-extern void tsc_early_delay_calibrate(void);
+extern void tsc_early_init(void);
 extern void tsc_init(void);
 extern void mark_tsc_unstable(char *reason);
 extern int unsynchronized_tsc(void);
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 7490de925a81..5d32c55aeb8b 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1014,6 +1014,7 @@ void __init setup_arch(char **cmdline_p)
 	 */
 	init_hypervisor_platform();
 
+	tsc_early_init();
 	x86_init.resources.probe_roms();
 
 	/* after parse_early_param, so could debug it */
@@ -1199,7 +1200,6 @@ void __init setup_arch(char **cmdline_p)
 
 	memblock_find_dma_reserve();
 
-	tsc_early_delay_calibrate();
 	if (!early_xdbc_setup_hardware())
 		early_xdbc_register_console();
 
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 186395041725..4cab2236169e 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -33,6 +33,8 @@ EXPORT_SYMBOL(cpu_khz);
 unsigned int __read_mostly tsc_khz;
 EXPORT_SYMBOL(tsc_khz);
 
+#define KHZ	1000
+
 /*
  * TSC can be unstable due to cpufreq or due to unsynced TSCs
  */
@@ -1335,34 +1337,10 @@ static int __init init_tsc_clocksource(void)
  */
 device_initcall(init_tsc_clocksource);
 
-void __init tsc_early_delay_calibrate(void)
-{
-	unsigned long lpj;
-
-	if (!boot_cpu_has(X86_FEATURE_TSC))
-		return;
-
-	cpu_khz = x86_platform.calibrate_cpu();
-	tsc_khz = x86_platform.calibrate_tsc();
-
-	tsc_khz = tsc_khz ? : cpu_khz;
-	if (!tsc_khz)
-		return;
-
-	lpj = tsc_khz * 1000;
-	do_div(lpj, HZ);
-	loops_per_jiffy = lpj;
-}
-
-void __init tsc_init(void)
+static bool __init determine_cpu_tsc_frequencies(void)
 {
-	u64 lpj, cyc;
-	int cpu;
-
-	if (!boot_cpu_has(X86_FEATURE_TSC)) {
-		setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE_TIMER);
-		return;
-	}
+	/* Make sure that cpu and tsc are not already calibrated */
+	WARN_ON(cpu_khz || tsc_khz);
 
 	cpu_khz = x86_platform.calibrate_cpu();
 	tsc_khz = x86_platform.calibrate_tsc();
@@ -1377,20 +1355,52 @@ void __init tsc_init(void)
 	else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
 		cpu_khz = tsc_khz;
 
-	if (!tsc_khz) {
-		mark_tsc_unstable("could not calculate TSC khz");
-		setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE_TIMER);
-		return;
-	}
+	if (tsc_khz == 0)
+		return false;
 
 	pr_info("Detected %lu.%03lu MHz processor\n",
-		(unsigned long)cpu_khz / 1000,
-		(unsigned long)cpu_khz % 1000);
+		(unsigned long)cpu_khz / KHZ,
+		(unsigned long)cpu_khz % KHZ);
 
 	if (cpu_khz != tsc_khz) {
 		pr_info("Detected %lu.%03lu MHz TSC",
-			(unsigned long)tsc_khz / 1000,
-			(unsigned long)tsc_khz % 1000);
+			(unsigned long)tsc_khz / KHZ,
+			(unsigned long)tsc_khz % KHZ);
+	}
+	return true;
+}
+
+static unsigned long __init get_loops_per_jiffy(void)
+{
+	unsigned long lpj = tsc_khz * KHZ;
+
+	do_div(lpj, HZ);
+	return lpj;
+}
+
+void __init tsc_early_init(void)
+{
+	if (!boot_cpu_has(X86_FEATURE_TSC))
+		return;
+	if (!determine_cpu_tsc_frequencies())
+		return;
+	loops_per_jiffy = get_loops_per_jiffy();
+}
+
+void __init tsc_init(void)
+{
+	if (!boot_cpu_has(X86_FEATURE_TSC)) {
+		setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE_TIMER);
+		return;
+	}
+
+	if (!tsc_khz) {
+		/* We failed to determine frequencies earlier, try again */
+		if (!determine_cpu_tsc_frequencies()) {
+			mark_tsc_unstable("could not calculate TSC khz");
+			setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE_TIMER);
+			return;
+		}
 	}
 
 	/* Sanitize TSC ADJUST before cyc2ns gets initialized */
@@ -1413,10 +1423,7 @@ void __init tsc_init(void)
 	if (!no_sched_irq_time)
 		enable_sched_clock_irqtime();
 
-	lpj = ((u64)tsc_khz * 1000);
-	do_div(lpj, HZ);
-	lpj_fine = lpj;
-
+	lpj_fine = get_loops_per_jiffy();
 	use_tsc_delay();
 
 	check_system_tsc_reliable();
-- 
2.18.0