Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1355595 > unrolled thread

[PATCH 0/2] Refactor MTRR and PAT initializations

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] Refactor MTRR and PAT initializations Toshi Kani <toshi.kani@hpe.com> - 2016-03-11 05:00 +0100
    [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Toshi Kani <toshi.kani@hpe.com> - 2016-03-11 05:00 +0100
      Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Borislav Petkov <bp@alien8.de> - 2016-03-11 10:20 +0100
        Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT  table Toshi Kani <toshi.kani@hpe.com> - 2016-03-11 16:40 +0100
          Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Borislav Petkov <bp@alien8.de> - 2016-03-11 17:00 +0100
            Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT  table Toshi Kani <toshi.kani@hpe.com> - 2016-03-11 19:40 +0100
              Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Borislav Petkov <bp@alien8.de> - 2016-03-12 13:00 +0100
                Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT  table Toshi Kani <toshi.kani@hpe.com> - 2016-03-14 21:50 +0100
                  Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Borislav Petkov <bp@alien8.de> - 2016-03-15 12:10 +0100
                    Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT  table Toshi Kani <toshi.kani@hpe.com> - 2016-03-15 22:10 +0100
                Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-03-15 01:30 +0100
                  Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT  table Toshi Kani <toshi.kani@hpe.com> - 2016-03-15 03:20 +0100
                    Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Borislav Petkov <bp@alien8.de> - 2016-03-15 12:10 +0100
                      Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT  table Toshi Kani <toshi.kani@hpe.com> - 2016-03-15 16:00 +0100
                        Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Borislav Petkov <bp@alien8.de> - 2016-03-15 16:50 +0100
                          Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT  table Toshi Kani <toshi.kani@hpe.com> - 2016-03-15 17:20 +0100
                            Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table Borislav Petkov <bp@alien8.de> - 2016-03-15 17:40 +0100
                    Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-03-15 22:40 +0100
    [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-11 05:00 +0100
      Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Ingo Molnar <mingo@kernel.org> - 2016-03-11 10:10 +0100
        Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Ingo Molnar <mingo@kernel.org> - 2016-03-11 10:20 +0100
          Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-11 18:50 +0100
            Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Ingo Molnar <mingo@kernel.org> - 2016-03-12 17:20 +0100
              Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-14 20:00 +0100
            Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code "Luis R. Rodriguez" <mcgrof@suse.com> - 2016-03-15 00:00 +0100
              Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-15 00:50 +0100
                Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Borislav Petkov <bp@suse.de> - 2016-03-15 17:00 +0100
      Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Borislav Petkov <bp@alien8.de> - 2016-03-11 10:30 +0100
        Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-11 19:10 +0100
          Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-03-11 23:20 +0100
            Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-12 00:10 +0100
              Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-03-12 00:40 +0100
                Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-12 01:30 +0100
                  Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-03-15 01:20 +0100
                    Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code Toshi Kani <toshi.kani@hpe.com> - 2016-03-16 00:00 +0100
                      Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-03-16 00:30 +0100

Page 1 of 2  [1] 2  Next page →

#1355595 — [PATCH 0/2] Refactor MTRR and PAT initializations

Since 'commit 9cd25aac1f44 ("x86/mm/pat: Emulate PAT when it
is disabled")', we emulate a PAT table when PAT is disabled.
This requires pat_init() be called even if PAT is disabled,
which revealed a long standing issue that PAT is left enabled
without calling pat_init() at all.

pat_init() is called from MTRR code since it relies on MTRR's
rendezvous handler to initialize PAT for all APs .  However,
when CPU does not support MTRR, ex. qemu32's virtual CPU, MTRR
is set disabled and does not call pat_init().  There is no
interface available for MTRR to disable PAT, either.

This patch-set refactors MTRR and PAT initializations to make
sure that PAT is properly initialized in all cases.

---
Toshi Kani (2):
 1/2 x86/mm/pat: Change pat_disable() to emulate PAT table
 2/2 x86/mtrr: Refactor PAT initialization code 

---
 arch/x86/include/asm/mtrr.h        |  6 ++-
 arch/x86/include/asm/pat.h         |  1 +
 arch/x86/kernel/cpu/mtrr/generic.c | 24 ++++++-----
 arch/x86/kernel/cpu/mtrr/main.c    | 13 +++++-
 arch/x86/kernel/cpu/mtrr/mtrr.h    |  1 +
 arch/x86/mm/pat.c                  | 84 +++++++++++++++++++++++---------------
 6 files changed, 84 insertions(+), 45 deletions(-)

[toc] | [next] | [standalone]

#1355596 — [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

Since 'commit 9cd25aac1f44 ("x86/mm/pat: Emulate PAT when it
is disabled")', we emulate a PAT table when PAT is disabled.
This requires pat_init() be called even if PAT is disabled,
which revealed a long standing issue that PAT is left enabled
without calling pat_init() at all.

pat_init() is called from MTRR code since it relies on MTRR's
rendezvous handler to initialize PAT for all APs .  However,
when CPU does not support MTRR, ex. qemu32's virtual CPU, MTRR
is set disabled and does not call pat_init().  There is no
interface available for MTRR to disable PAT, either.

Change pat_disable() to a regular function (from an inline func)
so that MTRR can call it to disable PAT when MTRR is disabled.
pat_disable() sets PAT disabled, and calls pat_disable_init()
to emulate the PAT table.

link: https://lkml.org/lkml/2016/3/10/402
Reported-by: Paul Gortmaker <paul.gortmaker@windriver.com>
Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Luis R. Rodriguez <mcgrof@suse.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/pat.h |    1 +
 arch/x86/mm/pat.c          |   84 +++++++++++++++++++++++++++-----------------
 2 files changed, 52 insertions(+), 33 deletions(-)

diff --git a/arch/x86/include/asm/pat.h b/arch/x86/include/asm/pat.h
index ca6c228..016142b 100644
--- a/arch/x86/include/asm/pat.h
+++ b/arch/x86/include/asm/pat.h
@@ -5,6 +5,7 @@
 #include <asm/pgtable_types.h>
 
 bool pat_enabled(void);
+void pat_disable(const char *reason);
 extern void pat_init(void);
 void pat_init_cache_modes(u64);
 
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index f4ae536..1ff8aa9 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -40,11 +40,19 @@
 static bool boot_cpu_done;
 
 static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT);
+static void pat_disable_init(void);
 
-static inline void pat_disable(const char *reason)
+void pat_disable(const char *reason)
 {
+	if (boot_cpu_done) {
+		pr_info("x86/PAT: PAT cannot be disabled after initialized\n");
+		return;
+	}
+
 	__pat_enabled = 0;
 	pr_info("x86/PAT: %s\n", reason);
+
+	pat_disable_init();
 }
 
 static int __init nopat(char *str)
@@ -207,9 +215,6 @@ static void pat_bsp_init(u64 pat)
 		return;
 	}
 
