Re: [PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-07-04 Thread Thomas Gleixner 
On Tue, 4 Jul 2017, Thomas Gleixner wrote:
>   wrmsrl(MSR_IA32_CR_PAT, pat);
> + __pat_initialized = true;

That should be

> + pat_initialized = true;

of course.

Re: [PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-07-04 Thread Thomas Gleixner 
On Tue, 4 Jul 2017, Thomas Gleixner wrote:
>   wrmsrl(MSR_IA32_CR_PAT, pat);
> + __pat_initialized = true;

That should be

> + pat_initialized = true;

of course.

Re: [PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-07-04 Thread Thomas Gleixner 
On Mon, 3 Jul 2017, Mikulas Patocka wrote:
> Is there any progress with this patch? Will you accept it or do you want 
> some changes to it?

Aside of the unparseable changelog, that patch is mostly duct tape.

1) __pat_enabled should be renamed to pat_disabled, as that is the actual
   purpose of that variable

2) Making the call to init_cache_modes() conditional in setup_arch() is
   pointless. init_cache_modes() has it's own protection against multiple
   invocations.

3) It adds yet another invocation of init_cache_modes() instead of getting
   rid of the ones in pat_disable() and the pat disabled case in pat_init().

I've reworked the whole thing into the patch below.

Thanks,

tglx

8<-
Subject: x86/mm/pat: Don't report PAT on CPUs that don't support it
From: Mikulas Patocka 
Date: Tue, 6 Jun 2017 18:49:39 -0400 (EDT)

The pat_enabled() logic is broken on CPUs which do not support PAT and
where the initialization code fails to call pat_init(). Due to that the
enabled flag stays true and pat_enabled() returns true wrongfully.

As a consequence the mappings, e.g. for Xorg, are set up with the wrong
caching mode and the required MTRR setups are omitted.

To cure this the following changes are required:

  1) Make pat_enabled() return true only if PAT initialization was
 invoked and successful.

  2) Invoke init_cache_modes() unconditionally in setup_arch() and
 remove the extra callsites in pat_disable() and the pat disabled
 code path in pat_init().

Also rename __pat_enabled to pat_disabled to reflect the real purpose of
this variable.

Signed-off-by: Mikulas Patocka 
Cc: Bernhard Held 
Cc: Toshi Kani 
Cc: Peter Zijlstra 
Cc: Brian Gerst 
Cc: "Luis R. Rodriguez" 
Cc: Borislav Petkov 
Cc: Andy Lutomirski 
Cc: Josh Poimboeuf 
Cc: Denys Vlasenko 
Cc: Andrew Morton 
Cc: Linus Torvalds 

---
 arch/x86/include/asm/pat.h |1 +
 arch/x86/kernel/setup.c|7 +++
 arch/x86/mm/pat.c  |   22 +-
 3 files changed, 17 insertions(+), 13 deletions(-)

--- a/arch/x86/include/asm/pat.h
+++ b/arch/x86/include/asm/pat.h
@@ -7,6 +7,7 @@
 bool pat_enabled(void);
 void pat_disable(const char *reason);
 extern void pat_init(void);
+extern void init_cache_modes(void);
 
 extern int reserve_memtype(u64 start, u64 end,
enum page_cache_mode req_pcm, enum page_cache_mode *ret_pcm);
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1076,6 +1076,13 @@ void __init setup_arch(char **cmdline_p)
max_possible_pfn = max_pfn;
 
/*
+* This call is required when the CPU does not support PAT. If
+* mtrr_bp_init() invoked it already via pat_init() the call has no
+* effect.
+*/
+   init_cache_modes();
+
+   /*
 * Define random base addresses for memory sections after max_pfn is
 * defined and before each memory section base is used.
 */
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -37,14 +37,13 @@
 #undef pr_fmt
 #define pr_fmt(fmt) "" fmt
 
-static bool boot_cpu_done;
-
-static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT);
-static void init_cache_modes(void);
+static bool __read_mostly boot_cpu_done;
+static bool __read_mostly pat_disabled = !IS_ENABLED(CONFIG_X86_PAT);
+static bool __read_mostly pat_initialized;
 
 void pat_disable(const char *reason)
 {
-   if (!__pat_enabled)
+   if (pat_disabled)
return;
 
if (boot_cpu_done) {
@@ -52,10 +51,8 @@ void pat_disable(const char *reason)
return;
}
 
-   __pat_enabled = 0;
+   pat_disabled = true;
pr_info("x86/PAT: %s\n", reason);
-
-   init_cache_modes();
 }
 
 static int __init nopat(char *str)
@@ -67,7 +64,7 @@ early_param("nopat", nopat);
 
 bool pat_enabled(void)
 {
-   return !!__pat_enabled;
+   return pat_initialized;
 }
 EXPORT_SYMBOL_GPL(pat_enabled);
 
@@ -225,6 +222,7 @@ static void pat_bsp_init(u64 pat)
}
 
wrmsrl(MSR_IA32_CR_PAT, pat);
+   __pat_initialized = true;
 
__init_cache_modes(pat);
 }
@@ -242,7 +240,7 @@ static void pat_ap_init(u64 pat)
wrmsrl(MSR_IA32_CR_PAT, pat);
 }
 
-static void init_cache_modes(void)
+void init_cache_modes(void)
 {
u64 pat = 0;
static int init_cm_done;
@@ -306,10 +304,8 @@ void pat_init(void)
u64 pat;
struct cpuinfo_x86 *c = _cpu_data;
 
-   if (!pat_enabled()) {
-   init_cache_modes();
+   if (pat_disabled)
return;
-   }
 
if ((c->x86_vendor == X86_VENDOR_INTEL) &&
(((c->x86 == 0x6) && (c->x86_model <= 0xd)) ||

Re: [PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-07-04 Thread Thomas Gleixner 
On Mon, 3 Jul 2017, Mikulas Patocka wrote:
> Is there any progress with this patch? Will you accept it or do you want 
> some changes to it?

Aside of the unparseable changelog, that patch is mostly duct tape.

1) __pat_enabled should be renamed to pat_disabled, as that is the actual
   purpose of that variable

2) Making the call to init_cache_modes() conditional in setup_arch() is
   pointless. init_cache_modes() has it's own protection against multiple
   invocations.

3) It adds yet another invocation of init_cache_modes() instead of getting
   rid of the ones in pat_disable() and the pat disabled case in pat_init().

I've reworked the whole thing into the patch below.

Thanks,

tglx

8<-
Subject: x86/mm/pat: Don't report PAT on CPUs that don't support it
From: Mikulas Patocka 
Date: Tue, 6 Jun 2017 18:49:39 -0400 (EDT)

The pat_enabled() logic is broken on CPUs which do not support PAT and
where the initialization code fails to call pat_init(). Due to that the
enabled flag stays true and pat_enabled() returns true wrongfully.

As a consequence the mappings, e.g. for Xorg, are set up with the wrong
caching mode and the required MTRR setups are omitted.

To cure this the following changes are required:

  1) Make pat_enabled() return true only if PAT initialization was
 invoked and successful.

  2) Invoke init_cache_modes() unconditionally in setup_arch() and
 remove the extra callsites in pat_disable() and the pat disabled
 code path in pat_init().

Also rename __pat_enabled to pat_disabled to reflect the real purpose of
this variable.

Signed-off-by: Mikulas Patocka 
Cc: Bernhard Held 
Cc: Toshi Kani 
Cc: Peter Zijlstra 
Cc: Brian Gerst 
Cc: "Luis R. Rodriguez" 
Cc: Borislav Petkov 
Cc: Andy Lutomirski 
Cc: Josh Poimboeuf 
Cc: Denys Vlasenko 
Cc: Andrew Morton 
Cc: Linus Torvalds 

---
 arch/x86/include/asm/pat.h |1 +
 arch/x86/kernel/setup.c|7 +++
 arch/x86/mm/pat.c  |   22 +-
 3 files changed, 17 insertions(+), 13 deletions(-)

--- a/arch/x86/include/asm/pat.h
+++ b/arch/x86/include/asm/pat.h
@@ -7,6 +7,7 @@
 bool pat_enabled(void);
 void pat_disable(const char *reason);
 extern void pat_init(void);
+extern void init_cache_modes(void);
 
 extern int reserve_memtype(u64 start, u64 end,
enum page_cache_mode req_pcm, enum page_cache_mode *ret_pcm);
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1076,6 +1076,13 @@ void __init setup_arch(char **cmdline_p)
max_possible_pfn = max_pfn;
 
