[PATCH v2 2/6] x86/mm/pat: Add pat_disable() interface

[PATCH v2 2/6] x86/mm/pat: Add pat_disable() interface

[Toshi Kani]

In preparation to fix a regression caused by 'commit 9cd25aac1f44
("x86/mm/pat: Emulate PAT when it is disabled")', PAT needs to
provide an interface that disables the OS to initialize PAT MSR.

PAT MSR initialization must be done on all CPUs with the specific
sequence of operations defined in Intel SDM.  This requires MTRR
to be enabled since pat_init() is called as part of MTRR init
from mtrr_rendezvous_handler().

Change pat_disable() as the interface to disable the OS to initialize
PAT MSR, and set PAT table with pat_keep_handoff_state().  This
interface can be called when PAT initialization may not be performed.

This also assures that pat_disable() called from pat_bsp_init()
to set PAT table properly when CPU does not support PAT.

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: Robert Elliott <elliott@hpe.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          |   21 ++++++++++++++++++---
 2 files changed, 19 insertions(+), 3 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 e0a34b0..48d1619 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -40,11 +40,26 @@
 static bool boot_cpu_done;
 
 static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT);
+static void pat_keep_handoff_state(void);
 
-static inline void pat_disable(const char *reason)
+/**
+ * pat_disable() - Disable the OS to initialize PAT MSR
+ *
+ * This function disables the OS to initialize PAT MSR, and calls
+ * pat_keep_handoff_state() to set PAT table to the handoff state.
+ */
+void pat_disable(const char *reason)
 {
+	if (boot_cpu_done) {
+		pr_err("x86/PAT: PAT cannot be disabled after initialization "
+		       "(attempting: %s)\n", reason);
+		return;
+	}
+
 	__pat_enabled = 0;
 	pr_info("x86/PAT: %s\n", reason);
+
+	pat_keep_handoff_state();
 }
 
 static int __init nopat(char *str)
@@ -202,7 +217,7 @@ static void pat_bsp_init(u64 pat)
 {
 	u64 tmp_pat;
 
-	if (!cpu_has_pat) {
+	if (!boot_cpu_has(X86_FEATURE_PAT)) {
 		pat_disable("PAT not supported by CPU.");
 		return;
 	}
@@ -220,7 +235,7 @@ static void pat_bsp_init(u64 pat)
 
 static void pat_ap_init(u64 pat)
 {
-	if (!cpu_has_pat) {
+	if (!boot_cpu_has(X86_FEATURE_PAT)) {
 		/*
 		 * If this happens we are on a secondary CPU, but switched to
 		 * PAT on the boot CPU. We have no way to undo PAT.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v2 3/6] x86/mtrr: Fix Xorg crashes in Qemu sessions

[Toshi Kani]

A Xorg failure on qemu32 was reported as a regression caused
by 'commit 9cd25aac1f44 ("x86/mm/pat: Emulate PAT when it is
disabled")'. [1]  This patch fixes the regression.

Negative effects of this regression were two failures in Xorg
on qemu32 env, which were triggered by the fact that its virtual
CPU does not support MTRR. [2]

 #1. copy_process() failed in the check in reserve_pfn_range()

    copy_process
     copy_mm
      dup_mm
       dup_mmap
        copy_page_range
         track_pfn_copy
          reserve_pfn_range

 #2. error path in copy_process() then hit WARN_ON_ONCE in
     untrack_pfn().

     x86/PAT: Xorg:509 map pfn expected mapping type uncached-
     minus for [mem 0xfd000000-0xfdffffff], got write-combining
      Call Trace:
     dump_stack+0x58/0x79
     warn_slowpath_common+0x8b/0xc0
     ? untrack_pfn+0x9f/0xb0
     ? untrack_pfn+0x9f/0xb0
     warn_slowpath_null+0x22/0x30
     untrack_pfn+0x9f/0xb0
     ? __kunmap_atomic+0x54/0x110
     unmap_single_vma+0x56f/0x580
     ? pagevec_move_tail_fn+0xa0/0xa0
     unmap_vmas+0x43/0x60
     exit_mmap+0x5f/0xf0
     mmput+0x2d/0xa0
     copy_process.part.47+0x1229/0x1430
     _do_fork+0xb4/0x3b0
     SyS_clone+0x2c/0x30
     do_syscall_32_irqs_on+0x54/0xb0
     entry_INT80_32+0x2a/0x2a

These negative effects are caused by two separate bugs, but they
can be dealt in lower priority.  Fixing the pat_init() issue below
avoids Xorg to hit these cases.

When the CPU does not support MTRR, MTRR does not call pat_init(),
which leaves PAT enabled without initializing PAT.  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 the 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().

This pat_init() issue existed before the commit, 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 it was designed to work.  When
PAT is set to disable properly, WC is converted to UC.  The use of
WT can result in a system crash if the target range does not support
WT.  Fortunately, nobody ran into such issue before.

To fix this pat_init() issue, PAT code has been enhanced to provide
pat_disable() interface, which disables the OS to initialize PAT MSR,
and sets PAT table to the BIOS handoff state.  This patch changes
MTRR code to call pat_disable() when MTRR is disabled as PAT cannot
be initialized in this case.  This sets PAT to disable properly, and
makes PAT code to bypass the memtype check.  This avoids issue #1
(which can be dealt in lower priority).

[1]: https://lkml.org/lkml/2016/3/3/828
[2]: https://lkml.org/lkml/2016/3/4/775
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/main.c |   10 +++++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index b94f6f6..634c593 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("Skip PAT initialization");
+}
 
 #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/main.c b/arch/x86/kernel/cpu/mtrr/main.c
index 10f8d47..2d7d8d7 100644
--- a/arch/x86/kernel/cpu/mtrr/main.c
+++ b/arch/x86/kernel/cpu/mtrr/main.c
@@ -759,8 +759,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.
+		 * Skip PAT init until the handler can initialize both
+		 * features independently.
+		 */
+		pat_disable("Skip PAT initialization");
+	}
 }
 
 void mtrr_ap_init(void)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v2 4/6] x86/mtrr: Fix PAT init handling when MTRR MSR is disabled

[Toshi Kani]

get_mtrr_state() calls pat_init() on BSP even if MTRR is disabled
by its MSR.  This causes pat_init() to be called on BSP only since
APs do not call pat_init() when MTRR is disabled.  This inconsistency
between BSP and APs leads undefined behavior.

Move BSP's PAT init code from get_mtrr_state() to mtrr_bp_pat_init().
Change mtrr_bp_init() to call mtrr_bp_pat_init() if MTRR is enabled.
This keeps BSP's calling condition to pat_init() consistent with AP's,
mtrr_ap_init() and mtrr_aps_init().

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/kernel/cpu/mtrr/generic.c |   24 ++++++++++++++----------
 arch/x86/kernel/cpu/mtrr/main.c    |    3 +++
 arch/x86/kernel/cpu/mtrr/mtrr.h    |    1 +
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index fcbcb2f..a9d2e54 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 2d7d8d7..6186ff4 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();
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);
 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v2 5/6] x86/xen, pat: Remove PAT table init code from Xen

