[PATCH] x86, pat: Add comments to cachemode translation tables

[PATCH] x86, pat: Add comments to cachemode translation tables

[Toshi Kani]

Add comments to the cachemode translation tables to clarify that
the default values are set as minimal supported mode, which are
necessary to handle WC and WT fallback to UC- when they are not
enabled.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
Ingo, please replace the patch below with this patch.
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg937806.html
---
 arch/x86/mm/init.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index 8533b46..1d8a83d 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -30,8 +30,11 @@
 /*
  * Tables translating between page_cache_type_t and pte encoding.
  *
- * Minimal supported modes are defined statically, they are modified
- * during bootup if more supported cache modes are available.
+ * The default values are defined statically as minimal supported mode;
+ * WC and WT fall back to UC-.  pat_init() updates these values to support
+ * more cache modes, WC and WT, when it is safe to do so.  See pat_init()
+ * for the details.  Note, __early_ioremap() used during early boot-time
+ * takes pgprot_t (pte encoding) and does not use these tables.
  *
  *   Index into __cachemode2pte_tbl[] is the cachemode.
  *

Re: [PATCH] x86, pat: Add comments to cachemode translation tables

[Jan Beulich]

>>> On 22.07.15 at 20:06, <toshi.kani@hp.com> wrote:
> Add comments to the cachemode translation tables to clarify that
> the default values are set as minimal supported mode, which are
> necessary to handle WC and WT fallback to UC- when they are not
> enabled.

Wait - shouldn't WT fall back to UC (so to not be affected by WC
MTRR settings)?

Jan

Re: [PATCH] x86, pat: Add comments to cachemode translation tables

[Toshi Kani]

On Thu, 2015-07-23 at 00:42 -0600, Jan Beulich wrote:
> > 
> > > > On 22.07.15 at 20:06, <toshi.kani@hp.com> wrote:
> > Add comments to the cachemode translation tables to clarify that
> > the default values are set as minimal supported mode, which are
> > necessary to handle WC and WT fallback to UC- when they are not
> > enabled.
> 
> Wait - shouldn't WT fall back to UC (so to not be affected by WC
> MTRR settings)?

Well, when MTRR is set to WC, WT will become UC.  So, it is not safe to
start with.

I know Luis is driving to support UC.  When UC can be used for both regular
memory and IO memory, yes, it can be changed to fall back to UC.  At this
point, however, UC cannot be set to regular memory.  See
reserve_ram_pages_type().

Thanks,
-Toshi 

Re: [PATCH] x86, pat: Add comments to cachemode translation tables

[Jan Beulich]

>>> On 23.07.15 at 16:27, <toshi.kani@hp.com> wrote:
> On Thu, 2015-07-23 at 00:42 -0600, Jan Beulich wrote:
>> > 
>> > > > On 22.07.15 at 20:06, <toshi.kani@hp.com> wrote:
>> > Add comments to the cachemode translation tables to clarify that
>> > the default values are set as minimal supported mode, which are
>> > necessary to handle WC and WT fallback to UC- when they are not
>> > enabled.
>> 
>> Wait - shouldn't WT fall back to UC (so to not be affected by WC
>> MTRR settings)?
> 
> Well, when MTRR is set to WC, WT will become UC.  So, it is not safe to
> start with.

Hmm, considering how my question was meant, your reply looks
contradictory to me. What I was talking about is the table as it is
in RC3, i.e. WT mapping to PAGE_PCD (matching WC and
UC_MINUS). I.e. if a page is mapped WT (i.e. UC-) and the
MTRRs for it say WC, then the result will be WC. Whereas if
WT translated to PAGE_PWT|PAGE_PCD (i.e. matching UC), an
MTRR setting of WC would have no effect - the resulting type
would still be UC.

> I know Luis is driving to support UC.  When UC can be used for both regular
> memory and IO memory, yes, it can be changed to fall back to UC.  At this
> point, however, UC cannot be set to regular memory.  See
> reserve_ram_pages_type().

With the above in mind, that's a problem then I would say.
Yet no matter whether UC can't be expressed as an
attribute on a page, what gets stored in the page table
entries should, together with how PAT is set, result in
something that doesn't weaken the requested memory
type. I.e. if WT was requested, the end result shouldn't
be WC. Yet any representable type may still be mapped to
UC (as being the strictest one possible), and hence if WT
can't be expressed it should be made UC.

Jan

Re: [PATCH] x86, pat: Add comments to cachemode translation tables

[Toshi Kani]

On Thu, 2015-07-23 at 08:50 -0600, Jan Beulich wrote:
> > 
> > > > On 23.07.15 at 16:27, <toshi.kani@hp.com> wrote:
> > On Thu, 2015-07-23 at 00:42 -0600, Jan Beulich wrote:
> > > > 
> > > > > > On 22.07.15 at 20:06, <toshi.kani@hp.com> wrote:
> > > > Add comments to the cachemode translation tables to clarify that
> > > > the default values are set as minimal supported mode, which are
> > > > necessary to handle WC and WT fallback to UC- when they are not
> > > > enabled.
> > > 
> > > Wait - shouldn't WT fall back to UC (so to not be affected by WC
> > > MTRR settings)?
> > 
> > Well, when MTRR is set to WC, WT will become UC.  So, it is not safe to
> > start with.
> 
> Hmm, considering how my question was meant, your reply looks
> contradictory to me. What I was talking about is the table as it is
> in RC3, i.e. WT mapping to PAGE_PCD (matching WC and
> UC_MINUS). I.e. if a page is mapped WT (i.e. UC-) and the
> MTRRs for it say WC, then the result will be WC. Whereas if
> WT translated to PAGE_PWT|PAGE_PCD (i.e. matching UC), an
> MTRR setting of WC would have no effect - the resulting type
> would still be UC.

Yes, I know what you meant.  I agree that WT should fall back to UC when
considering MTRR. 

In my reply, I was referring the regular case that WT is supported (i.e. no
fallback).  WT request to a range covered by MTRR with WC will become UC. 
 I think such code is not written properly (I should not have said "not
safe") since it won't get WT in the regular case, either.  A range with
MTRR covered by WC is a special memory, ex. frame buffer, and the code
should know what type of memory it is dealing with.

> > I know Luis is driving to support UC.  When UC can be used for both 
> > regular
> > memory and IO memory, yes, it can be changed to fall back to UC.  At 
> > this
> > point, however, UC cannot be set to regular memory.  See
> > reserve_ram_pages_type().
> 
> With the above in mind, that's a problem then I would say.
> Yet no matter whether UC can't be expressed as an
> attribute on a page, what gets stored in the page table
> entries should, together with how PAT is set, result in
> something that doesn't weaken the requested memory
> type. I.e. if WT was requested, the end result shouldn't
> be WC. Yet any representable type may still be mapped to
> UC (as being the strictest one possible), and hence if WT
> can't be expressed it should be made UC.

Yes, I agree with you.  But such risk is very low -- 1) the regular case
(no fallback) is used most of the cases, 2) the code using WT knows what
type of memory it is dealing with.  For example, pmem may map NVDIMM with
WT, and any sane BIOS sets MTRR to WB for NVDIMM. 

Thanks,
-Toshi

Re: [PATCH] x86, pat: Add comments to cachemode translation tables

[Jan Beulich]

>>> On 23.07.15 at 17:25, <toshi.kani@hp.com> wrote:
> Yes, I agree with you.  But such risk is very low -- 1) the regular case
> (no fallback) is used most of the cases, 2) the code using WT knows what
> type of memory it is dealing with.  For example, pmem may map NVDIMM with
> WT, and any sane BIOS sets MTRR to WB for NVDIMM. 

Do the words "sane" and "BIOS" really fit together in your opinion?

Jan

Re: [PATCH] x86, pat: Add comments to cachemode translation tables

[Toshi Kani]

On Thu, 2015-07-23 at 09:36 -0600, Jan Beulich wrote:
> > 
> > > > On 23.07.15 at 17:25, <toshi.kani@hp.com> wrote:
> > Yes, I agree with you.  But such risk is very low -- 1) the regular 
> > case
> > (no fallback) is used most of the cases, 2) the code using WT knows 
> > what
> > type of memory it is dealing with.  For example, pmem may map NVDIMM 
> > with
> > WT, and any sane BIOS sets MTRR to WB for NVDIMM. 
> 
> Do the words "sane" and "BIOS" really fit together in your opinion?

:-)

Anyway, I am not disagreeing with you... When UC is ready for both regular
memory and IO memory, it should be changed to fall back to UC.

Thanks,
-Toshi

Re: [PATCH] x86, pat: Add comments to cachemode translation tables

[Thomas Gleixner]

On Thu, 23 Jul 2015, Toshi Kani wrote:
> On Thu, 2015-07-23 at 09:36 -0600, Jan Beulich wrote:
> > > 
> > > > > On 23.07.15 at 17:25, <toshi.kani@hp.com> wrote:
> > > Yes, I agree with you.  But such risk is very low -- 1) the regular 
> > > case
> > > (no fallback) is used most of the cases, 2) the code using WT knows 
> > > what
> > > type of memory it is dealing with.  For example, pmem may map NVDIMM 
> > > with
> > > WT, and any sane BIOS sets MTRR to WB for NVDIMM. 
> > 
> > Do the words "sane" and "BIOS" really fit together in your opinion?
> 
> :-)
> 
> Anyway, I am not disagreeing with you... When UC is ready for both regular
> memory and IO memory, it should be changed to fall back to UC.

What's the resolution of this discussion? Is that patch correct as is
or do we get an updated version?

Thanks,

	tglx

Re: [PATCH] x86, pat: Add comments to cachemode translation tables

[Toshi Kani]

On Sun, 2015-08-02 at 12:07 +0200, Thomas Gleixner wrote:
> On Thu, 23 Jul 2015, Toshi Kani wrote:
> > On Thu, 2015-07-23 at 09:36 -0600, Jan Beulich wrote:
> > > > 
> > > > > > On 23.07.15 at 17:25, <toshi.kani@hp.com> wrote:
> > > > Yes, I agree with you.  But such risk is very low -- 1) the regular 
> > > > case
> > > > (no fallback) is used most of the cases, 2) the code using WT knows 
> > > > what
> > > > type of memory it is dealing with.  For example, pmem may map NVDIMM 
> > > > 
> > > > with
> > > > WT, and any sane BIOS sets MTRR to WB for NVDIMM. 
> > > 
> > > Do the words "sane" and "BIOS" really fit together in your opinion?
> > 
> > :-)
> > 
> > Anyway, I am not disagreeing with you... When UC is ready for both 
> > regular
> > memory and IO memory, it should be changed to fall back to UC.
> 
> What's the resolution of this discussion? Is that patch correct as is
> or do we get an updated version?

Yes, this patch is correct and we are in agreement.

What Jan mentioned about falling back to UC, instead of UC-, is a separate
item, and the code does not support it yet.

Thanks,
-Toshi

[tip:x86/mm] x86/mm/pat: Add comments to cachemode translation tables

[tip-bot for Toshi Kani]

Commit-ID:  d5dc861bd601b546ae6b36af54485142cca36a5e
Gitweb:     http://git.kernel.org/tip/d5dc861bd601b546ae6b36af54485142cca36a5e
Author:     Toshi Kani <toshi.kani@hp.com>
AuthorDate: Wed, 22 Jul 2015 12:06:11 -0600
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 20 Aug 2015 21:26:12 +0200

x86/mm/pat: Add comments to cachemode translation tables

Add comments to the cachemode translation tables to clarify that
the default values are set as minimal supported mode, which are
necessary to handle WC and WT fallback to UC- when they are not
enabled.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1437588371-28223-1-git-send-email-toshi.kani@hp.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/mm/init.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index 8533b46..1d8a83d 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -30,8 +30,11 @@
 /*
  * Tables translating between page_cache_type_t and pte encoding.
  *
- * Minimal supported modes are defined statically, they are modified
- * during bootup if more supported cache modes are available.
+ * The default values are defined statically as minimal supported mode;
+ * WC and WT fall back to UC-.  pat_init() updates these values to support
+ * more cache modes, WC and WT, when it is safe to do so.  See pat_init()
+ * for the details.  Note, __early_ioremap() used during early boot-time
+ * takes pgprot_t (pte encoding) and does not use these tables.
  *
  *   Index into __cachemode2pte_tbl[] is the cachemode.
  *