/*
+* This call is required when the CPU does not support PAT. If
+* mtrr_bp_init() invoked it already via pat_init() the call has no
+* effect.
+*/
+   init_cache_modes();
+
+   /*
 * Define random base addresses for memory sections after max_pfn is
 * defined and before each memory section base is used.
 */
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -37,14 +37,13 @@
 #undef pr_fmt
 #define pr_fmt(fmt) "" fmt
 
-static bool boot_cpu_done;
-
-static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT);
-static void init_cache_modes(void);
+static bool __read_mostly boot_cpu_done;
+static bool __read_mostly pat_disabled = !IS_ENABLED(CONFIG_X86_PAT);
+static bool __read_mostly pat_initialized;
 
 void pat_disable(const char *reason)
 {
-   if (!__pat_enabled)
+   if (pat_disabled)
return;
 
if (boot_cpu_done) {
@@ -52,10 +51,8 @@ void pat_disable(const char *reason)
return;
}
 
-   __pat_enabled = 0;
+   pat_disabled = true;
pr_info("x86/PAT: %s\n", reason);
-
-   init_cache_modes();
 }
 
 static int __init nopat(char *str)
@@ -67,7 +64,7 @@ early_param("nopat", nopat);
 
 bool pat_enabled(void)
 {
-   return !!__pat_enabled;
+   return pat_initialized;
 }
 EXPORT_SYMBOL_GPL(pat_enabled);
 
@@ -225,6 +222,7 @@ static void pat_bsp_init(u64 pat)
}
 
wrmsrl(MSR_IA32_CR_PAT, pat);
+   __pat_initialized = true;
 
__init_cache_modes(pat);
 }
@@ -242,7 +240,7 @@ static void pat_ap_init(u64 pat)
wrmsrl(MSR_IA32_CR_PAT, pat);
 }
 