[Toshi Kani]

Xen supports PAT without MTRR for its guests.  In order to
enable WC attribute, it was necessary for xen_start_kernel()
to call pat_init_cache_modes() to update PAT table before
starting guest kernel.

Now that the kernel initializes PAT table to the BIOS handoff
state when MTRR is disabled, this Xen-specific PAT init code
is no longer necessary.  Delete it from xen_start_kernel().

Also change pat_init_cache_modes() to a static function since
PAT table should not be tweaked by other modules.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.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          |    2 +-
 arch/x86/xen/enlighten.c   |    9 ---------
 3 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/pat.h b/arch/x86/include/asm/pat.h
index 016142b..0b1ff4c 100644
--- a/arch/x86/include/asm/pat.h
+++ b/arch/x86/include/asm/pat.h
@@ -7,7 +7,6 @@
 bool pat_enabled(void);
 void pat_disable(const char *reason);
 extern void pat_init(void);
-void pat_init_cache_modes(u64);
 
 extern int reserve_memtype(u64 start, u64 end,
 		enum page_cache_mode req_pcm, enum page_cache_mode *ret_pcm);
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index 48d1619..02bf0e4 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -196,7 +196,7 @@ static enum page_cache_mode pat_get_cache_mode(unsigned pat_val, char *msg)
  * configuration.
  * Using lower indices is preferred, so we start with highest index.
  */
-void pat_init_cache_modes(u64 pat)
+static void pat_init_cache_modes(u64 pat)
 {
 	enum page_cache_mode cache;
 	char pat_msg[33];
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 2c26108..4d21d69 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -74,7 +74,6 @@
 #include <asm/mach_traps.h>
 #include <asm/mwait.h>
 #include <asm/pci_x86.h>
-#include <asm/pat.h>
 #include <asm/cpu.h>
 
 #ifdef CONFIG_ACPI
@@ -1510,7 +1509,6 @@ asmlinkage __visible void __init xen_start_kernel(void)
 {
 	struct physdev_set_iopl set_iopl;
 	unsigned long initrd_start = 0;
-	u64 pat;
 	int rc;
 
 	if (!xen_start_info)
@@ -1617,13 +1615,6 @@ asmlinkage __visible void __init xen_start_kernel(void)
 				   xen_start_info->nr_pages);
 	xen_reserve_special_pages();
 
-	/*
-	 * Modify the cache mode translation tables to match Xen's PAT
-	 * configuration.
-	 */
-	rdmsrl(MSR_IA32_CR_PAT, pat);
-	pat_init_cache_modes(pat);
-
 	/* keep using Xen gdt for now; no urgent need to change it */
 
 #ifdef CONFIG_X86_32

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v2 6/6] x86/pat: Document PAT initializations

[Toshi Kani]

Update PAT documentation to describe how PAT is initialized under
various configurations.

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>
---
 Documentation/x86/pat.txt |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/Documentation/x86/pat.txt b/Documentation/x86/pat.txt
index 54944c7..f619e1d 100644
--- a/Documentation/x86/pat.txt
+++ b/Documentation/x86/pat.txt
@@ -196,3 +196,35 @@ Another, more verbose way of getting PAT related debug messages is with
 "debugpat" boot parameter. With this parameter, various debug messages are
 printed to dmesg log.
 
