Date:	Mon, 4 May 2015 17:22:08 +0200
From:	Borislav Petkov <bp@...e.de>
To:	"Luis R. Rodriguez" <mcgrof@...not-panic.com>
Cc:	mingo@...e.hu, tglx@...utronix.de, hpa@...or.com,
	plagnioj@...osoft.com, tomi.valkeinen@...com,
	daniel.vetter@...el.com, airlied@...ux.ie, dledford@...hat.com,
	awalls@...metrocast.net, syrjala@....fi, luto@...capital.net,
	mst@...hat.com, cocci@...teme.lip6.fr,
	linux-kernel@...r.kernel.org,
	"Luis R. Rodriguez" <mcgrof@...e.com>,
	Christoph Lameter <cl@...ux.com>,
	Kyle McMartin <kyle@...nel.org>,
	Juergen Gross <jgross@...e.com>,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	Dave Airlie <airlied@...hat.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>, x86@...nel.org
Subject: Re: [PATCH v5 2/6] x86/mm/pat: redefine pat_enabled

On Thu, Apr 30, 2015 at 01:25:16PM -0700, Luis R. Rodriguez wrote:
> diff --git a/arch/x86/kernel/cpu/mtrr/main.c b/arch/x86/kernel/cpu/mtrr/main.c
> index bfef424..f094d36 100644
> --- a/arch/x86/kernel/cpu/mtrr/main.c
> +++ b/arch/x86/kernel/cpu/mtrr/main.c
> @@ -556,7 +556,7 @@ int arch_phys_wc_add(unsigned long base, unsigned long size)
>  {
>  	int ret;
>  
> -	if (pat_enabled || !mtrr_enabled)
> +	if (pat_enabled() || !mtrr_enabled)

What's going on here? I got a reject about mtrr_enabled which is nowhere
to be found. Am I missing a patch?

Anyway, I applied that:

---
From: "Luis R. Rodriguez" <mcgrof@...e.com>
Date: Thu, 30 Apr 2015 13:25:16 -0700
Subject: [PATCH] x86/mm/pat: Redefine pat_enabled

We use pat_enabled in x86 specific code to see if PAT is enabled or
not, we however are granting full access to the variable even though
readers do not need to set it. If for instance we granted access to it
to modules later they then could override the variable setting... no
bueno.

This renames pat_enabled to a new static variable __pat_enabled. To
see if PAT is enabled / disabled folks can just use the nice accessor
pat_enabled() now.

Code that sets this can only be internal to pat.c. Apart from the early
kernel parameter "nopat" to disable PAT we also have a few cases that
disable it later and make use of a helper pat_disable(), this helper is
wrapped under an ifdef but since that code cannot run unless PAT was
enabled its not required to wrap it with ifdefs, unwrap that. Likewise
since "nopat" doesn't really change non-PAT systems just remove that
ifdef as well.

Although we could add and use an early_param_off() these helpers don't
use __read_mostly and we want to keep __read_mostly for __pat_enabled as
this is a hot path -- upon boot for instance a simple guest may see ~4k
accesses to pat_enabled(). Since __read_mostly early boot params are not
that common we don't add a helper for them just yet.

Signed-off-by: Luis R. Rodriguez <mcgrof@...e.com>
Cc: Christoph Lameter <cl@...ux.com>
Cc: Kyle McMartin <kyle@...nel.org>
Cc: Andy Walls <awalls@...metrocast.net>
Cc: Doug Ledford <dledford@...hat.com>
Cc: Andy Lutomirski <luto@...capital.net>
Cc: Ingo Molnar <mingo@...e.hu>
Cc: Thomas Gleixner <tglx@...utronix.de>
Cc: Juergen Gross <jgross@...e.com>
Cc: Daniel Vetter <daniel.vetter@...ll.ch>
Cc: Dave Airlie <airlied@...hat.com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>
Cc: Michael S. Tsirkin <mst@...hat.com>
Cc: syrjala@....fi
Cc: plagnioj@...osoft.com
Cc: tomi.valkeinen@...com
Link: http://lkml.kernel.org/r/1430425520-22275-3-git-send-email-mcgrof@do-not-panic.com
Signed-off-by: 
---
 arch/x86/include/asm/pat.h      |  7 +------
 arch/x86/kernel/cpu/mtrr/main.c |  2 +-
 arch/x86/mm/iomap_32.c          |  2 +-
 arch/x86/mm/ioremap.c           |  4 ++--
 arch/x86/mm/pageattr.c          |  2 +-
 arch/x86/mm/pat.c               | 33 +++++++++++++++------------------
 arch/x86/pci/i386.c             |  6 +++---
 7 files changed, 24 insertions(+), 32 deletions(-)

diff --git a/arch/x86/include/asm/pat.h b/arch/x86/include/asm/pat.h
index 91bc4ba95f91..cdcff7f7f694 100644
--- a/arch/x86/include/asm/pat.h
+++ b/arch/x86/include/asm/pat.h
@@ -4,12 +4,7 @@
 #include <linux/types.h>
 #include <asm/pgtable_types.h>
 
-#ifdef CONFIG_X86_PAT
-extern int pat_enabled;
-#else
-static const int pat_enabled;
-#endif
-
+bool pat_enabled(void);
 extern void pat_init(void);
 void pat_init_cache_modes(void);
 
diff --git a/arch/x86/kernel/cpu/mtrr/main.c b/arch/x86/kernel/cpu/mtrr/main.c
index ea5f363a1948..96fa7b38af5e 100644
--- a/arch/x86/kernel/cpu/mtrr/main.c
+++ b/arch/x86/kernel/cpu/mtrr/main.c
@@ -545,7 +545,7 @@ int arch_phys_wc_add(unsigned long base, unsigned long size)
 {
 	int ret;
 
-	if (pat_enabled)
+	if (pat_enabled())
 		return 0;  /* Success!  (We don't need to do anything.) */
 
 	ret = mtrr_add(base, size, MTRR_TYPE_WRCOMB, true);
diff --git a/arch/x86/mm/iomap_32.c b/arch/x86/mm/iomap_32.c
index 9ca35fc60cfe..3a2ec8790ca7 100644
--- a/arch/x86/mm/iomap_32.c
+++ b/arch/x86/mm/iomap_32.c
@@ -82,7 +82,7 @@ iomap_atomic_prot_pfn(unsigned long pfn, pgprot_t prot)
 	 * MTRR is UC or WC.  UC_MINUS 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 && pgprot_val(prot) ==
+	if (!pat_enabled() && pgprot_val(prot) ==
 	    (__PAGE_KERNEL | cachemode2protval(_PAGE_CACHE_MODE_WC)))
 		prot = __pgprot(__PAGE_KERNEL |
 				cachemode2protval(_PAGE_CACHE_MODE_UC_MINUS));
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index fc08431a387b..ea379c06cc4c 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -234,7 +234,7 @@ void __iomem *ioremap_nocache(resource_size_t phys_addr, unsigned long size)
 {
 	/*
 	 * Ideally, this should be:
-	 *	pat_enabled ? _PAGE_CACHE_MODE_UC : _PAGE_CACHE_MODE_UC_MINUS;
+	 *	pat_enabled() ? _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
@@ -292,7 +292,7 @@ EXPORT_SYMBOL_GPL(ioremap_uc);
  */
 void __iomem *ioremap_wc(resource_size_t phys_addr, unsigned long size)
 {
-	if (pat_enabled)
+	if (pat_enabled())
 		return __ioremap_caller(phys_addr, size, _PAGE_CACHE_MODE_WC,
 					__builtin_return_address(0));
 	else
diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index d35148acdc05..e07686633ce4 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -1573,7 +1573,7 @@ int set_memory_wc(unsigned long addr, int numpages)
 {
 	int ret;
 
-	if (!pat_enabled)
+	if (!pat_enabled())
 		return set_memory_uc(addr, numpages);
 
 	ret = reserve_memtype(__pa(addr), __pa(addr) + numpages * PAGE_SIZE,
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index 8269c784b61c..6e05a071ffc8 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -33,12 +33,11 @@
 #include "pat_internal.h"
 #include "mm_internal.h"
 
-#ifdef CONFIG_X86_PAT
-int __read_mostly pat_enabled = 1;
+static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT);
 
 static inline void pat_disable(const char *reason)
 {
-	pat_enabled = 0;
+	__pat_enabled = 0;
 	pr_info("x86/PAT: %s\n", reason);
 }
 
@@ -48,13 +47,11 @@ static int __init nopat(char *str)
 	return 0;
 }
 early_param("nopat", nopat);
-#else
-static inline void pat_disable(const char *reason)
+
+bool pat_enabled(void)
 {
-	(void)reason;
+	return !!__pat_enabled;
 }
-#endif
-
 
 int pat_debug_enable;
 
@@ -198,7 +195,7 @@ void pat_init(void)
 	u64 pat;
 	bool boot_cpu = !boot_pat_state;
 
-	if (!pat_enabled)
+	if (!pat_enabled())
 		return;
 
 	if (!cpu_has_pat) {
@@ -399,7 +396,7 @@ int reserve_memtype(u64 start, u64 end, enum page_cache_mode req_type,
 
 	BUG_ON(start >= end); /* end is exclusive */
 
-	if (!pat_enabled) {
+	if (!pat_enabled()) {
 		/* This is identical to page table setting without PAT */
 		if (new_type) {
 			if (req_type == _PAGE_CACHE_MODE_WC)
@@ -474,7 +471,7 @@ int free_memtype(u64 start, u64 end)
 	int is_range_ram;
 	struct memtype *entry;
 
-	if (!pat_enabled)
+	if (!pat_enabled())
 		return 0;
 
 	/* Low ISA region is always mapped WB. No need to track */
@@ -622,7 +619,7 @@ static inline int range_is_allowed(unsigned long pfn, unsigned long size)
 	u64 to = from + size;
 	u64 cursor = from;
 
-	if (!pat_enabled)
+	if (!pat_enabled())
 		return 1;
 
 	while (cursor < to) {
@@ -658,7 +655,7 @@ int phys_mem_access_prot_allowed(struct file *file, unsigned long pfn,
 	 * caching for the high addresses through the KEN pin, but
 	 * we maintain the tradition of paranoia in this code.
 	 */
-	if (!pat_enabled &&
+	if (!pat_enabled() &&
 	    !(boot_cpu_has(X86_FEATURE_MTRR) ||
 	      boot_cpu_has(X86_FEATURE_K6_MTRR) ||
 	      boot_cpu_has(X86_FEATURE_CYRIX_ARR) ||
@@ -727,7 +724,7 @@ static int reserve_pfn_range(u64 paddr, unsigned long size, pgprot_t *vma_prot,
 	 * the type requested matches the type of first page in the range.
 	 */
 	if (is_ram) {
-		if (!pat_enabled)
+		if (!pat_enabled())
 			return 0;
 
 		pcm = lookup_memtype(paddr);
@@ -841,7 +838,7 @@ int track_pfn_remap(struct vm_area_struct *vma, pgprot_t *prot,
 		return ret;
 	}
 
-	if (!pat_enabled)
+	if (!pat_enabled())
 		return 0;
 
 	/*
@@ -869,7 +866,7 @@ int track_pfn_insert(struct vm_area_struct *vma, pgprot_t *prot,
 {
 	enum page_cache_mode pcm;
 
-	if (!pat_enabled)
+	if (!pat_enabled())
 		return 0;
 
 	/* Set prot based on lookup */
@@ -910,7 +907,7 @@ void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn,
 
 pgprot_t pgprot_writecombine(pgprot_t prot)
 {
-	if (pat_enabled)
+	if (pat_enabled())
 		return __pgprot(pgprot_val(prot) |
 				cachemode2protval(_PAGE_CACHE_MODE_WC));
 	else
@@ -993,7 +990,7 @@ static const struct file_operations memtype_fops = {
 
 static int __init pat_memtype_list_init(void)
 {
-	if (pat_enabled) {
+	if (pat_enabled()) {
 		debugfs_create_file("pat_memtype_list", S_IRUSR,
 				    arch_debugfs_dir, NULL, &memtype_fops);
 	}
diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
index 349c0d32cc0b..0a9f2caf358f 100644
--- a/arch/x86/pci/i386.c
+++ b/arch/x86/pci/i386.c
@@ -429,12 +429,12 @@ int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
  	 * Caller can followup with UC MINUS request and add a WC mtrr if there
  	 * is a free mtrr slot.
  	 */
-	if (!pat_enabled && write_combine)
+	if (!pat_enabled() && write_combine)
 		return -EINVAL;
 
-	if (pat_enabled && write_combine)
+	if (pat_enabled() && write_combine)
 		prot |= cachemode2protval(_PAGE_CACHE_MODE_WC);
-	else if (pat_enabled || boot_cpu_data.x86 > 3)
+	else if (pat_enabled() || boot_cpu_data.x86 > 3)
 		/*
 		 * ioremap() and ioremap_nocache() defaults to UC MINUS for now.
 		 * To avoid attribute conflicts, request UC MINUS here
-- 
2.3.5

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists