Re: [PATCH] x86, tsc: Limit CPU frequency calibration on AMD

["H. Peter Anvin" <hpa@zytor.com>]

On 08/18/2010 09:16 AM, Borislav Petkov wrote:
> 6b37f5a20c0e5c334c010a587058354215433e92 introduced the CPU frequency
> calibration code for AMD CPUs whose TSCs didn't increment with the
> core's P0 frequency. From F10h, revB onward, the TSC increment rate is
> denoted by MSRC001_0015[24] and when this bit is set (which is normally
> done by the BIOS,) the TSC increments with the P0 frequency so the
> calibration is not needed and booting can be a couple of mcecs faster on
> those machines.
> 
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  arch/x86/kernel/tsc.c |   14 ++++++++++++--
>  1 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index ce8e502..41b2b8b 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -927,8 +927,18 @@ void __init tsc_init(void)
>  	}
>  
>  	if (cpu_has(&boot_cpu_data, X86_FEATURE_CONSTANT_TSC) &&
> -			(boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
> -		cpu_khz = calibrate_cpu();
> +			(boot_cpu_data.x86_vendor == X86_VENDOR_AMD)) {
> +
> +		if (boot_cpu_data.x86 > 0x10 ||
> +		    (boot_cpu_data.x86 == 0x10 &&
> +		     boot_cpu_data.x86_model >= 0x2)) {
> +			u64 val;
> +
> +			rdmsrl(MSR_K7_HWCR, val);
> +			if (!(val & BIT(24)))
> +				cpu_khz = calibrate_cpu();
> +		}
> +	}
>  
>  	printk("Detected %lu.%03lu MHz processor.\n",
>  			(unsigned long)cpu_khz / 1000,

Yuck!  There are a number of problems with this code, quite a few of
which are, of course, pre-existing, but this makes it worse.

calibrate_cpu() is AMD-specific, despite the generic name.  (It is also,
strangely enough, only compiled on 64 bits for some reason???)  Either
which way, it is definitely not okay for the test for when the code
applies to be this distant from the code itself.

The easy way to fix this is to rename it amd_calibrate_cpu() and move
the applicability test into the routine itself.  That is probably okay
as long as there are no other users.  However, if there are other users,
then this really should move into x86_init and have a function pointer
associated with it.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.