Re: Re: [PATCH] x86: set Pentium M as PAE capable

[Borislav Petkov <bp@alien8.de>]

On Mon, Mar 03, 2014 at 03:04:35PM +0700, Chris Bainbridge wrote:
> On 3 March 2014 02:05, Roland Kletzing <devzero@web.de> wrote:
> > i would recommend adding the newly introduced param to
> > Documentation/kernel-
> > parameters.txt , though.
> 
> Done.
> 
> Signed-off-by: Chris Bainbridge <chris.bainbridge@gmail.com>
> ---
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index b9e9bd8..388b5e9 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -962,6 +962,13 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			parameter will force ia64_sal_cache_flush to call
>  			ia64_pal_cache_flush instead of SAL_CACHE_FLUSH.
>  
> +	forcepae [X86-32]
> +			Forcefully enable Physical Address Extension (PAE).
> +			Many Pentium M systems disable PAE but may have a
> +			functionally usable PAE implementation.
> +			Note: This parameter is unsupported, may cause unknown

What does "unsupported" mean here exactly?

> +			problems, and will taint the kernel.
> +
>  	ftrace=[tracer]
>  			[FTRACE] will set and start the specified tracer
>  			as early as possible in order to facilitate early
> diff --git a/arch/x86/boot/cpucheck.c b/arch/x86/boot/cpucheck.c
> index 4d3ff03..93ba160 100644
> --- a/arch/x86/boot/cpucheck.c
> +++ b/arch/x86/boot/cpucheck.c
> @@ -69,6 +69,13 @@ static int is_transmeta(void)
>  	       cpu_vendor[2] == A32('M', 'x', '8', '6');
>  }
>  
> +static int is_intel(void)
> +{
> +	return cpu_vendor[0] == A32('G', 'e', 'n', 'u') &&
> +	       cpu_vendor[1] == A32('i', 'n', 'e', 'I') &&
> +	       cpu_vendor[2] == A32('n', 't', 'e', 'l');
> +}
> +
>  static int has_fpu(void)
>  {
>  	u16 fcw = -1, fsw = -1;
> @@ -239,6 +246,24 @@ int check_cpu(int *cpu_level_ptr, int *req_level_ptr, u32 **err_flags_ptr)
>  		asm("wrmsr" : : "a" (eax), "d" (edx), "c" (ecx));
>  
>  		err = check_flags();
> +	} else if (err == 0x01 &&
> +		   !(err_flags[0] & ~(1 << X86_FEATURE_PAE)) &&
> +		   is_intel() && cpu.level == 6 &&
> +		   (cpu.model == 9 || cpu.model == 13)) {
> +		/* PAE is disabled on this Pentium M but can be forced */
> +		if (cmdline_find_option_bool("forcepae")) {
> +			puts("WARNING: Forcing PAE in CPU flags\n");
> +			set_bit(X86_FEATURE_PAE, cpu.flags);
> +			err = check_flags();

This function is called check_cpuflags() now. You probably want to redo
your patch against tip/master, i.e.:

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git#master

> +		}
> +		else {
> +			puts("ERROR: PAE is disabled on this Pentium M\n"
> +				"(PAE can potentially be enabled with "
> +				"kernel parameter\n"
> +				"\"forcepae\" - this is unsupported, may "
> +				"cause unknown\n"
> +				"problems, and will taint the kernel)\n");

This string could definitely violate the 80 cols rule so that it is much
more readable:

		}
		else
			puts("WARNING: PAE disabled. Use \"forcepae\" to enable at your own risk!\n");

I've shortened it to the most relevant info only. No need to say we're
tainting the kernel because LOCKDEP_NOW_UNRELIABLE will cause that
anyway below.

> +		}
>  	}
>  
>  	if (err_flags_ptr)
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index bbe1b8b..271686d 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -196,6 +196,14 @@ static void intel_smp_check(struct cpuinfo_x86 *c)
>  	}
>  }
>  
> +static int forcepae;
> +static int __init forcepae_setup(char *__unused)
> +{
> +	forcepae = 1;
> +	return 1;
> +}
> +__setup("forcepae", forcepae_setup);

Yeah, why not simply call it "pae"? It is smaller and the letter
combination is not used yet and it means the same.

> +
>  static void intel_workarounds(struct cpuinfo_x86 *c)
>  {
>  	unsigned long lo, hi;
> @@ -226,6 +234,17 @@ static void intel_workarounds(struct cpuinfo_x86 *c)
>  		clear_cpu_cap(c, X86_FEATURE_SEP);
>  
>  	/*
> +	 * PAE CPUID issue: many Pentium M report no PAE but may have a
> +	 * functionally usable PAE implementation.
> +	 * Forcefully enable PAE if kernel parameter "forcepae" is present.
> +	 */
> +	if (forcepae) {
> +		printk(KERN_WARNING "PAE forced!\n");
> +		set_cpu_cap(c, X86_FEATURE_PAE);
> +		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_NOW_UNRELIABLE);

Right, this implies Dave's patch is preceding yours. I guess hpa can
fish it out from the thread when applying.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--