Message-ID: <alpine.LRH.2.02.1706141538370.20127@file01.intranet.prod.int.rdu2.redhat.com>
Date:   Wed, 14 Jun 2017 16:24:16 -0400 (EDT)
From:   Mikulas Patocka <mpatocka@...hat.com>
To:     Andy Lutomirski <luto@...nel.org>
cc:     Bernhard Held <berny156@....de>, Toshi Kani <toshi.kani@...com>,
        Borislav Petkov <bp@...en8.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Brian Gerst <brgerst@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        "Luis R. Rodriguez" <mcgrof@...e.com>,
        Denys Vlasenko <dvlasenk@...hat.com>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] X86: don't report PAT on CPUs that don't support it



On Tue, 13 Jun 2017, Andy Lutomirski wrote:

> On Tue, Jun 6, 2017 at 4:21 PM, Mikulas Patocka <mpatocka@...hat.com> wrote:
> >
> > On Tue, 6 Jun 2017, Andy Lutomirski wrote:
> >
> >> On Tue, Jun 6, 2017 at 3:49 PM, Mikulas Patocka <mpatocka@...hat.com> 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, 
&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 0xe4000000-0xe5ffffff], 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 0xe4000000-0xe5ffffff], 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 0xe4000000-0xe5ffffff], got write-combining

Signed-off-by: Mikulas Patocka <mpatocka@...hat.com>
Cc: stable@...r.kernel.org	# v4.2+

---
 arch/x86/include/asm/pat.h        |    3 ++-
 arch/x86/include/asm/pci.h        |    2 +-
 arch/x86/kernel/cpu/mtrr/main.c   |    2 +-
 arch/x86/kernel/setup.c           |    2 ++
 arch/x86/mm/iomap_32.c            |    2 +-
 arch/x86/mm/ioremap.c             |    2 +-
 arch/x86/mm/pat.c                 |   28 +++++++++++++++-------------
 drivers/infiniband/hw/mlx5/main.c |    2 +-
 drivers/media/pci/ivtv/ivtvfb.c   |    2 +-
 9 files changed, 25 insertions(+), 20 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,11 +64,13 @@ static int __init nopat(char *str)
 }
 early_param("nopat", nopat);
 
-bool pat_enabled(void)
+static bool __read_mostly __pat_has_wc = false;
+
+bool pat_has_wc(void)
 {
-	return !!__pat_enabled;
+	return __pat_has_wc;
 }
-EXPORT_SYMBOL_GPL(pat_enabled);
+EXPORT_SYMBOL_GPL(pat_has_wc);
 
 int pat_debug_enable;
 
@@ -225,6 +226,7 @@ static void pat_bsp_init(u64 pat)
 	}
 
 	wrmsrl(MSR_IA32_CR_PAT, pat);
+	__pat_has_wc = 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 = &boot_cpu_data;
 