-	if (!pat_enabled())
-		goto done;
-
 	rdmsrl(MSR_IA32_CR_PAT, tmp_pat);
 	if (!tmp_pat) {
 		pat_disable("PAT MSR is 0, disabled.");
@@ -218,15 +223,11 @@ static void pat_bsp_init(u64 pat)
 
 	wrmsrl(MSR_IA32_CR_PAT, pat);
 
-done:
 	pat_init_cache_modes(pat);
 }
 
 static void pat_ap_init(u64 pat)
 {
-	if (!pat_enabled())
-		return;
-
 	if (!cpu_has_pat) {
 		/*
 		 * If this happens we are on a secondary CPU, but switched to
@@ -238,38 +239,55 @@ static void pat_ap_init(u64 pat)
 	wrmsrl(MSR_IA32_CR_PAT, pat);
 }
 
+static void pat_disable_init(void)
+{
+	u64 pat;
+	static int disable_init_done;
+
+	if (disable_init_done)
+		return;
+
+	/*
+	 * No PAT. Emulate the PAT table that corresponds to the two
+	 * cache bits, PWT (Write Through) and PCD (Cache Disable). This
+	 * setup is the same as the BIOS default setup when the system
+	 * has PAT but the "nopat" boot option has been specified. This
+	 * emulated PAT table is used when MSR_IA32_CR_PAT returns 0.
+	 *
+	 * PTE encoding:
+	 *
+	 *       PCD
+	 *       |PWT  PAT
+	 *       ||    slot
+	 *       00    0    WB : _PAGE_CACHE_MODE_WB
+	 *       01    1    WT : _PAGE_CACHE_MODE_WT
+	 *       10    2    UC-: _PAGE_CACHE_MODE_UC_MINUS
+	 *       11    3    UC : _PAGE_CACHE_MODE_UC
+	 *
+	 * NOTE: When WC or WP is used, it is redirected to UC- per
+	 * the default setup in __cachemode2pte_tbl[].
+	 */
+	pat = PAT(0, WB) | PAT(1, WT) | PAT(2, UC_MINUS) | PAT(3, UC) |
+	      PAT(4, WB) | PAT(5, WT) | PAT(6, UC_MINUS) | PAT(7, UC);
+
+	pat_init_cache_modes(pat);
+
+	disable_init_done = 1;
+}
+
 void pat_init(void)
 {
 	u64 pat;
 	struct cpuinfo_x86 *c = &boot_cpu_data;
 
 	if (!pat_enabled()) {
-		/*
-		 * No PAT. Emulate the PAT table that corresponds to the two
-		 * cache bits, PWT (Write Through) and PCD (Cache Disable). This
-		 * setup is the same as the BIOS default setup when the system
-		 * has PAT but the "nopat" boot option has been specified. This
-		 * emulated PAT table is used when MSR_IA32_CR_PAT returns 0.
-		 *
-		 * PTE encoding:
-		 *
-		 *       PCD
-		 *       |PWT  PAT
-		 *       ||    slot
-		 *       00    0    WB : _PAGE_CACHE_MODE_WB
-		 *       01    1    WT : _PAGE_CACHE_MODE_WT
-		 *       10    2    UC-: _PAGE_CACHE_MODE_UC_MINUS
-		 *       11    3    UC : _PAGE_CACHE_MODE_UC
-		 *
-		 * NOTE: When WC or WP is used, it is redirected to UC- per
-		 * the default setup in __cachemode2pte_tbl[].
-		 */
-		pat = PAT(0, WB) | PAT(1, WT) | PAT(2, UC_MINUS) | PAT(3, UC) |
-		      PAT(4, WB) | PAT(5, WT) | PAT(6, UC_MINUS) | PAT(7, UC);
+		pat_disable_init();
+		return;
+	}
 
-	} else if ((c->x86_vendor == X86_VENDOR_INTEL) &&
-		   (((c->x86 == 0x6) && (c->x86_model <= 0xd)) ||
-		    ((c->x86 == 0xf) && (c->x86_model <= 0x6)))) {
+	if ((c->x86_vendor == X86_VENDOR_INTEL) &&
+	    (((c->x86 == 0x6) && (c->x86_model <= 0xd)) ||
+	     ((c->x86 == 0xf) && (c->x86_model <= 0x6)))) {
 		/*
 		 * PAT support with the lower four entries. Intel Pentium 2,
 		 * 3, M, and 4 are affected by PAT errata, which makes the

[toc] | [prev] | [next] | [standalone]

#1355749 — Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

On Thu, Mar 10, 2016 at 09:45:45PM -0700, Toshi Kani wrote:
> Since 'commit 9cd25aac1f44 ("x86/mm/pat: Emulate PAT when it
> is disabled")', we emulate a PAT table when PAT is disabled.
> This requires pat_init() be called even if PAT is disabled,
> which revealed a long standing issue that PAT is left enabled
> without calling pat_init() at all.
> 
> pat_init() is called from MTRR code since it relies on MTRR's
> rendezvous handler to initialize PAT for all APs .  However,
> when CPU does not support MTRR, ex. qemu32's virtual CPU, MTRR
> is set disabled and does not call pat_init().  There is no
> interface available for MTRR to disable PAT, either.
> 
> Change pat_disable() to a regular function (from an inline func)
> so that MTRR can call it to disable PAT when MTRR is disabled.
> pat_disable() sets PAT disabled, and calls pat_disable_init()
> to emulate the PAT table.
> 
> link: https://lkml.org/lkml/2016/3/10/402
> Reported-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Luis R. Rodriguez <mcgrof@suse.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/include/asm/pat.h |    1 +
>  arch/x86/mm/pat.c          |   84 +++++++++++++++++++++++++++-----------------
>  2 files changed, 52 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/x86/include/asm/pat.h b/arch/x86/include/asm/pat.h
> index ca6c228..016142b 100644
> --- a/arch/x86/include/asm/pat.h
> +++ b/arch/x86/include/asm/pat.h
> @@ -5,6 +5,7 @@
>  #include <asm/pgtable_types.h>
>  
>  bool pat_enabled(void);
> +void pat_disable(const char *reason);
>  extern void pat_init(void);
>  void pat_init_cache_modes(u64);
>  
> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> index f4ae536..1ff8aa9 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -40,11 +40,19 @@
>  static bool boot_cpu_done;
>  
>  static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT);
> +static void pat_disable_init(void);
>  
> -static inline void pat_disable(const char *reason)
> +void pat_disable(const char *reason)
>  {
> +	if (boot_cpu_done) {
> +		pr_info("x86/PAT: PAT cannot be disabled after initialized\n");

		pr_err()

> +		return;
> +	}
> +
>  	__pat_enabled = 0;
>  	pr_info("x86/PAT: %s\n", reason);
> +
> +	pat_disable_init();

Why can't you call pat_init() here simply? It checks pat_enabled(). You
can call it pat_setup() or so if it looks confusing to call an init
function in a disable function...

Then you don't have to add yet another static disable_init_done but rely on
boot_cpu_done which gets set in pat_init().

Also, I don't see the static_cpu_has() check I suggested yesterday - we need to
check the feature bits if PAT gets disabled early on some old Intels.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1355984 — Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

On Fri, 2016-03-11 at 09:12 +0000, Borislav Petkov wrote:
> On Thu, Mar 10, 2016 at 09:45:45PM -0700, Toshi Kani wrote:
 :
> >  
> > -static inline void pat_disable(const char *reason)
> > +void pat_disable(const char *reason)
> >  {
> > +	if (boot_cpu_done) {
> > +		pr_info("x86/PAT: PAT cannot be disabled after
> > initialized\n");
> 
> 		pr_err()

Will do.

> 
> > +		return;
> > +	}
> > +
> >  	__pat_enabled = 0;
> >  	pr_info("x86/PAT: %s\n", reason);
> > +
> > +	pat_disable_init();
> 
> Why can't you call pat_init() here simply? It checks pat_enabled(). You
> can call it pat_setup() or so if it looks confusing to call an init
> function in a disable function...

How about pat_disable_setup()?  It's only used for the disabled case, so
I'd prefer to keep the word "disable".

Yes, calling pat_init() from pat_disable() works too. I changed it in this
way because:
 - pat_bsp_init() calls pat_disabled() in an error case. It is simpler to
avoid a recursive call to pat_init().
 - pat_bsp_init() has two different error paths, 1) call pat_disable() and
return, and 2) goto done and call pat_init_cache_modes(). We can remove
case 2) to keep the error handling consistent in this way.

> Then you don't have to add yet another static disable_init_done but rely
> on boot_cpu_done which gets set in pat_init().

Right, but it will do 'boot_cpu_done = true' twice, and this implicit
recursive call may cause an issue in future if someone makes change
carelessly.

> Also, I don't see the static_cpu_has() check I suggested yesterday - we
> need to check the feature bits if PAT gets disabled early on some old
> Intels.

Sorry, I should have mentioned it. I ended up not needing this change. The
table in patch 2/2 covers this case as:

  MTRR   PAT    ACTION
  ====================================================================
  E      D      MTRR calls pat_init() -> PAT disabled per cpu_has_pat

That is, the check with cpu_has_pat in pat_bsp_init() calls pat_disable()
in this case. I preferred this way because it will continue to log a
message "PAT not supported by CPU.", and keeps __pat_enabled as the single
variable to manage the PAT state.

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1356014 — Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

On Fri, Mar 11, 2016 at 09:27:40AM -0700, Toshi Kani wrote:
> How about pat_disable_setup()?  It's only used for the disabled case, so
> I'd prefer to keep the word "disable".

What for?

Renaming pat_init() to pat_setup() is perfectly fine as it sets up PAT
after looking at pat_disabled() setting and after looking at the CPU
vendor. Sounds like a perfectly sane design to me.

> Yes, calling pat_init() from pat_disable() works too. I changed it in this
> way because:
>  - pat_bsp_init() calls pat_disabled() in an error case. It is simpler to
> avoid a recursive call to pat_init().

So do this:

static inline void pat_disable(const char *reason)
{
	if (!__pat_enabled)
		return;


>  - pat_bsp_init() has two different error paths, 1) call pat_disable() and
> return, and 2) goto done and call pat_init_cache_modes(). We can remove
> case 2) to keep the error handling consistent in this way.

Above.

> > Then you don't have to add yet another static disable_init_done but rely
> > on boot_cpu_done which gets set in pat_init().
> 
> Right, but it will do 'boot_cpu_done = true' twice, and this implicit
> recursive call may cause an issue in future if someone makes change
> carelessly.

So move boot_cpu_done into pat_bsp_init() and make it protect that
function from a being called a second time.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1356127 — Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

On Fri, 2016-03-11 at 16:54 +0100, Borislav Petkov wrote:
> On Fri, Mar 11, 2016 at 09:27:40AM -0700, Toshi Kani wrote:
> > How about pat_disable_setup()?  It's only used for the disabled case,
> > so I'd prefer to keep the word "disable".
> 
> What for?
> 
> Renaming pat_init() to pat_setup() is perfectly fine as it sets up PAT
> after looking at pat_disabled() setting and after looking at the CPU
> vendor. Sounds like a perfectly sane design to me.

Sorry, I meant to say -- "How about renaming pat_disable_init() to
pat_disable_setup()?" since I thought you had suggested to rename
pat_disable_init() to pat_setup().  I am still in favor of having a
separate setup func for the disabled case.

> > Yes, calling pat_init() from pat_disable() works too. I changed it in
> > this way because:
> >  - pat_bsp_init() calls pat_disabled() in an error case. It is simpler
> > to avoid a recursive call to pat_init().
> 
> So do this:
> 
> static inline void pat_disable(const char *reason)
> {
> 	if (!__pat_enabled)
> 		return;

Hmm...  I do not think I understand this.  When pat_bsp_init() calls
pat_disable(), 'pat' has been set to the "Full PAT support" setup.  So, we
need to reset 'pat' to the "No PAT" setup.  How is this handled in your
case?


> >  - pat_bsp_init() has two different error paths, 1) call pat_disable()
> > and return, and 2) goto done and call pat_init_cache_modes(). We can
> > remove case 2) to keep the error handling consistent in this way.
> 
> Above.
> 
> > > Then you don't have to add yet another static disable_init_done but
> > > rely on boot_cpu_done which gets set in pat_init().
> > 
> > Right, but it will do 'boot_cpu_done = true' twice, and this implicit
> > recursive call may cause an issue in future if someone makes change
> > carelessly.
> 
> So move boot_cpu_done into pat_bsp_init() and make it protect that
> function from a being called a second time.

I think this leads more complication in the end.  pat_init() covers (too)
many scenarios already, and moving the disabled setup case out will
simplify it, IMHO.

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1356409 — Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

Ok, let's start again.

So what's wrong with this simpler, cleaner version below?

* It makes sure to run the rendezvous handler on the BP on init.

* It disables PAT otherwise.

* Oh, and it boots fine with Paul's reproducer, X is there (even though
vncviewer dies when X starts with "Rect too large: 640x480 at (0, 0)"
and I have to reconnect to see the X window).

In dmesg I see "x86/PAT: Disabled by MTRR".

---
diff --git a/arch/x86/include/asm/pat.h b/arch/x86/include/asm/pat.h
index ca6c228d5e62..3f3b1bc67391 100644
--- a/arch/x86/include/asm/pat.h
+++ b/arch/x86/include/asm/pat.h
@@ -5,7 +5,8 @@
 #include <asm/pgtable_types.h>
 
 bool pat_enabled(void);
-extern void pat_init(void);
+void pat_disable(const char *reason);
+void pat_setup(void);
 void pat_init_cache_modes(u64);
 
 extern int reserve_memtype(u64 start, u64 end,
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 19f57360dfd2..44bad29ebc9b 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -444,11 +444,24 @@ static void __init print_mtrr_state(void)
 		pr_debug("TOM2: %016llx aka %lldM\n", mtrr_tom2, mtrr_tom2>>20);
 }
 
+void __init mtrr_pat_setup_bp(void)
+{
+	unsigned long flags;
+
+	/* PAT setup for BP. We need to go through sync steps here */
+	local_irq_save(flags);
+	prepare_set();
+
+	pat_setup();
+
+	post_set();
+	local_irq_restore(flags);
+}
+
 /* Grab all of the MTRR state for this CPU into *state */
 bool __init get_mtrr_state(void)
 {
 	struct mtrr_var_range *vrs;
-	unsigned long flags;
 	unsigned lo, dummy;
 	unsigned int i;
 
@@ -481,15 +494,6 @@ bool __init get_mtrr_state(void)
 
 	mtrr_state_set = 1;
 
-	/* PAT setup for BP. We need to go through sync steps here */
-	local_irq_save(flags);
-	prepare_set();
-
-	pat_init();
-
-	post_set();
-	local_irq_restore(flags);
-
 	return !!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED);
 }
 
@@ -788,7 +792,7 @@ static void generic_set_all(void)
 	mask = set_mtrr_state();
 
 	/* also set PAT */
-	pat_init();
+	pat_setup();
 
 	post_set();
 	local_irq_restore(flags);
diff --git a/arch/x86/kernel/cpu/mtrr/main.c b/arch/x86/kernel/cpu/mtrr/main.c
index 10f8d4796240..5c442b4bd52a 100644
--- a/arch/x86/kernel/cpu/mtrr/main.c
+++ b/arch/x86/kernel/cpu/mtrr/main.c
@@ -752,6 +752,8 @@ void __init mtrr_bp_init(void)
 			/* BIOS may override */
 			__mtrr_enabled = get_mtrr_state();
 
+			mtrr_pat_setup_bp();
+
 			if (mtrr_cleanup(phys_addr)) {
 				changed_by_mtrr_cleanup = 1;
 				mtrr_if->set_all();
@@ -759,8 +761,11 @@ void __init mtrr_bp_init(void)
 		}
 	}
 
-	if (!mtrr_enabled())
+	if (!__mtrr_enabled) {
 		pr_info("MTRR: Disabled\n");
+		pat_disable("PAT disabled by MTRR");
+		pat_setup();
+	}
 }
 
 void mtrr_ap_init(void)
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.h b/arch/x86/kernel/cpu/mtrr/mtrr.h
index 951884dcc433..e60d87cceae5 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.h
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.h
@@ -52,6 +52,7 @@ void set_mtrr_prepare_save(struct set_mtrr_context *ctxt);
 void fill_mtrr_var_range(unsigned int index,
 		u32 base_lo, u32 base_hi, u32 mask_lo, u32 mask_hi);
 bool get_mtrr_state(void);
+void __init mtrr_pat_setup_bp(void);
 
 extern void set_mtrr_ops(const struct mtrr_ops *ops);
 
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index faec01e7a17d..ac8283bcf417 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -41,8 +41,11 @@ static bool boot_cpu_done;
 
 static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT);
 
-static inline void pat_disable(const char *reason)
+void pat_disable(const char *reason)
 {
+	if (!__pat_enabled)
+		return;
+
 	__pat_enabled = 0;
 	pr_info("x86/PAT: %s\n", reason);
 }
@@ -238,7 +241,7 @@ static void pat_ap_init(u64 pat)
 	wrmsrl(MSR_IA32_CR_PAT, pat);
 }
 