+PAT Initialization
+------------------
+
+The following table describes how PAT is initialized under various
+configurations. PAT must be set to enable to initialize PAT MSR in order
+to support WC and WT attributes. Otherwise, PAT keeps PAT MSR value set
+by BIOS. Note, Xen enables WC attribute in BIOS setup for guests.
+
+ MTRR PAT   Call Sequence               PAT State  PAT MSR
+ =========================================================
+ E    E     MTRR -> pat_init()          Enable     OS
+ E    D     MTRR -> pat_init()          Disable    -
+ D    E     MTRR -> pat_disable()       Disable    BIOS
+ D    D     MTRR -> pat_disable()       Disable    -
+ -    np/E  nopat() -> pat_disable()    Disable    BIOS
+ -    np/D  nopat() -> pat_disable()    Disable    -
+ E    !P/E  MTRR -> pat_init()          Disable    BIOS
+ D    !P/E  MTRR -> pat_disable()       Disable    BIOS
+ !M   !P/E  MTRR stub -> pat_disable()  Disable    BIOS
+
+ Legend
+ ------------------------------------------------
+ E        Feature enabled in CPU
+ D	  Feature disabled/unsupported in CPU
+ np	  "nopat" boot option specified
+ !P	  CONFIG_X86_PAT option unset
+ !M	  CONFIG_MTRR option unset
+ Enable   PAT state set to enable
+ Disable  PAT state set to disable
+ OS       PAT initializes PAT MSR with OS setup
+ BIOS     PAT keeps PAT MSR with BIOS setup
+

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 2/6] x86/mm/pat: Add pat_disable() interface

[Borislav Petkov]

On Wed, Mar 16, 2016 at 06:46:55PM -0600, Toshi Kani wrote:
> In preparation to fix a regression caused by 'commit 9cd25aac1f44
> ("x86/mm/pat: Emulate PAT when it is disabled")', PAT needs to
> provide an interface that disables the OS to initialize PAT MSR.

			prevents the OS from initializing the PAT MSR.

> 
> PAT MSR initialization must be done on all CPUs with the specific

s/with/using/

> sequence of operations defined in Intel SDM.  This requires MTRR
				   ^
				  the

s/MTRR/MTRRs/

> to be enabled since pat_init() is called as part of MTRR init
> from mtrr_rendezvous_handler().
> 
> Change pat_disable() as the interface to disable the OS to initialize
> PAT MSR, and set PAT table with pat_keep_handoff_state().  This
> interface can be called when PAT initialization may not be performed.

This paragraph reads funky and I can't really parse what it is trying to
say.