-	if (!pat_enabled()) {
+	if (!__pat_enabled) {
 		init_cache_modes();
 		return;
 	}
@@ -539,7 +541,7 @@ int reserve_memtype(u64 start, u64 end,
 
 	BUG_ON(start >= end); /* end is exclusive */
 
-	if (!pat_enabled()) {
+	if (!pat_has_wc()) {
 		/* This is identical to page table setting without PAT */
 		if (new_type)
 			*new_type = req_type;
@@ -610,7 +612,7 @@ int free_memtype(u64 start, u64 end)
 	int is_range_ram;
 	struct memtype *entry;
 
-	if (!pat_enabled())
+	if (!pat_has_wc())
 		return 0;
 
 	/* Low ISA region is always mapped WB. No need to track */
@@ -765,7 +767,7 @@ static inline int range_is_allowed(unsig
 	u64 to = from + size;
 	u64 cursor = from;
 
-	if (!pat_enabled())
+	if (!pat_has_wc())
 		return 1;
 
 	while (cursor < to) {
@@ -848,7 +850,7 @@ static int reserve_pfn_range(u64 paddr,
 	 * the type requested matches the type of first page in the range.
 	 */
 	if (is_ram) {
-		if (!pat_enabled())
+		if (!pat_has_wc())
 			return 0;
 
 		pcm = lookup_memtype(paddr);
@@ -964,7 +966,7 @@ int track_pfn_remap(struct vm_area_struc
 		return ret;
 	}
 
-	if (!pat_enabled())
+	if (!pat_has_wc())
 		return 0;
 
 	/*
@@ -991,7 +993,7 @@ void track_pfn_insert(struct vm_area_str
 {
 	enum page_cache_mode pcm;
 
-	if (!pat_enabled())
+	if (!pat_has_wc())
 		return;
 
 	/* Set prot based on lookup */
@@ -1128,7 +1130,7 @@ static const struct file_operations memt
 
 static int __init pat_memtype_list_init(void)
 {
-	if (pat_enabled()) {
+	if (pat_has_wc()) {
 		debugfs_create_file("pat_memtype_list", S_IRUSR,
 				    arch_debugfs_dir, NULL, &memtype_fops);
 	}
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
@@ -4,9 +4,10 @@
 #include <linux/types.h>
 #include <asm/pgtable_types.h>
 
-bool pat_enabled(void);
+bool pat_has_wc(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);
Index: linux-stable/arch/x86/kernel/setup.c
===================================================================
--- linux-stable.orig/arch/x86/kernel/setup.c
+++ linux-stable/arch/x86/kernel/setup.c
@@ -1070,6 +1070,8 @@ void __init setup_arch(char **cmdline_p)
 
 	/* update e820 for memory not covered by WB MTRRs */
 	mtrr_bp_init();
+	if (!pat_has_wc())
+		init_cache_modes();
 	if (mtrr_trim_uncached_memory(max_pfn))
 		max_pfn = e820__end_of_ram_pfn();
 
Index: linux-stable/arch/x86/include/asm/pci.h
===================================================================
--- linux-stable.orig/arch/x86/include/asm/pci.h
+++ linux-stable/arch/x86/include/asm/pci.h
@@ -103,7 +103,7 @@ int pcibios_set_irq_routing(struct pci_d
 
 
 #define HAVE_PCI_MMAP
-#define arch_can_pci_mmap_wc()	pat_enabled()
+#define arch_can_pci_mmap_wc()	pat_has_wc()
 #define ARCH_GENERIC_PCI_MMAP_RESOURCE
 
 #ifdef CONFIG_PCI
Index: linux-stable/arch/x86/kernel/cpu/mtrr/main.c
===================================================================
--- linux-stable.orig/arch/x86/kernel/cpu/mtrr/main.c
+++ linux-stable/arch/x86/kernel/cpu/mtrr/main.c
@@ -556,7 +556,7 @@ int arch_phys_wc_add(unsigned long base,
 {
 	int ret;
 
-	if (pat_enabled() || !mtrr_enabled())
+	if (pat_has_wc() || !mtrr_enabled())
 		return 0;  /* Success!  (We don't need to do anything.) */
 
 	ret = mtrr_add(base, size, MTRR_TYPE_WRCOMB, true);
Index: linux-stable/arch/x86/mm/iomap_32.c
===================================================================
--- linux-stable.orig/arch/x86/mm/iomap_32.c
+++ linux-stable/arch/x86/mm/iomap_32.c
@@ -84,7 +84,7 @@ iomap_atomic_prot_pfn(unsigned long pfn,
 	 * is UC or WC. UC- gets the real intention, of the user, which is
 	 * "WC if the MTRR is WC, UC if you can't do that."
 	 */
-	if (!pat_enabled() && pgprot2cachemode(prot) != _PAGE_CACHE_MODE_WB)
+	if (!pat_has_wc() && pgprot2cachemode(prot) != _PAGE_CACHE_MODE_WB)
 		prot = __pgprot(__PAGE_KERNEL |
 				cachemode2protval(_PAGE_CACHE_MODE_UC_MINUS));
 
Index: linux-stable/arch/x86/mm/ioremap.c
===================================================================
--- linux-stable.orig/arch/x86/mm/ioremap.c
+++ linux-stable/arch/x86/mm/ioremap.c
@@ -230,7 +230,7 @@ void __iomem *ioremap_nocache(resource_s
 {
 	/*
 	 * Ideally, this should be:
-	 *	pat_enabled() ? _PAGE_CACHE_MODE_UC : _PAGE_CACHE_MODE_UC_MINUS;
+	 *	pat_has_wc() ? _PAGE_CACHE_MODE_UC : _PAGE_CACHE_MODE_UC_MINUS;
 	 *
 	 * Till we fix all X drivers to use ioremap_wc(), we will use
 	 * UC MINUS. Drivers that are certain they need or can already
Index: linux-stable/drivers/infiniband/hw/mlx5/main.c
===================================================================
--- linux-stable.orig/drivers/infiniband/hw/mlx5/main.c
+++ linux-stable/drivers/infiniband/hw/mlx5/main.c
@@ -1602,7 +1602,7 @@ static int uar_mmap(struct mlx5_ib_dev *
 	case MLX5_IB_MMAP_WC_PAGE:
 /* Some architectures don't support WC memory */
 #if defined(CONFIG_X86)
-		if (!pat_enabled())
+		if (!pat_has_wc())
 			return -EPERM;
 #elif !(defined(CONFIG_PPC) || (defined(CONFIG_ARM) && defined(CONFIG_MMU)))
 			return -EPERM;
Index: linux-stable/drivers/media/pci/ivtv/ivtvfb.c
===================================================================
--- linux-stable.orig/drivers/media/pci/ivtv/ivtvfb.c
+++ linux-stable/drivers/media/pci/ivtv/ivtvfb.c
@@ -1168,7 +1168,7 @@ static int ivtvfb_init_card(struct ivtv
 	int rc;
 
 #ifdef CONFIG_X86_64
-	if (pat_enabled()) {
+	if (pat_has_wc()) {
 		pr_warn("ivtvfb needs PAT disabled, boot with nopat kernel parameter\n");
 		return -ENODEV;
 	}

Powered by blists - more mailing lists