-void pat_init(void)
+void pat_setup(void)
 {
 	u64 pat;
 	struct cpuinfo_x86 *c = &boot_cpu_data;


-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1357614 — Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

On Sat, 2016-03-12 at 12:55 +0100, Borislav Petkov wrote:
> Ok, let's start again.
> 
> So what's wrong with this simpler, cleaner version below?
> 
> * It makes sure to run the rendezvous handler on the BP on init.
> 
> * It disables PAT otherwise.
> 
> * Oh, and it boots fine with Paul's reproducer, X is there (even though
> vncviewer dies when X starts with "Rect too large: 640x480 at (0, 0)"
> and I have to reconnect to see the X window).
> 
> In dmesg I see "x86/PAT: Disabled by MTRR".

Your patch is a simplified version of mine.  So, yes, it fixes the Paul's
issue, but it does not address other issues that my patchset also
addressed.  In specific, I think your patch has the following issues.

- pat_disable() is now callable from other modules. So, it needs to check
with boot_cpu_done. We cannot disable PAT once it is initialized.

- mtrr_bp_init() needs to check with mtrr_enabled() when it
calls mtrr_pat_setup_bp(). Otherwise, PAT is left initialized on BSP only
when MTRR is disabled by its MSR. In your patch, mtrr_bp_init() calls
pat_setup() again, but it does not help since boot_cpu_done is set.