> This also assures that pat_disable() called from pat_bsp_init()
> to set PAT table properly when CPU does not support PAT.
> 
> 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: Robert Elliott <elliott@hpe.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          |   21 ++++++++++++++++++---
>  2 files changed, 19 insertions(+), 3 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 e0a34b0..48d1619 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -40,11 +40,26 @@
>  static bool boot_cpu_done;
>  
>  static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT);
> +static void pat_keep_handoff_state(void);
>  
> -static inline void pat_disable(const char *reason)
> +/**
> + * pat_disable() - Disable the OS to initialize PAT MSR

			^^^^

Err, what? The function name can't be more clear.

> + *
> + * This function disables the OS to initialize PAT MSR, and calls

		    "prevents the OS from initializing the PAT MSR..."

> + * pat_keep_handoff_state() to set PAT table to the handoff state.

We can see what is calls. You're explaining *what* the code does instead
of *why* again.

> + */
> +void pat_disable(const char *reason)
>  {

Why aren't you checking __pat_enabled here?

	if (!__pat_enabled)
		return;

You can save yourself the other guards in that function, especially that
pr_err() below.

> +	if (boot_cpu_done) {
> +		pr_err("x86/PAT: PAT cannot be disabled after initialization "
> +		       "(attempting: %s)\n", reason);

Please integrate checkpatch.pl into your patch creation workflow as it
sometimes has valid complaints:

WARNING: quoted string split across lines
#79: FILE: arch/x86/mm/pat.c:55:
+               pr_err("x86/PAT: PAT cannot be disabled after initialization "
+                      "(attempting: %s)\n", reason);

More to the point: why do we need that pr_err() call? What is that
supposed to tell the user?

I think it is more for the programmer to catch wrong use of
pat_disable() and then it should be WARN_ONCE() or so...

> +		return;
> +	}
> +
>  	__pat_enabled = 0;
>  	pr_info("x86/PAT: %s\n", reason);
> +
> +	pat_keep_handoff_state();
>  }
>  
>  static int __init nopat(char *str)
> @@ -202,7 +217,7 @@ static void pat_bsp_init(u64 pat)
>  {
>  	u64 tmp_pat;
>  
> -	if (!cpu_has_pat) {
> +	if (!boot_cpu_has(X86_FEATURE_PAT)) {
>  		pat_disable("PAT not supported by CPU.");
>  		return;
>  	}
> @@ -220,7 +235,7 @@ static void pat_bsp_init(u64 pat)
>  
>  static void pat_ap_init(u64 pat)
>  {
> -	if (!cpu_has_pat) {
> +	if (!boot_cpu_has(X86_FEATURE_PAT)) {
>  		/*
>  		 * If this happens we are on a secondary CPU, but switched to
>  		 * PAT on the boot CPU. We have no way to undo PAT.

Those last two hunks are unrelated changes and should be a separate
patch.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 3/6] x86/mtrr: Fix Xorg crashes in Qemu sessions

[Borislav Petkov]

On Wed, Mar 16, 2016 at 06:46:56PM -0600, Toshi Kani wrote:
> A Xorg failure on qemu32 was reported as a regression caused
> by 'commit 9cd25aac1f44 ("x86/mm/pat: Emulate PAT when it is
> disabled")'. [1]  This patch fixes the regression.

I hope so.

> Negative effects of this regression were two failures in Xorg
> on qemu32 env, which were triggered by the fact that its virtual

"... with QEMU CPU model "qemu32" (-cpu qemu32) ... "

> CPU does not support MTRR. [2]
> 
>  #1. copy_process() failed in the check in reserve_pfn_range()
> 
>     copy_process
>      copy_mm
>       dup_mm
>        dup_mmap
>         copy_page_range
>          track_pfn_copy
>           reserve_pfn_range

Here's where you describe why it failed the check and which check.

> 
>  #2. error path in copy_process() then hit WARN_ON_ONCE in
>      untrack_pfn().
> 
>      x86/PAT: Xorg:509 map pfn expected mapping type uncached-
>      minus for [mem 0xfd000000-0xfdffffff], got write-combining
>       Call Trace:
>      dump_stack+0x58/0x79
>      warn_slowpath_common+0x8b/0xc0
>      ? untrack_pfn+0x9f/0xb0
>      ? untrack_pfn+0x9f/0xb0
>      warn_slowpath_null+0x22/0x30
>      untrack_pfn+0x9f/0xb0
>      ? __kunmap_atomic+0x54/0x110
>      unmap_single_vma+0x56f/0x580
>      ? pagevec_move_tail_fn+0xa0/0xa0
>      unmap_vmas+0x43/0x60
>      exit_mmap+0x5f/0xf0
>      mmput+0x2d/0xa0
>      copy_process.part.47+0x1229/0x1430
>      _do_fork+0xb4/0x3b0
>      SyS_clone+0x2c/0x30
>      do_syscall_32_irqs_on+0x54/0xb0
>      entry_INT80_32+0x2a/0x2a

You can delete the offsets after the "+" - they're useless.

> These negative effects are caused by two separate bugs, but they
> can be dealt in lower priority.

??

> Fixing the pat_init() issue below
> avoids Xorg to hit these cases.
> 
> When the CPU does not support MTRR, MTRR does not call pat_init(),
> which leaves PAT enabled without initializing PAT.  This pat_init()
> issue is a long-standing issue, but manifested as issue #1 (and then
> hit issue #2) with the commit

commit 9cd25aac1f44 ?

> 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 the 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().

Good.

> This pat_init() issue existed before the commit, 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 it was designed to work.  When
> PAT is set to disable properly, WC is converted to UC.  The use of
> WT can result in a system crash if the target range does not support
> WT.  Fortunately, nobody ran into such issue before.
> 
> To fix this pat_init() issue, PAT code has been enhanced to provide
> pat_disable() interface, which disables the OS to initialize PAT MSR,

				... prevents the OS from initializing the PAT MSR.

> and sets PAT table to the BIOS handoff state.

> This patch changes
> MTRR code to call pat_disable() when MTRR is disabled as PAT cannot
> be initialized in this case.  This sets PAT to disable properly, and
> makes PAT code to bypass the memtype check.  This avoids issue #1
> (which can be dealt in lower priority).

You don't need all that text from "This patch ..." on - we can see that
in the diff. The commit message needs to contain "why" not "what".

> 
> [1]: https://lkml.org/lkml/2016/3/3/828
> [2]: https://lkml.org/lkml/2016/3/4/775

I believe I already mentioned that links should be supplied like this:

Link: http://lkml.kernel.org/r/<Message-ID>

> 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/main.c |   10 +++++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
> index b94f6f6..634c593 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("Skip PAT initialization");

Make that more user-friendly:

		   "MTRRs disabled, skipping PAT initialization too."

> +}
>  
>  #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/main.c b/arch/x86/kernel/cpu/mtrr/main.c
> index 10f8d47..2d7d8d7 100644
> --- a/arch/x86/kernel/cpu/mtrr/main.c
> +++ b/arch/x86/kernel/cpu/mtrr/main.c
> @@ -759,8 +759,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.
> +		 * Skip PAT init until the handler can initialize both
> +		 * features independently.
> +		 */
> +		pat_disable("Skip PAT initialization");

Ditto: you can merge the pr_info text with the pat_disable() string.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 4/6] x86/mtrr: Fix PAT init handling when MTRR MSR is disabled

[Borislav Petkov]

Subject: [PATCH v2 4/6] x86/mtrr: Fix PAT init handling when MTRR MSR is disabled

s/ MSR//

On Wed, Mar 16, 2016 at 06:46:57PM -0600, Toshi Kani wrote:
> get_mtrr_state() calls pat_init() on BSP even if MTRR is disabled
> by its MSR.

s/by its MSR//

> This causes pat_init() to be called on BSP only since APs do not call

This doesn't cause that - get_mtrr_state() is called only on the BSP by
mtrr_bp_init().

> pat_init() when MTRR is disabled. This inconsistency between BSP and
> APs leads undefined behavior.

      leads to


> Move BSP's PAT init code from get_mtrr_state() to mtrr_bp_pat_init().
> Change mtrr_bp_init() to call mtrr_bp_pat_init() if MTRR is enabled.

No need for those.

> This keeps BSP's calling condition to pat_init() consistent with AP's,
> mtrr_ap_init() and mtrr_aps_init().

This one is fine.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 5/6] x86/xen, pat: Remove PAT table init code from Xen

[Borislav Petkov]

On Wed, Mar 16, 2016 at 06:46:58PM -0600, Toshi Kani wrote:
> Xen supports PAT without MTRR for its guests.  In order to
> enable WC attribute, it was necessary for xen_start_kernel()
> to call pat_init_cache_modes() to update PAT table before
> starting guest kernel.
> 
> Now that the kernel initializes PAT table to the BIOS handoff
> state when MTRR is disabled, this Xen-specific PAT init code
> is no longer necessary.  Delete it from xen_start_kernel().
> 
> Also change pat_init_cache_modes() to a static function since
> PAT table should not be tweaked by other modules.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.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          |    2 +-
>  arch/x86/xen/enlighten.c   |    9 ---------
>  3 files changed, 1 insertion(+), 11 deletions(-)

Jürgen, ack?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 6/6] x86/pat: Document PAT initializations

[Borislav Petkov]

On Wed, Mar 16, 2016 at 06:46:59PM -0600, Toshi Kani wrote:
> Update PAT documentation to describe how PAT is initialized under
> various configurations.
> 
> 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>
> ---
>  Documentation/x86/pat.txt |   32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/Documentation/x86/pat.txt b/Documentation/x86/pat.txt
> index 54944c7..f619e1d 100644
> --- a/Documentation/x86/pat.txt
> +++ b/Documentation/x86/pat.txt
> @@ -196,3 +196,35 @@ Another, more verbose way of getting PAT related debug messages is with
>  "debugpat" boot parameter. With this parameter, various debug messages are
>  printed to dmesg log.
>  
> +PAT Initialization
> +------------------
> +
> +The following table describes how PAT is initialized under various
> +configurations. PAT must be set to enable to initialize PAT MSR in order

Err "PAT MSR must be updated by Linux in order to support WC and WT" ... or so?

> +to support WC and WT attributes. Otherwise, PAT keeps PAT MSR value set
> +by BIOS.

"Otherwise, the PAT MSR has the value programmed in it by the firmware."

> Note, Xen enables WC attribute in BIOS setup for guests.
> +
> + MTRR PAT   Call Sequence               PAT State  PAT MSR
> + =========================================================
> + E    E     MTRR -> pat_init()          Enable     OS

s/Enable/Enabled/

MTRR->pat_init() - either use function names for both or do pseudo like
so:

MTRR init -> PAT init

> + E    D     MTRR -> pat_init()          Disable    -

s/Disable/Disabled/. Ditto for the rest.

> + D    E     MTRR -> pat_disable()       Disable    BIOS
> + D    D     MTRR -> pat_disable()       Disable    -
> + -    np/E  nopat() -> pat_disable()    Disable    BIOS
> + -    np/D  nopat() -> pat_disable()    Disable    -
> + E    !P/E  MTRR -> pat_init()          Disable    BIOS
> + D    !P/E  MTRR -> pat_disable()       Disable    BIOS
> + !M   !P/E  MTRR stub -> pat_disable()  Disable    BIOS
> +
> + Legend
> + ------------------------------------------------
> + E        Feature enabled in CPU
> + D	  Feature disabled/unsupported in CPU
> + np	  "nopat" boot option specified
> + !P	  CONFIG_X86_PAT option unset
> + !M	  CONFIG_MTRR option unset
> + Enable   PAT state set to enable
> + Disable  PAT state set to disable
> + OS       PAT initializes PAT MSR with OS setup
> + BIOS     PAT keeps PAT MSR with BIOS setup
> +

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 2/6] x86/mm/pat: Add pat_disable() interface

[Toshi Kani]

On Tue, 2016-03-22 at 17:59 +0100, Borislav Petkov wrote:
> On Wed, Mar 16, 2016 at 06:46:55PM -0600, Toshi Kani wrote:
> > In preparation to fix a regression caused by 'commit 9cd25aac1f44
> > ("x86/mm/pat: Emulate PAT when it is disabled")', PAT needs to
> > provide an interface that disables the OS to initialize PAT MSR.
> 
> 			prevents the OS from initializing the PAT MSR.

Right. Will do.

> > 
> > PAT MSR initialization must be done on all CPUs with the specific
> 
> s/with/using/

Ditto.

> > sequence of operations defined in Intel SDM.  This requires MTRR
> 				   ^
> 				  the
> 
> s/MTRR/MTRRs/

Ditto.

> > to be enabled since pat_init() is called as part of MTRR init
> > from mtrr_rendezvous_handler().
> > 
> > Change pat_disable() as the interface to disable the OS to initialize
> > PAT MSR, and set PAT table with pat_keep_handoff_state().  This
> > interface can be called when PAT initialization may not be performed.
> 
> This paragraph reads funky and I can't really parse what it is trying to
> say.

Sorry... Here is a retry:

Make pat_disable() as the interface that prevents the OS from initializing
the PAT MSR.  MTRR will call this interface when it cannot provide the SDM-
defined sequence to initialize PAT.

> > This also assures that pat_disable() called from pat_bsp_init()
> > to set PAT table properly when CPU does not support PAT.
> > 
 :
> >  
> > -static inline void pat_disable(const char *reason)
> > +/**
> > + * pat_disable() - Disable the OS to initialize PAT MSR
> 
> 			^^^^
> 
> Err, what? The function name can't be more clear.

Will change to "Prevent the OS from initializing the PAT MSR".

I wanted to clarify that "disable" does not mean to disable PAT MSR.

> > + *
> > + * This function disables the OS to initialize PAT MSR, and calls
> 
> 		    "prevents the OS from initializing the PAT MSR..."

Will do.

> > + * pat_keep_handoff_state() to set PAT table to the handoff state.
> 
> We can see what is calls. You're explaining *what* the code does instead
> of *why* again.

Right...

> > + */
> > +void pat_disable(const char *reason)
> >  {
> 
> Why aren't you checking __pat_enabled here?
> 
> 	if (!__pat_enabled)
> 		return;

pat_keep_handoff_state() is a no-op after the initial call, but I agree
that having this check is better.  Will do.

> You can save yourself the other guards in that function, especially that
> pr_err() below.

The pr_err() below is for a difference case -- PAT is enabled, but a call
is made to disable it after pat_init() is called.  We cannot allow this
case.

> > +	if (boot_cpu_done) {
> > +		pr_err("x86/PAT: PAT cannot be disabled after
> > initialization "
> > +		       "(attempting: %s)\n", reason);
> 
> Please integrate checkpatch.pl into your patch creation workflow as it
> sometimes has valid complaints:
> 
> WARNING: quoted string split across lines
> #79: FILE: arch/x86/mm/pat.c:55:
> +               pr_err("x86/PAT: PAT cannot be disabled after
> initialization "
> +                      "(attempting: %s)\n", reason);

I've run checkpatch.pl and thought it was OK to have this warning (instead
of a >80 warning) since the error message part was not split.  The
"attempting" part is for debugging and its string is passed from the
caller. 

> More to the point: why do we need that pr_err() call? What is that
> supposed to tell the user?
> 
> I think it is more for the programmer to catch wrong use of
> pat_disable() and then it should be WARN_ONCE() or so...

Yes, this case is for the programmer to catch wrong use.  I will change it
to use WARN_ONCE() and remove the "(attempting: %s)\n" part of the message.

> > +		return;
> > +	}
> > +
> >  	__pat_enabled = 0;
> >  	pr_info("x86/PAT: %s\n", reason);
> > +
> > +	pat_keep_handoff_state();
> >  }
> >  
> >  static int __init nopat(char *str)
> > @@ -202,7 +217,7 @@ static void pat_bsp_init(u64 pat)
> >  {
> >  	u64 tmp_pat;
> >  
> > -	if (!cpu_has_pat) {
> > +	if (!boot_cpu_has(X86_FEATURE_PAT)) {
> >  		pat_disable("PAT not supported by CPU.");
> >  		return;
> >  	}
> > @@ -220,7 +235,7 @@ static void pat_bsp_init(u64 pat)
> >  
> >  static void pat_ap_init(u64 pat)
> >  {
> > -	if (!cpu_has_pat) {
> > +	if (!boot_cpu_has(X86_FEATURE_PAT)) {
> >  		/*
> >  		 * If this happens we are on a secondary CPU, but
> > switched to
> >  		 * PAT on the boot CPU. We have no way to undo PAT.
> 
> Those last two hunks are unrelated changes and should be a separate
> patch.

Will do.

Thanks,
-Toshi

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 3/6] x86/mtrr: Fix Xorg crashes in Qemu sessions

[Toshi Kani]

On Tue, 2016-03-22 at 18:00 +0100, Borislav Petkov wrote:
> On Wed, Mar 16, 2016 at 06:46:56PM -0600, Toshi Kani wrote:
> > A Xorg failure on qemu32 was reported as a regression caused
> > by 'commit 9cd25aac1f44 ("x86/mm/pat: Emulate PAT when it is
> > disabled")'. [1]  This patch fixes the regression.
> 
> I hope so.
> 
> > Negative effects of this regression were two failures in Xorg
> > on qemu32 env, which were triggered by the fact that its virtual
> 
> "... with QEMU CPU model "qemu32" (-cpu qemu32) ... "

Will add this description.

> > CPU does not support MTRR. [2]
> > 
> >  #1. copy_process() failed in the check in reserve_pfn_range()
> > 
> >     copy_process
> >      copy_mm
> >       dup_mm
> >        dup_mmap
> >         copy_page_range
> >          track_pfn_copy
> >           reserve_pfn_range
> 
> Here's where you describe why it failed the check and which check.

Will do.

> >  #2. error path in copy_process() then hit WARN_ON_ONCE in
> >      untrack_pfn().
> > 
> >      x86/PAT: Xorg:509 map pfn expected mapping type uncached-
> >      minus for [mem 0xfd000000-0xfdffffff], got write-combining
> >       Call Trace:
> >      dump_stack+0x58/0x79
> >      warn_slowpath_common+0x8b/0xc0
> >      ? untrack_pfn+0x9f/0xb0
> >      ? untrack_pfn+0x9f/0xb0
> >      warn_slowpath_null+0x22/0x30
> >      untrack_pfn+0x9f/0xb0
> >      ? __kunmap_atomic+0x54/0x110
> >      unmap_single_vma+0x56f/0x580
> >      ? pagevec_move_tail_fn+0xa0/0xa0
> >      unmap_vmas+0x43/0x60
> >      exit_mmap+0x5f/0xf0
> >      mmput+0x2d/0xa0
> >      copy_process.part.47+0x1229/0x1430
> >      _do_fork+0xb4/0x3b0
> >      SyS_clone+0x2c/0x30
> >      do_syscall_32_irqs_on+0x54/0xb0
> >      entry_INT80_32+0x2a/0x2a
> 
> You can delete the offsets after the "+" - they're useless.

Will do.

> > These negative effects are caused by two separate bugs, but they
> > can be dealt in lower priority.
> 
> ??

Will change to "they can be addressed in separate patches."

> > Fixing the pat_init() issue below
> > avoids Xorg to hit these cases.
> > 
> > When the CPU does not support MTRR, MTRR does not call pat_init(),
> > which leaves PAT enabled without initializing PAT.  This pat_init()
> > issue is a long-standing issue, but manifested as issue #1 (and then
> > hit issue #2) with the commit
> 
> commit 9cd25aac1f44 ?

Yes. I had to remove this number since checkpatch complained that I needed
to quote the whole patch tile again.  I will ignore this checkpatch error
and add this commit number here.

> > 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 the 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().
> 
> Good.
> 
> > This pat_init() issue existed before the commit, 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 it was designed to work.  When
> > PAT is set to disable properly, WC is converted to UC.  The use of
> > WT can result in a system crash if the target range does not support
> > WT.  Fortunately, nobody ran into such issue before.
> > 
> > To fix this pat_init() issue, PAT code has been enhanced to provide
> > pat_disable() interface, which disables the OS to initialize PAT MSR,
> 
> 				... prevents the OS from initializing the
> PAT MSR.

Will do.

> > and sets PAT table to the BIOS handoff state.
> 
> > This patch changes
> > MTRR code to call pat_disable() when MTRR is disabled as PAT cannot
> > be initialized in this case.  This sets PAT to disable properly, and
> > makes PAT code to bypass the memtype check.  This avoids issue #1
> > (which can be dealt in lower priority).
> 
> You don't need all that text from "This patch ..." on - we can see that
> in the diff. The commit message needs to contain "why" not "what".

OK.

> >  
> >  /*
> > @@ -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("Skip PAT initialization");
> 
> Make that more user-friendly:
> 
> 		   "MTRRs disabled, skipping PAT initialization too."

Agreed. Will do.

> > +}
> >  
> >  #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/main.c
> > b/arch/x86/kernel/cpu/mtrr/main.c
> > index 10f8d47..2d7d8d7 100644
> > --- a/arch/x86/kernel/cpu/mtrr/main.c
> > +++ b/arch/x86/kernel/cpu/mtrr/main.c
> > @@ -759,8 +759,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.
> > +		 * Skip PAT init until the handler can initialize both
> > +		 * features independently.
> > +		 */
> > +		pat_disable("Skip PAT initialization");
> 
> Ditto: you can merge the pr_info text with the pat_disable() string.

Will do.

Thanks,
-Toshi

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 4/6] x86/mtrr: Fix PAT init handling when MTRR MSR is disabled

[Toshi Kani]

On Tue, 2016-03-22 at 18:01 +0100, Borislav Petkov wrote:
> Subject: [PATCH v2 4/6] x86/mtrr: Fix PAT init handling when MTRR MSR is
> disabled
> 
> s/ MSR//

Will do.

> On Wed, Mar 16, 2016 at 06:46:57PM -0600, Toshi Kani wrote:
> > get_mtrr_state() calls pat_init() on BSP even if MTRR is disabled
> > by its MSR.
> 
> s/by its MSR//

Will do.

> > This causes pat_init() to be called on BSP only since APs do not call
> 
> This doesn't cause that - get_mtrr_state() is called only on the BSP by
> mtrr_bp_init().

Right, I will change it to "results" or something.

> > pat_init() when MTRR is disabled. This inconsistency between BSP and
> > APs leads undefined behavior.
> 
>       leads to

Will do.

> > Move BSP's PAT init code from get_mtrr_state() to mtrr_bp_pat_init().
> > Change mtrr_bp_init() to call mtrr_bp_pat_init() if MTRR is enabled.
> 
> No need for those.

OK.

> > This keeps BSP's calling condition to pat_init() consistent with AP's,
> > mtrr_ap_init() and mtrr_aps_init().
> 
> This one is fine.

:-)

Thanks,
-Toshi

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 6/6] x86/pat: Document PAT initializations

[Toshi Kani]

On Tue, 2016-03-22 at 18:02 +0100, Borislav Petkov wrote:
> On Wed, Mar 16, 2016 at 06:46:59PM -0600, Toshi Kani wrote:
> > Update PAT documentation to describe how PAT is initialized under
> > various configurations.
> > 
 :
> >  
> > +PAT Initialization
> > +------------------
> > +
> > +The following table describes how PAT is initialized under various
> > +configurations. PAT must be set to enable to initialize PAT MSR in
> > order
> 
> Err "PAT MSR must be updated by Linux in order to support WC and WT" ...
> or so?

Right. Will do.

> > +to support WC and WT attributes. Otherwise, PAT keeps PAT MSR value
> > set
> > +by BIOS.
> 
> "Otherwise, the PAT MSR has the value programmed in it by the firmware."

Will do.

> > Note, Xen enables WC attribute in BIOS setup for guests.
> > +
> > + MTRR PAT   Call Sequence               PAT State  PAT MSR
> > + =========================================================
> > + E    E     MTRR -> pat_init()          Enable     OS
> 
> s/Enable/Enabled/

Will do.

> MTRR->pat_init() - either use function names for both or do pseudo like
> so:
> 
> MTRR init -> PAT init

OK, I will change all to pseudo.  MTRR has multiple caller functions, and
we do not have enough space to write them all.

> > + E    D     MTRR -> pat_init()          Disable    -
> 
> s/Disable/Disabled/. Ditto for the rest.

Will do.

> > + D    E     MTRR -> pat_disable()       Disable    BIOS
> > + D    D     MTRR -> pat_disable()       Disable    -
> > + -    np/E  nopat() -> pat_disable()    Disable    BIOS
> > + -    np/D  nopat() -> pat_disable()    Disable    -
> > + E    !P/E  MTRR -> pat_init()          Disable    BIOS
> > + D    !P/E  MTRR -> pat_disable()       Disable    BIOS
> > + !M   !P/E  MTRR stub -> pat_disable()  Disable    BIOS
> > +
> > + Legend
> > + ------------------------------------------------
> > + E        Feature enabled in CPU
> > + D	  Feature disabled/unsupported in CPU
> > + np	  "nopat" boot option specified
> > + !P	  CONFIG_X86_PAT option unset
> > + !M	  CONFIG_MTRR option unset
> > + Enable   PAT state set to enable
> > + Disable  PAT state set to disable
> > + OS       PAT initializes PAT MSR with OS setup
> > + BIOS     PAT keeps PAT MSR with BIOS setup
> > +

Thanks,
-Toshi

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 5/6] x86/xen, pat: Remove PAT table init code from Xen

[Juergen Gross]

On 22/03/16 18:02, Borislav Petkov wrote:
> On Wed, Mar 16, 2016 at 06:46:58PM -0600, Toshi Kani wrote:
>> Xen supports PAT without MTRR for its guests.  In order to
>> enable WC attribute, it was necessary for xen_start_kernel()
>> to call pat_init_cache_modes() to update PAT table before
>> starting guest kernel.
>>
>> Now that the kernel initializes PAT table to the BIOS handoff
>> state when MTRR is disabled, this Xen-specific PAT init code
>> is no longer necessary.  Delete it from xen_start_kernel().
>>
>> Also change pat_init_cache_modes() to a static function since
>> PAT table should not be tweaked by other modules.
>>
>> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.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          |    2 +-
>>  arch/x86/xen/enlighten.c   |    9 ---------
>>  3 files changed, 1 insertion(+), 11 deletions(-)
> 
> Jürgen, ack?

Yes, seems to be okay.

Acked-by: Juergen Gross <jgross@suse.com>


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 3/6] x86/mtrr: Fix Xorg crashes in Qemu sessions

