[PATCH 0/4] x86/mtrr: Allow MTRR updates on multiple CPUs in parallel

[PATCH 0/4] x86/mtrr: Allow MTRR updates on multiple CPUs in parallel

[Juergen Gross]

Today MTRR updates are serialized to not happen on multiple CPUs at the
same time, as the related coding is using global variables.

On huge machines with lots of CPUs this can result in problems, as such
updates are happening through stop_machine(), which will call the MTRR
update function with interrupts off on all CPUs at the same time. The
interrupts will be switched on only after the last CPU has finished
the MTRR update. As the update is required to run in uncached mode, it
can take easily several milliseconds on each CPU, resulting in the
whole process to need several seconds. This in turn can cause the
watchdog to trigger and to recognize a hard system lockup.

This series is changing the behavior by allowing the MTRR update to
happen on all CPUs in parallel.

Juergen Gross (4):
  x86/mtrr: Move cache_enable() and cache_disable() to mtrr/generic.c
  x86/mtrr: Introduce MTRR work state structure
  x86/mtrr: Add a prepare_set hook to mtrr_ops
  x86/mtrr: Drop cache_disable_lock

 arch/x86/include/asm/cacheinfo.h   |   2 -
 arch/x86/include/asm/mtrr.h        |   2 -
 arch/x86/kernel/cpu/cacheinfo.c    |  80 +----------------
 arch/x86/kernel/cpu/mtrr/generic.c | 137 ++++++++++++++++++++++++-----
 arch/x86/kernel/cpu/mtrr/mtrr.c    |   3 +
 arch/x86/kernel/cpu/mtrr/mtrr.h    |   2 +
 6 files changed, 120 insertions(+), 106 deletions(-)

-- 
2.52.0

[PATCH 1/4] x86/mtrr: Move cache_enable() and cache_disable() to mtrr/generic.c

[Juergen Gross]

cache_enable() and cache_disable() are used for generic MTRR code only.
Move them and related stuff to mtrr/generic.c, allowing to make them
static. This requires to move the cache_enable() and cache_disable()
calls from cache_cpu_init() into mtrr_generic_set_state().

This allows to make mtrr_enable() and mtrr_disable() static, too.

While moving the code, drop the comment related to the PAT MSR, as this
is not true anymore.

No change of functionality.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/include/asm/cacheinfo.h   |  2 -
 arch/x86/include/asm/mtrr.h        |  2 -
 arch/x86/kernel/cpu/cacheinfo.c    | 80 +---------------------------
 arch/x86/kernel/cpu/mtrr/generic.c | 83 +++++++++++++++++++++++++++++-
 4 files changed, 82 insertions(+), 85 deletions(-)

diff --git a/arch/x86/include/asm/cacheinfo.h b/arch/x86/include/asm/cacheinfo.h
index 5aa061199866..07b0e5e6d5bb 100644
--- a/arch/x86/include/asm/cacheinfo.h
+++ b/arch/x86/include/asm/cacheinfo.h
@@ -7,8 +7,6 @@ extern unsigned int memory_caching_control;
 #define CACHE_MTRR 0x01
 #define CACHE_PAT  0x02
 
-void cache_disable(void);
-void cache_enable(void);
 void set_cache_aps_delayed_init(bool val);
 bool get_cache_aps_delayed_init(void);
 void cache_bp_init(void);
diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index 76b95bd1a405..d547b364ce65 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -58,8 +58,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 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 void guest_force_mtrr_state(struct mtrr_var_range *var,
diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index 51a95b07831f..0d2150de0120 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -635,92 +635,14 @@ 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);
-
-/*
- * Cache flushing is the most time-consuming step when programming the
- * MTRRs.  On many Intel CPUs without known erratas, it can be skipped
- * if the CPU declares cache self-snooping support.
- */
-static void maybe_flush_caches(void)
-{
-	if (!static_cpu_has(X86_FEATURE_SELFSNOOP))
-		wbinvd();
-}
-
-void cache_disable(void) __acquires(cache_disable_lock)
-{
-	unsigned long cr0;
-
-	/*
-	 * 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);
-
-	maybe_flush_caches();
-
-	/* Save value of CR4 and clear Page Global Enable (bit 7) */
-	if (cpu_feature_enabled(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 (cpu_feature_enabled(X86_FEATURE_MTRR))
-		mtrr_disable();
-
-	maybe_flush_caches();
-}
-
-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 (cpu_feature_enabled(X86_FEATURE_MTRR))
-		mtrr_enable();
-
-	/* Enable caches */
-	write_cr0(read_cr0() & ~X86_CR0_CD);
-
-	/* Restore value of CR4 */
-	if (cpu_feature_enabled(X86_FEATURE_PGE))
-		__write_cr4(saved_cr4);
-
-	raw_spin_unlock(&cache_disable_lock);
-}
-
 static void cache_cpu_init(void)
 {
 	unsigned long flags;
 
 	local_irq_save(flags);
 
-	if (memory_caching_control & CACHE_MTRR) {
-		cache_disable();
+	if (memory_caching_control & CACHE_MTRR)
 		mtrr_generic_set_state();
-		cache_enable();
-	}
 
 	if (memory_caching_control & CACHE_PAT)
 		pat_cpu_init();
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 0863733858dc..2c874b88e12c 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -945,7 +945,7 @@ static unsigned long set_mtrr_state(void)
 	return change_mask;
 }
 
-void mtrr_disable(void)
+static void mtrr_disable(void)
 {
 	/* Save MTRR state */
 	rdmsr(MSR_MTRRdefType, deftype_lo, deftype_hi);
@@ -954,16 +954,93 @@ void mtrr_disable(void)
 	mtrr_wrmsr(MSR_MTRRdefType, deftype_lo & MTRR_DEF_TYPE_DISABLE, deftype_hi);
 }
 
-void mtrr_enable(void)
+static void mtrr_enable(void)
 {
 	/* Intel (P6) standard MTRRs */
 	mtrr_wrmsr(MSR_MTRRdefType, deftype_lo, deftype_hi);
 }
 
+/*
+ * Disable and enable caches. Needed for changing MTRRs.
+ *
+ * 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);
+
+/*
+ * Cache flushing is the most time-consuming step when programming the
+ * MTRRs.  On many Intel CPUs without known erratas, it can be skipped
+ * if the CPU declares cache self-snooping support.
+ */
+static void maybe_flush_caches(void)
+{
+	if (!static_cpu_has(X86_FEATURE_SELFSNOOP))
+		wbinvd();
+}
+
+static void cache_disable(void) __acquires(cache_disable_lock)
+{
+	unsigned long cr0;
+
+	/*
+	 * 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);
+
+	maybe_flush_caches();
+
+	/* Save value of CR4 and clear Page Global Enable (bit 7) */
+	if (cpu_feature_enabled(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 (cpu_feature_enabled(X86_FEATURE_MTRR))
+		mtrr_disable();
+
+	maybe_flush_caches();
+}
+
+static 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 (cpu_feature_enabled(X86_FEATURE_MTRR))
+		mtrr_enable();
+
+	/* Enable caches */
+	write_cr0(read_cr0() & ~X86_CR0_CD);
+
+	/* Restore value of CR4 */
+	if (cpu_feature_enabled(X86_FEATURE_PGE))
+		__write_cr4(saved_cr4);
+
+	raw_spin_unlock(&cache_disable_lock);
+}
+
 void mtrr_generic_set_state(void)
 {
 	unsigned long mask, count;
 
+	cache_disable();
+
 	/* Actually set the state */
 	mask = set_mtrr_state();
 
@@ -973,6 +1050,8 @@ void mtrr_generic_set_state(void)
 			set_bit(count, &smp_changes_mask);
 		mask >>= 1;
 	}
+
+	cache_enable();
 }
 
 /**
-- 
2.52.0

[PATCH 2/4] x86/mtrr: Introduce MTRR work state structure

[Juergen Gross]

Instead of using static variables for storing local state across
cache_disable() ... cache_enable(), use a structure allocated on the
stack for the same purpose.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/kernel/cpu/mtrr/generic.c | 58 ++++++++++++++++--------------
 1 file changed, 31 insertions(+), 27 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 2c874b88e12c..ac95b19b01d0 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -905,18 +905,19 @@ static bool set_mtrr_var_ranges(unsigned int index, struct mtrr_var_range *vr)
 	return changed;
 }
 
-static u32 deftype_lo, deftype_hi;
+struct mtrr_work_state {
+	unsigned long cr4;
+	u32 lo;
+	u32 hi;
+};
 
 /**
  * set_mtrr_state - Set the MTRR state for this CPU.
  *
- * 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 cache_disable_lock.
+ * NOTE: The CPU must already be in a safe state for MTRR changes.
  * RETURNS: 0 if no changes made, else a mask indicating what was changed.
  */
-static unsigned long set_mtrr_state(void)
+static unsigned long set_mtrr_state(struct mtrr_work_state *state)
 {
 	unsigned long change_mask = 0;
 	unsigned int i;
@@ -933,10 +934,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 & MTRR_DEF_TYPE_TYPE) != mtrr_state.def_type ||
-	    ((deftype_lo & MTRR_DEF_TYPE_ENABLE) >> MTRR_STATE_SHIFT) != mtrr_state.enabled) {
+	if ((state->lo & MTRR_DEF_TYPE_TYPE) != mtrr_state.def_type ||
+	    ((state->lo & MTRR_DEF_TYPE_ENABLE) >> MTRR_STATE_SHIFT) != mtrr_state.enabled) {
 
-		deftype_lo = (deftype_lo & MTRR_DEF_TYPE_DISABLE) |
+		state->lo = (state->lo & MTRR_DEF_TYPE_DISABLE) |
 			     mtrr_state.def_type |
 			     (mtrr_state.enabled << MTRR_STATE_SHIFT);
 		change_mask |= MTRR_CHANGE_MASK_DEFTYPE;
@@ -945,19 +946,19 @@ static unsigned long set_mtrr_state(void)
 	return change_mask;
 }
 
-static void mtrr_disable(void)
+static void mtrr_disable(struct mtrr_work_state *state)
 {
 	/* Save MTRR state */
-	rdmsr(MSR_MTRRdefType, deftype_lo, deftype_hi);
+	rdmsr(MSR_MTRRdefType, state->lo, state->hi);
 
 	/* Disable MTRRs, and set the default type to uncached */
-	mtrr_wrmsr(MSR_MTRRdefType, deftype_lo & MTRR_DEF_TYPE_DISABLE, deftype_hi);
+	mtrr_wrmsr(MSR_MTRRdefType, state->lo & MTRR_DEF_TYPE_DISABLE, state->hi);
 }
 
-static void mtrr_enable(void)
+static void mtrr_enable(struct mtrr_work_state *state)
 {
 	/* Intel (P6) standard MTRRs */
-	mtrr_wrmsr(MSR_MTRRdefType, deftype_lo, deftype_hi);
+	mtrr_wrmsr(MSR_MTRRdefType, state->lo, state->hi);
 }
 
 /*
@@ -969,7 +970,6 @@ static void mtrr_enable(void)
  * 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);
 
 /*
@@ -983,7 +983,8 @@ static void maybe_flush_caches(void)
 		wbinvd();
 }
 
-static void cache_disable(void) __acquires(cache_disable_lock)
+static void cache_disable(struct mtrr_work_state *state)
+	__acquires(cache_disable_lock)
 {
 	unsigned long cr0;
 
@@ -1002,8 +1003,8 @@ static void cache_disable(void) __acquires(cache_disable_lock)
 
 	/* Save value of CR4 and clear Page Global Enable (bit 7) */
 	if (cpu_feature_enabled(X86_FEATURE_PGE)) {
-		saved_cr4 = __read_cr4();
-		__write_cr4(saved_cr4 & ~X86_CR4_PGE);
+		state->cr4 = __read_cr4();
+		__write_cr4(state->cr4 & ~X86_CR4_PGE);
 	}
 
 	/* Flush all TLBs via a mov %cr3, %reg; mov %reg, %cr3 */
@@ -1011,26 +1012,27 @@ static void cache_disable(void) __acquires(cache_disable_lock)
 	flush_tlb_local();
 
 	if (cpu_feature_enabled(X86_FEATURE_MTRR))
-		mtrr_disable();
+		mtrr_disable(state);
 
 	maybe_flush_caches();
 }
 
