Re: [tip:x86/urgent] x86/PAT: Fix Xorg regression on CPUs that don't support PAT

[Doug Smythies]

Note Before:
I might not have the address list correct, as I have re-created this
e-mail from the web page archive, having found the thread after bisecting the
kernel.

On 2017.05.29 18:50:57 -0400 (EDT) Mikulas Patocka wrote:
> On Sun, 28 May 2017, Andy Lutomirski wrote:
>> On Sun, May 28, 2017 at 11:18 AM, Bernhard Held <berny156@xxxxxx> wrote:
>>> Hi,
>>>
>>> this patch breaks the boot of my kernel. The last message is "Booting
>>> the kernel.".

It breaks my kernel boot also, and I know of at least two others with
the same, or similar, problem as of kernel 4.12-rc3.

>>> My setup might be unusual: I'm running a Xenon E5450 (LGA 771) in a
>>> Gigbayte G33-DS3R board (LGA 775). The BIOS is patched with the
>>> microcode of the E5450 and recognizes the CPU.

I do not think my test server setup is unusual.
I use Ubuntu 16.04.2 server edition as my distro, and
steal Ubuntu kernel configurations for compiling.
My processor is an older model i7
(Intel(R) Core(TM) i7-2600K CPU @ 3.40GHz)

> Hi
>
> Please do the following three tests and test if the kernel boots.
>
> 1. use the PAT patch and revert the change to the function pat_enabled()
> - i.e. change it to the original:
> bool pat_enabled(void)
> {
>	return !!__pat_enabled;
> }

Test 1 result: fail

>
> 2. use the PAT patch and revert the change to the function pat_ap_init
> - i.e. change it to the original:
> static void pat_ap_init(u64 pat)
> {
>	if (!boot_cpu_has(X86_FEATURE_PAT)) {

Test 2 result: pass

> 3. use the full PAT patch and apply the below patch on the top of it.
>

Test 3 result: fail

>> I think this patch is bogus.  pat_enabled() sure looks like it's
>> supposed to return true if PAT is *enabled*, and these days PAT is
>> "enabled" even if there's no HW PAT support.  Even if the patch were
>> somehow correct, it should have been split up into two patches, one to
>> change pat_enabled() and one to use this_cpu_has().
>> 
>> Ingo, I'd suggest reverting the patch, cc-ing stable on the revert so
>> -stable knows not to backport it, and starting over with the fix.
>> From very brief inspection, the right fix is to make sure that
>> pat_init(), or at least init_cache_modes(), gets called on the
>>
> pat_init() needs to be called with cache disabled - and the cache disable 
> code (functions prepare_set() and post_set()) exists in 
> arch/x86/kernel/cpu/mtrr/generic.c - it may not be compiled if CONFIG_MTRR 
> is not set.
>
> Though, it is possible to call init_cache_modes() - see the patch below. 
> init_cache_modes() does nothing if it is called multiple times.
>
>> affected CPUs.
>> 
>> As a future cleanup, I think that pat_enabled() could be deleted
>> outright and, if needed, replaced by functions like have_memtype_wc()
>> or similar.  (Do we already have helpers like that?)  Toshi, am I
>> right?
>> 
>> --Andy