- When PAT is disabled in CPU feature, pat_bsp_init() calls pat_disable()
and returns. However, it does not initialize a PAT table by calling
pat_init_cache_modes().

- When CONFIG_MTRR is unset, it does not call pat_setup().

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1357988 — Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

On Mon, Mar 14, 2016 at 03:37:23PM -0600, Toshi Kani wrote:
> Your patch is a simplified version of mine.  So, yes, it fixes the Paul's
> issue, but it does not address other issues that my patchset also
> addressed.  In specific, I think your patch has the following issues.

You couldnt've structured your reply better: remember how I split a
convoluted patch of yours already? A patch which was trying to do a
bunch of things in one go.

The situation here is the same. You need to do *one* *logical*
*non-trivial* thing in a patch. If there's something else that needs to
be done, add it in a *separate* patch which explains why that new change
is needed.

> - pat_disable() is now callable from other modules. So, it needs to check
> with boot_cpu_done. We cannot disable PAT once it is initialized.

That should be a separate patch which explains *why* the change is being
done.

> - mtrr_bp_init() needs to check with mtrr_enabled() when it
> calls mtrr_pat_setup_bp(). Otherwise, PAT is left initialized on BSP only
> when MTRR is disabled by its MSR. In your patch, mtrr_bp_init() calls
> pat_setup() again, but it does not help since boot_cpu_done is set.

The code which you carved out from get_mtrr_state() didn't check
mtrr_enabled() before. That needs to be another patch *again* with
explanations.

> - When PAT is disabled in CPU feature, pat_bsp_init() calls pat_disable()
> and returns. However, it does not initialize a PAT table by calling
> pat_init_cache_modes().

Yet another patch.

> - When CONFIG_MTRR is unset, it does not call pat_setup().

Aaaand... can you guess what I'm going to say here?

I hope it is coming across as I intend it: please use my hunk to do a
single fix and then prepare all those changes above in separate patches
with explanations:

"Problem is A. We need to do B. I'm doing it/I'm doing C because."

Ok?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1358243 — Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

On Tue, 2016-03-15 at 12:00 +0100, Borislav Petkov wrote:
> On Mon, Mar 14, 2016 at 03:37:23PM -0600, Toshi Kani wrote:
> > Your patch is a simplified version of mine.  So, yes, it fixes the
> > Paul's issue, but it does not address other issues that my patchset
> > also addressed.  In specific, I think your patch has the following
> > issues.
> 
> You couldnt've structured your reply better: remember how I split a
> convoluted patch of yours already? A patch which was trying to do a
> bunch of things in one go.
> 
> The situation here is the same. You need to do *one* *logical*
> *non-trivial* thing in a patch. If there's something else that needs to
> be done, add it in a *separate* patch which explains why that new change
> is needed.

Got it!

> > - pat_disable() is now callable from other modules. So, it needs to
> > check with boot_cpu_done. We cannot disable PAT once it is initialized.
> 
> That should be a separate patch which explains *why* the change is being
> done.
> 
> > - mtrr_bp_init() needs to check with mtrr_enabled() when it
> > calls mtrr_pat_setup_bp(). Otherwise, PAT is left initialized on BSP
> > only when MTRR is disabled by its MSR. In your patch, mtrr_bp_init()
> > calls pat_setup() again, but it does not help since boot_cpu_done is
> > set.
> 
> The code which you carved out from get_mtrr_state() didn't check
> mtrr_enabled() before. That needs to be another patch *again* with
> explanations.
> 
> > - When PAT is disabled in CPU feature, pat_bsp_init() calls
> > pat_disable() and returns. However, it does not initialize a PAT table
> > by calling pat_init_cache_modes().
> 
> Yet another patch.
> 
> > - When CONFIG_MTRR is unset, it does not call pat_setup().
> 
> Aaaand... can you guess what I'm going to say here?
> 
> I hope it is coming across as I intend it: please use my hunk to do a
> single fix and then prepare all those changes above in separate patches
> with explanations:

Unfortunately, this single fix will break Xen.  So, I think we will need to
make a few enhancements first before making the fix.

> "Problem is A. We need to do B. I'm doing it/I'm doing C because."
> 
> Ok?

Yes, I will try to separate the patches to change one logical thing at a
time. 

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1357717 — Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

I like this approach more as it stuff more PAT setup on its own type
of calls, but:

On Sat, Mar 12, 2016 at 12:55:44PM +0100, Borislav Petkov wrote:
> diff --git a/arch/x86/kernel/cpu/mtrr/main.c b/arch/x86/kernel/cpu/mtrr/main.c
> index 10f8d4796240..5c442b4bd52a 100644
> --- a/arch/x86/kernel/cpu/mtrr/main.c
> +++ b/arch/x86/kernel/cpu/mtrr/main.c
> @@ -759,8 +761,11 @@ void __init mtrr_bp_init(void)
>  		}
>  	}
>  
> -	if (!mtrr_enabled())
> +	if (!__mtrr_enabled) {
>  		pr_info("MTRR: Disabled\n");
> +		pat_disable("PAT disabled by MTRR");
> +		pat_setup();
> +	}
>  }

