Re: [RFC patch 0/4] TSC calibration improvements

[Ingo Molnar <mingo@elte.hu>]

> +static unsigned long quick_pit_calibrate(void)
> +{
[...]
> +	if (pit_expect_msb(0xff)) {
> +		int i;
> +		u64 t1, t2, delta;
> +		unsigned char expect = 0xfe;
> +
> +		t1 = get_cycles();
> +		for (i = 0; i < QUICK_PIT_ITERATIONS; i++, expect--) {
> +			if (!pit_expect_msb(expect))
> +				goto failed;
> +		}
> +		t2 = get_cycles();

hm, unless i'm missing something i think here we still have a small 
window for an SMI or some virtualization delay to slip in and cause 
massive inaccuracy: if the delay happens _after_ the last 
pit_expect_msb() and _before_ the external get_cycles() call. Right?

i fixed that by adding one more pit_expect_msb() call.

plus i think QUICK_PIT_ITERATIONS is quite close to overflowing 255 
which is built into the u32 'expect' variable (the MSB will only 
overflow to 10 bits or so) - so i've added a BUILD_BUG_ON() to make sure 
anyone tuning QUICK_PIT_MS above 60msec or so would get a build error 
instead of some hard(er) to track down calibration error.

but it's getting late here so please double-check me ... The commit is 
below.

	Ingo

------------>
>From 40d2650256289d3ba59c4fd146b86b972db6ec40 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Thu, 4 Sep 2008 22:47:47 +0200
Subject: [PATCH] x86: quick TSC calibration, improve

- make sure the final TSC timestamp is reliable too

- make sure nobody increases QUICK_PIT_MS so that
  QUICK_PIT_ITERATIONS can get larger than 0xff, breaking the iteration.
  (It would take about 60 msecs to reach that limit.)

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/tsc.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 839070b..4832a40 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -304,6 +304,11 @@ static unsigned long quick_pit_calibrate(void)
 	outb(0xff, 0x42);
 	outb(0xff, 0x42);
 
+	/*
+	 * The iteration assumes that expect never goes below zero:
+	 */
+	BUILD_BUG_ON(QUICK_PIT_ITERATIONS >= 0xff);
+
 	if (pit_expect_msb(0xff)) {
 		int i;
 		u64 t1, t2, delta;
@@ -317,6 +322,12 @@ static unsigned long quick_pit_calibrate(void)
 		t2 = get_cycles();
 
 		/*
+		 * Make sure we can rely on the second TSC timestamp:
+		 */
+		if (!pit_expect_msb(--expect))
+			goto failed;
+
+		/*
 		 * Ok, if we get here, then we've seen the
 		 * MSB of the PIT decrement QUICK_PIT_ITERATIONS
 		 * times, and each MSB had many hits, so we never