[Borislav Petkov]

On Tue, Mar 22, 2016 at 03:53:30PM -0600, Toshi Kani wrote:
> Yes. I had to remove this number since checkpatch complained that I needed
> to quote the whole patch tile again.  I will ignore this checkpatch error
> and add this commit number here.

Actually, checkpatch is right. We do quote the commit IDs *together*
with their names so that the reader knows which commit the text is
talking about.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 2/6] x86/mm/pat: Add pat_disable() interface

[Borislav Petkov]

On Tue, Mar 22, 2016 at 03:40:45PM -0600, Toshi Kani wrote:
> Will change to "Prevent the OS from initializing the PAT MSR".
> 
> I wanted to clarify that "disable" does not mean to disable PAT MSR.

How do you "disable PAT MSR" ?

I think you're overdocumenting this. pat_disable() is as clear as day
what it does. It doesn't need any commenting...

> I've run checkpatch.pl and thought it was OK to have this warning (instead
> of a >80 warning) since the error message part was not split.  The
> "attempting" part is for debugging and its string is passed from the
> caller. 

We always put the quoted strings on a single line for easier grepping.
Forget the 80-cols rule.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 2/6] x86/mm/pat: Add pat_disable() interface

[Toshi Kani]

On Wed, 2016-03-23 at 09:51 +0100, Borislav Petkov wrote:
> On Tue, Mar 22, 2016 at 03:40:45PM -0600, Toshi Kani wrote:
> > Will change to "Prevent the OS from initializing the PAT MSR".
> > 
> > I wanted to clarify that "disable" does not mean to disable PAT MSR.
> 
> How do you "disable PAT MSR" ?

