Re: [PATCH 2/2] x86/cpu/intel: Add EIST workaround for Lightning Mountain.

[Dave Hansen <dave.hansen@intel.com>]

So what's weird about these systems? Do they not have a "normal" BIOS
based on the Intel reference one?

I don't know much about this specific feature, but this patch is doing
some unusual things. I'll elaborate below:

> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 98ae4c37c93eccf775d5632acf122603a19918a8..e49df04e8d491158cc48f8d8bef824c434256d09 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -466,6 +466,29 @@ static void intel_workarounds(struct cpuinfo_x86 *c)
>  #else
>  static void intel_workarounds(struct cpuinfo_x86 *c)
>  {
> +	u64 misc_enable;
> +
> +	/*
> +	 * Intel / MaxLinear Lightning Mountain workaround to enable Enhanced
> +	 * Intel SpeedStep Technology (EIST) for each cpu. Otherwise, the
> +	 * frequency on some cpus is locked to the minimum value of 624 MHz.
> +	 * This usually would be the job of the BIOS / bootloader, but U-Boot
> +	 * only enables it on the cpu on which it is running.
> +	 */
> +	if (c->x86_vfm == INTEL_ATOM_AIRMONT_NP) {

Model checks area kinda a last resort. A quick search in the SDM found:

CPUID.01H:ECX[7]: If 1, supports Enhanced Intel SpeedStep® technology.

But there's other chit chat in the "Runtime Mutable CPUID Fields"
section that makes it seem that it's not a really feature enumeration
bit, but a flag to tell if the BIOS enabled it:

	CPUID.01H:ECX[7] -- This feature flag reflects the setting in
			    IA32_MISC_ENABLE[16]

But the plot thickens because the *existing* code does this:

static int centrino_cpu_init(struct cpufreq_policy *policy)
{
...
        /* Only Intel makes Enhanced Speedstep-capable CPUs */
        if (cpu->x86_vendor != X86_VENDOR_INTEL ||
            !cpu_has(cpu, X86_FEATURE_EST))
                return -ENODEV;
...
        if (!(l & MSR_IA32_MISC_ENABLE_ENHANCED_SPEEDSTEP)) {

Which, again, makes it seem like X86_FEATURE_EST (aka. CPUID.01H:ECX[7])
tells you if the MSR bit is supported, not whether it is enabled.

I'd tend to trust the existing kernel code over quibbling with the SDM
wording in general. It's also possible the old code was just confused or
something was buggy.

> +		rdmsrq(MSR_IA32_MISC_ENABLE, misc_enable);
> +		if (!(misc_enable & MSR_IA32_MISC_ENABLE_ENHANCED_SPEEDSTEP)) {
> +			misc_enable |= MSR_IA32_MISC_ENABLE_ENHANCED_SPEEDSTEP;
> +			wrmsrq(MSR_IA32_MISC_ENABLE, misc_enable);
> +
> +			/* check to see if it was enabled successfully */
> +			rdmsrq(MSR_IA32_MISC_ENABLE, misc_enable);
> +			if (!(misc_enable & MSR_IA32_MISC_ENABLE_ENHANCED_SPEEDSTEP)) {
> +				pr_info("CPU%d: Can't enable Enhanced SpeedStep\n",
> +					c->cpu_index);
> +			}
> +		}
> +	}
>  }


This is also not written in the normal kernel style which minimizes
indentation. For instance, the function should have opened with:

	if (c->x86_vfm != INTEL_ATOM_AIRMONT_NP)
		return;

It also needs to be reconciled with centrino_cpu_init() (at least).
Having *a* single place to go in and say "If this CPU supports 'EST',
turn it on" would be a minimal refactoring that could be shared by your
new workaround and the old centrino code.

centrino_cpu_init() does look gated on X86_FEATURE_EST already, though
because of the centrino_ids[]. So, you still need to figure out the
interaction with X86_FEATURE_EST for when you call the workaround.