DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C556785545
Date: Tue, 18 Apr 2017 16:47:47 -0400 (EDT)
From: Mikulas Patocka <mpatocka@redhat.com>
To: "H. Peter Anvin" <hpa@zytor.com>
cc: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
        x86@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] X86: don't report PAT on CPUs that don't support it
In-Reply-To: <f359fc2e-0ef3-3173-5e78-36bcb5a04316@zytor.com>
Message-ID: <alpine.LRH.2.02.1704181639410.14899@file01.intranet.prod.int.rdu2.redhat.com>
References: <alpine.LRH.2.02.1704181501450.26399@file01.intranet.prod.int.rdu2.redhat.com> <f359fc2e-0ef3-3173-5e78-36bcb5a04316@zytor.com>
User-Agent: Alpine 2.02 (LRH 1266 2009-07-14)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1994
Lines: 48


On Tue, 18 Apr 2017, H. Peter Anvin wrote:

> On 04/18/17 12:07, Mikulas Patocka wrote:
> > In the file arch/x86/mm/pat.c, there's a variable __pat_enabled. The
> > variable is set to 1 by default and the function pat_init() sets
> > __pat_enabled to 0 if the CPU doesn't support PAT.
> > 
> > However, on AMD K6-3 CPU, the processor initialization code never calls
> > pat_init() and so __pat_enabled stays 1 and the function pat_enabled()
> > returns true, even though the K6-3 CPU doesn't support PAT.
> > 
> > The result of this bug is that this warning is produced when attemting to
> > start the Xserver and the Xserver doesn't start (fork() returns ENOMEM).
> > Another symptom of this bug is that the framebuffer driver doesn't set the
> > K6-3 MTRR registers.
> > 
> > This patch changes pat_enabled() so that it returns true only if pat
> > initialization was actually done.
> > 
> > Also, I changed boot_cpu_has(X86_FEATURE_PAT) to
> > this_cpu_has(X86_FEATURE_PAT) in pat_ap_init, so that we check the PAT
> > feature on the processor that is being initialized.
> > 
> 
> I'm thinking it would be better to replace __pat_enabled with
> static_cpu_has(X86_FEATURE_PAT) and disable the feature bit if the user
> has disabled it on the command line, just as we do with other features.
> 
> 	-hpa

If MTRR initialization fails for whatever reason, then pat_init() won't be 
called and the kernel would mistakenly believe that PAT is working 
(because there would be no one to clear X86_FEATURE_PAT).

I think that pat should be reported only if pat_init() is called and 
succeeds.


Another strange thing: pat_disable() calls init_cache_modes() - but since 
pat_disable() may not be called at all, it is possible that 
init_cache_modes() is also not called at all. It doesn't produce any 
visible misbehavior on my machine, but it doesn't seem right - we should 
not call init_cache_modes() from pat_disable() and do the initialization 
elsewhere, where it is always called.

Mikulas