-static void init_cache_modes(void)
+void init_cache_modes(void)
 {
u64 pat = 0;
static int init_cm_done;
@@ -306,10 +304,8 @@ void pat_init(void)
u64 pat;
struct cpuinfo_x86 *c = _cpu_data;
 
-   if (!pat_enabled()) {
-   init_cache_modes();
+   if (pat_disabled)
return;
-   }
 
if ((c->x86_vendor == X86_VENDOR_INTEL) &&
(((c->x86 == 0x6) && (c->x86_model <= 0xd)) ||

Re: [PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-07-02 Thread Mikulas Patocka 


On Tue, 6 Jun 2017, Mikulas Patocka wrote:

> Hi
> 
> Here I send the second version of the patch. It drops the change from 
> boot_cpu_has(X86_FEATURE_PAT) to this_cpu_has(X86_FEATURE_PAT) (that 
> caused kernel to be unbootable for some people).
> 
> Another change is that setup_arch() calls init_cache_modes() if PAT is 
> disabled, so that init_cache_modes() is always called.
> 
> Mikulas

Is there any progress with this patch? Will you accept it or do you want 
some changes to it?

> From: Mikulas Patocka 
> 
> In the file arch/x86/mm/pat.c, there's a variable __pat_enabled. The
> variable is set to 1 by default and the function pat_init() sets
> __pat_enabled to 0 if the CPU doesn't support PAT.
> 
> However, on AMD K6-3 CPU, the processor initialization code never calls
> pat_init() and so __pat_enabled stays 1 and the function pat_enabled()
> returns true, even though the K6-3 CPU doesn't support PAT.
> 
> The result of this bug is that this warning is produced when attemting to
> start the Xserver and the Xserver doesn't start (fork() returns ENOMEM).
> Another symptom of this bug is that the framebuffer driver doesn't set the
> K6-3 MTRR registers.
> 
> This patch changes pat_enabled() so that it returns true only if pat
> initialization was actually done.
> 
> As Andy Lutomirski suggested, we also need to call init_cache_modes() if
> pat was not initialized, so we call it after mtrr_bp_init()
> (mtrr_bp_init() would or wouldn't call pat_init()). Note that
> init_cache_modes() detects if it was called more than once and does
> nothing in that case.
> 
> x86/PAT: Xorg:3891 map pfn expected mapping type uncached-minus for [mem 
> 0xe400-0xe5ff], got write-combining
> [ cut here ]
> WARNING: CPU: 0 PID: 3891 at arch/x86/mm/pat.c:1020 untrack_pfn+0x5c/0x9f
> Modules linked in: blowfish_generic blowfish_common cbc dm_crypt dm_loop msr 
> configfs cpufreq_userspace cpufreq_powersave cpufreq_ondemand 
> cpufreq_conservative ext4 crc16 jbd2 mbcache hpfs nls_cp852 msdos fat 
> matroxfb_base matroxfb_g450 matroxfb_accel cfbfillrect cfbimgblt cfbcopyarea 
> matroxfb_DAC1064 g450_pll matroxfb_misc floppy snd_usb_audio snd_hwdep 
> snd_usbmidi_lib snd_seq_midi snd_seq_midi_event snd_rawmidi snd_pcm snd_seq 
> snd_seq_device snd_timer hid_generic snd usbhid hid soundcore powernow_k6 
> pcspkr evdev e1000 ehci_pci uhci_hcd ehci_hcd usbcore usb_common dm_mod 
> pata_it821x unix
> CPU: 0 PID: 3891 Comm: Xorg Not tainted 4.11.0-rc6-test+ #35
> Hardware name: System Manufacturer Product Name/VA-503A, BIOS 4.51 PG 08/02/00
> Call Trace:
>  ? __warn+0xab/0xc0
>  ? untrack_pfn+0x5c/0x9f
>  ? warn_slowpath_null+0xf/0x13
>  ? untrack_pfn+0x5c/0x9f
>  ? unmap_single_vma+0x43/0x66
>  ? unmap_vmas+0x24/0x30
>  ? exit_mmap+0x3c/0xa5
>  ? __mmput+0xf/0x76
>  ? copy_process.part.76+0xb43/0x1129
>  ? _do_fork+0x96/0x177
>  ? do_int80_syscall_32+0x3e/0x4c
>  ? entry_INT80_32+0x2a/0x2a
> ---[ end trace 8726f9d9fa90d702 ]---
> x86/PAT: Xorg:3891 map pfn expected mapping type uncached-minus for [mem 
> 0xe400-0xe5ff], got write-combining
> 
> Signed-off-by: Mikulas Patocka 
> Cc: sta...@vger.kernel.org# v4.2+
> 
> ---
>  arch/x86/include/asm/pat.h |1 +
>  arch/x86/kernel/setup.c|2 ++
>  arch/x86/mm/pat.c  |   10 ++
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> Index: linux-stable/arch/x86/mm/pat.c
> ===
> --- linux-stable.orig/arch/x86/mm/pat.c
> +++ linux-stable/arch/x86/mm/pat.c
> @@ -40,7 +40,6 @@
>  static bool boot_cpu_done;
>  
>  static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT);
> -static void init_cache_modes(void);
>  
>  void pat_disable(const char *reason)
>  {
> @@ -65,9 +64,11 @@ static int __init nopat(char *str)
>  }
>  early_param("nopat", nopat);
>  
> +static bool __read_mostly __pat_initialized = false;
> +
>  bool pat_enabled(void)
>  {
> - return !!__pat_enabled;
> + return __pat_initialized;
>  }
>  EXPORT_SYMBOL_GPL(pat_enabled);
>  
> @@ -225,6 +226,7 @@ static void pat_bsp_init(u64 pat)
>   }
>  
>   wrmsrl(MSR_IA32_CR_PAT, pat);
> + __pat_initialized = true;
>  
>   __init_cache_modes(pat);
>  }
> @@ -242,7 +244,7 @@ static void pat_ap_init(u64 pat)
>   wrmsrl(MSR_IA32_CR_PAT, pat);
>  }
>  
> -static void init_cache_modes(void)
> +void init_cache_modes(void)
>  {
>   u64 pat = 0;
>   static int init_cm_done;
> @@ -306,7 +308,7 @@ void pat_init(void)
>   u64 pat;
>   struct cpuinfo_x86 *c = _cpu_data;
>  
> - if (!pat_enabled()) {
> + if (!__pat_enabled) {
>   init_cache_modes();
>   return;
>   }
> Index: linux-stable/arch/x86/include/asm/pat.h
> ===
> --- linux-stable.orig/arch/x86/include/asm/pat.h
> +++ linux-stable/arch/x86/include/asm/pat.h
> @@ -7,6

Re: [PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-07-02 Thread Mikulas Patocka 


On Tue, 6 Jun 2017, Mikulas Patocka wrote:

> Hi
> 
> Here I send the second version of the patch. It drops the change from 
> boot_cpu_has(X86_FEATURE_PAT) to this_cpu_has(X86_FEATURE_PAT) (that 
> caused kernel to be unbootable for some people).
> 
> Another change is that setup_arch() calls init_cache_modes() if PAT is 
> disabled, so that init_cache_modes() is always called.
> 
> Mikulas

Is there any progress with this patch? Will you accept it or do you want 
some changes to it?

> From: Mikulas Patocka 
> 
> In the file arch/x86/mm/pat.c, there's a variable __pat_enabled. The
> variable is set to 1 by default and the function pat_init() sets
> __pat_enabled to 0 if the CPU doesn't support PAT.
> 
> However, on AMD K6-3 CPU, the processor initialization code never calls
> pat_init() and so __pat_enabled stays 1 and the function pat_enabled()
> returns true, even though the K6-3 CPU doesn't support PAT.
> 
> The result of this bug is that this warning is produced when attemting to
> start the Xserver and the Xserver doesn't start (fork() returns ENOMEM).
> Another symptom of this bug is that the framebuffer driver doesn't set the
> K6-3 MTRR registers.
> 
> This patch changes pat_enabled() so that it returns true only if pat
> initialization was actually done.
> 
> As Andy Lutomirski suggested, we also need to call init_cache_modes() if
> pat was not initialized, so we call it after mtrr_bp_init()
> (mtrr_bp_init() would or wouldn't call pat_init()). Note that
> init_cache_modes() detects if it was called more than once and does
> nothing in that case.
> 
> x86/PAT: Xorg:3891 map pfn expected mapping type uncached-minus for [mem 
> 0xe400-0xe5ff], got write-combining
> [ cut here ]
> WARNING: CPU: 0 PID: 3891 at arch/x86/mm/pat.c:1020 untrack_pfn+0x5c/0x9f
> Modules linked in: blowfish_generic blowfish_common cbc dm_crypt dm_loop msr 
> configfs cpufreq_userspace cpufreq_powersave cpufreq_ondemand 
> cpufreq_conservative ext4 crc16 jbd2 mbcache hpfs nls_cp852 msdos fat 
> matroxfb_base matroxfb_g450 matroxfb_accel cfbfillrect cfbimgblt cfbcopyarea 
> matroxfb_DAC1064 g450_pll matroxfb_misc floppy snd_usb_audio snd_hwdep 
> snd_usbmidi_lib snd_seq_midi snd_seq_midi_event snd_rawmidi snd_pcm snd_seq 
> snd_seq_device snd_timer hid_generic snd usbhid hid soundcore powernow_k6 
> pcspkr evdev e1000 ehci_pci uhci_hcd ehci_hcd usbcore usb_common dm_mod 
> pata_it821x unix
> CPU: 0 PID: 3891 Comm: Xorg Not tainted 4.11.0-rc6-test+ #35
> Hardware name: System Manufacturer Product Name/VA-503A, BIOS 4.51 PG 08/02/00
> Call Trace:
>  ? __warn+0xab/0xc0
>  ? untrack_pfn+0x5c/0x9f
>  ? warn_slowpath_null+0xf/0x13
>  ? untrack_pfn+0x5c/0x9f
>  ? unmap_single_vma+0x43/0x66
>  ? unmap_vmas+0x24/0x30
>  ? exit_mmap+0x3c/0xa5
>  ? __mmput+0xf/0x76
>  ? copy_process.part.76+0xb43/0x1129
>  ? _do_fork+0x96/0x177
>  ? do_int80_syscall_32+0x3e/0x4c
>  ? entry_INT80_32+0x2a/0x2a
> ---[ end trace 8726f9d9fa90d702 ]---
> x86/PAT: Xorg:3891 map pfn expected mapping type uncached-minus for [mem 
> 0xe400-0xe5ff], got write-combining
> 
> Signed-off-by: Mikulas Patocka 
> Cc: sta...@vger.kernel.org# v4.2+
> 
> ---
>  arch/x86/include/asm/pat.h |1 +
>  arch/x86/kernel/setup.c|2 ++
>  arch/x86/mm/pat.c  |   10 ++
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> Index: linux-stable/arch/x86/mm/pat.c
> ===
> --- linux-stable.orig/arch/x86/mm/pat.c
> +++ linux-stable/arch/x86/mm/pat.c
> @@ -40,7 +40,6 @@
>  static bool boot_cpu_done;
>  
>  static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT);
> -static void init_cache_modes(void);
>  
>  void pat_disable(const char *reason)
>  {
> @@ -65,9 +64,11 @@ static int __init nopat(char *str)
>  }
>  early_param("nopat", nopat);
>  
> +static bool __read_mostly __pat_initialized = false;
> +
>  bool pat_enabled(void)
>  {
> - return !!__pat_enabled;
> + return __pat_initialized;
>  }
>  EXPORT_SYMBOL_GPL(pat_enabled);
>  
> @@ -225,6 +226,7 @@ static void pat_bsp_init(u64 pat)
>   }
>  
>   wrmsrl(MSR_IA32_CR_PAT, pat);
> + __pat_initialized = true;
>  
>   __init_cache_modes(pat);
>  }
> @@ -242,7 +244,7 @@ static void pat_ap_init(u64 pat)
>   wrmsrl(MSR_IA32_CR_PAT, pat);
>  }
>  
> -static void init_cache_modes(void)
> +void init_cache_modes(void)
>  {
>   u64 pat = 0;
>   static int init_cm_done;
> @@ -306,7 +308,7 @@ void pat_init(void)
>   u64 pat;
>   struct cpuinfo_x86 *c = _cpu_data;
>  
> - if (!pat_enabled()) {
> + if (!__pat_enabled) {
>   init_cache_modes();
>   return;
>   }
> Index: linux-stable/arch/x86/include/asm/pat.h
> ===
> --- linux-stable.orig/arch/x86/include/asm/pat.h
> +++ linux-stable/arch/x86/include/asm/pat.h
> @@ -7,6 +7,7 @@
>  bool pat_enabled(void);
>  void

Re: [PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-06-14 Thread Mikulas Patocka 


On Tue, 13 Jun 2017, Andy Lutomirski wrote:

> On Tue, Jun 6, 2017 at 4:21 PM, Mikulas Patocka  wrote:
> >
> > On Tue, 6 Jun 2017, Andy Lutomirski wrote:
> >
> >> On Tue, Jun 6, 2017 at 3:49 PM, Mikulas Patocka  
> >> wrote:
> >> >
> >> > This patch changes pat_enabled() so that it returns true only if pat
> >> > initialization was actually done.
> >>
> >> Why?  Shouldn't calling init_cache_modes() be sufficient?
> >>
> >> --Andy
> >
> > See the function arch_phys_wc_add():
> >
> > if (pat_enabled() || !mtrr_enabled())
> > return 0;  /* Success!  (We don't need to do anything.) */
> > ret = mtrr_add(base, size, MTRR_TYPE_WRCOMB, true);
> >
> > - if pat_enabled() returns true, that function doesn't set MTRRs.
> > pat_enabled() must return false on systems without PAT, so that MTRRs are
> > set.
> 
> It still sounds to me like there are two bugs here that should be
> treated separately.
> 
> Bug 1: A warning fires.  Have you figured out why the warning fires?

The Xserver maps videoram and attempts to call fork() to spawn some 
utility.

reserve_pfn_range is called. At the beginning of reserve_pfn_range, 
want_pcm is set to 2 (_PAGE_CACHE_MODE_UC_MINUS).

reserve_pfn_range calls reserve_memtype(paddr, paddr + size, want_pcm, 
);

reserve_memtype contains this code at the beginning:
if (!pat_enabled()) {
/* This is identical to page table setting without PAT */
if (new_type)
*new_type = req_type;
return 0;
} --- so, if pat_enabled() return false, it sets pcm equal to want_pcm and 
there is no problem.

However, if pat_enabled() returns true, reserve_memtype goes on, it sets
actual_type = pat_x_mtrr_type(start, end, req_type); (actual_type is 2)
if (new_type)
*new_type = actual_type; (*new_type is 2)

and finally it calls rbt_memtype_check_insert(new, new_type); 

rbt_memtype_check_insert calls memtype_rb_check_conflict 
memtype_rb_check_conflict sets *newtype to the value 1 
(_PAGE_CACHE_MODE_WC) (the pointer newtype points to the variable "pcm" in 
the function reserve_pfn_range)

Then, we go back to reserve_pfn_range, the variable want_pcm is 2 and the 
variable pcm is 1. reserve_pfn_range checks "if (pcm != want_pcm)", prints 
a warning "x86/PAT: Xorg:3986 map pfn expected mapping type uncached-minus 
for [mem 0xe400-0xe5ff], got write-combining", returns an error - 
and this causes fork failure.

> Bug 2: arch_phys_wc_add() appears to be checking the wrong condition.
> How about checking the right condition?  It doesn't actually want to
> know if PAT is enabled -- it wants to know if the PAT contains a
> usable WC entry.  Something like pat_has_wc() would be better, I
> think.
>
> --Andy

The function pat_init() always programs the PAT with the WC type. So - 
surely we can rename pat_enabled() to pat_has_wc(). But renaming the 
function doesn't change functionality.

This is the same patch with pat_enabled() renamed to pat_has_wc(). Note 
also that this renaming causes conflicts when backporting the patch to 
stable kernels.

Mikulas





X86: don't report PAT on CPUs that don't support it

In the file arch/x86/mm/pat.c, there's a variable __pat_enabled. The
variable is set to 1 by default and the function pat_init() sets
__pat_enabled to 0 if the CPU doesn't support PAT.

However, on AMD K6-3 CPU, the processor initialization code never calls
pat_init() and so __pat_enabled stays 1 and the function pat_enabled()
returns true, even though the K6-3 CPU doesn't support PAT.

The result of this bug is that this warning is produced when attemting to
start the Xserver and the Xserver doesn't start (fork() returns ENOMEM).
Another symptom of this bug is that the framebuffer driver doesn't set the
K6-3 MTRR registers.

This patch changes pat_enabled() so that it returns true only if pat
initialization was actually done.

As Andy Lutomirski suggested, we also need to call init_cache_modes() if
pat was not initialized, so we call it after mtrr_bp_init()
(mtrr_bp_init() would or wouldn't call pat_init()). Note that
init_cache_modes() detects if it was called more than once and does
nothing in that case.

x86/PAT: Xorg:3891 map pfn expected mapping type uncached-minus for [mem 
0xe400-0xe5ff], got write-combining
[ cut here ]
WARNING: CPU: 0 PID: 3891 at arch/x86/mm/pat.c:1020 untrack_pfn+0x5c/0x9f
Modules linked in: blowfish_generic blowfish_common cbc dm_crypt dm_loop msr 
configfs cpufreq_userspace cpufreq_powersave cpufreq_ondemand 
cpufreq_conservative ext4 crc16 jbd2 mbcache hpfs nls_cp852 msdos fat 
matroxfb_base matroxfb_g450 matroxfb_accel cfbfillrect cfbimgblt cfbcopyarea 
matroxfb_DAC1064 g450_pll matroxfb_misc floppy snd_usb_audio snd_hwdep 
snd_usbmidi_lib snd_seq_midi snd_seq_midi_event snd_rawmidi snd_pcm snd_seq 
snd_seq_device snd_timer hid_generic snd usbhid hid soundcore powernow_k6 
pcspkr evdev e1000 ehci_pci uhci_hcd ehci_hcd usbcore usb_common dm_mod

Re: [PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-06-14 Thread Mikulas Patocka 


On Tue, 13 Jun 2017, Andy Lutomirski wrote:

> On Tue, Jun 6, 2017 at 4:21 PM, Mikulas Patocka  wrote:
> >
> > On Tue, 6 Jun 2017, Andy Lutomirski wrote:
> >
> >> On Tue, Jun 6, 2017 at 3:49 PM, Mikulas Patocka  
> >> wrote:
> >> >
> >> > This patch changes pat_enabled() so that it returns true only if pat
> >> > initialization was actually done.
> >>
> >> Why?  Shouldn't calling init_cache_modes() be sufficient?
> >>
> >> --Andy
> >
> > See the function arch_phys_wc_add():
> >
> > if (pat_enabled() || !mtrr_enabled())
> > return 0;  /* Success!  (We don't need to do anything.) */
> > ret = mtrr_add(base, size, MTRR_TYPE_WRCOMB, true);
> >
> > - if pat_enabled() returns true, that function doesn't set MTRRs.
> > pat_enabled() must return false on systems without PAT, so that MTRRs are
> > set.
> 
> It still sounds to me like there are two bugs here that should be
> treated separately.
> 
> Bug 1: A warning fires.  Have you figured out why the warning fires?

The Xserver maps videoram and attempts to call fork() to spawn some 
utility.

reserve_pfn_range is called. At the beginning of reserve_pfn_range, 
want_pcm is set to 2 (_PAGE_CACHE_MODE_UC_MINUS).

reserve_pfn_range calls reserve_memtype(paddr, paddr + size, want_pcm, 
);

reserve_memtype contains this code at the beginning:
if (!pat_enabled()) {
/* This is identical to page table setting without PAT */
if (new_type)
*new_type = req_type;
return 0;
} --- so, if pat_enabled() return false, it sets pcm equal to want_pcm and 
there is no problem.

However, if pat_enabled() returns true, reserve_memtype goes on, it sets
actual_type = pat_x_mtrr_type(start, end, req_type); (actual_type is 2)
if (new_type)
*new_type = actual_type; (*new_type is 2)

and finally it calls rbt_memtype_check_insert(new, new_type); 

rbt_memtype_check_insert calls memtype_rb_check_conflict 
memtype_rb_check_conflict sets *newtype to the value 1 
(_PAGE_CACHE_MODE_WC) (the pointer newtype points to the variable "pcm" in 
the function reserve_pfn_range)

Then, we go back to reserve_pfn_range, the variable want_pcm is 2 and the 
variable pcm is 1. reserve_pfn_range checks "if (pcm != want_pcm)", prints 
a warning "x86/PAT: Xorg:3986 map pfn expected mapping type uncached-minus 
for [mem 0xe400-0xe5ff], got write-combining", returns an error - 
and this causes fork failure.

> Bug 2: arch_phys_wc_add() appears to be checking the wrong condition.
> How about checking the right condition?  It doesn't actually want to
> know if PAT is enabled -- it wants to know if the PAT contains a
> usable WC entry.  Something like pat_has_wc() would be better, I
> think.
>
> --Andy

The function pat_init() always programs the PAT with the WC type. So - 
surely we can rename pat_enabled() to pat_has_wc(). But renaming the 
function doesn't change functionality.

This is the same patch with pat_enabled() renamed to pat_has_wc(). Note 
also that this renaming causes conflicts when backporting the patch to 
stable kernels.

Mikulas





X86: don't report PAT on CPUs that don't support it

In the file arch/x86/mm/pat.c, there's a variable __pat_enabled. The
variable is set to 1 by default and the function pat_init() sets
__pat_enabled to 0 if the CPU doesn't support PAT.

However, on AMD K6-3 CPU, the processor initialization code never calls
pat_init() and so __pat_enabled stays 1 and the function pat_enabled()
returns true, even though the K6-3 CPU doesn't support PAT.

The result of this bug is that this warning is produced when attemting to
start the Xserver and the Xserver doesn't start (fork() returns ENOMEM).
Another symptom of this bug is that the framebuffer driver doesn't set the
K6-3 MTRR registers.

This patch changes pat_enabled() so that it returns true only if pat
initialization was actually done.

As Andy Lutomirski suggested, we also need to call init_cache_modes() if
pat was not initialized, so we call it after mtrr_bp_init()
(mtrr_bp_init() would or wouldn't call pat_init()). Note that
init_cache_modes() detects if it was called more than once and does
nothing in that case.

x86/PAT: Xorg:3891 map pfn expected mapping type uncached-minus for [mem 
0xe400-0xe5ff], got write-combining
[ cut here ]
WARNING: CPU: 0 PID: 3891 at arch/x86/mm/pat.c:1020 untrack_pfn+0x5c/0x9f
Modules linked in: blowfish_generic blowfish_common cbc dm_crypt dm_loop msr 
configfs cpufreq_userspace cpufreq_powersave cpufreq_ondemand 
cpufreq_conservative ext4 crc16 jbd2 mbcache hpfs nls_cp852 msdos fat 
matroxfb_base matroxfb_g450 matroxfb_accel cfbfillrect cfbimgblt cfbcopyarea 
matroxfb_DAC1064 g450_pll matroxfb_misc floppy snd_usb_audio snd_hwdep 
snd_usbmidi_lib snd_seq_midi snd_seq_midi_event snd_rawmidi snd_pcm snd_seq 
snd_seq_device snd_timer hid_generic snd usbhid hid soundcore powernow_k6 
pcspkr evdev e1000 ehci_pci uhci_hcd ehci_hcd usbcore usb_common dm_mod 
pata_it821x unix
CPU: 0 PID: 3891 Comm: Xorg

Re: [PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-06-13 Thread Andy Lutomirski 
On Tue, Jun 6, 2017 at 4:21 PM, Mikulas Patocka  wrote:
>
>
> On Tue, 6 Jun 2017, Andy Lutomirski wrote:
>
>> On Tue, Jun 6, 2017 at 3:49 PM, Mikulas Patocka  wrote:
>> >
>> >
>> > On Sun, 28 May 2017, Andy Lutomirski wrote:
>> >
>> >> On Sun, May 28, 2017 at 11:18 AM, Bernhard Held  wrote:
>> >> > Hi,
>> >> >
>> >> > this patch breaks the boot of my kernel. The last message is "Booting
>> >> > the kernel.".
>> >> >
>> >> > My setup might be unusual: I'm running a Xenon E5450 (LGA 771) in a
>> >> > Gigbayte G33-DS3R board (LGA 775). The BIOS is patched with the
>> >> > microcode of the E5450 and recognizes the CPU.
>> >> >
>> >> > Please find below the dmesg of a the latest kernel w/o the PAT-patch.
>> >> > I'm happy to provide more information or to test patches.
>> >>
>> >> I think this patch is bogus.  pat_enabled() sure looks like it's
>> >> supposed to return true if PAT is *enabled*, and these days PAT is
>> >> "enabled" even if there's no HW PAT support.  Even if the patch were
>> >> somehow correct, it should have been split up into two patches, one to
>> >> change pat_enabled() and one to use this_cpu_has().
>> >>
>> >> Ingo, I'd suggest reverting the patch, cc-ing stable on the revert so
>> >> -stable knows not to backport it, and starting over with the fix.
>> >> >From very brief inspection, the right fix is to make sure that
>> >> pat_init(), or at least init_cache_modes(), gets called on the
>> >> affected CPUs.
>> >>
>> >> --Andy
>> >
>> > Hi
>> >
>> > Here I send the second version of the patch. It drops the change from
>> > boot_cpu_has(X86_FEATURE_PAT) to this_cpu_has(X86_FEATURE_PAT) (that
>> > caused kernel to be unbootable for some people).
>> >
>> > Another change is that setup_arch() calls init_cache_modes() if PAT is
>> > disabled, so that init_cache_modes() is always called.
>> >
>> > Mikulas
>> >
>> >
>> >
>> > From: Mikulas Patocka 
>> >
>> > In the file arch/x86/mm/pat.c, there's a variable __pat_enabled. The
>> > variable is set to 1 by default and the function pat_init() sets
>> > __pat_enabled to 0 if the CPU doesn't support PAT.
>> >
>> > However, on AMD K6-3 CPU, the processor initialization code never calls
>> > pat_init() and so __pat_enabled stays 1 and the function pat_enabled()
>> > returns true, even though the K6-3 CPU doesn't support PAT.
>> >
>> > The result of this bug is that this warning is produced when attemting to
>> > start the Xserver and the Xserver doesn't start (fork() returns ENOMEM).
>> > Another symptom of this bug is that the framebuffer driver doesn't set the
>> > K6-3 MTRR registers.
>> >
>> > This patch changes pat_enabled() so that it returns true only if pat
>> > initialization was actually done.
>>
>> Why?  Shouldn't calling init_cache_modes() be sufficient?
>>
>> --Andy
>
> See the function arch_phys_wc_add():
>
> if (pat_enabled() || !mtrr_enabled())
> return 0;  /* Success!  (We don't need to do anything.) */
> ret = mtrr_add(base, size, MTRR_TYPE_WRCOMB, true);
>
> - if pat_enabled() returns true, that function doesn't set MTRRs.
> pat_enabled() must return false on systems without PAT, so that MTRRs are
> set.

It still sounds to me like there are two bugs here that should be
treated separately.

Bug 1: A warning fires.  Have you figured out why the warning fires?

Bug 2: arch_phys_wc_add() appears to be checking the wrong condition.
How about checking the right condition?  It doesn't actually want to
know if PAT is enabled -- it wants to know if the PAT contains a
usable WC entry.  Something like pat_has_wc() would be better, I
think.

--Andy

Re: [PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-06-13 Thread Andy Lutomirski 
On Tue, Jun 6, 2017 at 4:21 PM, Mikulas Patocka  wrote:
>
>
> On Tue, 6 Jun 2017, Andy Lutomirski wrote:
>
>> On Tue, Jun 6, 2017 at 3:49 PM, Mikulas Patocka  wrote:
>> >
>> >
>> > On Sun, 28 May 2017, Andy Lutomirski wrote:
>> >
>> >> On Sun, May 28, 2017 at 11:18 AM, Bernhard Held  wrote:
>> >> > Hi,
>> >> >
>> >> > this patch breaks the boot of my kernel. The last message is "Booting
>> >> > the kernel.".
>> >> >
>> >> > My setup might be unusual: I'm running a Xenon E5450 (LGA 771) in a
>> >> > Gigbayte G33-DS3R board (LGA 775). The BIOS is patched with the
>> >> > microcode of the E5450 and recognizes the CPU.
>> >> >
>> >> > Please find below the dmesg of a the latest kernel w/o the PAT-patch.
>> >> > I'm happy to provide more information or to test patches.
>> >>
>> >> I think this patch is bogus.  pat_enabled() sure looks like it's
>> >> supposed to return true if PAT is *enabled*, and these days PAT is
>> >> "enabled" even if there's no HW PAT support.  Even if the patch were
>> >> somehow correct, it should have been split up into two patches, one to
>> >> change pat_enabled() and one to use this_cpu_has().
>> >>
>> >> Ingo, I'd suggest reverting the patch, cc-ing stable on the revert so
>> >> -stable knows not to backport it, and starting over with the fix.
>> >> >From very brief inspection, the right fix is to make sure that
>> >> pat_init(), or at least init_cache_modes(), gets called on the
>> >> affected CPUs.
>> >>
>> >> --Andy
>> >
>> > Hi
>> >
>> > Here I send the second version of the patch. It drops the change from
>> > boot_cpu_has(X86_FEATURE_PAT) to this_cpu_has(X86_FEATURE_PAT) (that
>> > caused kernel to be unbootable for some people).
>> >
>> > Another change is that setup_arch() calls init_cache_modes() if PAT is
>> > disabled, so that init_cache_modes() is always called.
>> >
>> > Mikulas
>> >
>> >
>> >
>> > From: Mikulas Patocka 
>> >
>> > In the file arch/x86/mm/pat.c, there's a variable __pat_enabled. The
>> > variable is set to 1 by default and the function pat_init() sets
>> > __pat_enabled to 0 if the CPU doesn't support PAT.
>> >
>> > However, on AMD K6-3 CPU, the processor initialization code never calls
>> > pat_init() and so __pat_enabled stays 1 and the function pat_enabled()
>> > returns true, even though the K6-3 CPU doesn't support PAT.
>> >
>> > The result of this bug is that this warning is produced when attemting to
>> > start the Xserver and the Xserver doesn't start (fork() returns ENOMEM).
>> > Another symptom of this bug is that the framebuffer driver doesn't set the
>> > K6-3 MTRR registers.
>> >
>> > This patch changes pat_enabled() so that it returns true only if pat
>> > initialization was actually done.
>>
>> Why?  Shouldn't calling init_cache_modes() be sufficient?
>>
>> --Andy
>
> See the function arch_phys_wc_add():
>
> if (pat_enabled() || !mtrr_enabled())
> return 0;  /* Success!  (We don't need to do anything.) */
> ret = mtrr_add(base, size, MTRR_TYPE_WRCOMB, true);
>
> - if pat_enabled() returns true, that function doesn't set MTRRs.
> pat_enabled() must return false on systems without PAT, so that MTRRs are
> set.

It still sounds to me like there are two bugs here that should be
treated separately.

Bug 1: A warning fires.  Have you figured out why the warning fires?

Bug 2: arch_phys_wc_add() appears to be checking the wrong condition.
How about checking the right condition?  It doesn't actually want to
know if PAT is enabled -- it wants to know if the PAT contains a
usable WC entry.  Something like pat_has_wc() would be better, I
think.

--Andy

Re: [PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-06-07 Thread Bernhard Held 

On 06/07/2017 at 12:49 AM, Mikulas Patocka wrote:

Hi

Here I send the second version of the patch. It drops the change from
boot_cpu_has(X86_FEATURE_PAT) to this_cpu_has(X86_FEATURE_PAT) (that
caused kernel to be unbootable for some people).

Another change is that setup_arch() calls init_cache_modes() if PAT is
disabled, so that init_cache_modes() is always called.

Mikulas


[PATCH v2] Works for me!

CHeers,
Bernhard

Re: [PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-06-07 Thread Bernhard Held 

On 06/07/2017 at 12:49 AM, Mikulas Patocka wrote:

Hi

Here I send the second version of the patch. It drops the change from
boot_cpu_has(X86_FEATURE_PAT) to this_cpu_has(X86_FEATURE_PAT) (that
caused kernel to be unbootable for some people).

Another change is that setup_arch() calls init_cache_modes() if PAT is
disabled, so that init_cache_modes() is always called.

Mikulas


[PATCH v2] Works for me!

CHeers,
Bernhard

Re: [PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-06-06 Thread Mikulas Patocka 


On Tue, 6 Jun 2017, Andy Lutomirski wrote:

> On Tue, Jun 6, 2017 at 3:49 PM, Mikulas Patocka  wrote:
> >
> >
> > On Sun, 28 May 2017, Andy Lutomirski wrote:
> >
> >> On Sun, May 28, 2017 at 11:18 AM, Bernhard Held  wrote:
> >> > Hi,
> >> >
> >> > this patch breaks the boot of my kernel. The last message is "Booting
> >> > the kernel.".
> >> >
> >> > My setup might be unusual: I'm running a Xenon E5450 (LGA 771) in a
> >> > Gigbayte G33-DS3R board (LGA 775). The BIOS is patched with the
> >> > microcode of the E5450 and recognizes the CPU.
> >> >
> >> > Please find below the dmesg of a the latest kernel w/o the PAT-patch.
> >> > I'm happy to provide more information or to test patches.
> >>
> >> I think this patch is bogus.  pat_enabled() sure looks like it's
> >> supposed to return true if PAT is *enabled*, and these days PAT is
> >> "enabled" even if there's no HW PAT support.  Even if the patch were
> >> somehow correct, it should have been split up into two patches, one to
> >> change pat_enabled() and one to use this_cpu_has().
> >>
> >> Ingo, I'd suggest reverting the patch, cc-ing stable on the revert so
> >> -stable knows not to backport it, and starting over with the fix.
> >> >From very brief inspection, the right fix is to make sure that
> >> pat_init(), or at least init_cache_modes(), gets called on the
> >> affected CPUs.
> >>
> >> --Andy
> >
> > Hi
> >
> > Here I send the second version of the patch. It drops the change from
> > boot_cpu_has(X86_FEATURE_PAT) to this_cpu_has(X86_FEATURE_PAT) (that
> > caused kernel to be unbootable for some people).
> >
> > Another change is that setup_arch() calls init_cache_modes() if PAT is
> > disabled, so that init_cache_modes() is always called.
> >
> > Mikulas
> >
> >
> >
> > From: Mikulas Patocka 
> >
> > In the file arch/x86/mm/pat.c, there's a variable __pat_enabled. The
> > variable is set to 1 by default and the function pat_init() sets
> > __pat_enabled to 0 if the CPU doesn't support PAT.
> >
> > However, on AMD K6-3 CPU, the processor initialization code never calls
> > pat_init() and so __pat_enabled stays 1 and the function pat_enabled()
> > returns true, even though the K6-3 CPU doesn't support PAT.
> >
> > The result of this bug is that this warning is produced when attemting to
> > start the Xserver and the Xserver doesn't start (fork() returns ENOMEM).
> > Another symptom of this bug is that the framebuffer driver doesn't set the
> > K6-3 MTRR registers.
> >
> > This patch changes pat_enabled() so that it returns true only if pat
> > initialization was actually done.
> 
> Why?  Shouldn't calling init_cache_modes() be sufficient?
> 
> --Andy

See the function arch_phys_wc_add():

if (pat_enabled() || !mtrr_enabled())
return 0;  /* Success!  (We don't need to do anything.) */
ret = mtrr_add(base, size, MTRR_TYPE_WRCOMB, true);

- if pat_enabled() returns true, that function doesn't set MTRRs. 
pat_enabled() must return false on systems without PAT, so that MTRRs are 
set.

Mikulas

Re: [PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-06-06 Thread Mikulas Patocka 


On Tue, 6 Jun 2017, Andy Lutomirski wrote:

> On Tue, Jun 6, 2017 at 3:49 PM, Mikulas Patocka  wrote:
> >
> >
> > On Sun, 28 May 2017, Andy Lutomirski wrote:
> >
> >> On Sun, May 28, 2017 at 11:18 AM, Bernhard Held  wrote:
> >> > Hi,
> >> >
> >> > this patch breaks the boot of my kernel. The last message is "Booting
> >> > the kernel.".
> >> >
> >> > My setup might be unusual: I'm running a Xenon E5450 (LGA 771) in a
> >> > Gigbayte G33-DS3R board (LGA 775). The BIOS is patched with the
> >> > microcode of the E5450 and recognizes the CPU.
> >> >
> >> > Please find below the dmesg of a the latest kernel w/o the PAT-patch.
> >> > I'm happy to provide more information or to test patches.
> >>
> >> I think this patch is bogus.  pat_enabled() sure looks like it's
> >> supposed to return true if PAT is *enabled*, and these days PAT is
> >> "enabled" even if there's no HW PAT support.  Even if the patch were
> >> somehow correct, it should have been split up into two patches, one to
> >> change pat_enabled() and one to use this_cpu_has().
> >>
> >> Ingo, I'd suggest reverting the patch, cc-ing stable on the revert so
> >> -stable knows not to backport it, and starting over with the fix.
> >> >From very brief inspection, the right fix is to make sure that
> >> pat_init(), or at least init_cache_modes(), gets called on the
> >> affected CPUs.
> >>
> >> --Andy
> >
> > Hi
> >
> > Here I send the second version of the patch. It drops the change from
> > boot_cpu_has(X86_FEATURE_PAT) to this_cpu_has(X86_FEATURE_PAT) (that
> > caused kernel to be unbootable for some people).
> >
> > Another change is that setup_arch() calls init_cache_modes() if PAT is
> > disabled, so that init_cache_modes() is always called.
> >
> > Mikulas
> >
> >
> >
> > From: Mikulas Patocka 
> >
> > In the file arch/x86/mm/pat.c, there's a variable __pat_enabled. The
> > variable is set to 1 by default and the function pat_init() sets
> > __pat_enabled to 0 if the CPU doesn't support PAT.
> >
> > However, on AMD K6-3 CPU, the processor initialization code never calls
> > pat_init() and so __pat_enabled stays 1 and the function pat_enabled()
> > returns true, even though the K6-3 CPU doesn't support PAT.
> >
> > The result of this bug is that this warning is produced when attemting to
> > start the Xserver and the Xserver doesn't start (fork() returns ENOMEM).
> > Another symptom of this bug is that the framebuffer driver doesn't set the
> > K6-3 MTRR registers.
> >
> > This patch changes pat_enabled() so that it returns true only if pat
> > initialization was actually done.
> 
> Why?  Shouldn't calling init_cache_modes() be sufficient?
> 
> --Andy

See the function arch_phys_wc_add():

if (pat_enabled() || !mtrr_enabled())
return 0;  /* Success!  (We don't need to do anything.) */
ret = mtrr_add(base, size, MTRR_TYPE_WRCOMB, true);

- if pat_enabled() returns true, that function doesn't set MTRRs. 
pat_enabled() must return false on systems without PAT, so that MTRRs are 
set.

Mikulas

Re: [PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-06-06 Thread Andy Lutomirski 
On Tue, Jun 6, 2017 at 3:49 PM, Mikulas Patocka  wrote:
>
>
> On Sun, 28 May 2017, Andy Lutomirski wrote:
>
>> On Sun, May 28, 2017 at 11:18 AM, Bernhard Held  wrote:
>> > Hi,
>> >
>> > this patch breaks the boot of my kernel. The last message is "Booting
>> > the kernel.".
>> >
>> > My setup might be unusual: I'm running a Xenon E5450 (LGA 771) in a
>> > Gigbayte G33-DS3R board (LGA 775). The BIOS is patched with the
>> > microcode of the E5450 and recognizes the CPU.
>> >
>> > Please find below the dmesg of a the latest kernel w/o the PAT-patch.
>> > I'm happy to provide more information or to test patches.
>>
>> I think this patch is bogus.  pat_enabled() sure looks like it's
>> supposed to return true if PAT is *enabled*, and these days PAT is
>> "enabled" even if there's no HW PAT support.  Even if the patch were
>> somehow correct, it should have been split up into two patches, one to
>> change pat_enabled() and one to use this_cpu_has().
>>
>> Ingo, I'd suggest reverting the patch, cc-ing stable on the revert so
>> -stable knows not to backport it, and starting over with the fix.
>> >From very brief inspection, the right fix is to make sure that
>> pat_init(), or at least init_cache_modes(), gets called on the
>> affected CPUs.
>>
>> --Andy
>
> Hi
>
> Here I send the second version of the patch. It drops the change from
> boot_cpu_has(X86_FEATURE_PAT) to this_cpu_has(X86_FEATURE_PAT) (that
> caused kernel to be unbootable for some people).
>
> Another change is that setup_arch() calls init_cache_modes() if PAT is
> disabled, so that init_cache_modes() is always called.
>
> Mikulas
>
>
>
> From: Mikulas Patocka 
>
> In the file arch/x86/mm/pat.c, there's a variable __pat_enabled. The
> variable is set to 1 by default and the function pat_init() sets
> __pat_enabled to 0 if the CPU doesn't support PAT.
>
> However, on AMD K6-3 CPU, the processor initialization code never calls
> pat_init() and so __pat_enabled stays 1 and the function pat_enabled()
> returns true, even though the K6-3 CPU doesn't support PAT.
>
> The result of this bug is that this warning is produced when attemting to
> start the Xserver and the Xserver doesn't start (fork() returns ENOMEM).
> Another symptom of this bug is that the framebuffer driver doesn't set the
> K6-3 MTRR registers.
>
> This patch changes pat_enabled() so that it returns true only if pat
> initialization was actually done.

Why?  Shouldn't calling init_cache_modes() be sufficient?

--Andy

Re: [PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-06-06 Thread Andy Lutomirski 
On Tue, Jun 6, 2017 at 3:49 PM, Mikulas Patocka  wrote:
>
>
> On Sun, 28 May 2017, Andy Lutomirski wrote:
>
>> On Sun, May 28, 2017 at 11:18 AM, Bernhard Held  wrote:
>> > Hi,
>> >
>> > this patch breaks the boot of my kernel. The last message is "Booting
>> > the kernel.".
>> >
>> > My setup might be unusual: I'm running a Xenon E5450 (LGA 771) in a
>> > Gigbayte G33-DS3R board (LGA 775). The BIOS is patched with the
>> > microcode of the E5450 and recognizes the CPU.
>> >
>> > Please find below the dmesg of a the latest kernel w/o the PAT-patch.
>> > I'm happy to provide more information or to test patches.
>>
>> I think this patch is bogus.  pat_enabled() sure looks like it's
>> supposed to return true if PAT is *enabled*, and these days PAT is
>> "enabled" even if there's no HW PAT support.  Even if the patch were
>> somehow correct, it should have been split up into two patches, one to
>> change pat_enabled() and one to use this_cpu_has().
>>
>> Ingo, I'd suggest reverting the patch, cc-ing stable on the revert so
>> -stable knows not to backport it, and starting over with the fix.
>> >From very brief inspection, the right fix is to make sure that
>> pat_init(), or at least init_cache_modes(), gets called on the
>> affected CPUs.
>>
>> --Andy
>
> Hi
>
> Here I send the second version of the patch. It drops the change from
> boot_cpu_has(X86_FEATURE_PAT) to this_cpu_has(X86_FEATURE_PAT) (that
> caused kernel to be unbootable for some people).
>
> Another change is that setup_arch() calls init_cache_modes() if PAT is
> disabled, so that init_cache_modes() is always called.
>
> Mikulas
>
>
>
> From: Mikulas Patocka 
>
> In the file arch/x86/mm/pat.c, there's a variable __pat_enabled. The
> variable is set to 1 by default and the function pat_init() sets
> __pat_enabled to 0 if the CPU doesn't support PAT.
>
> However, on AMD K6-3 CPU, the processor initialization code never calls
> pat_init() and so __pat_enabled stays 1 and the function pat_enabled()
> returns true, even though the K6-3 CPU doesn't support PAT.
>
> The result of this bug is that this warning is produced when attemting to
> start the Xserver and the Xserver doesn't start (fork() returns ENOMEM).
> Another symptom of this bug is that the framebuffer driver doesn't set the
> K6-3 MTRR registers.
>
> This patch changes pat_enabled() so that it returns true only if pat
> initialization was actually done.

Why?  Shouldn't calling init_cache_modes() be sufficient?

--Andy

[PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-06-06 Thread Mikulas Patocka 


On Sun, 28 May 2017, Andy Lutomirski wrote:

> On Sun, May 28, 2017 at 11:18 AM, Bernhard Held  wrote:
> > Hi,
> >
> > this patch breaks the boot of my kernel. The last message is "Booting
> > the kernel.".
> >
> > My setup might be unusual: I'm running a Xenon E5450 (LGA 771) in a
> > Gigbayte G33-DS3R board (LGA 775). The BIOS is patched with the
> > microcode of the E5450 and recognizes the CPU.
> >
> > Please find below the dmesg of a the latest kernel w/o the PAT-patch.
> > I'm happy to provide more information or to test patches.
> 
> I think this patch is bogus.  pat_enabled() sure looks like it's
> supposed to return true if PAT is *enabled*, and these days PAT is
> "enabled" even if there's no HW PAT support.  Even if the patch were
> somehow correct, it should have been split up into two patches, one to
> change pat_enabled() and one to use this_cpu_has().
> 
> Ingo, I'd suggest reverting the patch, cc-ing stable on the revert so
> -stable knows not to backport it, and starting over with the fix.
> >From very brief inspection, the right fix is to make sure that
> pat_init(), or at least init_cache_modes(), gets called on the
> affected CPUs.
> 
> --Andy

Hi

Here I send the second version of the patch. It drops the change from 
boot_cpu_has(X86_FEATURE_PAT) to this_cpu_has(X86_FEATURE_PAT) (that 
caused kernel to be unbootable for some people).

Another change is that setup_arch() calls init_cache_modes() if PAT is 
disabled, so that init_cache_modes() is always called.

Mikulas



From: Mikulas Patocka 

In the file arch/x86/mm/pat.c, there's a variable __pat_enabled. The
variable is set to 1 by default and the function pat_init() sets
__pat_enabled to 0 if the CPU doesn't support PAT.

However, on AMD K6-3 CPU, the processor initialization code never calls
pat_init() and so __pat_enabled stays 1 and the function pat_enabled()
returns true, even though the K6-3 CPU doesn't support PAT.

The result of this bug is that this warning is produced when attemting to
start the Xserver and the Xserver doesn't start (fork() returns ENOMEM).
Another symptom of this bug is that the framebuffer driver doesn't set the
K6-3 MTRR registers.

This patch changes pat_enabled() so that it returns true only if pat
initialization was actually done.

As Andy Lutomirski suggested, we also need to call init_cache_modes() if
pat was not initialized, so we call it after mtrr_bp_init()
(mtrr_bp_init() would or wouldn't call pat_init()). Note that
init_cache_modes() detects if it was called more than once and does
nothing in that case.

x86/PAT: Xorg:3891 map pfn expected mapping type uncached-minus for [mem 
0xe400-0xe5ff], got write-combining
[ cut here ]
WARNING: CPU: 0 PID: 3891 at arch/x86/mm/pat.c:1020 untrack_pfn+0x5c/0x9f
Modules linked in: blowfish_generic blowfish_common cbc dm_crypt dm_loop msr 
configfs cpufreq_userspace cpufreq_powersave cpufreq_ondemand 
cpufreq_conservative ext4 crc16 jbd2 mbcache hpfs nls_cp852 msdos fat 
matroxfb_base matroxfb_g450 matroxfb_accel cfbfillrect cfbimgblt cfbcopyarea 
matroxfb_DAC1064 g450_pll matroxfb_misc floppy snd_usb_audio snd_hwdep 
snd_usbmidi_lib snd_seq_midi snd_seq_midi_event snd_rawmidi snd_pcm snd_seq 
snd_seq_device snd_timer hid_generic snd usbhid hid soundcore powernow_k6 
pcspkr evdev e1000 ehci_pci uhci_hcd ehci_hcd usbcore usb_common dm_mod 
pata_it821x unix
CPU: 0 PID: 3891 Comm: Xorg Not tainted 4.11.0-rc6-test+ #35
Hardware name: System Manufacturer Product Name/VA-503A, BIOS 4.51 PG 08/02/00
Call Trace:
 ? __warn+0xab/0xc0
 ? untrack_pfn+0x5c/0x9f
 ? warn_slowpath_null+0xf/0x13
 ? untrack_pfn+0x5c/0x9f
 ? unmap_single_vma+0x43/0x66
 ? unmap_vmas+0x24/0x30
 ? exit_mmap+0x3c/0xa5
 ? __mmput+0xf/0x76
 ? copy_process.part.76+0xb43/0x1129
 ? _do_fork+0x96/0x177
 ? do_int80_syscall_32+0x3e/0x4c
 ? entry_INT80_32+0x2a/0x2a
---[ end trace 8726f9d9fa90d702 ]---
x86/PAT: Xorg:3891 map pfn expected mapping type uncached-minus for [mem 
0xe400-0xe5ff], got write-combining

Signed-off-by: Mikulas Patocka 
Cc: sta...@vger.kernel.org  # v4.2+

---
 arch/x86/include/asm/pat.h |1 +
 arch/x86/kernel/setup.c|2 ++
 arch/x86/mm/pat.c  |   10 ++
 3 files changed, 9 insertions(+), 4 deletions(-)

Index: linux-stable/arch/x86/mm/pat.c
===
--- linux-stable.orig/arch/x86/mm/pat.c
+++ linux-stable/arch/x86/mm/pat.c
@@ -40,7 +40,6 @@
 static bool boot_cpu_done;
 
 static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT);
-static void init_cache_modes(void);
 
 void pat_disable(const char *reason)
 {
@@ -65,9 +64,11 @@ static int __init nopat(char *str)
 }
 early_param("nopat", nopat);
 
+static bool __read_mostly __pat_initialized = false;
+
 bool pat_enabled(void)
 {
-   return !!__pat_enabled;
+   return __pat_initialized;
 }
 EXPORT_SYMBOL_GPL(pat_enabled);
 
@@

[PATCH v2] X86: don't report PAT on CPUs that don't support it

2017-06-06 Thread Mikulas Patocka 


On Sun, 28 May 2017, Andy Lutomirski wrote:

> On Sun, May 28, 2017 at 11:18 AM, Bernhard Held  wrote:
> > Hi,
> >
> > this patch breaks the boot of my kernel. The last message is "Booting
> > the kernel.".
> >
> > My setup might be unusual: I'm running a Xenon E5450 (LGA 771) in a
> > Gigbayte G33-DS3R board (LGA 775). The BIOS is patched with the
> > microcode of the E5450 and recognizes the CPU.
> >
> > Please find below the dmesg of a the latest kernel w/o the PAT-patch.
> > I'm happy to provide more information or to test patches.
> 
> I think this patch is bogus.  pat_enabled() sure looks like it's
> supposed to return true if PAT is *enabled*, and these days PAT is
> "enabled" even if there's no HW PAT support.  Even if the patch were
> somehow correct, it should have been split up into two patches, one to
> change pat_enabled() and one to use this_cpu_has().
> 
> Ingo, I'd suggest reverting the patch, cc-ing stable on the revert so
> -stable knows not to backport it, and starting over with the fix.
> >From very brief inspection, the right fix is to make sure that
> pat_init(), or at least init_cache_modes(), gets called on the
> affected CPUs.
> 
> --Andy

Hi

Here I send the second version of the patch. It drops the change from 
boot_cpu_has(X86_FEATURE_PAT) to this_cpu_has(X86_FEATURE_PAT) (that 
caused kernel to be unbootable for some people).

Another change is that setup_arch() calls init_cache_modes() if PAT is 
disabled, so that init_cache_modes() is always called.

Mikulas



From: Mikulas Patocka 

In the file arch/x86/mm/pat.c, there's a variable __pat_enabled. The
variable is set to 1 by default and the function pat_init() sets
__pat_enabled to 0 if the CPU doesn't support PAT.

However, on AMD K6-3 CPU, the processor initialization code never calls
pat_init() and so __pat_enabled stays 1 and the function pat_enabled()
returns true, even though the K6-3 CPU doesn't support PAT.

The result of this bug is that this warning is produced when attemting to
start the Xserver and the Xserver doesn't start (fork() returns ENOMEM).
Another symptom of this bug is that the framebuffer driver doesn't set the
K6-3 MTRR registers.

This patch changes pat_enabled() so that it returns true only if pat
initialization was actually done.

As Andy Lutomirski suggested, we also need to call init_cache_modes() if
pat was not initialized, so we call it after mtrr_bp_init()
(mtrr_bp_init() would or wouldn't call pat_init()). Note that
init_cache_modes() detects if it was called more than once and does
nothing in that case.

x86/PAT: Xorg:3891 map pfn expected mapping type uncached-minus for [mem 
0xe400-0xe5ff], got write-combining
[ cut here ]
WARNING: CPU: 0 PID: 3891 at arch/x86/mm/pat.c:1020 untrack_pfn+0x5c/0x9f
Modules linked in: blowfish_generic blowfish_common cbc dm_crypt dm_loop msr 
configfs cpufreq_userspace cpufreq_powersave cpufreq_ondemand 
cpufreq_conservative ext4 crc16 jbd2 mbcache hpfs nls_cp852 msdos fat 
matroxfb_base matroxfb_g450 matroxfb_accel cfbfillrect cfbimgblt cfbcopyarea 
matroxfb_DAC1064 g450_pll matroxfb_misc floppy snd_usb_audio snd_hwdep 
snd_usbmidi_lib snd_seq_midi snd_seq_midi_event snd_rawmidi snd_pcm snd_seq 
snd_seq_device snd_timer hid_generic snd usbhid hid soundcore powernow_k6 
pcspkr evdev e1000 ehci_pci uhci_hcd ehci_hcd usbcore usb_common dm_mod 
pata_it821x unix
CPU: 0 PID: 3891 Comm: Xorg Not tainted 4.11.0-rc6-test+ #35
Hardware name: System Manufacturer Product Name/VA-503A, BIOS 4.51 PG 08/02/00
Call Trace:
 ? __warn+0xab/0xc0
 ? untrack_pfn+0x5c/0x9f
 ? warn_slowpath_null+0xf/0x13
 ? untrack_pfn+0x5c/0x9f
 ? unmap_single_vma+0x43/0x66
 ? unmap_vmas+0x24/0x30
 ? exit_mmap+0x3c/0xa5
 ? __mmput+0xf/0x76
 ? copy_process.part.76+0xb43/0x1129
 ? _do_fork+0x96/0x177
 ? do_int80_syscall_32+0x3e/0x4c
 ? entry_INT80_32+0x2a/0x2a
---[ end trace 8726f9d9fa90d702 ]---
x86/PAT: Xorg:3891 map pfn expected mapping type uncached-minus for [mem 
0xe400-0xe5ff], got write-combining

Signed-off-by: Mikulas Patocka 
Cc: sta...@vger.kernel.org  # v4.2+

---
 arch/x86/include/asm/pat.h |1 +
 arch/x86/kernel/setup.c|2 ++
 arch/x86/mm/pat.c  |   10 ++
 3 files changed, 9 insertions(+), 4 deletions(-)

Index: linux-stable/arch/x86/mm/pat.c
===
--- linux-stable.orig/arch/x86/mm/pat.c
+++ linux-stable/arch/x86/mm/pat.c
@@ -40,7 +40,6 @@
 static bool boot_cpu_done;
 
 static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT);
-static void init_cache_modes(void);
 
 void pat_disable(const char *reason)
 {
@@ -65,9 +64,11 @@ static int __init nopat(char *str)
 }
 early_param("nopat", nopat);
 
+static bool __read_mostly __pat_initialized = false;
+
 bool pat_enabled(void)
 {
-   return !!__pat_enabled;
+   return __pat_initialized;
 }
 EXPORT_SYMBOL_GPL(pat_enabled);
 
@@ -225,6 +226,7 @@ static void pat_bsp_init(u64 pat)
}