-static void cache_enable(void) __releases(cache_disable_lock)
+static void cache_enable(struct mtrr_work_state *state)
+	__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 (cpu_feature_enabled(X86_FEATURE_MTRR))
-		mtrr_enable();
+		mtrr_enable(state);
 
 	/* Enable caches */
 	write_cr0(read_cr0() & ~X86_CR0_CD);
 
 	/* Restore value of CR4 */
 	if (cpu_feature_enabled(X86_FEATURE_PGE))
-		__write_cr4(saved_cr4);
+		__write_cr4(state->cr4);
 
 	raw_spin_unlock(&cache_disable_lock);
 }
@@ -1038,11 +1040,12 @@ static void cache_enable(void) __releases(cache_disable_lock)
 void mtrr_generic_set_state(void)
 {
 	unsigned long mask, count;
+	struct mtrr_work_state state;
 
-	cache_disable();
+	cache_disable(&state);
 
 	/* Actually set the state */
-	mask = set_mtrr_state();
+	mask = set_mtrr_state(&state);
 
 	/* Use the atomic bitops to update the global mask */
 	for (count = 0; count < sizeof(mask) * 8; ++count) {
@@ -1051,7 +1054,7 @@ void mtrr_generic_set_state(void)
 		mask >>= 1;
 	}
 
-	cache_enable();
+	cache_enable(&state);
 }
 
 /**
@@ -1069,11 +1072,12 @@ static void generic_set_mtrr(unsigned int reg, unsigned long base,
 {
 	unsigned long flags;
 	struct mtrr_var_range *vr;
+	struct mtrr_work_state state;
 
 	vr = &mtrr_state.var_ranges[reg];
 
 	local_irq_save(flags);
-	cache_disable();
+	cache_disable(&state);
 
 	if (size == 0) {
 		/*
@@ -1092,7 +1096,7 @@ static void generic_set_mtrr(unsigned int reg, unsigned long base,
 		mtrr_wrmsr(MTRRphysMask_MSR(reg), vr->mask_lo, vr->mask_hi);
 	}
 
-	cache_enable();
+	cache_enable(&state);
 	local_irq_restore(flags);
 }
 
-- 
2.52.0

[PATCH 3/4] x86/mtrr: Add a prepare_set hook to mtrr_ops

[Juergen Gross]

In order to prepare dropping the cache_disable_lock add a new hook
to struct mtrr_ops, allowing to set some global state before calling
the .set hook on all active CPUs.

Move setting of mtrr_state.var_ranges[] from generic_set_mtrr() to
the new prepare hook. Note that doing that only once outside the
cache_disable_lock is fine, as generic_set_mtrr() is called via
set_mtrr() only and this call is protected by mtrr_mutex.

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

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index ac95b19b01d0..bfd5a7ba17cb 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -1057,6 +1057,31 @@ void mtrr_generic_set_state(void)
 	cache_enable(&state);
 }
 
+/**
+ * generic_prepare_set_mtrr - set variable MTRR register data in mtrr_state
+ *
+ * @reg: The register to set.
+ * @base: The base address of the region.
+ * @size: The size of the region. If this is 0 the region is disabled.
+ * @type: The type of the region.
+ *
+ * Returns nothing.
+ */
+static void generic_prepare_set_mtrr(unsigned int reg, unsigned long base,
+				     unsigned long size, mtrr_type type)
+{
+	struct mtrr_var_range *vr = &mtrr_state.var_ranges[reg];
+
+	if (size == 0) {
+		memset(vr, 0, sizeof(struct mtrr_var_range));
+	} else {
+		vr->base_lo = base << PAGE_SHIFT | type;
+		vr->base_hi = (base >> (32 - PAGE_SHIFT)) & ~phys_hi_rsvd;
+		vr->mask_lo = -size << PAGE_SHIFT | MTRR_PHYSMASK_V;
+		vr->mask_hi = (-size >> (32 - PAGE_SHIFT)) & ~phys_hi_rsvd;
+	}
+}
+
 /**
  * generic_set_mtrr - set variable MTRR register on the local CPU.
  *
@@ -1085,13 +1110,7 @@ static void generic_set_mtrr(unsigned int reg, unsigned long base,
 		 * clear the relevant mask register to disable a range.
 		 */
 		mtrr_wrmsr(MTRRphysMask_MSR(reg), 0, 0);
