Re: [PATCH] x86 (64): make calibrate_APIC_clock() SMI-safe (take 2)
From: Cyrill Gorcunov
Date: Fri Jul 25 2008 - 06:09:00 EST
[Martin Wilck - Fri, Jul 25, 2008 at 11:02:06AM +0200]
> I wrote:
>
>> This patch fixes this by two separate measures:
>> a) make sure that no significant interruption occurs between APIC and
>> TSC reads
>> b) make sure that the measurement loop isn't significantly longer
>> than originally intended.
>
> Here is a new, simplified version of our patch that only uses measure a).
> We verified that this is sufficient for accurate calibration.
>
> If we fail to determine the start or end time of the calibration
> correctly 10 times in a row, we will print a critical error message and
> go on. One might as well argue that this should cause a kernel panic (it
> is impossible to run on the CPU for only a few cycles without being
> interrupted by an SMI!), but Cyrill probably won't agree.
>
> Martin
>
> --
> Martin Wilck
> PRIMERGY System Software Engineer
> FSC IP ESP DEV 6
>
> Fujitsu Siemens Computers GmbH
> Heinz-Nixdorf-Ring 1
> 33106 Paderborn
> Germany
>
> Tel: ++49 5251 8 15113
> Fax: ++49 5251 8 20209
> Email: mailto:martin.wilck@xxxxxxxxxxxxxxxxxxx
> Internet: http://www.fujitsu-siemens.com
> Company Details: http://www.fujitsu-siemens.com/imprint.html
| [PATCH] x86 (64): make calibrate_APIC_clock() SMI-safe (take 2)
|
| Non-maskable asynchronous events (e.g. SMIs) which occur during the APIC
| timer calibration can cause timer miscalibrations, sometimes by large amounts.
| This patch fixes this by making sure that no significant interruption occurs
| between APIC and TSC reads. SMIs may still occur at some stage in the
| calibration loop, causing the loop to last longer than intended. This
| doesn't matter though, as long as the start and end values are both
| taken simultaneously.
|
| Signed-off-by: Martin Wilck <martin.wilck@xxxxxxxxxxxxxxxxxxx>
| Signed-off-by: Gerhard Wichert <gerhard.wichert@xxxxxxxxxxxxxxxxxxx>
|
| --- arch/x86/kernel/apic_64.c 2008-07-25 10:45:09.000000000 +0200
| +++ arch/x86/kernel/apic_64.c.new 2008-07-25 10:45:19.000000000 +0200
| @@ -300,6 +300,31 @@ static void setup_APIC_timer(void)
| }
|
| /*
| + * Helper function for calibrate_APIC_clock(): Make sure that
| + * APIC TMCTT and TSC are read at the same time, to reasonable
| + * accuracy. On any sane system, the retry loop won't need more
| + * than a single retry, given that the rdtsc/apic_read/rdtsc
| + * sequence won't take more than a few cycles.
| + */
| +
| +#define MAX_DIFFERENCE 1000UL
| +#define MAX_ITER 10
| +static inline int
| +__read_tsc_and_apic(unsigned long *tsc, unsigned *apic)
| +{
| + unsigned long tsc0, tsc1, diff;
| + int i = 0;
| + do {
| + rdtscll(tsc0);
| + *apic = apic_read(APIC_TMCCT);
| + rdtscll(tsc1);
| + diff = tsc1 - tsc0;
| + } while (diff > MAX_DIFFERENCE && ++i < MAX_ITER);
| + *tsc = tsc0 + (diff >> 1);
| + return diff > MAX_DIFFERENCE ? -EIO : 0;
| +}
| +
| +/*
| * In this function we calibrate APIC bus clocks to the external
| * timer. Unfortunately we cannot use jiffies and the timer irq
| * to calibrate, since some later bootup code depends on getting
| @@ -318,7 +343,7 @@ static void __init calibrate_APIC_clock(
| {
| unsigned apic, apic_start;
| unsigned long tsc, tsc_start;
| - int result;
| + int result, err_start, err;
|
| local_irq_disable();
|
| @@ -331,23 +356,25 @@ static void __init calibrate_APIC_clock(
| */
| __setup_APIC_LVTT(250000000, 0, 0);
|
| - apic_start = apic_read(APIC_TMCCT);
| #ifdef CONFIG_X86_PM_TIMER
| if (apic_calibrate_pmtmr && pmtmr_ioport) {
| + apic_start = apic_read(APIC_TMCCT);
| pmtimer_wait(5000); /* 5ms wait */
| apic = apic_read(APIC_TMCCT);
| result = (apic_start - apic) * 1000L / 5;
| } else
| #endif
| {
| - rdtscll(tsc_start);
| + err_start = __read_tsc_and_apic(&tsc_start, &apic_start);
|
| do {
| - apic = apic_read(APIC_TMCCT);
| - rdtscll(tsc);
| + err = __read_tsc_and_apic(&tsc, &apic);
| } while ((tsc - tsc_start) < TICK_COUNT &&
| (apic_start - apic) < TICK_COUNT);
|
| + if (err_start || err)
| + printk(KERN_CRIT "calibrate_APIC_clock: SMI flood - "
| + "the APIC timer calibration may be wrong!\n");
| result = (apic_start - apic) * 1000L * tsc_khz /
| (tsc - tsc_start);
| }
Hi Martin, what about the patch below - I simplified it a bit.
Actually we have to handle 32bit mode as well I think. Anyway,
take a look. I don't really mind against your patch but we better
should wait until Maciej could take a look (he will be able in
a week or maybe a bit later).
- Cyrill -
---
Index: linux-2.6.git/arch/x86/kernel/apic_64.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic_64.c 2008-07-25 13:38:11.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/apic_64.c 2008-07-25 14:01:43.000000000 +0400
@@ -378,6 +378,35 @@ static void setup_APIC_timer(void)
}
/*
+ * Helper function for calibrate_APIC_clock(): Make sure that
+ * APIC TMCTT and TSC are read at the same time, to reasonable
+ * accuracy. On any sane system, the retry loop won't need more
+ * than a single retry, given that the rdtsc/apic_read/rdtsc
+ * sequence won't take more than a few cycles.
+ */
+
+#define MAX_DIFFERENCE 1000UL
+#define MAX_ITER 10
+static inline int __read_tsc_and_apic(unsigned long *tsc, unsigned *apic)
+{
+ unsigned long tsc0, tsc1, diff;
+ int i = 0;
+
+ for (i = 0; i < MAX_ITER; i++) {
+ rdtscll(tsc0);
+ *apic = apic_read(APIC_TMCCT);
+ rdtscll(tsc1);
+ diff = tsc1 - tsc0;
+ if (diff < MAX_DIFFERENCE) {
+ *tsc = tsc0 + diff / 2;
+ return 0;
+ }
+ }
+
+ return -EIO ;
+}
+
+/*
* In this function we calibrate APIC bus clocks to the external
* timer. Unfortunately we cannot use jiffies and the timer irq
* to calibrate, since some later bootup code depends on getting
@@ -396,7 +425,7 @@ static int __init calibrate_APIC_clock(v
{
unsigned apic, apic_start;
unsigned long tsc, tsc_start;
- int result;
+ int result, err_start, err;
local_irq_disable();
@@ -409,23 +438,25 @@ static int __init calibrate_APIC_clock(v
*/
__setup_APIC_LVTT(250000000, 0, 0);
- apic_start = apic_read(APIC_TMCCT);
#ifdef CONFIG_X86_PM_TIMER
if (apic_calibrate_pmtmr && pmtmr_ioport) {
+ apic_start = apic_read(APIC_TMCCT);
pmtimer_wait(5000); /* 5ms wait */
apic = apic_read(APIC_TMCCT);
result = (apic_start - apic) * 1000L / 5;
} else
#endif
{
- rdtscll(tsc_start);
+ err_start = __read_tsc_and_apic(&tsc_start, &apic_start);
do {
- apic = apic_read(APIC_TMCCT);
- rdtscll(tsc);
+ err = __read_tsc_and_apic(&tsc, &apic);
} while ((tsc - tsc_start) < TICK_COUNT &&
(apic_start - apic) < TICK_COUNT);
+ if (err_start || err)
+ printk(KERN_CRIT "calibrate_APIC_clock: SMI flood - "
+ "the APIC timer calibration may be wrong!\n");
result = (apic_start - apic) * 1000L * tsc_khz /
(tsc - tsc_start);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/