This hunk would break PAT on Xen.

  Luis

[toc] | [prev] | [next] | [standalone]

#1357757 — Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

[Multipart message — attachments visible in raw view] — view raw

On Tue, 2016-03-15 at 01:29 +0100, Luis R. Rodriguez wrote:
> I like this approach more as it stuff more PAT setup on its own type
> of calls, but:
> 
> On Sat, Mar 12, 2016 at 12:55:44PM +0100, Borislav Petkov wrote:
> > diff --git a/arch/x86/kernel/cpu/mtrr/main.c
> > b/arch/x86/kernel/cpu/mtrr/main.c
> > index 10f8d4796240..5c442b4bd52a 100644
> > --- a/arch/x86/kernel/cpu/mtrr/main.c
> > +++ b/arch/x86/kernel/cpu/mtrr/main.c
> > @@ -759,8 +761,11 @@ void __init mtrr_bp_init(void)
> >  		}
> >  	}
> >  
> > -	if (!mtrr_enabled())
> > +	if (!__mtrr_enabled) {
> >  		pr_info("MTRR: Disabled\n");
> > +		pat_disable("PAT disabled by MTRR");
> > +		pat_setup();
> > +	}
> >  }
> 
> This hunk would break PAT on Xen.

Can you try the attached patches?  They apply on top of my original patch-
set.  With this change, PAT code generally supports Xen, and the PAT init
code in Xen is now removed.  If they look OK, I will reorganize the patch
series.

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1357990 — Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

On Mon, Mar 14, 2016 at 09:11:16PM -0600, Toshi Kani wrote:
> -	pat = PAT(0, WB) | PAT(1, WT) | PAT(2, UC_MINUS) | PAT(3, UC) |
> -	      PAT(4, WB) | PAT(5, WT) | PAT(6, UC_MINUS) | PAT(7, UC);
> +	if (cpu_has_pat) {

Please use on init paths boot_cpu_has(X86_FEATURE_PAT) and on fast paths
static_cpu_has(X86_FEATURE_PAT). No more of that cpu_has_XXX ugliness.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1358086 — Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

On Tue, 2016-03-15 at 11:01 +0000, Borislav Petkov wrote:
> On Mon, Mar 14, 2016 at 09:11:16PM -0600, Toshi Kani wrote:
> > -	pat = PAT(0, WB) | PAT(1, WT) | PAT(2, UC_MINUS) | PAT(3, UC)
> > |
> > -	      PAT(4, WB) | PAT(5, WT) | PAT(6, UC_MINUS) | PAT(7, UC);
> > +	if (cpu_has_pat) {
> 
> Please use on init paths boot_cpu_has(X86_FEATURE_PAT) and on fast paths
> static_cpu_has(X86_FEATURE_PAT). No more of that cpu_has_XXX ugliness.

'cpu_has_pat' is defined as 'boot_cpu_has(X86_FEATURE_PAT)'.  Do you mean
it should explicitly use 'boot_cpu_has(X86_FEATURE_PAT)'?

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1358120 — Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

On Tue, Mar 15, 2016 at 09:43:15AM -0600, Toshi Kani wrote:
> > Please use on init paths boot_cpu_has(X86_FEATURE_PAT) and on fast paths
> > static_cpu_has(X86_FEATURE_PAT). No more of that cpu_has_XXX ugliness.
> 
> 'cpu_has_pat' is defined as 'boot_cpu_has(X86_FEATURE_PAT)'.  Do you mean
> it should explicitly use 'boot_cpu_has(X86_FEATURE_PAT)'?

No, read what I said.

We use boot_cpu_has(<feature_bit>) on slow paths (i.e., init, bootup,
etc), where speed is not that important. static_cpu_has(<feature_bit>)
is an optimized version which should be used in hot paths.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1358134 — Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

On Tue, 2016-03-15 at 16:47 +0100, Borislav Petkov wrote:
> On Tue, Mar 15, 2016 at 09:43:15AM -0600, Toshi Kani wrote:
> > > Please use on init paths boot_cpu_has(X86_FEATURE_PAT) and on fast
> > > paths static_cpu_has(X86_FEATURE_PAT). No more of that cpu_has_XXX
> > > ugliness.
> > 
> > 'cpu_has_pat' is defined as 'boot_cpu_has(X86_FEATURE_PAT)'.  Do you
> > mean it should explicitly use 'boot_cpu_has(X86_FEATURE_PAT)'?
> 
> No, read what I said.
> 
> We use boot_cpu_has(<feature_bit>) on slow paths (i.e., init, bootup,
> etc), where speed is not that important. static_cpu_has(<feature_bit>)
> is an optimized version which should be used in hot paths.

Yes, I understand that part.  Let me rephrase my question.

This PAT code is on init paths and speed is not that important.  So, it
needs to use 'boot_cpu_has()' here.  'cpu_has_pat' is defined
as boot_cpu_has(X86_FEATURE_PAT), and hence it uses boot_cpu_has() already.
 
While cpu_has_pat is the same as boot_cpu_has(X86_FEATURE_PAT), cpu_has_XXX
should not be used.  So, this code needs to be changed to use
boot_cpu_has(X86_FEATURE_PAT) directly.

Is this right?

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1358144 — Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

On Tue, Mar 15, 2016 at 11:11:23AM -0600, Toshi Kani wrote:
> While cpu_has_pat is the same as boot_cpu_has(X86_FEATURE_PAT), cpu_has_XXX
> should not be used.  So, this code needs to be changed to use
> boot_cpu_has(X86_FEATURE_PAT) directly.
> 
> Is this right?

Yes.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1358261 — Re: [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

On Mon, Mar 14, 2016 at 09:11:16PM -0600, Toshi Kani wrote:
> On Tue, 2016-03-15 at 01:29 +0100, Luis R. Rodriguez wrote:
> > I like this approach more as it stuff more PAT setup on its own type
> > of calls, but:
> > 
> > On Sat, Mar 12, 2016 at 12:55:44PM +0100, Borislav Petkov wrote:
> > > diff --git a/arch/x86/kernel/cpu/mtrr/main.c
> > > b/arch/x86/kernel/cpu/mtrr/main.c
> > > index 10f8d4796240..5c442b4bd52a 100644
> > > --- a/arch/x86/kernel/cpu/mtrr/main.c
> > > +++ b/arch/x86/kernel/cpu/mtrr/main.c
> > > @@ -759,8 +761,11 @@ void __init mtrr_bp_init(void)
> > >  		}
> > >  	}
> > >  
> > > -	if (!mtrr_enabled())
> > > +	if (!__mtrr_enabled) {
> > >  		pr_info("MTRR: Disabled\n");
> > > +		pat_disable("PAT disabled by MTRR");
> > > +		pat_setup();
> > > +	}
> > >  }
> > 
> > This hunk would break PAT on Xen.
> 
> Can you try the attached patches?  They apply on top of my original patch-
> set.  With this change, PAT code generally supports Xen, and the PAT init
> code in Xen is now removed.  If they look OK, I will reorganize the patch
> series.

