Message-ID: <1457721266.6393.123.camel@hpe.com>
Subject: Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code
From: Toshi Kani <toshi.kani@hpe.com>
To: Ingo Molnar <mingo@kernel.org>
Cc: "bp@suse.de" <bp@suse.de>, "hpa@zytor.com" <hpa@zytor.com>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "mcgrof@suse.com" <mcgrof@suse.com>,
        "jgross@suse.com" <jgross@suse.com>,
        "paul.gortmaker@windriver.com" <paul.gortmaker@windriver.com>,
        "x86@kernel.org" <x86@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Date: Fri, 11 Mar 2016 11:34:26 -0700
In-Reply-To: <20160311091301.GA11595@gmail.com>
References: <1457671546-13486-1-git-send-email-toshi.kani@hpe.com>
	 <1457671546-13486-3-git-send-email-toshi.kani@hpe.com>
	 <20160311090140.GA4873@gmail.com> <20160311091301.GA11595@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3505
Lines: 77

On Fri, 2016-03-11 at 09:13 +0000, Ingo Molnar wrote:
> * Ingo Molnar <mingo@kernel.org> wrote:
> 
> > 
> > * Toshi Kani <toshi.kani@hpe.com> wrote:
> > 
> > > MTRR manages PAT initialization as it implements a rendezvous
> > > handler that initializes PAT as part of MTRR initialization.
> > > 
> > > When CPU does not support MTRR, ex. qemu32 virtual CPU, MTRR
> > > simply skips PAT init, which causes PAT left enabled without
> > > initialization. [...]
> > 
> > What practical effects does this have to the user? Does the kernel
> > crash?
> 
> Btw., I find this omission _highly_ annoying: describing what negative
> effects a bug _causes in practice_ is the most important part of a
> changelog. How on earth can an experienced contributor omit such an
> important component from a patch description?
> 
> Most readers of changelogs couldn't care less about technical details of
> how the bug is fixed (of course others will read it so it's nice to have
> too), but what symptoms a bug causes, how serious is it, whether it
> should be backported are like super important compared to everything else
> you wrote - and both the description and the changelogs are totally
> silent on those topics ...
> 
> I've seen this in other PAT patches - please try to improve this.

My apology. I agree the importance of describing the negative effect of the
issue. This case is complicated to describe thoroughly, but here is a
summary.

The issue was reported as a regression caused by 'commit 9cd25aac1f44
("x86/mm/pat: Emulate PAT when it is disabled")'. So, the goal of this
patchset is to fix this regression.
https://lkml.org/lkml/2016/3/3/828

The negative effects of the issue were two failures in Xorg on qemu32 env,
which was triggered by the fact that its virtual CPU does not support MTRR.
https://lkml.org/lkml/2016/3/4/775
 #1. copy_process() failed in the check in reserve_pfn_range()
 #2. error path in copy_process() then hit WARN_ON_ONCE in untrack_pfn().

These negative effects are also caused by two different bugs, but they can
be dealt in lower priority. Fixing the pat_init() issue will avoid Xorg
hitting these cases.

When the CPU does not support MTRR, MTRR does not call pat_init(), but
leaves PAT enabled. This pat_init() issue is a long-standing issue, but
manifested as issue #1 (and then hit issue #2) with the commit because the
memtype now tracks cache attribute with 'page_cache_mode'. A WC map request
is tracked as WC in memtype, but sets a PTE as UC (pgprot) per
__cachemode2pte_tbl[]. This caused an error in reserve_pfn_range() when it
was called from track_pfn_copy(), which obtained pgprot from a PTE. It
converts pgprot to page_cache_mode, which does not necessarily result in
the original page_cache_mode since __cachemode2pte_tbl[] redirects multiple
types to UC. This is a separate issue in reserve_pfn_range().

If PAT is set to disabled properly, the code bypasses the memtype check.
So, #1 is a non-issue as a result (although it should be fixed).
  
This pat_init() issue existed before commit 9cd25aac1f44, but we used
pgprot in memtype. Hence, we did not have issue #1 before. But WC request
resulted in WT in effect because WC pgrot is actually WT when PAT is not
initialized. This is not how the code was designed to work. When PAT is set
to disabled properly, WC gets converted to UC. The use of WT can result in
a system crash if the target range does not support WT, but fortunately
people did not run into such issue before.

Thanks,
-Toshi