Re: [PATCH] x86 (64): make calibrate_APIC_clock() SMI-safe (take 3)

[Cyrill Gorcunov <gorcunov@gmail.com>]

[Martin Wilck - Fri, Jul 25, 2008 at 05:13:21PM +0200]
> Cyrill Gorcunov wrote:
>> Martin, it seems you forgot to attach the patch :) 
>
> I am sorry, here it is. Thanks :)
>
> 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@fujitsu-siemens.com
> 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 3)
| 
| 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.
| 
| Changed wrt take 2: Use max. possible start value for the APIC timer 
| to avoid underflow.
| 
| Signed-off-by: Martin Wilck <martin.wilck@fujitsu-siemens.com>
| Signed-off-by: Gerhard Wichert <gerhard.wichert@fujitsu-siemens.com>
| 
| --- arch/x86/kernel/apic_64.c	2008-07-25 15:39:51.000000000 +0200
| +++ arch/x86/kernel/apic_64.c.new	2008-07-25 15:55:08.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();
|  
| @@ -329,25 +354,27 @@ static void __init calibrate_APIC_clock(
|  	 *
|  	 * No interrupt enable !
|  	 */
| -	__setup_APIC_LVTT(250000000, 0, 0);
| +	__setup_APIC_LVTT(0xffffffff, 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);
|  	}

Thanks, Martin! Patch looks good for me (actually I would better
wait for Maciej opinion since I'm not that specialist in this area).
Thanks!

		- Cyrill -