I don't have time to test this at this time but on a cursory review this should
in theory work, a few nitpicks:

> 
> Thanks,
> -Toshi

> From: Toshi Kani <toshi.kani@hpe.com>
> 
> Add support of PAT emulation that matches with the PAT MSR.
> 
> ---
>  arch/x86/mm/pat.c |   73 +++++++++++++++++++++++++++++++----------------------
>  1 file changed, 43 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> index 1ff8aa9..565a478 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -40,7 +40,7 @@
>  static bool boot_cpu_done;
>  
>  static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT);
> -static void pat_disable_init(void);
> +static void pat_emu_init(void);
>  
>  void pat_disable(const char *reason)
>  {
> @@ -52,7 +52,7 @@ void pat_disable(const char *reason)
>  	__pat_enabled = 0;
>  	pr_info("x86/PAT: %s\n", reason);
>  
> -	pat_disable_init();
> +	pat_emu_init();
>  }
>  
>  static int __init nopat(char *str)
> @@ -239,40 +239,53 @@ static void pat_ap_init(u64 pat)
>  	wrmsrl(MSR_IA32_CR_PAT, pat);
>  }
>  
> -static void pat_disable_init(void)
> +static void pat_emu_init(void)
>  {
> -	u64 pat;
> -	static int disable_init_done;
> +	u64 pat = 0;
> +	static int emu_init_done;
>  
> -	if (disable_init_done)
> +	if (emu_init_done)
>  		return;
>  
> -	/*
> -	 * No PAT. Emulate the PAT table that corresponds to the two
> -	 * cache bits, PWT (Write Through) and PCD (Cache Disable). This
> -	 * setup is the same as the BIOS default setup when the system
> -	 * has PAT but the "nopat" boot option has been specified. This
> -	 * emulated PAT table is used when MSR_IA32_CR_PAT returns 0.
> -	 *
> -	 * PTE encoding:
> -	 *
> -	 *       PCD
> -	 *       |PWT  PAT
> -	 *       ||    slot
> -	 *       00    0    WB : _PAGE_CACHE_MODE_WB
> -	 *       01    1    WT : _PAGE_CACHE_MODE_WT
> -	 *       10    2    UC-: _PAGE_CACHE_MODE_UC_MINUS
> -	 *       11    3    UC : _PAGE_CACHE_MODE_UC
> -	 *
> -	 * NOTE: When WC or WP is used, it is redirected to UC- per
> -	 * the default setup in __cachemode2pte_tbl[].
> -	 */
> -	pat = PAT(0, WB) | PAT(1, WT) | PAT(2, UC_MINUS) | PAT(3, UC) |
> -	      PAT(4, WB) | PAT(5, WT) | PAT(6, UC_MINUS) | PAT(7, UC);
> +	if (cpu_has_pat) {

First, this is not emulation, to be clear.

> +		/*
> +		 * CPU supports PAT. Initialize the PAT table to match with
> +		 * the PAT MSR value. This setup is used by "nopat" boot

Did you mean "nomtrr" option ? If not why would "nopat" land you here and if
"nopat" was used and you ended up here why would we want to go ahead and
read the MSR to keep the PAT set up ?

> +		 * option, or by virtual machine environments which do not
> +		 * support MTRRs but support PAT.

This might end up supporting PAT for some other virtual environments ;)

> +		 *
> +		 * If the MSR returns 0, it is considered invalid and emulate
> +		 * as No PAT.
> +		 */
> +		rdmsrl(MSR_IA32_CR_PAT, pat);
> +	}
> +
> +	if (!pat) {
> +		/*
> +		 * No PAT. Emulate the PAT table that corresponds to the two
> +		 * cache bits, PWT (Write Through) and PCD (Cache Disable).
> +		 * This setup is also the same as the BIOS default setup.
> +		 *
> +		 * PTE encoding:
> +		 *
> +		 *       PCD
> +		 *       |PWT  PAT
> +		 *       ||    slot
> +		 *       00    0    WB : _PAGE_CACHE_MODE_WB
> +		 *       01    1    WT : _PAGE_CACHE_MODE_WT
> +		 *       10    2    UC-: _PAGE_CACHE_MODE_UC_MINUS
> +		 *       11    3    UC : _PAGE_CACHE_MODE_UC
> +		 *
> +		 * NOTE: When WC or WP is used, it is redirected to UC- per
> +		 * the default setup in __cachemode2pte_tbl[].
> +		 */
> +		pat = PAT(0, WB) | PAT(1, WT) | PAT(2, UC_MINUS) | PAT(3, UC) |
> +		      PAT(4, WB) | PAT(5, WT) | PAT(6, UC_MINUS) | PAT(7, UC);
> +	}

a) pat_emu_init() -- this should probably be renamed to something that includes
   not just emulation as for virtual environments that's not true and using
   emulation is very misleading for Xen.

b) Will this mean that then you can support bare metal with no MTRR but with PAT
   enabled too?

c) Why not just split this up to enable PAT init to be a first class citizen ?

  Luis

