linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/10] x86: make pat and mtrr independent from each other
@ 2022-08-20  9:25 Juergen Gross
  2022-08-20  9:25 ` [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs Juergen Gross
                   ` (9 more replies)
  0 siblings, 10 replies; 23+ messages in thread
From: Juergen Gross @ 2022-08-20  9:25 UTC (permalink / raw)
  To: xen-devel, x86, linux-kernel, linux-pm
  Cc: Juergen Gross, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, H. Peter Anvin, stable, Rafael J. Wysocki,
	Pavel Machek, Andy Lutomirski, Peter Zijlstra

Today PAT can't be used without MTRR being available, unless MTRR is at
least configured via CONFIG_MTRR and the system is running as Xen PV
guest. In this case PAT is automatically available via the hypervisor,
but the PAT MSR can't be modified by the kernel and MTRR is disabled.

The same applies to a kernel built with no MTRR support: it won't
allow to use the PAT MSR, even if there is no technical reason for
that, other than setting up PAT on all cpus the same way (which is a
requirement of the processor's cache management) is relying on some
MTRR specific code.

Fix all of that by:

- moving the function needed by PAT from MTRR specific code one level
  up
- reworking the init sequences of MTRR and PAT to be more similar to
  each other without calling PAT from MTRR code
- removing the dependency of PAT on MTRR

While working on that I discovered two minor bugs in MTRR code, which
are fixed, too.

Changes in V2:
- complete rework of the patches based on comments by Boris Petkov
- added several patches to the series

Juergen Gross (10):
  x86/mtrr: fix MTRR fixup on APs
  x86/mtrr: remove unused cyrix_set_all() function
  x86/mtrr: replace use_intel() with a local flag
  x86: move some code out of arch/x86/kernel/cpu/mtrr
  x86/mtrr: split generic_set_all()
  x86/mtrr: remove set_all callback from struct mtrr_ops
  x86/mtrr: simplify mtrr_bp_init()
  x86/mtrr: let cache_aps_delayed_init replace mtrr_aps_delayed_init
  x86/mtrr: add a stop_machine() handler calling only cache_cpu_init()
  x86: decouple pat and mtrr handling

 arch/x86/include/asm/cacheinfo.h   |  14 +++
 arch/x86/include/asm/memtype.h     |   5 +-
 arch/x86/include/asm/mtrr.h        |  12 +--
 arch/x86/kernel/cpu/cacheinfo.c    | 159 +++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/common.c       |   3 +-
 arch/x86/kernel/cpu/mtrr/cyrix.c   |  34 ------
 arch/x86/kernel/cpu/mtrr/generic.c | 120 ++++------------------
 arch/x86/kernel/cpu/mtrr/mtrr.c    | 158 +++++-----------------------
 arch/x86/kernel/cpu/mtrr/mtrr.h    |   5 -
 arch/x86/kernel/setup.c            |  14 +--
 arch/x86/kernel/smpboot.c          |   9 +-
 arch/x86/mm/pat/memtype.c          | 127 +++++++----------------
 arch/x86/power/cpu.c               |   3 +-
 13 files changed, 274 insertions(+), 389 deletions(-)

-- 
2.35.3


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs
  2022-08-20  9:25 [PATCH v2 00/10] x86: make pat and mtrr independent from each other Juergen Gross
@ 2022-08-20  9:25 ` Juergen Gross
  2022-08-20 10:28   ` Greg KH
                     ` (2 more replies)
  2022-08-20  9:25 ` [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function Juergen Gross
                   ` (8 subsequent siblings)
  9 siblings, 3 replies; 23+ messages in thread
From: Juergen Gross @ 2022-08-20  9:25 UTC (permalink / raw)
  To: xen-devel, x86, linux-kernel
  Cc: Juergen Gross, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, H. Peter Anvin, stable

When booting or resuming the system MTRR state is saved on the boot
processor and then this state is loaded into MTRRs of all other cpus.
During update of the MTRRs the MTRR mechanism needs to be disabled by
writing the related MSR. The old contents of this MSR are saved in a
set of static variables and later those static variables are used to
restore the MSR.

In case the MSR contents need to be modified on a cpu due to the MSR
not having been initialized properly by the BIOS, the related update
function is modifying the static variables accordingly.

Unfortunately the MTRR state update is usually running on all cpus
at the same time, so using just one set of static variables for all
cpus is racy in case the MSR contents differ across cpus.

Fix that by using percpu variables for saving the MSR contents.

Cc: stable@vger.kernel.org
Signed-off-by: Juergen Gross <jgross@suse.com>
---
I thought adding a "Fixes:" tag for the kernel's initial git commit
would maybe be entertaining, but without being really helpful.
The percpu variables were preferred over on-stack ones in order to
avoid more code churn in followup patches decoupling PAT from MTRR
support.
V2:
- new patch
---
 arch/x86/kernel/cpu/mtrr/generic.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 558108296f3c..3d185fcf08ca 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -679,7 +679,8 @@ static bool set_mtrr_var_ranges(unsigned int index, struct mtrr_var_range *vr)
 	return changed;
 }
 
-static u32 deftype_lo, deftype_hi;
+static DEFINE_PER_CPU(u32, deftype_lo);
+static DEFINE_PER_CPU(u32, deftype_hi);
 
 /**
  * set_mtrr_state - Set the MTRR state for this CPU.
@@ -691,6 +692,7 @@ static unsigned long set_mtrr_state(void)
 {
 	unsigned long change_mask = 0;
 	unsigned int i;
+	u32 *lo = this_cpu_ptr(&deftype_lo);
 
 	for (i = 0; i < num_var_ranges; i++) {
 		if (set_mtrr_var_ranges(i, &mtrr_state.var_ranges[i]))
@@ -704,10 +706,10 @@ static unsigned long set_mtrr_state(void)
 	 * Set_mtrr_restore restores the old value of MTRRdefType,
 	 * so to set it we fiddle with the saved value:
 	 */
-	if ((deftype_lo & 0xff) != mtrr_state.def_type
-	    || ((deftype_lo & 0xc00) >> 10) != mtrr_state.enabled) {
+	if ((*lo & 0xff) != mtrr_state.def_type
+	    || ((*lo & 0xc00) >> 10) != mtrr_state.enabled) {
 
-		deftype_lo = (deftype_lo & ~0xcff) | mtrr_state.def_type |
+		*lo = (*lo & ~0xcff) | mtrr_state.def_type |
 			     (mtrr_state.enabled << 10);
 		change_mask |= MTRR_CHANGE_MASK_DEFTYPE;
 	}
@@ -729,6 +731,8 @@ static DEFINE_RAW_SPINLOCK(set_atomicity_lock);
 static void prepare_set(void) __acquires(set_atomicity_lock)
 {
 	unsigned long cr0;
+	u32 *lo = this_cpu_ptr(&deftype_lo);
+	u32 *hi = this_cpu_ptr(&deftype_hi);
 
 	/*
 	 * Note that this is not ideal
@@ -763,10 +767,10 @@ static void prepare_set(void) __acquires(set_atomicity_lock)
 	flush_tlb_local();
 
 	/* Save MTRR state */
-	rdmsr(MSR_MTRRdefType, deftype_lo, deftype_hi);
+	rdmsr(MSR_MTRRdefType, *lo, *hi);
 
 	/* Disable MTRRs, and set the default type to uncached */
-	mtrr_wrmsr(MSR_MTRRdefType, deftype_lo & ~0xcff, deftype_hi);
+	mtrr_wrmsr(MSR_MTRRdefType, *lo & ~0xcff, *hi);
 
 	/* Again, only flush caches if we have to. */
 	if (!static_cpu_has(X86_FEATURE_SELFSNOOP))
@@ -775,12 +779,15 @@ static void prepare_set(void) __acquires(set_atomicity_lock)
 
 static void post_set(void) __releases(set_atomicity_lock)
 {
+	u32 *lo = this_cpu_ptr(&deftype_lo);
+	u32 *hi = this_cpu_ptr(&deftype_hi);
+
 	/* Flush TLBs (no need to flush caches - they are disabled) */
 	count_vm_tlb_event(NR_TLB_LOCAL_FLUSH_ALL);
 	flush_tlb_local();
 
 	/* Intel (P6) standard MTRRs */
-	mtrr_wrmsr(MSR_MTRRdefType, deftype_lo, deftype_hi);
+	mtrr_wrmsr(MSR_MTRRdefType, *lo, *hi);
 
 	/* Enable caches */
 	write_cr0(read_cr0() & ~X86_CR0_CD);
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function
  2022-08-20  9:25 [PATCH v2 00/10] x86: make pat and mtrr independent from each other Juergen Gross
  2022-08-20  9:25 ` [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs Juergen Gross
@ 2022-08-20  9:25 ` Juergen Gross
  2022-08-25 10:31   ` Borislav Petkov
  2022-08-20  9:25 ` [PATCH v2 03/10] x86/mtrr: replace use_intel() with a local flag Juergen Gross
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Juergen Gross @ 2022-08-20  9:25 UTC (permalink / raw)
  To: xen-devel, x86, linux-kernel
  Cc: Juergen Gross, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, H. Peter Anvin

The Cyrix cpu specific MTRR function cyrix_set_all() will never be
called, as the struct mtrr_ops set_all() callback will only be called
in the use_intel() case, which would require the use_intel_if member
of struct mtrr_ops to be set, which isn't the case for Cyrix.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
 arch/x86/kernel/cpu/mtrr/cyrix.c | 34 --------------------------------
 1 file changed, 34 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/cyrix.c b/arch/x86/kernel/cpu/mtrr/cyrix.c
index ca670919b561..c77d3b0a5bf2 100644
--- a/arch/x86/kernel/cpu/mtrr/cyrix.c
+++ b/arch/x86/kernel/cpu/mtrr/cyrix.c
@@ -234,42 +234,8 @@ static void cyrix_set_arr(unsigned int reg, unsigned long base,
 	post_set();
 }
 
-typedef struct {
-	unsigned long	base;
-	unsigned long	size;
-	mtrr_type	type;
-} arr_state_t;
-
-static arr_state_t arr_state[8] = {
-	{0UL, 0UL, 0UL}, {0UL, 0UL, 0UL}, {0UL, 0UL, 0UL}, {0UL, 0UL, 0UL},
-	{0UL, 0UL, 0UL}, {0UL, 0UL, 0UL}, {0UL, 0UL, 0UL}, {0UL, 0UL, 0UL}
-};
-
-static unsigned char ccr_state[7] = { 0, 0, 0, 0, 0, 0, 0 };
-
-static void cyrix_set_all(void)
-{
-	int i;
-
-	prepare_set();
-
-	/* the CCRs are not contiguous */
-	for (i = 0; i < 4; i++)
-		setCx86(CX86_CCR0 + i, ccr_state[i]);
-	for (; i < 7; i++)
-		setCx86(CX86_CCR4 + i, ccr_state[i]);
-
-	for (i = 0; i < 8; i++) {
-		cyrix_set_arr(i, arr_state[i].base,
-			      arr_state[i].size, arr_state[i].type);
-	}
-
-	post_set();
-}
-
 static const struct mtrr_ops cyrix_mtrr_ops = {
 	.vendor            = X86_VENDOR_CYRIX,
-	.set_all	   = cyrix_set_all,
 	.set               = cyrix_set_arr,
 	.get               = cyrix_get_arr,
 	.get_free_region   = cyrix_get_free_region,
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 03/10] x86/mtrr: replace use_intel() with a local flag
  2022-08-20  9:25 [PATCH v2 00/10] x86: make pat and mtrr independent from each other Juergen Gross
  2022-08-20  9:25 ` [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs Juergen Gross
  2022-08-20  9:25 ` [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function Juergen Gross
@ 2022-08-20  9:25 ` Juergen Gross
  2022-08-20  9:25 ` [PATCH v2 04/10] x86: move some code out of arch/x86/kernel/cpu/mtrr Juergen Gross
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2022-08-20  9:25 UTC (permalink / raw)
  To: xen-devel, x86, linux-kernel
  Cc: Juergen Gross, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, H. Peter Anvin

In MTRR code use_intel() is only used in one source file, and the
relevant use_intel_if member of struct mtrr_ops is set only in
generic_mtrr_ops.

Replace use_intel() with a single flag in cacheinfo.c, which can be set
when assigning generic_mtrr_ops to mtrr_if. This allows to drop
use_intel_if from mtrr_ops, while preparing to support PAT without
MTRR. As another preparation for the PAT/MTRR decoupling use a bit for
MTRR control and one for PAT control. For now set both bits together,
this can be changed later.

As the new flag will be set only if mtrr_enabled is set, the test for
mtrr_enabled can be dropped at some places.

At the same time drop the local mtrr_enabled() function and rename
the __mtrr_enabled flag to mtrr_enabled.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
 arch/x86/include/asm/cacheinfo.h   |  5 +++
 arch/x86/kernel/cpu/cacheinfo.c    |  3 ++
 arch/x86/kernel/cpu/mtrr/generic.c |  1 -
 arch/x86/kernel/cpu/mtrr/mtrr.c    | 58 ++++++++++++++----------------
 arch/x86/kernel/cpu/mtrr/mtrr.h    |  2 --
 5 files changed, 35 insertions(+), 34 deletions(-)

diff --git a/arch/x86/include/asm/cacheinfo.h b/arch/x86/include/asm/cacheinfo.h
index 86b2e0dcc4bf..1aeafa9888f7 100644
--- a/arch/x86/include/asm/cacheinfo.h
+++ b/arch/x86/include/asm/cacheinfo.h
@@ -2,6 +2,11 @@
 #ifndef _ASM_X86_CACHEINFO_H
 #define _ASM_X86_CACHEINFO_H
 
+/* Kernel controls MTRR and/or PAT MSRs. */
+extern unsigned int cache_generic;
+#define CACHE_GENERIC_MTRR 0x01
+#define CACHE_GENERIC_PAT  0x02
+
 void cacheinfo_amd_init_llc_id(struct cpuinfo_x86 *c, int cpu);
 void cacheinfo_hygon_init_llc_id(struct cpuinfo_x86 *c, int cpu);
 
diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index 66556833d7af..3b05d3ade7a6 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -35,6 +35,9 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_llc_shared_map);
 /* Shared L2 cache maps */
 DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_l2c_shared_map);
 
+/* Kernel controls MTRR and/or PAT MSRs. */
+unsigned int cache_generic;
+
 struct _cache_table {
 	unsigned char descriptor;
 	char cache_type;
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 3d185fcf08ca..3c3c7919de5b 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -921,7 +921,6 @@ int positive_have_wrcomb(void)
  * Generic structure...
  */
 const struct mtrr_ops generic_mtrr_ops = {
-	.use_intel_if		= 1,
 	.set_all		= generic_set_all,
 	.get			= generic_get_mtrr,
 	.get_free_region	= generic_get_free_region,
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 2746cac9d8a9..4cd9c8d1473e 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -46,6 +46,7 @@
 #include <linux/syscore_ops.h>
 #include <linux/rcupdate.h>
 
+#include <asm/cacheinfo.h>
 #include <asm/cpufeature.h>
 #include <asm/e820/api.h>
 #include <asm/mtrr.h>
@@ -58,12 +59,7 @@
 #define MTRR_TO_PHYS_WC_OFFSET 1000
 
 u32 num_var_ranges;
-static bool __mtrr_enabled;
-
-static bool mtrr_enabled(void)
-{
-	return __mtrr_enabled;
-}
+static bool mtrr_enabled;
 
 unsigned int mtrr_usage_table[MTRR_MAX_VAR_RANGES];
 static DEFINE_MUTEX(mtrr_mutex);
@@ -119,11 +115,11 @@ static int have_wrcomb(void)
 }
 
 /*  This function returns the number of variable MTRRs  */
-static void __init set_num_var_ranges(void)
+static void __init set_num_var_ranges(bool use_generic)
 {
 	unsigned long config = 0, dummy;
 
-	if (use_intel())
+	if (use_generic)
 		rdmsr(MSR_MTRRcap, config, dummy);
 	else if (is_cpu(AMD) || is_cpu(HYGON))
 		config = 2;
@@ -303,7 +299,7 @@ int mtrr_add_page(unsigned long base, unsigned long size,
 	int i, replace, error;
 	mtrr_type ltype;
 
-	if (!mtrr_enabled())
+	if (!mtrr_enabled)
 		return -ENXIO;
 
 	error = mtrr_if->validate_add_page(base, size, type);
@@ -451,7 +447,7 @@ static int mtrr_check(unsigned long base, unsigned long size)
 int mtrr_add(unsigned long base, unsigned long size, unsigned int type,
 	     bool increment)
 {
-	if (!mtrr_enabled())
+	if (!mtrr_enabled)
 		return -ENODEV;
 	if (mtrr_check(base, size))
 		return -EINVAL;
@@ -480,7 +476,7 @@ int mtrr_del_page(int reg, unsigned long base, unsigned long size)
 	unsigned long lbase, lsize;
 	int error = -EINVAL;
 
-	if (!mtrr_enabled())
+	if (!mtrr_enabled)
 		return -ENODEV;
 
 	max = num_var_ranges;
@@ -540,7 +536,7 @@ int mtrr_del_page(int reg, unsigned long base, unsigned long size)
  */
 int mtrr_del(int reg, unsigned long base, unsigned long size)
 {
-	if (!mtrr_enabled())
+	if (!mtrr_enabled)
 		return -ENODEV;
 	if (mtrr_check(base, size))
 		return -EINVAL;
@@ -566,7 +562,7 @@ int arch_phys_wc_add(unsigned long base, unsigned long size)
 {
 	int ret;
 
-	if (pat_enabled() || !mtrr_enabled())
+	if (pat_enabled() || !mtrr_enabled)
 		return 0;  /* Success!  (We don't need to do anything.) */
 
 	ret = mtrr_add(base, size, MTRR_TYPE_WRCOMB, true);
@@ -687,6 +683,7 @@ int __initdata changed_by_mtrr_cleanup;
 void __init mtrr_bp_init(void)
 {
 	u32 phys_addr;
+	bool use_generic = false;
 
 	init_ifs();
 
@@ -694,6 +691,7 @@ void __init mtrr_bp_init(void)
 
 	if (boot_cpu_has(X86_FEATURE_MTRR)) {
 		mtrr_if = &generic_mtrr_ops;
+		use_generic = true;
 		size_or_mask = SIZE_OR_MASK_BITS(36);
 		size_and_mask = 0x00f00000;
 		phys_addr = 36;
@@ -755,15 +753,18 @@ void __init mtrr_bp_init(void)
 	}
 
 	if (mtrr_if) {
-		__mtrr_enabled = true;
-		set_num_var_ranges();
+		mtrr_enabled = true;
+		set_num_var_ranges(use_generic);
 		init_table();
-		if (use_intel()) {
+		if (use_generic) {
 			/* BIOS may override */
-			__mtrr_enabled = get_mtrr_state();
+			mtrr_enabled = get_mtrr_state();
 
-			if (mtrr_enabled())
+			if (mtrr_enabled) {
 				mtrr_bp_pat_init();
+				cache_generic |= CACHE_GENERIC_MTRR |
+						 CACHE_GENERIC_PAT;
+			}
 
 			if (mtrr_cleanup(phys_addr)) {
 				changed_by_mtrr_cleanup = 1;
@@ -772,7 +773,7 @@ void __init mtrr_bp_init(void)
 		}
 	}
 
-	if (!mtrr_enabled()) {
+	if (!mtrr_enabled) {
 		pr_info("Disabled\n");
 
 		/*
@@ -786,10 +787,7 @@ void __init mtrr_bp_init(void)
 
 void mtrr_ap_init(void)
 {
-	if (!mtrr_enabled())
-		return;
-
-	if (!use_intel() || mtrr_aps_delayed_init)
+	if (!cache_generic || mtrr_aps_delayed_init)
 		return;
 
 	/*
@@ -816,7 +814,7 @@ void mtrr_save_state(void)
 {
 	int first_cpu;
 
-	if (!mtrr_enabled())
+	if (!mtrr_enabled)
 		return;
 
 	first_cpu = cpumask_first(cpu_online_mask);
@@ -825,9 +823,7 @@ void mtrr_save_state(void)
 
 void set_mtrr_aps_delayed_init(void)
 {
-	if (!mtrr_enabled())
-		return;
-	if (!use_intel())
+	if (!cache_generic)
 		return;
 
 	mtrr_aps_delayed_init = true;
@@ -838,7 +834,7 @@ void set_mtrr_aps_delayed_init(void)
  */
 void mtrr_aps_init(void)
 {
-	if (!use_intel() || !mtrr_enabled())
+	if (!cache_generic)
 		return;
 
 	/*
@@ -855,7 +851,7 @@ void mtrr_aps_init(void)
 
 void mtrr_bp_restore(void)
 {
-	if (!use_intel() || !mtrr_enabled())
+	if (!cache_generic)
 		return;
 
 	mtrr_if->set_all();
@@ -863,10 +859,10 @@ void mtrr_bp_restore(void)
 
 static int __init mtrr_init_finialize(void)
 {
-	if (!mtrr_enabled())
+	if (!mtrr_enabled)
 		return 0;
 
-	if (use_intel()) {
+	if (cache_generic & CACHE_GENERIC_MTRR) {
 		if (!changed_by_mtrr_cleanup)
 			mtrr_state_warn();
 		return 0;
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.h b/arch/x86/kernel/cpu/mtrr/mtrr.h
index 2ac99e561181..88b1c4b6174a 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.h
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.h
@@ -14,7 +14,6 @@ extern unsigned int mtrr_usage_table[MTRR_MAX_VAR_RANGES];
 
 struct mtrr_ops {
 	u32	vendor;
-	u32	use_intel_if;
 	void	(*set)(unsigned int reg, unsigned long base,
 		       unsigned long size, mtrr_type type);
 	void	(*set_all)(void);
@@ -61,7 +60,6 @@ extern u64 size_or_mask, size_and_mask;
 extern const struct mtrr_ops *mtrr_if;
 
 #define is_cpu(vnd)	(mtrr_if && mtrr_if->vendor == X86_VENDOR_##vnd)
-#define use_intel()	(mtrr_if && mtrr_if->use_intel_if == 1)
 
 extern unsigned int num_var_ranges;
 extern u64 mtrr_tom2;
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 04/10] x86: move some code out of arch/x86/kernel/cpu/mtrr
  2022-08-20  9:25 [PATCH v2 00/10] x86: make pat and mtrr independent from each other Juergen Gross
                   ` (2 preceding siblings ...)
  2022-08-20  9:25 ` [PATCH v2 03/10] x86/mtrr: replace use_intel() with a local flag Juergen Gross
@ 2022-08-20  9:25 ` Juergen Gross
  2022-08-20  9:25 ` [PATCH v2 05/10] x86/mtrr: split generic_set_all() Juergen Gross
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2022-08-20  9:25 UTC (permalink / raw)
  To: xen-devel, x86, linux-kernel
  Cc: Juergen Gross, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, H. Peter Anvin

Prepare making PAT and MTRR support independent from each other by
moving some code needed by both out of the MTRR specific sources.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- move code from cpu/common.c to cpu/cacheinfo.c (Boris Petkov)
---
 arch/x86/include/asm/cacheinfo.h   |  3 ++
 arch/x86/include/asm/mtrr.h        |  4 ++
 arch/x86/kernel/cpu/cacheinfo.c    | 77 ++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/mtrr/generic.c | 80 ++++--------------------------
 4 files changed, 93 insertions(+), 71 deletions(-)

diff --git a/arch/x86/include/asm/cacheinfo.h b/arch/x86/include/asm/cacheinfo.h
index 1aeafa9888f7..313a6920d0f9 100644
--- a/arch/x86/include/asm/cacheinfo.h
+++ b/arch/x86/include/asm/cacheinfo.h
@@ -10,4 +10,7 @@ extern unsigned int cache_generic;
 void cacheinfo_amd_init_llc_id(struct cpuinfo_x86 *c, int cpu);
 void cacheinfo_hygon_init_llc_id(struct cpuinfo_x86 *c, int cpu);
 
+void cache_disable(void);
+void cache_enable(void);
+
 #endif /* _ASM_X86_CACHEINFO_H */
diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index 76d726074c16..12a16caed395 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -48,6 +48,8 @@ extern void mtrr_aps_init(void);
 extern void mtrr_bp_restore(void);
 extern int mtrr_trim_uncached_memory(unsigned long end_pfn);
 extern int amd_special_default_mtrr(void);
+void mtrr_disable(void);
+void mtrr_enable(void);
 #  else
 static inline u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform)
 {
@@ -87,6 +89,8 @@ static inline void mtrr_centaur_report_mcr(int mcr, u32 lo, u32 hi)
 #define set_mtrr_aps_delayed_init() do {} while (0)
 #define mtrr_aps_init() do {} while (0)
 #define mtrr_bp_restore() do {} while (0)
+#define mtrr_disable() do {} while (0)
+#define mtrr_enable() do {} while (0)
 #  endif
 
 #ifdef CONFIG_COMPAT
diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index 3b05d3ade7a6..47e2c72fa8a4 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -20,6 +20,8 @@
 #include <asm/cacheinfo.h>
 #include <asm/amd_nb.h>
 #include <asm/smp.h>
+#include <asm/mtrr.h>
+#include <asm/tlbflush.h>
 
 #include "cpu.h"
 
@@ -1043,3 +1045,78 @@ int populate_cache_leaves(unsigned int cpu)
 
 	return 0;
 }
+
+/*
+ * Disable and enable caches. Needed for changing MTRRs and the PAT MSR.
+ *
+ * Since we are disabling the cache don't allow any interrupts,
+ * they would run extremely slow and would only increase the pain.
+ *
+ * The caller must ensure that local interrupts are disabled and
+ * are reenabled after cache_enable() has been called.
+ */
+static unsigned long saved_cr4;
+static DEFINE_RAW_SPINLOCK(cache_disable_lock);
+
+void cache_disable(void) __acquires(cache_disable_lock)
+{
+	unsigned long cr0;
+
+	/*
+	 * Note that this is not ideal
+	 * since the cache is only flushed/disabled for this CPU while the
+	 * MTRRs are changed, but changing this requires more invasive
+	 * changes to the way the kernel boots
+	 */
+
+	raw_spin_lock(&cache_disable_lock);
+
+	/* Enter the no-fill (CD=1, NW=0) cache mode and flush caches. */
+	cr0 = read_cr0() | X86_CR0_CD;
+	write_cr0(cr0);
+
+	/*
+	 * Cache flushing is the most time-consuming step when programming
+	 * the MTRRs. Fortunately, as per the Intel Software Development
+	 * Manual, we can skip it if the processor supports cache self-
+	 * snooping.
+	 */
+	if (!static_cpu_has(X86_FEATURE_SELFSNOOP))
+		wbinvd();
+
+	/* Save value of CR4 and clear Page Global Enable (bit 7) */
+	if (boot_cpu_has(X86_FEATURE_PGE)) {
+		saved_cr4 = __read_cr4();
+		__write_cr4(saved_cr4 & ~X86_CR4_PGE);
+	}
+
+	/* Flush all TLBs via a mov %cr3, %reg; mov %reg, %cr3 */
+	count_vm_tlb_event(NR_TLB_LOCAL_FLUSH_ALL);
+	flush_tlb_local();
+
+	if (boot_cpu_has(X86_FEATURE_MTRR))
+		mtrr_disable();
+
+	/* Again, only flush caches if we have to. */
+	if (!static_cpu_has(X86_FEATURE_SELFSNOOP))
+		wbinvd();
+}
+
+void cache_enable(void) __releases(cache_disable_lock)
+{
+	/* Flush TLBs (no need to flush caches - they are disabled) */
+	count_vm_tlb_event(NR_TLB_LOCAL_FLUSH_ALL);
+	flush_tlb_local();
+
+	if (boot_cpu_has(X86_FEATURE_MTRR))
+		mtrr_enable();
+
+	/* Enable caches */
+	write_cr0(read_cr0() & ~X86_CR0_CD);
+
+	/* Restore value of CR4 */
+	if (boot_cpu_has(X86_FEATURE_PGE))
+		__write_cr4(saved_cr4);
+
+	raw_spin_unlock(&cache_disable_lock);
+}
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 3c3c7919de5b..a9ac159b1566 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -10,6 +10,7 @@
 #include <linux/mm.h>
 
 #include <asm/processor-flags.h>
+#include <asm/cacheinfo.h>
 #include <asm/cpufeature.h>
 #include <asm/tlbflush.h>
 #include <asm/mtrr.h>
@@ -396,9 +397,6 @@ print_fixed(unsigned base, unsigned step, const mtrr_type *types)
 	}
 }
 
-static void prepare_set(void);
-static void post_set(void);
-
 static void __init print_mtrr_state(void)
 {
 	unsigned int i;
@@ -450,11 +448,11 @@ void __init mtrr_bp_pat_init(void)
 	unsigned long flags;
 
 	local_irq_save(flags);
-	prepare_set();
+	cache_disable();
 
 	pat_init();
 
-	post_set();
+	cache_enable();
 	local_irq_restore(flags);
 }
 
@@ -717,85 +715,25 @@ static unsigned long set_mtrr_state(void)
 	return change_mask;
 }
 
-
-static unsigned long cr4;
-static DEFINE_RAW_SPINLOCK(set_atomicity_lock);
-
-/*
- * Since we are disabling the cache don't allow any interrupts,
- * they would run extremely slow and would only increase the pain.
- *
- * The caller must ensure that local interrupts are disabled and
- * are reenabled after post_set() has been called.
- */
-static void prepare_set(void) __acquires(set_atomicity_lock)
+void mtrr_disable(void)
 {
-	unsigned long cr0;
 	u32 *lo = this_cpu_ptr(&deftype_lo);
 	u32 *hi = this_cpu_ptr(&deftype_hi);
 
-	/*
-	 * Note that this is not ideal
-	 * since the cache is only flushed/disabled for this CPU while the
-	 * MTRRs are changed, but changing this requires more invasive
-	 * changes to the way the kernel boots
-	 */
-
-	raw_spin_lock(&set_atomicity_lock);
-
-	/* Enter the no-fill (CD=1, NW=0) cache mode and flush caches. */
-	cr0 = read_cr0() | X86_CR0_CD;
-	write_cr0(cr0);
-
-	/*
-	 * Cache flushing is the most time-consuming step when programming
-	 * the MTRRs. Fortunately, as per the Intel Software Development
-	 * Manual, we can skip it if the processor supports cache self-
-	 * snooping.
-	 */
-	if (!static_cpu_has(X86_FEATURE_SELFSNOOP))
-		wbinvd();
-
-	/* Save value of CR4 and clear Page Global Enable (bit 7) */
-	if (boot_cpu_has(X86_FEATURE_PGE)) {
-		cr4 = __read_cr4();
-		__write_cr4(cr4 & ~X86_CR4_PGE);
-	}
-
-	/* Flush all TLBs via a mov %cr3, %reg; mov %reg, %cr3 */
-	count_vm_tlb_event(NR_TLB_LOCAL_FLUSH_ALL);
-	flush_tlb_local();
-
 	/* Save MTRR state */
 	rdmsr(MSR_MTRRdefType, *lo, *hi);
 
 	/* Disable MTRRs, and set the default type to uncached */
 	mtrr_wrmsr(MSR_MTRRdefType, *lo & ~0xcff, *hi);
-
-	/* Again, only flush caches if we have to. */
-	if (!static_cpu_has(X86_FEATURE_SELFSNOOP))
-		wbinvd();
 }
 
-static void post_set(void) __releases(set_atomicity_lock)
+void mtrr_enable(void)
 {
 	u32 *lo = this_cpu_ptr(&deftype_lo);
 	u32 *hi = this_cpu_ptr(&deftype_hi);
 
-	/* Flush TLBs (no need to flush caches - they are disabled) */
-	count_vm_tlb_event(NR_TLB_LOCAL_FLUSH_ALL);
-	flush_tlb_local();
-
 	/* Intel (P6) standard MTRRs */
 	mtrr_wrmsr(MSR_MTRRdefType, *lo, *hi);
-
-	/* Enable caches */
-	write_cr0(read_cr0() & ~X86_CR0_CD);
-
-	/* Restore value of CR4 */
-	if (boot_cpu_has(X86_FEATURE_PGE))
-		__write_cr4(cr4);
-	raw_spin_unlock(&set_atomicity_lock);
 }
 
 static void generic_set_all(void)
@@ -804,7 +742,7 @@ static void generic_set_all(void)
 	unsigned long flags;
 
 	local_irq_save(flags);
-	prepare_set();
+	cache_disable();
 
 	/* Actually set the state */
 	mask = set_mtrr_state();
@@ -812,7 +750,7 @@ static void generic_set_all(void)
 	/* also set PAT */
 	pat_init();
 
-	post_set();
+	cache_enable();
 	local_irq_restore(flags);
 
 	/* Use the atomic bitops to update the global mask */
@@ -843,7 +781,7 @@ static void generic_set_mtrr(unsigned int reg, unsigned long base,
 	vr = &mtrr_state.var_ranges[reg];
 
 	local_irq_save(flags);
-	prepare_set();
+	cache_disable();
 
 	if (size == 0) {
 		/*
@@ -862,7 +800,7 @@ static void generic_set_mtrr(unsigned int reg, unsigned long base,
 		mtrr_wrmsr(MTRRphysMask_MSR(reg), vr->mask_lo, vr->mask_hi);
 	}
 
-	post_set();
+	cache_enable();
 	local_irq_restore(flags);
 }
 
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 05/10] x86/mtrr: split generic_set_all()
  2022-08-20  9:25 [PATCH v2 00/10] x86: make pat and mtrr independent from each other Juergen Gross
                   ` (3 preceding siblings ...)
  2022-08-20  9:25 ` [PATCH v2 04/10] x86: move some code out of arch/x86/kernel/cpu/mtrr Juergen Gross
@ 2022-08-20  9:25 ` Juergen Gross
  2022-08-20  9:25 ` [PATCH v2 06/10] x86/mtrr: remove set_all callback from struct mtrr_ops Juergen Gross
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2022-08-20  9:25 UTC (permalink / raw)
  To: xen-devel, x86, linux-kernel
  Cc: Juergen Gross, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, H. Peter Anvin

Split generic_set_all() into multiple parts, while moving the main
function body into cacheinfo.c.

This prepares the support of PAT without needing MTRR support by
moving the main function body of generic_set_all() into cacheinfo.c
while renaming it to cache_cpu_init(). The MTRR specific parts are
moved into a dedicated small function called by cache_cpu_init().
The PAT and MTRR specific functions are called conditionally based
on the cache_generic bit settings.

The setting of smp_changes_mask is merged into the (new) function
mtrr_generic_set_state() used to call set_mtrr_state(). It was
probably split in ancient times, as atomic operations while running
uncached might be quite expensive, but OTOH only systems with a
broken BIOS should ever require to set any bit in smp_changes_mask,
so just hurting those devices with a penalty of a few microseconds
during boot shouldn't be a real issue.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
 arch/x86/include/asm/cacheinfo.h   |  1 +
 arch/x86/include/asm/mtrr.h        |  2 ++
 arch/x86/kernel/cpu/cacheinfo.c    | 19 +++++++++++++++++++
 arch/x86/kernel/cpu/mtrr/generic.c | 15 ++-------------
 4 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/cacheinfo.h b/arch/x86/include/asm/cacheinfo.h
index 313a6920d0f9..563d9cb5fcf5 100644
--- a/arch/x86/include/asm/cacheinfo.h
+++ b/arch/x86/include/asm/cacheinfo.h
@@ -12,5 +12,6 @@ void cacheinfo_hygon_init_llc_id(struct cpuinfo_x86 *c, int cpu);
 
 void cache_disable(void);
 void cache_enable(void);
+void cache_cpu_init(void);
 
 #endif /* _ASM_X86_CACHEINFO_H */
diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index 12a16caed395..986249a2b9b6 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -50,6 +50,7 @@ extern int mtrr_trim_uncached_memory(unsigned long end_pfn);
 extern int amd_special_default_mtrr(void);
 void mtrr_disable(void);
 void mtrr_enable(void);
+void mtrr_generic_set_state(void);
 #  else
 static inline u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform)
 {
@@ -91,6 +92,7 @@ static inline void mtrr_centaur_report_mcr(int mcr, u32 lo, u32 hi)
 #define mtrr_bp_restore() do {} while (0)
 #define mtrr_disable() do {} while (0)
 #define mtrr_enable() do {} while (0)
+#define mtrr_generic_set_state() do {} while (0)
 #  endif
 
 #ifdef CONFIG_COMPAT
diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index 47e2c72fa8a4..36378604ec61 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -1120,3 +1120,22 @@ void cache_enable(void) __releases(cache_disable_lock)
 
 	raw_spin_unlock(&cache_disable_lock);
 }
+
+void cache_cpu_init(void)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+	cache_disable();
+
+	/* Set MTRR state. */
+	if (cache_generic & CACHE_GENERIC_MTRR)
+		mtrr_generic_set_state();
+
+	/* Set PAT. */
+	if (cache_generic & CACHE_GENERIC_PAT)
+		pat_init();
+
+	cache_enable();
+	local_irq_restore(flags);
+}
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index a9ac159b1566..0be6d43ec1fe 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -736,30 +736,19 @@ void mtrr_enable(void)
 	mtrr_wrmsr(MSR_MTRRdefType, *lo, *hi);
 }
 
-static void generic_set_all(void)
+void mtrr_generic_set_state(void)
 {
 	unsigned long mask, count;
-	unsigned long flags;
-
-	local_irq_save(flags);
-	cache_disable();
 
 	/* Actually set the state */
 	mask = set_mtrr_state();
 
-	/* also set PAT */
-	pat_init();
-
-	cache_enable();
-	local_irq_restore(flags);
-
 	/* Use the atomic bitops to update the global mask */
 	for (count = 0; count < sizeof(mask) * 8; ++count) {
 		if (mask & 0x01)
 			set_bit(count, &smp_changes_mask);
 		mask >>= 1;
 	}
-
 }
 
 /**
@@ -859,7 +848,7 @@ int positive_have_wrcomb(void)
  * Generic structure...
  */
 const struct mtrr_ops generic_mtrr_ops = {
-	.set_all		= generic_set_all,
+	.set_all		= cache_cpu_init,
 	.get			= generic_get_mtrr,
 	.get_free_region	= generic_get_free_region,
 	.set			= generic_set_mtrr,
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 06/10] x86/mtrr: remove set_all callback from struct mtrr_ops
  2022-08-20  9:25 [PATCH v2 00/10] x86: make pat and mtrr independent from each other Juergen Gross
                   ` (4 preceding siblings ...)
  2022-08-20  9:25 ` [PATCH v2 05/10] x86/mtrr: split generic_set_all() Juergen Gross
@ 2022-08-20  9:25 ` Juergen Gross
  2022-08-20  9:25 ` [PATCH v2 07/10] x86/mtrr: simplify mtrr_bp_init() Juergen Gross
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2022-08-20  9:25 UTC (permalink / raw)
  To: xen-devel, x86, linux-kernel
  Cc: Juergen Gross, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, H. Peter Anvin

Instead of using an indirect call to mtrr_if->set_all just call the
only possible target cache_cpu_init() directly. This enables to remove
the set_all callback from struct mtrr_ops.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/kernel/cpu/mtrr/generic.c |  1 -
 arch/x86/kernel/cpu/mtrr/mtrr.c    | 10 +++++-----
 arch/x86/kernel/cpu/mtrr/mtrr.h    |  2 --
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 0be6d43ec1fe..1c6260b94cb9 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -848,7 +848,6 @@ int positive_have_wrcomb(void)
  * Generic structure...
  */
 const struct mtrr_ops generic_mtrr_ops = {
-	.set_all		= cache_cpu_init,
 	.get			= generic_get_mtrr,
 	.get_free_region	= generic_get_free_region,
 	.set			= generic_set_mtrr,
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 4cd9c8d1473e..19662ab057be 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -165,15 +165,15 @@ static int mtrr_rendezvous_handler(void *info)
 	 * saved, and we want to replicate that across all the cpus that come
 	 * online (either at the end of boot or resume or during a runtime cpu
 	 * online). If we're doing that, @reg is set to something special and on
-	 * all the cpu's we do mtrr_if->set_all() (On the logical cpu that
+	 * all the cpu's we do cache_cpu_init() (On the logical cpu that
 	 * started the boot/resume sequence, this might be a duplicate
-	 * set_all()).
+	 * cache_cpu_init()).
 	 */
 	if (data->smp_reg != ~0U) {
 		mtrr_if->set(data->smp_reg, data->smp_base,
 			     data->smp_size, data->smp_type);
 	} else if (mtrr_aps_delayed_init || !cpu_online(smp_processor_id())) {
-		mtrr_if->set_all();
+		cache_cpu_init();
 	}
 	return 0;
 }
@@ -768,7 +768,7 @@ void __init mtrr_bp_init(void)
 
 			if (mtrr_cleanup(phys_addr)) {
 				changed_by_mtrr_cleanup = 1;
-				mtrr_if->set_all();
+				cache_cpu_init();
 			}
 		}
 	}
@@ -854,7 +854,7 @@ void mtrr_bp_restore(void)
 	if (!cache_generic)
 		return;
 
-	mtrr_if->set_all();
+	cache_cpu_init();
 }
 
 static int __init mtrr_init_finialize(void)
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.h b/arch/x86/kernel/cpu/mtrr/mtrr.h
index 88b1c4b6174a..3b1883185185 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.h
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.h
@@ -16,8 +16,6 @@ struct mtrr_ops {
 	u32	vendor;
 	void	(*set)(unsigned int reg, unsigned long base,
 		       unsigned long size, mtrr_type type);
-	void	(*set_all)(void);
-
 	void	(*get)(unsigned int reg, unsigned long *base,
 		       unsigned long *size, mtrr_type *type);
 	int	(*get_free_region)(unsigned long base, unsigned long size,
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 07/10] x86/mtrr: simplify mtrr_bp_init()
  2022-08-20  9:25 [PATCH v2 00/10] x86: make pat and mtrr independent from each other Juergen Gross
                   ` (5 preceding siblings ...)
  2022-08-20  9:25 ` [PATCH v2 06/10] x86/mtrr: remove set_all callback from struct mtrr_ops Juergen Gross
@ 2022-08-20  9:25 ` Juergen Gross
  2022-08-20  9:25 ` [PATCH v2 08/10] x86/mtrr: let cache_aps_delayed_init replace mtrr_aps_delayed_init Juergen Gross
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2022-08-20  9:25 UTC (permalink / raw)
  To: xen-devel, x86, linux-kernel
  Cc: Juergen Gross, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, H. Peter Anvin

In case of the generic cache interface being used (Intel cpus or a
64-bit system), the initialization sequence of the boot cpu is more
complicated as necessary:

- check if MTRR enabled, if yes, call mtrr_bp_pat_init() which will
  disable caching, set the PAT MSR, and reenable caching

- call mtrr_cleanup(), in case that changed anything, call
  cache_cpu_init() doing the same caching disable/enable dance as
  above, but this time with setting the (modified) MTRR state (even
  if MTRR was disabled) AND setting the PAT MSR (again even with
  disabled MTRR)

The sequence can be simplified a lot while removing potential
inconsistencies:

- check if MTRR enabled, if yes, call mtrr_cleanup() and then
  cache_cpu_init()

This ensures to:

- no longer disable/enable caching more than once

- avoid to set MTRRs and/or the PAT MSR on the boot processor in case
  of MTRR cleanups even if MTRRs meant to be disabled

With that mtrr_bp_pat_init() can be removed.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
 arch/x86/kernel/cpu/mtrr/generic.c | 14 --------------
 arch/x86/kernel/cpu/mtrr/mtrr.c    |  7 ++-----
 arch/x86/kernel/cpu/mtrr/mtrr.h    |  1 -
 3 files changed, 2 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 1c6260b94cb9..b6be476c77af 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -442,20 +442,6 @@ 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);
-	cache_disable();
-
-	pat_init();
-
-	cache_enable();
-	local_irq_restore(flags);
-}
-
 /* Grab all of the MTRR state for this CPU into *state */
 bool __init get_mtrr_state(void)
 {
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 19662ab057be..ca348dcc5bf8 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -761,13 +761,10 @@ void __init mtrr_bp_init(void)
 			mtrr_enabled = get_mtrr_state();
 
 			if (mtrr_enabled) {
-				mtrr_bp_pat_init();
 				cache_generic |= CACHE_GENERIC_MTRR |
 						 CACHE_GENERIC_PAT;
-			}
-
-			if (mtrr_cleanup(phys_addr)) {
-				changed_by_mtrr_cleanup = 1;
+				changed_by_mtrr_cleanup =
+					mtrr_cleanup(phys_addr);
 				cache_cpu_init();
 			}
 		}
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.h b/arch/x86/kernel/cpu/mtrr/mtrr.h
index 3b1883185185..c98928ceee6a 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.h
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.h
@@ -50,7 +50,6 @@ 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 __init set_mtrr_ops(const struct mtrr_ops *ops);
 
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 08/10] x86/mtrr: let cache_aps_delayed_init replace mtrr_aps_delayed_init
  2022-08-20  9:25 [PATCH v2 00/10] x86: make pat and mtrr independent from each other Juergen Gross
                   ` (6 preceding siblings ...)
  2022-08-20  9:25 ` [PATCH v2 07/10] x86/mtrr: simplify mtrr_bp_init() Juergen Gross
@ 2022-08-20  9:25 ` Juergen Gross
  2022-08-20  9:25 ` [PATCH v2 09/10] x86/mtrr: add a stop_machine() handler calling only cache_cpu_init() Juergen Gross
  2022-08-20  9:25 ` [PATCH v2 10/10] x86: decouple pat and mtrr handling Juergen Gross
  9 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2022-08-20  9:25 UTC (permalink / raw)
  To: xen-devel, x86, linux-kernel
  Cc: Juergen Gross, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, H. Peter Anvin

In order to prepare decoupling MTRR and PAT replace the MTRR specific
mtrr_aps_delayed_init flag with a more generic cache_aps_delayed_init
one.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
 arch/x86/include/asm/cacheinfo.h |  2 ++
 arch/x86/include/asm/mtrr.h      |  2 --
 arch/x86/kernel/cpu/cacheinfo.c  |  2 ++
 arch/x86/kernel/cpu/mtrr/mtrr.c  | 17 ++++-------------
 arch/x86/kernel/smpboot.c        |  5 +++--
 5 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/cacheinfo.h b/arch/x86/include/asm/cacheinfo.h
index 563d9cb5fcf5..e80ed3c523c8 100644
--- a/arch/x86/include/asm/cacheinfo.h
+++ b/arch/x86/include/asm/cacheinfo.h
@@ -7,6 +7,8 @@ extern unsigned int cache_generic;
 #define CACHE_GENERIC_MTRR 0x01
 #define CACHE_GENERIC_PAT  0x02
 
+extern bool cache_aps_delayed_init;
+
 void cacheinfo_amd_init_llc_id(struct cpuinfo_x86 *c, int cpu);
 void cacheinfo_hygon_init_llc_id(struct cpuinfo_x86 *c, int cpu);
 
diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index 986249a2b9b6..5d31219c8529 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -43,7 +43,6 @@ extern int mtrr_del(int reg, unsigned long base, unsigned long size);
 extern int mtrr_del_page(int reg, unsigned long base, unsigned long size);
 extern void mtrr_centaur_report_mcr(int mcr, u32 lo, u32 hi);
 extern void mtrr_ap_init(void);
-extern void set_mtrr_aps_delayed_init(void);
 extern void mtrr_aps_init(void);
 extern void mtrr_bp_restore(void);
 extern int mtrr_trim_uncached_memory(unsigned long end_pfn);
@@ -87,7 +86,6 @@ static inline void mtrr_centaur_report_mcr(int mcr, u32 lo, u32 hi)
 {
 }
 #define mtrr_ap_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)
 #define mtrr_disable() do {} while (0)
diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index 36378604ec61..c6e7c93e45e8 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -1139,3 +1139,5 @@ void cache_cpu_init(void)
 	cache_enable();
 	local_irq_restore(flags);
 }
+
+bool cache_aps_delayed_init;
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index ca348dcc5bf8..b68ec43e33a2 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -65,7 +65,6 @@ unsigned int mtrr_usage_table[MTRR_MAX_VAR_RANGES];
 static DEFINE_MUTEX(mtrr_mutex);
 
 u64 size_or_mask, size_and_mask;
-static bool mtrr_aps_delayed_init;
 
 static const struct mtrr_ops *mtrr_ops[X86_VENDOR_NUM] __ro_after_init;
 
@@ -172,7 +171,7 @@ static int mtrr_rendezvous_handler(void *info)
 	if (data->smp_reg != ~0U) {
 		mtrr_if->set(data->smp_reg, data->smp_base,
 			     data->smp_size, data->smp_type);
-	} else if (mtrr_aps_delayed_init || !cpu_online(smp_processor_id())) {
+	} else if (cache_aps_delayed_init || !cpu_online(smp_processor_id())) {
 		cache_cpu_init();
 	}
 	return 0;
@@ -784,7 +783,7 @@ void __init mtrr_bp_init(void)
 
 void mtrr_ap_init(void)
 {
-	if (!cache_generic || mtrr_aps_delayed_init)
+	if (!cache_generic || cache_aps_delayed_init)
 		return;
 
 	/*
@@ -818,14 +817,6 @@ void mtrr_save_state(void)
 	smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1);
 }
 
-void set_mtrr_aps_delayed_init(void)
-{
-	if (!cache_generic)
-		return;
-
-	mtrr_aps_delayed_init = true;
-}
-
 /*
  * Delayed MTRR initialization for all AP's
  */
@@ -839,11 +830,11 @@ void mtrr_aps_init(void)
 	 * by doing set_mtrr_aps_delayed_init(), prior to this point. If not,
 	 * then we are done.
 	 */
-	if (!mtrr_aps_delayed_init)
+	if (!cache_aps_delayed_init)
 		return;
 
 	set_mtrr(~0U, 0, 0, 0);
-	mtrr_aps_delayed_init = false;
+	cache_aps_delayed_init = false;
 }
 
 void mtrr_bp_restore(void)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index f24227bc3220..ef7bce21cbe8 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -58,6 +58,7 @@
 #include <linux/overflow.h>
 
 #include <asm/acpi.h>
+#include <asm/cacheinfo.h>
 #include <asm/desc.h>
 #include <asm/nmi.h>
 #include <asm/irq.h>
@@ -1428,7 +1429,7 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
 
 	uv_system_init();
 
-	set_mtrr_aps_delayed_init();
+	cache_aps_delayed_init = true;
 
 	smp_quirk_init_udelay();
 
@@ -1439,7 +1440,7 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
 
 void arch_thaw_secondary_cpus_begin(void)
 {
-	set_mtrr_aps_delayed_init();
+	cache_aps_delayed_init = true;
 }
 
 void arch_thaw_secondary_cpus_end(void)
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 09/10] x86/mtrr: add a stop_machine() handler calling only cache_cpu_init()
  2022-08-20  9:25 [PATCH v2 00/10] x86: make pat and mtrr independent from each other Juergen Gross
                   ` (7 preceding siblings ...)
  2022-08-20  9:25 ` [PATCH v2 08/10] x86/mtrr: let cache_aps_delayed_init replace mtrr_aps_delayed_init Juergen Gross
@ 2022-08-20  9:25 ` Juergen Gross
  2022-08-20  9:25 ` [PATCH v2 10/10] x86: decouple pat and mtrr handling Juergen Gross
  9 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2022-08-20  9:25 UTC (permalink / raw)
  To: xen-devel, x86, linux-kernel, linux-pm
  Cc: Juergen Gross, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, H. Peter Anvin, Rafael J. Wysocki, Pavel Machek

Instead of having a stop_machine() handler for either a specific MTRR
register or all state at once, add a handler just for calling
cache_cpu_init() if appropriate.

Add functions for calling stop_machine() with this handler as well.

Add a generic replacements for mtrr_bp_restore() and a wrapper for
mtrr_bp_init().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- completely new replacement of former patch 2
---
 arch/x86/include/asm/cacheinfo.h |  5 +-
 arch/x86/include/asm/mtrr.h      |  4 --
 arch/x86/kernel/cpu/cacheinfo.c  | 59 +++++++++++++++++++++-
 arch/x86/kernel/cpu/common.c     |  3 +-
 arch/x86/kernel/cpu/mtrr/mtrr.c  | 87 +-------------------------------
 arch/x86/kernel/setup.c          |  3 +-
 arch/x86/kernel/smpboot.c        |  4 +-
 arch/x86/power/cpu.c             |  3 +-
 8 files changed, 72 insertions(+), 96 deletions(-)

diff --git a/arch/x86/include/asm/cacheinfo.h b/arch/x86/include/asm/cacheinfo.h
index e80ed3c523c8..a122a1aad936 100644
--- a/arch/x86/include/asm/cacheinfo.h
+++ b/arch/x86/include/asm/cacheinfo.h
@@ -14,6 +14,9 @@ void cacheinfo_hygon_init_llc_id(struct cpuinfo_x86 *c, int cpu);
 
 void cache_disable(void);
 void cache_enable(void);
-void cache_cpu_init(void);
+void cache_bp_init(void);
+void cache_bp_restore(void);
+void cache_ap_init(void);
+void cache_aps_init(void);
 
 #endif /* _ASM_X86_CACHEINFO_H */
diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index 5d31219c8529..ec73d1e5bafb 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -42,8 +42,6 @@ extern int mtrr_add_page(unsigned long base, unsigned long size,
 extern int mtrr_del(int reg, unsigned long base, unsigned long size);
 extern int mtrr_del_page(int reg, unsigned long base, unsigned long size);
 extern void mtrr_centaur_report_mcr(int mcr, u32 lo, u32 hi);
-extern void mtrr_ap_init(void);
-extern void mtrr_aps_init(void);
 extern void mtrr_bp_restore(void);
 extern int mtrr_trim_uncached_memory(unsigned long end_pfn);
 extern int amd_special_default_mtrr(void);
@@ -85,8 +83,6 @@ static inline int mtrr_trim_uncached_memory(unsigned long end_pfn)
 static inline void mtrr_centaur_report_mcr(int mcr, u32 lo, u32 hi)
 {
 }
-#define mtrr_ap_init() do {} while (0)
-#define mtrr_aps_init() do {} while (0)
 #define mtrr_bp_restore() do {} while (0)
 #define mtrr_disable() do {} while (0)
 #define mtrr_enable() do {} while (0)
diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index c6e7c93e45e8..4946f93eb16f 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -15,6 +15,7 @@
 #include <linux/capability.h>
 #include <linux/sysfs.h>
 #include <linux/pci.h>
+#include <linux/stop_machine.h>
 
 #include <asm/cpufeature.h>
 #include <asm/cacheinfo.h>
@@ -1121,7 +1122,7 @@ void cache_enable(void) __releases(cache_disable_lock)
 	raw_spin_unlock(&cache_disable_lock);
 }
 
-void cache_cpu_init(void)
+static void cache_cpu_init(void)
 {
 	unsigned long flags;
 
@@ -1141,3 +1142,59 @@ void cache_cpu_init(void)
 }
 
 bool cache_aps_delayed_init;
+
+static int cache_rendezvous_handler(void *unused)
+{
+	if (cache_aps_delayed_init || !cpu_online(smp_processor_id()))
+		cache_cpu_init();
+
+	return 0;
+}
+
+void __init cache_bp_init(void)
+{
+	mtrr_bp_init();
+
+	if (cache_generic)
+		cache_cpu_init();
+}
+
+void cache_bp_restore(void)
+{
+	if (cache_generic)
+		cache_cpu_init();
+}
+
+void cache_ap_init(void)
+{
+	if (!cache_generic || cache_aps_delayed_init)
+		return;
+
+	/*
+	 * Ideally we should hold mtrr_mutex here to avoid mtrr entries
+	 * changed, but this routine will be called in cpu boot time,
+	 * holding the lock breaks it.
+	 *
+	 * This routine is called in two cases:
+	 *
+	 *   1. very early time of software resume, when there absolutely
+	 *      isn't mtrr entry changes;
+	 *
+	 *   2. cpu hotadd time. We let mtrr_add/del_page hold cpuhotplug
+	 *      lock to prevent mtrr entry changes
+	 */
+	stop_machine_from_inactive_cpu(cache_rendezvous_handler, NULL,
+				       cpu_callout_mask);
+}
+
+/*
+ * Delayed cache initialization for all AP's
+ */
+void cache_aps_init(void)
+{
+	if (!cache_generic || !cache_aps_delayed_init)
+		return;
+
+	stop_machine(cache_rendezvous_handler, NULL, cpu_online_mask);
+	cache_aps_delayed_init = false;
+}
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 64a73f415f03..5c122dd17ec2 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -52,6 +52,7 @@
 #include <asm/cpu.h>
 #include <asm/mce.h>
 #include <asm/msr.h>
+#include <asm/cacheinfo.h>
 #include <asm/memtype.h>
 #include <asm/microcode.h>
 #include <asm/microcode_intel.h>
@@ -1936,7 +1937,7 @@ void identify_secondary_cpu(struct cpuinfo_x86 *c)
 #ifdef CONFIG_X86_32
 	enable_sep_cpu();
 #endif
-	mtrr_ap_init();
+	cache_ap_init();
 	validate_apic_and_package_id(c);
 	x86_spec_ctrl_setup_ap();
 	update_srbds_msr();
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index b68ec43e33a2..38531e021581 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -70,9 +70,6 @@ static const struct mtrr_ops *mtrr_ops[X86_VENDOR_NUM] __ro_after_init;
 
 const struct mtrr_ops *mtrr_if;
 
-static void set_mtrr(unsigned int reg, unsigned long base,
-		     unsigned long size, mtrr_type type);
-
 void __init set_mtrr_ops(const struct mtrr_ops *ops)
 {
 	if (ops->vendor && ops->vendor < X86_VENDOR_NUM)
@@ -155,25 +152,8 @@ static int mtrr_rendezvous_handler(void *info)
 {
 	struct set_mtrr_data *data = info;
 
-	/*
-	 * We use this same function to initialize the mtrrs during boot,
-	 * resume, runtime cpu online and on an explicit request to set a
-	 * specific MTRR.
-	 *
-	 * During boot or suspend, the state of the boot cpu's mtrrs has been
-	 * saved, and we want to replicate that across all the cpus that come
-	 * online (either at the end of boot or resume or during a runtime cpu
-	 * online). If we're doing that, @reg is set to something special and on
-	 * all the cpu's we do cache_cpu_init() (On the logical cpu that
-	 * started the boot/resume sequence, this might be a duplicate
-	 * cache_cpu_init()).
-	 */
-	if (data->smp_reg != ~0U) {
-		mtrr_if->set(data->smp_reg, data->smp_base,
-			     data->smp_size, data->smp_type);
-	} else if (cache_aps_delayed_init || !cpu_online(smp_processor_id())) {
-		cache_cpu_init();
-	}
+	mtrr_if->set(data->smp_reg, data->smp_base,
+		     data->smp_size, data->smp_type);
 	return 0;
 }
 
@@ -243,19 +223,6 @@ static void set_mtrr_cpuslocked(unsigned int reg, unsigned long base,
 	stop_machine_cpuslocked(mtrr_rendezvous_handler, &data, cpu_online_mask);
 }
 
-static void set_mtrr_from_inactive_cpu(unsigned int reg, unsigned long base,
-				      unsigned long size, mtrr_type type)
-{
-	struct set_mtrr_data data = { .smp_reg = reg,
-				      .smp_base = base,
-				      .smp_size = size,
-				      .smp_type = type
-				    };
-
-	stop_machine_from_inactive_cpu(mtrr_rendezvous_handler, &data,
-				       cpu_callout_mask);
-}
-
 /**
  * mtrr_add_page - Add a memory type region
  * @base: Physical base address of region in pages (in units of 4 kB!)
@@ -764,7 +731,6 @@ void __init mtrr_bp_init(void)
 						 CACHE_GENERIC_PAT;
 				changed_by_mtrr_cleanup =
 					mtrr_cleanup(phys_addr);
-				cache_cpu_init();
 			}
 		}
 	}
@@ -781,27 +747,6 @@ void __init mtrr_bp_init(void)
 	}
 }
 
-void mtrr_ap_init(void)
-{
-	if (!cache_generic || cache_aps_delayed_init)
-		return;
-
-	/*
-	 * Ideally we should hold mtrr_mutex here to avoid mtrr entries
-	 * changed, but this routine will be called in cpu boot time,
-	 * holding the lock breaks it.
-	 *
-	 * This routine is called in two cases:
-	 *
-	 *   1. very early time of software resume, when there absolutely
-	 *      isn't mtrr entry changes;
-	 *
-	 *   2. cpu hotadd time. We let mtrr_add/del_page hold cpuhotplug
-	 *      lock to prevent mtrr entry changes
-	 */
-	set_mtrr_from_inactive_cpu(~0U, 0, 0, 0);
-}
-
 /**
  * mtrr_save_state - Save current fixed-range MTRR state of the first
  *	cpu in cpu_online_mask.
@@ -817,34 +762,6 @@ void mtrr_save_state(void)
 	smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1);
 }
 
-/*
- * Delayed MTRR initialization for all AP's
- */
-void mtrr_aps_init(void)
-{
-	if (!cache_generic)
-		return;
-
-	/*
-	 * Check if someone has requested the delay of AP MTRR initialization,
-	 * by doing set_mtrr_aps_delayed_init(), prior to this point. If not,
-	 * then we are done.
-	 */
-	if (!cache_aps_delayed_init)
-		return;
-
-	set_mtrr(~0U, 0, 0, 0);
-	cache_aps_delayed_init = false;
-}
-
-void mtrr_bp_restore(void)
-{
-	if (!cache_generic)
-		return;
-
-	cache_cpu_init();
-}
-
 static int __init mtrr_init_finialize(void)
 {
 	if (!mtrr_enabled)
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 216fee7144ee..e0e185ee0229 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -34,6 +34,7 @@
 #include <asm/numa.h>
 #include <asm/bios_ebda.h>
 #include <asm/bugs.h>
+#include <asm/cacheinfo.h>
 #include <asm/cpu.h>
 #include <asm/efi.h>
 #include <asm/gart.h>
@@ -1075,7 +1076,7 @@ void __init setup_arch(char **cmdline_p)
 
 	/* update e820 for memory not covered by WB MTRRs */
 	if (IS_ENABLED(CONFIG_MTRR))
-		mtrr_bp_init();
+		cache_bp_init();
 	else
 		pat_disable("PAT support disabled because CONFIG_MTRR is disabled in the kernel.");
 
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index ef7bce21cbe8..ff793f436904 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1445,7 +1445,7 @@ void arch_thaw_secondary_cpus_begin(void)
 
 void arch_thaw_secondary_cpus_end(void)
 {
-	mtrr_aps_init();
+	cache_aps_init();
 }
 
 /*
@@ -1488,7 +1488,7 @@ void __init native_smp_cpus_done(unsigned int max_cpus)
 
 	nmi_selftest();
 	impress_friends();
-	mtrr_aps_init();
+	cache_aps_init();
 }
 
 static int __initdata setup_possible_cpus = -1;
diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
index bb176c72891c..754221c9a1c3 100644
--- a/arch/x86/power/cpu.c
+++ b/arch/x86/power/cpu.c
@@ -23,6 +23,7 @@
 #include <asm/fpu/api.h>
 #include <asm/debugreg.h>
 #include <asm/cpu.h>
+#include <asm/cacheinfo.h>
 #include <asm/mmu_context.h>
 #include <asm/cpu_device_id.h>
 #include <asm/microcode.h>
@@ -261,7 +262,7 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
 	do_fpu_end();
 	tsc_verify_tsc_adjust(true);
 	x86_platform.restore_sched_clock_state();
-	mtrr_bp_restore();
+	cache_bp_restore();
 	perf_restore_debug_store();
 
 	c = &cpu_data(smp_processor_id());
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 10/10] x86: decouple pat and mtrr handling
  2022-08-20  9:25 [PATCH v2 00/10] x86: make pat and mtrr independent from each other Juergen Gross
                   ` (8 preceding siblings ...)
  2022-08-20  9:25 ` [PATCH v2 09/10] x86/mtrr: add a stop_machine() handler calling only cache_cpu_init() Juergen Gross
@ 2022-08-20  9:25 ` Juergen Gross
  9 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2022-08-20  9:25 UTC (permalink / raw)
  To: xen-devel, x86, linux-kernel
  Cc: Juergen Gross, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, H. Peter Anvin, Andy Lutomirski, Peter Zijlstra

Today PAT is usable only with MTRR being active, with some nasty tweaks
to make PAT usable when running as Xen PV guest, which doesn't support
MTRR.

The reason for this coupling is, that both, PAT MSR changes and MTRR
changes, require a similar sequence and so full PAT support was added
using the already available MTRR handling.

Xen PV PAT handling can work without MTRR, as it just needs to consume
the PAT MSR setting done by the hypervisor without the ability and need
to change it. This in turn has resulted in a convoluted initialization
sequence and wrong decisions regarding cache mode availability due to
misguiding PAT availability flags.

Fix all of that by allowing to use PAT without MTRR and by reworking
the current PAT initialization sequence to match better with the newly
introduced generic cache initialization.

This removes the need of the recently added pat_force_disabled flag, so
remove the remnants of the patch adding it.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- former patch 3 completely reworked
---
 arch/x86/include/asm/memtype.h  |   5 +-
 arch/x86/kernel/cpu/cacheinfo.c |   3 +-
 arch/x86/kernel/cpu/mtrr/mtrr.c |  13 +---
 arch/x86/kernel/setup.c         |  13 +---
 arch/x86/mm/pat/memtype.c       | 127 ++++++++++----------------------
 5 files changed, 45 insertions(+), 116 deletions(-)

diff --git a/arch/x86/include/asm/memtype.h b/arch/x86/include/asm/memtype.h
index 9ca760e430b9..113b2fa51849 100644
--- a/arch/x86/include/asm/memtype.h
+++ b/arch/x86/include/asm/memtype.h
@@ -6,9 +6,8 @@
 #include <asm/pgtable_types.h>
 
 extern bool pat_enabled(void);
-extern void pat_disable(const char *reason);
-extern void pat_init(void);
-extern void init_cache_modes(void);
+extern void pat_bp_init(void);
+extern void pat_cpu_init(void);
 
 extern int memtype_reserve(u64 start, u64 end,
 		enum page_cache_mode req_pcm, enum page_cache_mode *ret_pcm);
diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index 4946f93eb16f..08130919d55d 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -1135,7 +1135,7 @@ static void cache_cpu_init(void)
 
 	/* Set PAT. */
 	if (cache_generic & CACHE_GENERIC_PAT)
-		pat_init();
+		pat_cpu_init();
 
 	cache_enable();
 	local_irq_restore(flags);
@@ -1154,6 +1154,7 @@ static int cache_rendezvous_handler(void *unused)
 void __init cache_bp_init(void)
 {
 	mtrr_bp_init();
+	pat_bp_init();
 
 	if (cache_generic)
 		cache_cpu_init();
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 38531e021581..90aa15610692 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -727,24 +727,15 @@ void __init mtrr_bp_init(void)
 			mtrr_enabled = get_mtrr_state();
 
 			if (mtrr_enabled) {
-				cache_generic |= CACHE_GENERIC_MTRR |
-						 CACHE_GENERIC_PAT;
+				cache_generic |= CACHE_GENERIC_MTRR;
 				changed_by_mtrr_cleanup =
 					mtrr_cleanup(phys_addr);
 			}
 		}
 	}
 
-	if (!mtrr_enabled) {
+	if (!mtrr_enabled)
 		pr_info("Disabled\n");
-
-		/*
-		 * PAT initialization relies on MTRR's rendezvous handler.
-		 * Skip PAT init until the handler can initialize both
-		 * features independently.
-		 */
-		pat_disable("MTRRs disabled, skipping PAT initialization too.");
-	}
 }
 
 /**
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index e0e185ee0229..aacaa96f0195 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1075,23 +1075,12 @@ void __init setup_arch(char **cmdline_p)
 	max_pfn = e820__end_of_ram_pfn();
 
 	/* update e820 for memory not covered by WB MTRRs */
-	if (IS_ENABLED(CONFIG_MTRR))
-		cache_bp_init();
-	else
-		pat_disable("PAT support disabled because CONFIG_MTRR is disabled in the kernel.");
-
+	cache_bp_init();
 	if (mtrr_trim_uncached_memory(max_pfn))
 		max_pfn = e820__end_of_ram_pfn();
 
 	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.
diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
index 66a209f7eb86..891f7c5f0f09 100644
--- a/arch/x86/mm/pat/memtype.c
+++ b/arch/x86/mm/pat/memtype.c
@@ -43,6 +43,7 @@
 #include <linux/rbtree.h>
 
 #include <asm/cacheflush.h>
+#include <asm/cacheinfo.h>
 #include <asm/processor.h>
 #include <asm/tlbflush.h>
 #include <asm/x86_init.h>
@@ -60,41 +61,34 @@
 #undef pr_fmt
 #define pr_fmt(fmt) "" fmt
 
-static bool __read_mostly pat_bp_initialized;
 static bool __read_mostly pat_disabled = !IS_ENABLED(CONFIG_X86_PAT);
-static bool __initdata pat_force_disabled = !IS_ENABLED(CONFIG_X86_PAT);
-static bool __read_mostly pat_bp_enabled;
-static bool __read_mostly pat_cm_initialized;
+static u64 __read_mostly pat_msr_val;
 
 /*
  * PAT support is enabled by default, but can be disabled for
  * various user-requested or hardware-forced reasons:
  */
-void pat_disable(const char *msg_reason)
+static void __init pat_disable(const char *msg_reason)
 {
 	if (pat_disabled)
 		return;
 
-	if (pat_bp_initialized) {
-		WARN_ONCE(1, "x86/PAT: PAT cannot be disabled after initialization\n");
-		return;
-	}
-
 	pat_disabled = true;
 	pr_info("x86/PAT: %s\n", msg_reason);
+
+	cache_generic &= ~CACHE_GENERIC_PAT;
 }
 
 static int __init nopat(char *str)
 {
 	pat_disable("PAT support disabled via boot option.");
-	pat_force_disabled = true;
 	return 0;
 }
 early_param("nopat", nopat);
 
 bool pat_enabled(void)
 {
-	return pat_bp_enabled;
+	return !pat_disabled;
 }
 EXPORT_SYMBOL_GPL(pat_enabled);
 
@@ -192,7 +186,8 @@ enum {
 
 #define CM(c) (_PAGE_CACHE_MODE_ ## c)
 
-static enum page_cache_mode pat_get_cache_mode(unsigned pat_val, char *msg)
+static enum page_cache_mode __init pat_get_cache_mode(unsigned int pat_val,
+						      char *msg)
 {
 	enum page_cache_mode cache;
 	char *cache_mode;
@@ -219,14 +214,12 @@ 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.
  */
-static void __init_cache_modes(u64 pat)
+static void __init init_cache_modes(u64 pat)
 {
 	enum page_cache_mode cache;
 	char pat_msg[33];
 	int i;
 
-	WARN_ON_ONCE(pat_cm_initialized);
-
 	pat_msg[32] = 0;
 	for (i = 7; i >= 0; i--) {
 		cache = pat_get_cache_mode((pat >> (i * 8)) & 7,
@@ -234,34 +227,11 @@ static void __init_cache_modes(u64 pat)
 		update_cache_mode_entry(i, cache);
 	}
 	pr_info("x86/PAT: Configuration [0-7]: %s\n", pat_msg);
-
-	pat_cm_initialized = true;
 }
 
 #define PAT(x, y)	((u64)PAT_ ## y << ((x)*8))
 
-static void pat_bp_init(u64 pat)
-{
-	u64 tmp_pat;
-
-	if (!boot_cpu_has(X86_FEATURE_PAT)) {
-		pat_disable("PAT not supported by the CPU.");
-		return;
-	}
-
-	rdmsrl(MSR_IA32_CR_PAT, tmp_pat);
-	if (!tmp_pat) {
-		pat_disable("PAT support disabled by the firmware.");
-		return;
-	}
-
-	wrmsrl(MSR_IA32_CR_PAT, pat);
-	pat_bp_enabled = true;
-
-	__init_cache_modes(pat);
-}
-
-static void pat_ap_init(u64 pat)
+void pat_cpu_init(void)
 {
 	if (!boot_cpu_has(X86_FEATURE_PAT)) {
 		/*
@@ -271,30 +241,35 @@ static void pat_ap_init(u64 pat)
 		panic("x86/PAT: PAT enabled, but not supported by secondary CPU\n");
 	}
 
-	wrmsrl(MSR_IA32_CR_PAT, pat);
+	wrmsrl(MSR_IA32_CR_PAT, pat_msr_val);
 }
 
-void __init init_cache_modes(void)
+/**
+ * pat_bp_init - Initialize the PAT MSR value and PAT table
+ *
+ * This function initializes PAT MSR value and PAT table with an OS-defined
+ * value to enable additional cache attributes, WC, WT and WP.
+ *
+ * This function prepares the calls of pat_cpu_init() via cache_cpu_init()
+ * on all cpus.
+ */
+void __init pat_bp_init(void)
 {
 	u64 pat = 0;
+	struct cpuinfo_x86 *c = &boot_cpu_data;
 
-	if (pat_cm_initialized)
-		return;
+#ifndef CONFIG_X86_PAT
+	pr_info_once("x86/PAT: PAT support disabled because CONFIG_X86_PAT is disabled in the kernel.\n");
+#endif
 
-	if (boot_cpu_has(X86_FEATURE_PAT)) {
-		/*
-		 * CPU supports PAT. Set PAT table to be consistent with
-		 * PAT MSR. This case supports "nopat" boot option, and
-		 * virtual machine environments which support PAT without
-		 * MTRRs. In specific, Xen has unique setup to PAT MSR.
-		 *
-		 * If PAT MSR returns 0, it is considered invalid and emulates
-		 * as No PAT.
-		 */
+	if (!boot_cpu_has(X86_FEATURE_PAT))
+		pat_disable("PAT not supported by the CPU.");
+	else
 		rdmsrl(MSR_IA32_CR_PAT, pat);
-	}
 
 	if (!pat) {
+		pat_disable("PAT support disabled by the firmware.");
+
 		/*
 		 * No PAT. Emulate the PAT table that corresponds to the two
 		 * cache bits, PWT (Write Through) and PCD (Cache Disable).
@@ -315,38 +290,14 @@ void __init init_cache_modes(void)
 		 */
 		pat = PAT(0, WB) | PAT(1, WT) | PAT(2, UC_MINUS) | PAT(3, UC) |
 		      PAT(4, WB) | PAT(5, WT) | PAT(6, UC_MINUS) | PAT(7, UC);
-	} else if (!pat_force_disabled && cpu_feature_enabled(X86_FEATURE_HYPERVISOR)) {
-		/*
-		 * Clearly PAT is enabled underneath. Allow pat_enabled() to
-		 * reflect this.
-		 */
-		pat_bp_enabled = true;
 	}
 
-	__init_cache_modes(pat);
-}
-
-/**
- * pat_init - Initialize the PAT MSR and PAT table on the current CPU
- *
- * This function initializes PAT MSR and PAT table with an OS-defined value
- * to enable additional cache attributes, WC, WT and WP.
- *
- * This function must be called on all CPUs using the specific sequence of
- * operations defined in Intel SDM. mtrr_rendezvous_handler() provides this
- * procedure for PAT.
- */
-void pat_init(void)
-{
-	u64 pat;
-	struct cpuinfo_x86 *c = &boot_cpu_data;
-
-#ifndef CONFIG_X86_PAT
-	pr_info_once("x86/PAT: PAT support disabled because CONFIG_X86_PAT is disabled in the kernel.\n");
-#endif
+	/* Xen PV doesn't allow to set PAT MSR, but all cache modes are fine. */
+	if (pat_disabled || cpu_feature_enabled(X86_FEATURE_XENPV)) {
+		init_cache_modes(pat);
 
-	if (pat_disabled)
 		return;
+	}
 
 	if ((c->x86_vendor == X86_VENDOR_INTEL) &&
 	    (((c->x86 == 0x6) && (c->x86_model <= 0xd)) ||
@@ -404,12 +355,10 @@ void pat_init(void)
 		      PAT(4, WB) | PAT(5, WP) | PAT(6, UC_MINUS) | PAT(7, WT);
 	}
 
-	if (!pat_bp_initialized) {
-		pat_bp_init(pat);
-		pat_bp_initialized = true;
-	} else {
-		pat_ap_init(pat);
-	}
+	pat_msr_val = pat;
+	cache_generic |= CACHE_GENERIC_PAT;
+
+	init_cache_modes(pat);
 }
 
 #undef PAT
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs
  2022-08-20  9:25 ` [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs Juergen Gross
@ 2022-08-20 10:28   ` Greg KH
  2022-08-21 12:25   ` Borislav Petkov
  2022-10-19 18:45   ` [tip: x86/cpu] x86/mtrr: Add comment for set_mtrr_state() serialization tip-bot2 for Juergen Gross
  2 siblings, 0 replies; 23+ messages in thread
From: Greg KH @ 2022-08-20 10:28 UTC (permalink / raw)
  To: Juergen Gross
  Cc: xen-devel, x86, linux-kernel, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Dave Hansen, H. Peter Anvin, stable

On Sat, Aug 20, 2022 at 11:25:24AM +0200, Juergen Gross wrote:
> When booting or resuming the system MTRR state is saved on the boot
> processor and then this state is loaded into MTRRs of all other cpus.
> During update of the MTRRs the MTRR mechanism needs to be disabled by
> writing the related MSR. The old contents of this MSR are saved in a
> set of static variables and later those static variables are used to
> restore the MSR.
> 
> In case the MSR contents need to be modified on a cpu due to the MSR
> not having been initialized properly by the BIOS, the related update
> function is modifying the static variables accordingly.
> 
> Unfortunately the MTRR state update is usually running on all cpus
> at the same time, so using just one set of static variables for all
> cpus is racy in case the MSR contents differ across cpus.
> 
> Fix that by using percpu variables for saving the MSR contents.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> I thought adding a "Fixes:" tag for the kernel's initial git commit
> would maybe be entertaining, but without being really helpful.

So that means I will just do a "best guess" as to how far to backport
things.  Hopefully I guess well...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs
  2022-08-20  9:25 ` [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs Juergen Gross
  2022-08-20 10:28   ` Greg KH
@ 2022-08-21 12:25   ` Borislav Petkov
  2022-08-21 21:41     ` Borislav Petkov
  2022-10-19 18:45   ` [tip: x86/cpu] x86/mtrr: Add comment for set_mtrr_state() serialization tip-bot2 for Juergen Gross
  2 siblings, 1 reply; 23+ messages in thread
From: Borislav Petkov @ 2022-08-21 12:25 UTC (permalink / raw)
  To: Juergen Gross
  Cc: xen-devel, x86, linux-kernel, Thomas Gleixner, Ingo Molnar,
	Dave Hansen, H. Peter Anvin, stable

On Sat, Aug 20, 2022 at 11:25:24AM +0200, Juergen Gross wrote:
> When booting or resuming the system MTRR state is saved on the boot
> processor and then this state is loaded into MTRRs of all other cpus.

s/cpu/CPU/g

Pls check all commit messages.

> During update of the MTRRs the MTRR mechanism needs to be disabled by
> writing the related MSR. The old contents of this MSR are saved in a
> set of static variables and later those static variables are used to
> restore the MSR.
> 
> In case the MSR contents need to be modified on a cpu due to the MSR
> not having been initialized properly by the BIOS, the related update
> function is modifying the static variables accordingly.
> 
> Unfortunately the MTRR state update is usually running on all cpus
> at the same time, so using just one set of static variables for all
> cpus is racy in case the MSR contents differ across cpus.
> 
> Fix that by using percpu variables for saving the MSR contents.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> I thought adding a "Fixes:" tag for the kernel's initial git commit
> would maybe be entertaining, but without being really helpful.
> The percpu variables were preferred over on-stack ones in order to
> avoid more code churn in followup patches decoupling PAT from MTRR
> support.

So if that thing has been broken for so long and no one noticed, we
could just as well not backport to stable at all...

> V2:
> - new patch
> ---
>  arch/x86/kernel/cpu/mtrr/generic.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> index 558108296f3c..3d185fcf08ca 100644
> --- a/arch/x86/kernel/cpu/mtrr/generic.c
> +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> @@ -679,7 +679,8 @@ static bool set_mtrr_var_ranges(unsigned int index, struct mtrr_var_range *vr)
>  	return changed;
>  }
>  
> -static u32 deftype_lo, deftype_hi;
> +static DEFINE_PER_CPU(u32, deftype_lo);
> +static DEFINE_PER_CPU(u32, deftype_hi);

My APM says that the high 32 bits of the MTRRdefType MSR are reserved
and MBZ. So you can drop the _hi thing and use 0 everywhere. Or rather a
dummy = 0 var because of that damn rdmsr() macro.

Or simply use a

u64 deftype;

use the rdmsrl/wrmsrl() variants and split it when passing to
umtrr_wrmsr() because that thing wants 2 32s.

>  /**
>   * set_mtrr_state - Set the MTRR state for this CPU.
> @@ -691,6 +692,7 @@ static unsigned long set_mtrr_state(void)
>  {
>  	unsigned long change_mask = 0;
>  	unsigned int i;
> +	u32 *lo = this_cpu_ptr(&deftype_lo);

The tip-tree preferred ordering of variable declarations at the
beginning of a function is reverse fir tree order::

	struct long_struct_name *descriptive_name;
	unsigned long foo, bar;
	unsigned int tmp;
	int ret;

The above is faster to parse than the reverse ordering::

	int ret;
	unsigned int tmp;
	unsigned long foo, bar;
	struct long_struct_name *descriptive_name;

And even more so than random ordering::

	unsigned long foo, bar;
	int ret;
	struct long_struct_name *descriptive_name;
	unsigned int tmp;

Please check all your patches.

>  	for (i = 0; i < num_var_ranges; i++) {
>  		if (set_mtrr_var_ranges(i, &mtrr_state.var_ranges[i]))
> @@ -704,10 +706,10 @@ static unsigned long set_mtrr_state(void)
>  	 * Set_mtrr_restore restores the old value of MTRRdefType,
>  	 * so to set it we fiddle with the saved value:
>  	 */
> -	if ((deftype_lo & 0xff) != mtrr_state.def_type
> -	    || ((deftype_lo & 0xc00) >> 10) != mtrr_state.enabled) {
> +	if ((*lo & 0xff) != mtrr_state.def_type
> +	    || ((*lo & 0xc00) >> 10) != mtrr_state.enabled) {
>  
> -		deftype_lo = (deftype_lo & ~0xcff) | mtrr_state.def_type |
> +		*lo = (*lo & ~0xcff) | mtrr_state.def_type |
>  			     (mtrr_state.enabled << 10);
>  		change_mask |= MTRR_CHANGE_MASK_DEFTYPE;
>  	}
> @@ -729,6 +731,8 @@ static DEFINE_RAW_SPINLOCK(set_atomicity_lock);
>  static void prepare_set(void) __acquires(set_atomicity_lock)
>  {
>  	unsigned long cr0;
> +	u32 *lo = this_cpu_ptr(&deftype_lo);
> +	u32 *hi = this_cpu_ptr(&deftype_hi);

You don't need the pointers here - this_cpu_read() should be enough.

>  	/*
>  	 * Note that this is not ideal
> @@ -763,10 +767,10 @@ static void prepare_set(void) __acquires(set_atomicity_lock)
>  	flush_tlb_local();
>  
>  	/* Save MTRR state */
> -	rdmsr(MSR_MTRRdefType, deftype_lo, deftype_hi);
> +	rdmsr(MSR_MTRRdefType, *lo, *hi);
>  
>  	/* Disable MTRRs, and set the default type to uncached */
> -	mtrr_wrmsr(MSR_MTRRdefType, deftype_lo & ~0xcff, deftype_hi);
> +	mtrr_wrmsr(MSR_MTRRdefType, *lo & ~0xcff, *hi);
>  
>  	/* Again, only flush caches if we have to. */
>  	if (!static_cpu_has(X86_FEATURE_SELFSNOOP))
> @@ -775,12 +779,15 @@ static void prepare_set(void) __acquires(set_atomicity_lock)
>  
>  static void post_set(void) __releases(set_atomicity_lock)
>  {
> +	u32 *lo = this_cpu_ptr(&deftype_lo);
> +	u32 *hi = this_cpu_ptr(&deftype_hi);

Ditto.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs
  2022-08-21 12:25   ` Borislav Petkov
@ 2022-08-21 21:41     ` Borislav Petkov
  2022-08-22  5:17       ` Juergen Gross
  0 siblings, 1 reply; 23+ messages in thread
From: Borislav Petkov @ 2022-08-21 21:41 UTC (permalink / raw)
  To: Juergen Gross
  Cc: xen-devel, x86, linux-kernel, Thomas Gleixner, Ingo Molnar,
	Dave Hansen, H. Peter Anvin, stable

On Sun, Aug 21, 2022 at 02:25:59PM +0200, Borislav Petkov wrote:
> > Fix that by using percpu variables for saving the MSR contents.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> > ---
> > I thought adding a "Fixes:" tag for the kernel's initial git commit
> > would maybe be entertaining, but without being really helpful.
> > The percpu variables were preferred over on-stack ones in order to
> > avoid more code churn in followup patches decoupling PAT from MTRR
> > support.
> 
> So if that thing has been broken for so long and no one noticed, we
> could just as well not backport to stable at all...

Yeah, you can't do that.

The whole day today I kept thinking that something's wrong with this
here. As in, why hasn't it been reported until now.

You say above:

"... for all cpus is racy in case the MSR contents differ across cpus."

But they don't differ:

"7.7.5 MTRRs in Multi-Processing Environments

In multi-processing environments, the MTRRs located in all processors
must characterize memory in the same way. Generally, this means that
identical values are written to the MTRRs used by the processors. This
also means that values CR0.CD and the PAT must be consistent across
processors. Failure to do so may result in coherency violations or loss
of atomicity. Processor implementations do not check the MTRR settings
in other processors to ensure consistency. It is the responsibility of
system software to initialize and maintain MTRR consistency across all
processors."

And you can't have different fixed MTRR type on each CPU - that would
lead to all kinds of nasty bugs.

And here's from a big fat box:

$ rdmsr -a 0x2ff | uniq -c
    256 c00

All 256 CPUs have the def type set to the same thing.

Now, if all CPUs go write that same deftype_lo variable in the
rendezvous handler, the only issue that could happen is if a read
sees a partial write. BUT, AFAIK, x86 doesn't tear 32-bit writes so I
*think* all CPUs see the same value being corrected by using mtrr_state
previously saved on the BSP.

As I said, we should've seen this exploding left and right otherwise...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs
  2022-08-21 21:41     ` Borislav Petkov
@ 2022-08-22  5:17       ` Juergen Gross
  2022-08-22  8:28         ` Borislav Petkov
  0 siblings, 1 reply; 23+ messages in thread
From: Juergen Gross @ 2022-08-22  5:17 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: xen-devel, x86, linux-kernel, Thomas Gleixner, Ingo Molnar,
	Dave Hansen, H. Peter Anvin, stable


[-- Attachment #1.1.1: Type: text/plain, Size: 2622 bytes --]

On 21.08.22 23:41, Borislav Petkov wrote:
> On Sun, Aug 21, 2022 at 02:25:59PM +0200, Borislav Petkov wrote:
>>> Fix that by using percpu variables for saving the MSR contents.
>>>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> I thought adding a "Fixes:" tag for the kernel's initial git commit
>>> would maybe be entertaining, but without being really helpful.
>>> The percpu variables were preferred over on-stack ones in order to
>>> avoid more code churn in followup patches decoupling PAT from MTRR
>>> support.
>>
>> So if that thing has been broken for so long and no one noticed, we
>> could just as well not backport to stable at all...
> 
> Yeah, you can't do that.
> 
> The whole day today I kept thinking that something's wrong with this
> here. As in, why hasn't it been reported until now.
> 
> You say above:
> 
> "... for all cpus is racy in case the MSR contents differ across cpus."
> 
> But they don't differ:
> 
> "7.7.5 MTRRs in Multi-Processing Environments
> 
> In multi-processing environments, the MTRRs located in all processors
> must characterize memory in the same way. Generally, this means that
> identical values are written to the MTRRs used by the processors. This
> also means that values CR0.CD and the PAT must be consistent across
> processors. Failure to do so may result in coherency violations or loss
> of atomicity. Processor implementations do not check the MTRR settings
> in other processors to ensure consistency. It is the responsibility of
> system software to initialize and maintain MTRR consistency across all
> processors."
> 
> And you can't have different fixed MTRR type on each CPU - that would
> lead to all kinds of nasty bugs.
> 
> And here's from a big fat box:
> 
> $ rdmsr -a 0x2ff | uniq -c
>      256 c00
> 
> All 256 CPUs have the def type set to the same thing.
> 
> Now, if all CPUs go write that same deftype_lo variable in the
> rendezvous handler, the only issue that could happen is if a read
> sees a partial write. BUT, AFAIK, x86 doesn't tear 32-bit writes so I
> *think* all CPUs see the same value being corrected by using mtrr_state
> previously saved on the BSP.
> 
> As I said, we should've seen this exploding left and right otherwise...

And then there is mtrr_state_warn() in arch/x86/kernel/cpu/mtrr/generic.c
which has a comment saying:

/* Some BIOS's are messed up and don't set all MTRRs the same! */

Yes, the chances are slim to hit such a box, but your reasoning suggests
I should remove the related code?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs
  2022-08-22  5:17       ` Juergen Gross
@ 2022-08-22  8:28         ` Borislav Petkov
  2022-08-22  8:32           ` Juergen Gross
  0 siblings, 1 reply; 23+ messages in thread
From: Borislav Petkov @ 2022-08-22  8:28 UTC (permalink / raw)
  To: Juergen Gross
  Cc: xen-devel, x86, linux-kernel, Thomas Gleixner, Ingo Molnar,
	Dave Hansen, H. Peter Anvin, stable

On Mon, Aug 22, 2022 at 07:17:40AM +0200, Juergen Gross wrote:
> And then there is mtrr_state_warn() in arch/x86/kernel/cpu/mtrr/generic.c
> which has a comment saying:
> 
> /* Some BIOS's are messed up and don't set all MTRRs the same! */

That thing also says:

        pr_info("mtrr: probably your BIOS does not setup all CPUs.\n");
        pr_info("mtrr: corrected configuration.\n");

because it'll go and force on all CPUs the MTRR state it read from the
BSP in mtrr_bp_init->get_mtrr_state.

> Yes, the chances are slim to hit such a box,

Well, my workstation says:

$ dmesg | grep -i mtrr
[    0.391514] mtrr: your CPUs had inconsistent variable MTRR settings
[    0.395199] mtrr: probably your BIOS does not setup all CPUs.
[    0.399199] mtrr: corrected configuration.

but that's the variable MTRRs.

> but your reasoning suggests I should remove the related code?

My reasoning says you should not do anything at all here - works as
advertized. :-)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs
  2022-08-22  8:28         ` Borislav Petkov
@ 2022-08-22  8:32           ` Juergen Gross
  0 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2022-08-22  8:32 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: xen-devel, x86, linux-kernel, Thomas Gleixner, Ingo Molnar,
	Dave Hansen, H. Peter Anvin, stable


[-- Attachment #1.1.1: Type: text/plain, Size: 1242 bytes --]

On 22.08.22 10:28, Borislav Petkov wrote:
> On Mon, Aug 22, 2022 at 07:17:40AM +0200, Juergen Gross wrote:
>> And then there is mtrr_state_warn() in arch/x86/kernel/cpu/mtrr/generic.c
>> which has a comment saying:
>>
>> /* Some BIOS's are messed up and don't set all MTRRs the same! */
> 
> That thing also says:
> 
>          pr_info("mtrr: probably your BIOS does not setup all CPUs.\n");
>          pr_info("mtrr: corrected configuration.\n");
> 
> because it'll go and force on all CPUs the MTRR state it read from the
> BSP in mtrr_bp_init->get_mtrr_state.
> 
>> Yes, the chances are slim to hit such a box,
> 
> Well, my workstation says:
> 
> $ dmesg | grep -i mtrr
> [    0.391514] mtrr: your CPUs had inconsistent variable MTRR settings
> [    0.395199] mtrr: probably your BIOS does not setup all CPUs.
> [    0.399199] mtrr: corrected configuration.
> 
> but that's the variable MTRRs.
> 
>> but your reasoning suggests I should remove the related code?
> 
> My reasoning says you should not do anything at all here - works as
> advertized. :-)
> 

And what about the:

   pr_warn("mtrr: your CPUs had inconsistent MTRRdefType settings\n");

This is the case the patch would fix.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function
  2022-08-20  9:25 ` [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function Juergen Gross
@ 2022-08-25 10:31   ` Borislav Petkov
  2022-08-25 10:38     ` Juergen Gross
  2022-08-25 10:41     ` Juergen Gross
  0 siblings, 2 replies; 23+ messages in thread
From: Borislav Petkov @ 2022-08-25 10:31 UTC (permalink / raw)
  To: Juergen Gross
  Cc: xen-devel, x86, linux-kernel, Thomas Gleixner, Ingo Molnar,
	Dave Hansen, H. Peter Anvin

On Sat, Aug 20, 2022 at 11:25:25AM +0200, Juergen Gross wrote:
> The Cyrix cpu specific MTRR function cyrix_set_all() will never be
> called, as the struct mtrr_ops set_all() callback will only be called
> in the use_intel() case, which would require the use_intel_if member
> of struct mtrr_ops to be set, which isn't the case for Cyrix.

Doing some git archeology:

So the commit which added mtrr_aps_delayed_init is

  d0af9eed5aa9 ("x86, pat/mtrr: Rendezvous all the cpus for MTRR/PAT init")

from 2009.

The IPI callback before it, looked like this:

static void ipi_handler(void *info)
{
#ifdef CONFIG_SMP
	struct set_mtrr_data *data = info;
	unsigned long flags;

	local_irq_save(flags);

	atomic_dec(&data->count);
	while (!atomic_read(&data->gate))
		cpu_relax();

	/*  The master has cleared me to execute  */
	if (data->smp_reg != ~0U) {
		mtrr_if->set(data->smp_reg, data->smp_base,
			     data->smp_size, data->smp_type);
	} else {
		mtrr_if->set_all();
		^^^^^^^^^

and that else branch would call ->set_all() on Cyrix too.

Suresh's patch changed it to do:

-	} else {
+	} else if (mtrr_aps_delayed_init) {
+		/*
+		 * Initialize the MTRRs inaddition to the synchronisation.
+		 */
 		mtrr_if->set_all();

BUT below in the set_mtrr() call, it did:

        /*
         * HACK!
         * We use this same function to initialize the mtrrs on boot.
         * The state of the boot cpu's mtrrs has been saved, and we want
         * to replicate across all the APs.
         * If we're doing that @reg is set to something special...
         */
        if (reg != ~0U)
                mtrr_if->set(reg, base, size, type);
        else if (!mtrr_aps_delayed_init)
                mtrr_if->set_all();
		^^^

and that would be the Cyrix case.

But then

  192d8857427d ("x86, mtrr: use stop_machine APIs for doing MTRR rendezvous")

came and "cleaned" all up by removing the "HACK" and doing ->set_all()
only in the rendezvous handler:

+       } else if (mtrr_aps_delayed_init || !cpu_online(smp_processor_id())) {
                mtrr_if->set_all();
        }

Which begs the question: why doesn't the second part of the else test
match on Cyrix? The "|| !cpu_online(smp_processor_id())" case.

If only we had a Cyrix machine somewhere...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function
  2022-08-25 10:31   ` Borislav Petkov
@ 2022-08-25 10:38     ` Juergen Gross
  2022-08-25 10:41     ` Juergen Gross
  1 sibling, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2022-08-25 10:38 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: xen-devel, x86, linux-kernel, Thomas Gleixner, Ingo Molnar,
	Dave Hansen, H. Peter Anvin


[-- Attachment #1.1.1: Type: text/plain, Size: 2726 bytes --]

On 25.08.22 12:31, Borislav Petkov wrote:
> On Sat, Aug 20, 2022 at 11:25:25AM +0200, Juergen Gross wrote:
>> The Cyrix cpu specific MTRR function cyrix_set_all() will never be
>> called, as the struct mtrr_ops set_all() callback will only be called
>> in the use_intel() case, which would require the use_intel_if member
>> of struct mtrr_ops to be set, which isn't the case for Cyrix.
> 
> Doing some git archeology:
> 
> So the commit which added mtrr_aps_delayed_init is
> 
>    d0af9eed5aa9 ("x86, pat/mtrr: Rendezvous all the cpus for MTRR/PAT init")
> 
> from 2009.
> 
> The IPI callback before it, looked like this:
> 
> static void ipi_handler(void *info)
> {
> #ifdef CONFIG_SMP
> 	struct set_mtrr_data *data = info;
> 	unsigned long flags;
> 
> 	local_irq_save(flags);
> 
> 	atomic_dec(&data->count);
> 	while (!atomic_read(&data->gate))
> 		cpu_relax();
> 
> 	/*  The master has cleared me to execute  */
> 	if (data->smp_reg != ~0U) {
> 		mtrr_if->set(data->smp_reg, data->smp_base,
> 			     data->smp_size, data->smp_type);
> 	} else {
> 		mtrr_if->set_all();
> 		^^^^^^^^^
> 
> and that else branch would call ->set_all() on Cyrix too.
> 
> Suresh's patch changed it to do:
> 
> -	} else {
> +	} else if (mtrr_aps_delayed_init) {
> +		/*
> +		 * Initialize the MTRRs inaddition to the synchronisation.
> +		 */
>   		mtrr_if->set_all();
> 
> BUT below in the set_mtrr() call, it did:
> 
>          /*
>           * HACK!
>           * We use this same function to initialize the mtrrs on boot.
>           * The state of the boot cpu's mtrrs has been saved, and we want
>           * to replicate across all the APs.
>           * If we're doing that @reg is set to something special...
>           */
>          if (reg != ~0U)
>                  mtrr_if->set(reg, base, size, type);
>          else if (!mtrr_aps_delayed_init)
>                  mtrr_if->set_all();
> 		^^^
> 
> and that would be the Cyrix case.
> 
> But then
> 
>    192d8857427d ("x86, mtrr: use stop_machine APIs for doing MTRR rendezvous")
> 
> came and "cleaned" all up by removing the "HACK" and doing ->set_all()
> only in the rendezvous handler:
> 
> +       } else if (mtrr_aps_delayed_init || !cpu_online(smp_processor_id())) {
>                  mtrr_if->set_all();
>          }
> 
> Which begs the question: why doesn't the second part of the else test
> match on Cyrix? The "|| !cpu_online(smp_processor_id())" case.
> 
> If only we had a Cyrix machine somewhere...
> 

You are missing one aspect here: there is no call path for Cyrix CPUs using
reg == ~0U.

So the condition of the "else if" will never be evaluated with Cyrix.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function
  2022-08-25 10:31   ` Borislav Petkov
  2022-08-25 10:38     ` Juergen Gross
@ 2022-08-25 10:41     ` Juergen Gross
  2022-08-25 11:42       ` Borislav Petkov
  1 sibling, 1 reply; 23+ messages in thread
From: Juergen Gross @ 2022-08-25 10:41 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: xen-devel, x86, linux-kernel, Thomas Gleixner, Ingo Molnar,
	Dave Hansen, H. Peter Anvin


[-- Attachment #1.1.1: Type: text/plain, Size: 2804 bytes --]

On 25.08.22 12:31, Borislav Petkov wrote:
> On Sat, Aug 20, 2022 at 11:25:25AM +0200, Juergen Gross wrote:
>> The Cyrix cpu specific MTRR function cyrix_set_all() will never be
>> called, as the struct mtrr_ops set_all() callback will only be called
>> in the use_intel() case, which would require the use_intel_if member
>> of struct mtrr_ops to be set, which isn't the case for Cyrix.
> 
> Doing some git archeology:
> 
> So the commit which added mtrr_aps_delayed_init is
> 
>    d0af9eed5aa9 ("x86, pat/mtrr: Rendezvous all the cpus for MTRR/PAT init")
> 
> from 2009.
> 
> The IPI callback before it, looked like this:
> 
> static void ipi_handler(void *info)
> {
> #ifdef CONFIG_SMP
> 	struct set_mtrr_data *data = info;
> 	unsigned long flags;
> 
> 	local_irq_save(flags);
> 
> 	atomic_dec(&data->count);
> 	while (!atomic_read(&data->gate))
> 		cpu_relax();
> 
> 	/*  The master has cleared me to execute  */
> 	if (data->smp_reg != ~0U) {
> 		mtrr_if->set(data->smp_reg, data->smp_base,
> 			     data->smp_size, data->smp_type);
> 	} else {
> 		mtrr_if->set_all();
> 		^^^^^^^^^
> 
> and that else branch would call ->set_all() on Cyrix too.
> 
> Suresh's patch changed it to do:
> 
> -	} else {
> +	} else if (mtrr_aps_delayed_init) {
> +		/*
> +		 * Initialize the MTRRs inaddition to the synchronisation.
> +		 */
>   		mtrr_if->set_all();
> 
> BUT below in the set_mtrr() call, it did:
> 
>          /*
>           * HACK!
>           * We use this same function to initialize the mtrrs on boot.
>           * The state of the boot cpu's mtrrs has been saved, and we want
>           * to replicate across all the APs.
>           * If we're doing that @reg is set to something special...
>           */
>          if (reg != ~0U)
>                  mtrr_if->set(reg, base, size, type);
>          else if (!mtrr_aps_delayed_init)
>                  mtrr_if->set_all();
> 		^^^
> 
> and that would be the Cyrix case.
> 
> But then
> 
>    192d8857427d ("x86, mtrr: use stop_machine APIs for doing MTRR rendezvous")
> 
> came and "cleaned" all up by removing the "HACK" and doing ->set_all()
> only in the rendezvous handler:
> 
> +       } else if (mtrr_aps_delayed_init || !cpu_online(smp_processor_id())) {
>                  mtrr_if->set_all();
>          }
> 
> Which begs the question: why doesn't the second part of the else test
> match on Cyrix? The "|| !cpu_online(smp_processor_id())" case.
> 
> If only we had a Cyrix machine somewhere...
> 

Maybe the alternative reasoning is much faster to understand: if the
Cyrix set_all() could be called, the AMD and Centaur ones would be callable,
too. Those being called would result in a NULL deref, so why should we keep
the Cyrix one?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function
  2022-08-25 10:41     ` Juergen Gross
@ 2022-08-25 11:42       ` Borislav Petkov
  2022-08-25 12:13         ` Juergen Gross
  0 siblings, 1 reply; 23+ messages in thread
From: Borislav Petkov @ 2022-08-25 11:42 UTC (permalink / raw)
  To: Juergen Gross
  Cc: xen-devel, x86, linux-kernel, Thomas Gleixner, Ingo Molnar,
	Dave Hansen, H. Peter Anvin

On Thu, Aug 25, 2022 at 12:41:05PM +0200, Juergen Gross wrote:
> Maybe the alternative reasoning is much faster to understand: if the
> Cyrix set_all() could be called, the AMD and Centaur ones would be callable,
> too.

Right.

> Those being called would result in a NULL deref, so why should we keep
> the Cyrix one?

I know you're eager to remove dead code - I'd love that too. But before
we do that, we need to find out whether some Cyrix hw out there would
not need this.

I know, I know, they should've complained by now ... maybe they have but
we haven't heard about it.

What it most likely looks like is that those machines - a commit from
before git

commit 8fbdcb188e31ac901e216b466b97e90e8b057daa
Author: Dave Jones <davej@suse.de>
Date:   Wed Aug 14 21:14:22 2002 -0700

    [PATCH] Modular x86 MTRR driver.

talks about

+/*
+ * On Cyrix 6x86(MX) and M II the ARR3 is special: it has connection
+ * with the SMM (System Management Mode) mode. So we need the following:
+ * Check whether SMI_LOCK (CCR3 bit 0) is set
+ *   if it is set, write a warning message: ARR3 cannot be changed!
+ *     (it cannot be changed until the next processor reset)

which sounds like old rust. And which no one uses or such machines are
long dead already.

Wikipedia says:

https://en.wikipedia.org/wiki/Cyrix_6x86

"The Cyrix 6x86 is a line of sixth-generation, 32-bit x86
microprocessors designed and released by Cyrix in 1995..."

So I'm thinking removing it would be ok...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function
  2022-08-25 11:42       ` Borislav Petkov
@ 2022-08-25 12:13         ` Juergen Gross
  0 siblings, 0 replies; 23+ messages in thread
From: Juergen Gross @ 2022-08-25 12:13 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: xen-devel, x86, linux-kernel, Thomas Gleixner, Ingo Molnar,
	Dave Hansen, H. Peter Anvin


[-- Attachment #1.1.1: Type: text/plain, Size: 700 bytes --]

On 25.08.22 13:42, Borislav Petkov wrote:
> On Thu, Aug 25, 2022 at 12:41:05PM +0200, Juergen Gross wrote:
>> Maybe the alternative reasoning is much faster to understand: if the
>> Cyrix set_all() could be called, the AMD and Centaur ones would be callable,
>> too.
> 
> Right.
> 
>> Those being called would result in a NULL deref, so why should we keep
>> the Cyrix one?
> 
> I know you're eager to remove dead code - I'd love that too. But before
> we do that, we need to find out whether some Cyrix hw out there would
> not need this.

Back to reasoning #1. Only the use_intel() case calls the code in question
with reg == ~0. And use_intel() is clearly not Cyrix.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [tip: x86/cpu] x86/mtrr: Add comment for set_mtrr_state() serialization
  2022-08-20  9:25 ` [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs Juergen Gross
  2022-08-20 10:28   ` Greg KH
  2022-08-21 12:25   ` Borislav Petkov
@ 2022-10-19 18:45   ` tip-bot2 for Juergen Gross
  2 siblings, 0 replies; 23+ messages in thread
From: tip-bot2 for Juergen Gross @ 2022-10-19 18:45 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Borislav Petkov, Juergen Gross, Borislav Petkov, x86, linux-kernel

The following commit has been merged into the x86/cpu branch of tip:

Commit-ID:     01c97c7303580682751b5aaae043b639bdcbacb3
Gitweb:        https://git.kernel.org/tip/01c97c7303580682751b5aaae043b639bdcbacb3
Author:        Juergen Gross <jgross@suse.com>
AuthorDate:    Tue, 04 Oct 2022 10:10:08 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Wed, 19 Oct 2022 20:37:23 +02:00

x86/mtrr: Add comment for set_mtrr_state() serialization

Add a comment about set_mtrr_state() needing serialization.

  [ bp: Touchups. ]

Suggested-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Juergen Gross <jgross@suse.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lore.kernel.org/r/20220820092533.29420-2-jgross@suse.com
---
 arch/x86/kernel/cpu/mtrr/generic.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 5581082..c8f8951 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -684,7 +684,10 @@ static u32 deftype_lo, deftype_hi;
 /**
  * set_mtrr_state - Set the MTRR state for this CPU.
  *
- * NOTE: The CPU must already be in a safe state for MTRR changes.
+ * NOTE: The CPU must already be in a safe state for MTRR changes, including
+ *       measures that only a single CPU can be active in set_mtrr_state() in
+ *       order to not be subject to races for usage of deftype_lo. This is
+ *       accomplished by taking set_atomicity_lock.
  * RETURNS: 0 if no changes made, else a mask indicating what was changed.
  */
 static unsigned long set_mtrr_state(void)

^ permalink raw reply related	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2022-10-19 18:45 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-20  9:25 [PATCH v2 00/10] x86: make pat and mtrr independent from each other Juergen Gross
2022-08-20  9:25 ` [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs Juergen Gross
2022-08-20 10:28   ` Greg KH
2022-08-21 12:25   ` Borislav Petkov
2022-08-21 21:41     ` Borislav Petkov
2022-08-22  5:17       ` Juergen Gross
2022-08-22  8:28         ` Borislav Petkov
2022-08-22  8:32           ` Juergen Gross
2022-10-19 18:45   ` [tip: x86/cpu] x86/mtrr: Add comment for set_mtrr_state() serialization tip-bot2 for Juergen Gross
2022-08-20  9:25 ` [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function Juergen Gross
2022-08-25 10:31   ` Borislav Petkov
2022-08-25 10:38     ` Juergen Gross
2022-08-25 10:41     ` Juergen Gross
2022-08-25 11:42       ` Borislav Petkov
2022-08-25 12:13         ` Juergen Gross
2022-08-20  9:25 ` [PATCH v2 03/10] x86/mtrr: replace use_intel() with a local flag Juergen Gross
2022-08-20  9:25 ` [PATCH v2 04/10] x86: move some code out of arch/x86/kernel/cpu/mtrr Juergen Gross
2022-08-20  9:25 ` [PATCH v2 05/10] x86/mtrr: split generic_set_all() Juergen Gross
2022-08-20  9:25 ` [PATCH v2 06/10] x86/mtrr: remove set_all callback from struct mtrr_ops Juergen Gross
2022-08-20  9:25 ` [PATCH v2 07/10] x86/mtrr: simplify mtrr_bp_init() Juergen Gross
2022-08-20  9:25 ` [PATCH v2 08/10] x86/mtrr: let cache_aps_delayed_init replace mtrr_aps_delayed_init Juergen Gross
2022-08-20  9:25 ` [PATCH v2 09/10] x86/mtrr: add a stop_machine() handler calling only cache_cpu_init() Juergen Gross
2022-08-20  9:25 ` [PATCH v2 10/10] x86: decouple pat and mtrr handling Juergen Gross

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).