-		memset(vr, 0, sizeof(struct mtrr_var_range));
 	} else {
-		vr->base_lo = base << PAGE_SHIFT | type;
-		vr->base_hi = (base >> (32 - PAGE_SHIFT)) & ~phys_hi_rsvd;
-		vr->mask_lo = -size << PAGE_SHIFT | MTRR_PHYSMASK_V;
-		vr->mask_hi = (-size >> (32 - PAGE_SHIFT)) & ~phys_hi_rsvd;
-
 		mtrr_wrmsr(MTRRphysBase_MSR(reg), vr->base_lo, vr->base_hi);
 		mtrr_wrmsr(MTRRphysMask_MSR(reg), vr->mask_lo, vr->mask_hi);
 	}
@@ -1156,6 +1175,7 @@ int positive_have_wrcomb(void)
 const struct mtrr_ops generic_mtrr_ops = {
 	.get			= generic_get_mtrr,
 	.get_free_region	= generic_get_free_region,
+	.prepare_set		= generic_prepare_set_mtrr,
 	.set			= generic_set_mtrr,
 	.validate_add_page	= generic_validate_add_page,
 	.have_wrcomb		= generic_have_wrcomb,
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 4b3d492afe17..32948fb4e742 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -175,6 +175,9 @@ static void set_mtrr(unsigned int reg, unsigned long base, unsigned long size,
 				      .smp_type = type
 				    };
 
+	if (mtrr_if->prepare_set)
+		mtrr_if->prepare_set(reg, base, size, type);
+
 	stop_machine_cpuslocked(mtrr_rendezvous_handler, &data, cpu_online_mask);
 
 	generic_rebuild_map();
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.h b/arch/x86/kernel/cpu/mtrr/mtrr.h
index 2de3bd2f95d1..4d32c095cfc5 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.h
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.h
@@ -17,6 +17,8 @@ extern unsigned int mtrr_usage_table[MTRR_MAX_VAR_RANGES];
 
 struct mtrr_ops {
 	u32	var_regs;
+	void	(*prepare_set)(unsigned int reg, unsigned long base,
+			       unsigned long size, mtrr_type type);
 	void	(*set)(unsigned int reg, unsigned long base,
 		       unsigned long size, mtrr_type type);
 	void	(*get)(unsigned int reg, unsigned long *base,
-- 
2.52.0

[PATCH 4/4] x86/mtrr: Drop cache_disable_lock

[Juergen Gross]

Now that no global state is modified under cache_disable_lock, it can
be dropped.

All required serialization is done via mtrr_mutex and cpus_read_lock(),
ensuring that only one set_mtrr() can be active at any time and that
this call can't run concurrently with CPU bringup.

The main advantages are a faster boot of machines with lots of CPUs,
and avoiding hard lockups on those machines in case
mtrr_generic_set_state() takes too long in uncached mode, resulting
in other CPUs waiting for seconds to get the cache_disable_lock.

This has been seen happening in more than 1% of all boots on an Intel
machine with 960 CPUs. With this patch applied boot was always
successful.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
I was considering applying a "Fixes:" tag, but this could only
reference the initial kernel git commit, as MTRR support predates
git, and the problem existed since MTRRs became a thing.

 arch/x86/kernel/cpu/mtrr/generic.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index bfd5a7ba17cb..280f9491ec2e 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -970,7 +970,6 @@ static void mtrr_enable(struct mtrr_work_state *state)
  * The caller must ensure that local interrupts are disabled and
  * are reenabled after cache_enable() has been called.
  */
-static DEFINE_RAW_SPINLOCK(cache_disable_lock);
 
 /*
  * Cache flushing is the most time-consuming step when programming the
@@ -984,17 +983,9 @@ static void maybe_flush_caches(void)
 }
 
 static void cache_disable(struct mtrr_work_state *state)
-	__acquires(cache_disable_lock)
 {
 	unsigned long cr0;
 
-	/*
-	 * 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);
@@ -1018,7 +1009,6 @@ static void cache_disable(struct mtrr_work_state *state)
 }
 
 static void cache_enable(struct mtrr_work_state *state)
-	__releases(cache_disable_lock)
 {
 	/* Flush TLBs (no need to flush caches - they are disabled) */
 	count_vm_tlb_event(NR_TLB_LOCAL_FLUSH_ALL);
@@ -1033,8 +1023,6 @@ static void cache_enable(struct mtrr_work_state *state)
 	/* Restore value of CR4 */
 	if (cpu_feature_enabled(X86_FEATURE_PGE))
 		__write_cr4(state->cr4);
-
-	raw_spin_unlock(&cache_disable_lock);
 }
 
 void mtrr_generic_set_state(void)
-- 
2.52.0

Re: [PATCH 2/4] x86/mtrr: Introduce MTRR work state structure

[kernel test robot]

Hi Juergen,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tip/x86/core]
[also build test WARNING on tip/master linus/master v6.19-rc6 next-20260121]
[cannot apply to tip/auto-latest]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Juergen-Gross/x86-mtrr-Move-cache_enable-and-cache_disable-to-mtrr-generic-c/20260121-230320
base:   tip/x86/core
patch link:    https://lore.kernel.org/r/20260121141106.755458-3-jgross%40suse.com
patch subject: [PATCH 2/4] x86/mtrr: Introduce MTRR work state structure
config: x86_64-allnoconfig (https://download.01.org/0day-ci/archive/20260122/202601220306.FavyG0wD-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260122/202601220306.FavyG0wD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202601220306.FavyG0wD-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Warning: arch/x86/kernel/cpu/mtrr/generic.c:920 function parameter 'state' not described in 'set_mtrr_state'
>> Warning: arch/x86/kernel/cpu/mtrr/generic.c:920 function parameter 'state' not described in 'set_mtrr_state'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/4] x86/mtrr: Introduce MTRR work state structure

[Jürgen Groß]

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

On 21.01.26 20:58, kernel test robot wrote:
> Hi Juergen,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on tip/x86/core]
> [also build test WARNING on tip/master linus/master v6.19-rc6 next-20260121]
> [cannot apply to tip/auto-latest]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Juergen-Gross/x86-mtrr-Move-cache_enable-and-cache_disable-to-mtrr-generic-c/20260121-230320
> base:   tip/x86/core
> patch link:    https://lore.kernel.org/r/20260121141106.755458-3-jgross%40suse.com
> patch subject: [PATCH 2/4] x86/mtrr: Introduce MTRR work state structure
> config: x86_64-allnoconfig (https://download.01.org/0day-ci/archive/20260122/202601220306.FavyG0wD-lkp@intel.com/config)
> compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260122/202601220306.FavyG0wD-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202601220306.FavyG0wD-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
>>> Warning: arch/x86/kernel/cpu/mtrr/generic.c:920 function parameter 'state' not described in 'set_mtrr_state'
>>> Warning: arch/x86/kernel/cpu/mtrr/generic.c:920 function parameter 'state' not described in 'set_mtrr_state'
> 

Just a note: I have a local fix for that, but will send V2 only after some
more feedback.


Juergen

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

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