[toc] | [prev] | [next] | [standalone]

#1355599 — [PATCH 2/2] x86/mtrr: Refactor PAT initialization code

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.  Also, get_mtrr_state() calls pat_init() on
BSP even if MTRR is disabled by its MSR.  This causes pat_init()
be called on BSP only.

The following changes are made to address these issues:
- Move BSP's PAT init code to mtrr_bp_pat_init(), and change
  mtrr_bp_init() to call it if MTRR is enabled.  This keeps
  the init condition consistent with mtrr_ap_init() and
  mtrr_aps_init() for APs.
- Change mtrr_bp_init() to call pat_disable() when MTRR is
  disabled.
- Change mtrr_bp_init() stub function to call pat_disable()
  when CONFIG_MTRR is unset.

The table below discribes how PAT is initialized in all possible
cases.

 Legend
 ----------------------------
 E       Enabled in CPU feature and MSR
 D       Disabled in CPU feature or MSR
 nopat   "nopat" boot option specified
 !PAT    CONFIG_X86_PAT unset
 !MTRR   CONFIG_MTRR unset

 MTRR   PAT    ACTION
 ====================================================================
 E      E      MTRR calls pat_init() -> PAT enabled
 E      D      MTRR calls pat_init() -> PAT disabled per cpu_has_pat
 D      E      MTRR calls pat_disable() -> PAT disabled (*)
 D      D      MTRR calls pat_disable() -> PAT disabled
 E      nopat  nopat() calls pat_disable() -> PAT disabled
 D      nopat  nopat() calls pat_disable() -> PAT disabled
 E      !PAT   MTRR calls pat_init() -> PAT disabled per __pat_enabled
 D      !PAT   MTRR calls pat_disable() -> PAT disabled
 !MTRR  !PAT   mtrr_bp_init() stub calls pat_disable() -> PAT disabled

 (*) Further enhancement may enable PAT if needed

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Luis R. Rodriguez <mcgrof@suse.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/mtrr.h        |    6 +++++-
 arch/x86/kernel/cpu/mtrr/generic.c |   24 ++++++++++++++----------
 arch/x86/kernel/cpu/mtrr/main.c    |   13 ++++++++++++-
 arch/x86/kernel/cpu/mtrr/mtrr.h    |    1 +
 4 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index b94f6f6..a965e74 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -24,6 +24,7 @@
 #define _ASM_X86_MTRR_H
 
 #include <uapi/asm/mtrr.h>
+#include <asm/pat.h>
 
 
 /*
@@ -83,9 +84,12 @@ static inline int mtrr_trim_uncached_memory(unsigned long end_pfn)
 static inline void mtrr_centaur_report_mcr(int mcr, u32 lo, u32 hi)
 {
 }
+static inline void mtrr_bp_init(void)
+{
+	pat_disable("PAT disabled by MTRR");
+}
 
 #define mtrr_ap_init() do {} while (0)
-#define mtrr_bp_init() do {} while (0)
 #define set_mtrr_aps_delayed_init() do {} while (0)
 #define mtrr_aps_init() do {} while (0)
 #define mtrr_bp_restore() do {} while (0)
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index c870af1..136ae86 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -444,11 +444,24 @@ static void __init print_mtrr_state(void)
 		pr_debug("TOM2: %016llx aka %lldM\n", mtrr_tom2, mtrr_tom2>>20);
 }
 
+/* PAT setup for BP. We need to go through sync steps here */
+void __init mtrr_bp_pat_init(void)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+	prepare_set();
+
+	pat_init();
+
+	post_set();
+	local_irq_restore(flags);
+}
+
 /* Grab all of the MTRR state for this CPU into *state */
 bool __init get_mtrr_state(void)
 {
 	struct mtrr_var_range *vrs;
-	unsigned long flags;
 	unsigned lo, dummy;
 	unsigned int i;
 
@@ -481,15 +494,6 @@ bool __init get_mtrr_state(void)
 
 	mtrr_state_set = 1;
 
-	/* PAT setup for BP. We need to go through sync steps here */
-	local_irq_save(flags);
-	prepare_set();
-
-	pat_init();
-
-	post_set();
-	local_irq_restore(flags);
-
 	return !!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED);
 }
 
diff --git a/arch/x86/kernel/cpu/mtrr/main.c b/arch/x86/kernel/cpu/mtrr/main.c
index 5c3d149..d9e91f1 100644
--- a/arch/x86/kernel/cpu/mtrr/main.c
+++ b/arch/x86/kernel/cpu/mtrr/main.c
@@ -752,6 +752,9 @@ void __init mtrr_bp_init(void)
 			/* BIOS may override */
 			__mtrr_enabled = get_mtrr_state();
 
+			if (mtrr_enabled())
+				mtrr_bp_pat_init();
+
 			if (mtrr_cleanup(phys_addr)) {
 				changed_by_mtrr_cleanup = 1;
 				mtrr_if->set_all();
@@ -759,8 +762,16 @@ void __init mtrr_bp_init(void)
 		}
 	}
 
-	if (!mtrr_enabled())
+	if (!mtrr_enabled()) {
 		pr_info("MTRR: Disabled\n");
+
+		/*
+		 * PAT initialization relies on MTRR's rendezvous handler.
+		 * Disable PAT until the handler can initialize both features
+		 * independently.
+		 */
+		pat_disable("PAT disabled by MTRR");
+	}
 }
 
 void mtrr_ap_init(void)
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.h b/arch/x86/kernel/cpu/mtrr/mtrr.h
index 951884d..6c7ced0 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.h
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.h
@@ -52,6 +52,7 @@ void set_mtrr_prepare_save(struct set_mtrr_context *ctxt);
 void fill_mtrr_var_range(unsigned int index,
 		u32 base_lo, u32 base_hi, u32 mask_lo, u32 mask_hi);
 bool get_mtrr_state(void);
+void mtrr_bp_pat_init(void);
 
 extern void set_mtrr_ops(const struct mtrr_ops *ops);
 

[toc] | [prev] | [next] | [standalone]

#1355740 — Re: [PATCH 2/2] x86/mtrr: Refactor PAT initialization code

* 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?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web