We can't, but I thought not everyone knows how it works...

> I think you're overdocumenting this. pat_disable() is as clear as day
> what it does. It doesn't need any commenting...

Right, maybe I am just paranoid.  I will remove the comment as you
suggested.

> > I've run checkpatch.pl and thought it was OK to have this warning
> > (instead of a >80 warning) since the error message part was not split.
> >  The "attempting" part is for debugging and its string is passed from
> > the caller. 
> 
> We always put the quoted strings on a single line for easier grepping.
> Forget the 80-cols rule.

OK.

Thanks,
-Toshi

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 3/6] x86/mtrr: Fix Xorg crashes in Qemu sessions

[Toshi Kani]

On Wed, 2016-03-23 at 09:44 +0100, Borislav Petkov wrote:
> On Tue, Mar 22, 2016 at 03:53:30PM -0600, Toshi Kani wrote:
> > Yes. I had to remove this number since checkpatch complained that I
> > needed to quote the whole patch tile again.  I will ignore this
> > checkpatch error and add this commit number here.
> 
> Actually, checkpatch is right. We do quote the commit IDs *together*
> with their names so that the reader knows which commit the text is
> talking about.

OK, I will use [1] to refer this patch.  This patch is fully quoted at the
top of this changelog, and it'd be verbose to repeat this full quote every
time I refers it...

Thanks,
-Toshi

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 3/6] x86/mtrr: Fix Xorg crashes in Qemu sessions

[Toshi Kani]

On Wed, 2016-03-23 at 09:53 -0600, Toshi Kani wrote:
> On Wed, 2016-03-23 at 09:44 +0100, Borislav Petkov wrote:
> > On Tue, Mar 22, 2016 at 03:53:30PM -0600, Toshi Kani wrote:
> > > Yes. I had to remove this number since checkpatch complained that I
> > > needed to quote the whole patch tile again.  I will ignore this
> > > checkpatch error and add this commit number here.
> > 
> > Actually, checkpatch is right. We do quote the commit IDs *together*
> > with their names so that the reader knows which commit the text is
> > talking about.
> 
> OK, I will use [1] to refer this patch.  This patch is fully quoted at
> the top of this changelog, and it'd be verbose to repeat this full quote
> every time I refers it...

I ended up with using "the above-mentioned patch" in v3.

Thanks,
-Toshi

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel