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

[PATCH v2 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.

Changes in V2:
- fix a function comment header in patch 2

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 | 139 ++++++++++++++++++++++++-----
 arch/x86/kernel/cpu/mtrr/mtrr.c    |   3 +
 arch/x86/kernel/cpu/mtrr/mtrr.h    |   2 +
 6 files changed, 122 insertions(+), 106 deletions(-)

-- 
2.52.0

[PATCH v2 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 v2 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>
---
V2:
- add parameter description to set_mtrr_state() (kernel test robot)
---
 arch/x86/kernel/cpu/mtrr/generic.c | 60 ++++++++++++++++--------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 2c874b88e12c..7d9e582a4048 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -905,18 +905,21 @@ 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.
+ *
+ * @state: pointer to mtrr_work_state
  * 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 +936,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 +948,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 +972,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 +985,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 +1005,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 +1014,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 +1042,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 +1056,7 @@ void mtrr_generic_set_state(void)
 		mask >>= 1;
 	}
 
-	cache_enable();
+	cache_enable(&state);
 }
 
 /**
@@ -1069,11 +1074,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 +1098,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 v2 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 7d9e582a4048..d49e1837a7af 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -1059,6 +1059,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.
  *
@@ -1087,13 +1112,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);
 	}
@@ -1158,6 +1177,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 v2 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.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 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 d49e1837a7af..236b83867bab 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -972,7 +972,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
@@ -986,17 +985,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);
@@ -1020,7 +1011,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);
@@ -1035,8 +1025,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 v2 0/4] x86/mtrr: Allow MTRR updates on multiple CPUs in parallel

[Juergen Gross]

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

Ping?

I'd really like to have this in 7.0, as it is fixing a real issue on
some machines ...


Juergen

On 30.01.26 12:36, Juergen Gross wrote:
> 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.
> 
> Changes in V2:
> - fix a function comment header in patch 2
> 
> 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 | 139 ++++++++++++++++++++++++-----
>   arch/x86/kernel/cpu/mtrr/mtrr.c    |   3 +
>   arch/x86/kernel/cpu/mtrr/mtrr.h    |   2 +
>   6 files changed, 122 insertions(+), 106 deletions(-)
> 


[-- 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 --]

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

[H. Peter Anvin]

On February 9, 2026 1:12:59 AM PST, Juergen Gross <jgross@suse.com> wrote:
>Ping?
>
>I'd really like to have this in 7.0, as it is fixing a real issue on
>some machines ...
>
>
>Juergen
>
>On 30.01.26 12:36, Juergen Gross wrote:
>> 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.
>> 
>> Changes in V2:
>> - fix a function comment header in patch 2
>> 
>> 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 | 139 ++++++++++++++++++++++++-----
>>   arch/x86/kernel/cpu/mtrr/mtrr.c    |   3 +
>>   arch/x86/kernel/cpu/mtrr/mtrr.h    |   2 +
>>   6 files changed, 122 insertions(+), 106 deletions(-)
>> 
>

First of all, what machines are even needing MTRR updates these days? 

This isn't a rhetorical question. It is important to understand what the underlying problem is.

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

[Jürgen Groß]

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

On 09.02.26 19:37, H. Peter Anvin wrote:
> On February 9, 2026 1:12:59 AM PST, Juergen Gross <jgross@suse.com> wrote:
>> Ping?
>>
>> I'd really like to have this in 7.0, as it is fixing a real issue on
>> some machines ...
>>
>>
>> Juergen
>>
>> On 30.01.26 12:36, Juergen Gross wrote:
>>> 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.
>>>
>>> Changes in V2:
>>> - fix a function comment header in patch 2
>>>
>>> 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 | 139 ++++++++++++++++++++++++-----
>>>    arch/x86/kernel/cpu/mtrr/mtrr.c    |   3 +
>>>    arch/x86/kernel/cpu/mtrr/mtrr.h    |   2 +
>>>    6 files changed, 122 insertions(+), 106 deletions(-)
>>>
>>
> 
> First of all, what machines are even needing MTRR updates these days?

I'm not aware this machine really needed an update.

> This isn't a rhetorical question. It is important to understand what the underlying problem is.

It just took several seconds for all CPUs to check if there is an update
needed. It might be an issue with firmware, topology, whatever. It happened
in a test doing 300 cold boots in a row after roughly 70 loop iterations,
always on one of the last CPUs.

The issue shows that there IS a potential problem with doing the MTRR
update one CPU after the other, instead just doing it in parallel (which
is the "official" recommendation anyway). See the comment in
cache_disable(). And it isn't as if the fix would be very complicated.


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 --]

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

[H. Peter Anvin]

On February 9, 2026 10:51:04 PM PST, "Jürgen Groß" <jgross@suse.com> wrote:
>On 09.02.26 19:37, H. Peter Anvin wrote:
>> On February 9, 2026 1:12:59 AM PST, Juergen Gross <jgross@suse.com> wrote:
>>> Ping?
>>> 
>>> I'd really like to have this in 7.0, as it is fixing a real issue on
>>> some machines ...
>>> 
>>> 
>>> Juergen
>>> 
>>> On 30.01.26 12:36, Juergen Gross wrote:
>>>> 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.
>>>> 
>>>> Changes in V2:
>>>> - fix a function comment header in patch 2
>>>> 
>>>> 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 | 139 ++++++++++++++++++++++++-----
>>>>    arch/x86/kernel/cpu/mtrr/mtrr.c    |   3 +
>>>>    arch/x86/kernel/cpu/mtrr/mtrr.h    |   2 +
>>>>    6 files changed, 122 insertions(+), 106 deletions(-)
>>>> 
>>> 
>> 
>> First of all, what machines are even needing MTRR updates these days?
>
>I'm not aware this machine really needed an update.
>
>> This isn't a rhetorical question. It is important to understand what the underlying problem is.
>
>It just took several seconds for all CPUs to check if there is an update
>needed. It might be an issue with firmware, topology, whatever. It happened
>in a test doing 300 cold boots in a row after roughly 70 loop iterations,
>always on one of the last CPUs.
>
>The issue shows that there IS a potential problem with doing the MTRR
>update one CPU after the other, instead just doing it in parallel (which
>is the "official" recommendation anyway). See the comment in
>cache_disable(). And it isn't as if the fix would be very complicated.
>
>
>Juergen

You are assuming that it won't break any fragile systems. I'm much more concerned about why this is happening at all.

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

[Jürgen Groß]

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

On 10.02.26 08:28, H. Peter Anvin wrote:
> On February 9, 2026 10:51:04 PM PST, "Jürgen Groß" <jgross@suse.com> wrote:
>> On 09.02.26 19:37, H. Peter Anvin wrote:
>>> On February 9, 2026 1:12:59 AM PST, Juergen Gross <jgross@suse.com> wrote:
>>>> Ping?
>>>>
>>>> I'd really like to have this in 7.0, as it is fixing a real issue on
>>>> some machines ...
>>>>
>>>>
>>>> Juergen
>>>>
>>>> On 30.01.26 12:36, Juergen Gross wrote:
>>>>> 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.
>>>>>
>>>>> Changes in V2:
>>>>> - fix a function comment header in patch 2
>>>>>
>>>>> 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 | 139 ++++++++++++++++++++++++-----
>>>>>     arch/x86/kernel/cpu/mtrr/mtrr.c    |   3 +
>>>>>     arch/x86/kernel/cpu/mtrr/mtrr.h    |   2 +
>>>>>     6 files changed, 122 insertions(+), 106 deletions(-)
>>>>>
>>>>
>>>
>>> First of all, what machines are even needing MTRR updates these days?
>>
>> I'm not aware this machine really needed an update.
>>
>>> This isn't a rhetorical question. It is important to understand what the underlying problem is.
>>
>> It just took several seconds for all CPUs to check if there is an update
>> needed. It might be an issue with firmware, topology, whatever. It happened
>> in a test doing 300 cold boots in a row after roughly 70 loop iterations,
>> always on one of the last CPUs.
>>
>> The issue shows that there IS a potential problem with doing the MTRR
>> update one CPU after the other, instead just doing it in parallel (which
>> is the "official" recommendation anyway). See the comment in
>> cache_disable(). And it isn't as if the fix would be very complicated.
>>
>>
>> Juergen
> 
> You are assuming that it won't break any fragile systems. I'm much more concerned about why this is happening at all.

I'm having a hard time seeing why my series would break fragile systems.
Its not as if I would change anything regarding the handling on each
cpu.

My main suspect why this is happening is the topology of the system
(8 socket NUMA machine), causing the uncached memory accesses to have a
rather high latency (multiple hops for accessing some memory), causing
each cpu to need some time for checking all MTRRs.


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 --]

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

[H. Peter Anvin]

On February 12, 2026 8:24:18 AM PST, "Jürgen Groß" <jgross@suse.com> wrote:
>On 10.02.26 08:28, H. Peter Anvin wrote:
>> On February 9, 2026 10:51:04 PM PST, "Jürgen Groß" <jgross@suse.com> wrote:
>>> On 09.02.26 19:37, H. Peter Anvin wrote:
>>>> On February 9, 2026 1:12:59 AM PST, Juergen Gross <jgross@suse.com> wrote:
>>>>> Ping?
>>>>> 
>>>>> I'd really like to have this in 7.0, as it is fixing a real issue on
>>>>> some machines ...
>>>>> 
>>>>> 
>>>>> Juergen
>>>>> 
>>>>> On 30.01.26 12:36, Juergen Gross wrote:
>>>>>> 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.
>>>>>> 
>>>>>> Changes in V2:
>>>>>> - fix a function comment header in patch 2
>>>>>> 
>>>>>> 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 | 139 ++++++++++++++++++++++++-----
>>>>>>     arch/x86/kernel/cpu/mtrr/mtrr.c    |   3 +
>>>>>>     arch/x86/kernel/cpu/mtrr/mtrr.h    |   2 +
>>>>>>     6 files changed, 122 insertions(+), 106 deletions(-)
>>>>>> 
>>>>> 
>>>> 
>>>> First of all, what machines are even needing MTRR updates these days?
>>> 
>>> I'm not aware this machine really needed an update.
>>> 
>>>> This isn't a rhetorical question. It is important to understand what the underlying problem is.
>>> 
>>> It just took several seconds for all CPUs to check if there is an update
>>> needed. It might be an issue with firmware, topology, whatever. It happened
>>> in a test doing 300 cold boots in a row after roughly 70 loop iterations,
>>> always on one of the last CPUs.
>>> 
>>> The issue shows that there IS a potential problem with doing the MTRR
>>> update one CPU after the other, instead just doing it in parallel (which
>>> is the "official" recommendation anyway). See the comment in
>>> cache_disable(). And it isn't as if the fix would be very complicated.
>>> 
>>> 
>>> Juergen
>> 
>> You are assuming that it won't break any fragile systems. I'm much more concerned about why this is happening at all.
>
>I'm having a hard time seeing why my series would break fragile systems.
>Its not as if I would change anything regarding the handling on each
>cpu.
>
>My main suspect why this is happening is the topology of the system
>(8 socket NUMA machine), causing the uncached memory accesses to have a
>rather high latency (multiple hops for accessing some memory), causing
>each cpu to need some time for checking all MTRRs.
>
>
>Juergen

Please stop avoiding the issue, which is WHY is this happening AT ALL on a recent production system.

The fastest way to do anything is to not do it at all. 

What do the logs look like, with sufficient verbosity, for one thing?

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

[Jürgen Groß]

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

On 12.02.26 17:54, H. Peter Anvin wrote:
> On February 12, 2026 8:24:18 AM PST, "Jürgen Groß" <jgross@suse.com> wrote:
>> On 10.02.26 08:28, H. Peter Anvin wrote:
>>> On February 9, 2026 10:51:04 PM PST, "Jürgen Groß" <jgross@suse.com> wrote:
>>>> On 09.02.26 19:37, H. Peter Anvin wrote:
>>>>> On February 9, 2026 1:12:59 AM PST, Juergen Gross <jgross@suse.com> wrote:
>>>>>> Ping?
>>>>>>
>>>>>> I'd really like to have this in 7.0, as it is fixing a real issue on
>>>>>> some machines ...
>>>>>>
>>>>>>
>>>>>> Juergen
>>>>>>
>>>>>> On 30.01.26 12:36, Juergen Gross wrote:
>>>>>>> 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.
>>>>>>>
>>>>>>> Changes in V2:
>>>>>>> - fix a function comment header in patch 2
>>>>>>>
>>>>>>> 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 | 139 ++++++++++++++++++++++++-----
>>>>>>>      arch/x86/kernel/cpu/mtrr/mtrr.c    |   3 +
>>>>>>>      arch/x86/kernel/cpu/mtrr/mtrr.h    |   2 +
>>>>>>>      6 files changed, 122 insertions(+), 106 deletions(-)
>>>>>>>
>>>>>>
>>>>>
>>>>> First of all, what machines are even needing MTRR updates these days?
>>>>
>>>> I'm not aware this machine really needed an update.
>>>>
>>>>> This isn't a rhetorical question. It is important to understand what the underlying problem is.
>>>>
>>>> It just took several seconds for all CPUs to check if there is an update
>>>> needed. It might be an issue with firmware, topology, whatever. It happened
>>>> in a test doing 300 cold boots in a row after roughly 70 loop iterations,
>>>> always on one of the last CPUs.
>>>>
>>>> The issue shows that there IS a potential problem with doing the MTRR
>>>> update one CPU after the other, instead just doing it in parallel (which
>>>> is the "official" recommendation anyway). See the comment in
>>>> cache_disable(). And it isn't as if the fix would be very complicated.
>>>>
>>>>
>>>> Juergen
>>>
>>> You are assuming that it won't break any fragile systems. I'm much more concerned about why this is happening at all.
>>
>> I'm having a hard time seeing why my series would break fragile systems.
>> Its not as if I would change anything regarding the handling on each
>> cpu.
>>
>> My main suspect why this is happening is the topology of the system
>> (8 socket NUMA machine), causing the uncached memory accesses to have a
>> rather high latency (multiple hops for accessing some memory), causing
>> each cpu to need some time for checking all MTRRs.
>>
>>
>> Juergen
> 
> Please stop avoiding the issue, which is WHY is this happening AT ALL on a recent production system.
> 
> The fastest way to do anything is to not do it at all.
> 
> What do the logs look like, with sufficient verbosity, for one thing?

I asked for more detailed logs. And indeed in the log with my patches applied
the following messages could be seen:

[   72.301321][    T1] mtrr: your CPUs had inconsistent MTRRdefType settings
[   72.310541][    T1] mtrr: probably your BIOS does not setup all CPUs.
[   72.318539][    T1] mtrr: corrected configuration.

While this probably is the reason for the long time MTRR setup on the APs
needed, I'm still thinking my patches are making a lot of sense.

I have asked they check their BIOS, of course.


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 --]

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

[Juergen Gross]

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

On 13.02.26 14:21, Jürgen Groß wrote:
> On 12.02.26 17:54, H. Peter Anvin wrote:
>> On February 12, 2026 8:24:18 AM PST, "Jürgen Groß" <jgross@suse.com> wrote:
>>> On 10.02.26 08:28, H. Peter Anvin wrote:
>>>> On February 9, 2026 10:51:04 PM PST, "Jürgen Groß" <jgross@suse.com> wrote:
>>>>> On 09.02.26 19:37, H. Peter Anvin wrote:
>>>>>> On February 9, 2026 1:12:59 AM PST, Juergen Gross <jgross@suse.com> wrote:
>>>>>>> Ping?
>>>>>>>
>>>>>>> I'd really like to have this in 7.0, as it is fixing a real issue on
>>>>>>> some machines ...
>>>>>>>
>>>>>>>
>>>>>>> Juergen
>>>>>>>
>>>>>>> On 30.01.26 12:36, Juergen Gross wrote:
>>>>>>>> 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.
>>>>>>>>
>>>>>>>> Changes in V2:
>>>>>>>> - fix a function comment header in patch 2
>>>>>>>>
>>>>>>>> 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 | 139 ++++++++++++++++++++++++-----
>>>>>>>>      arch/x86/kernel/cpu/mtrr/mtrr.c    |   3 +
>>>>>>>>      arch/x86/kernel/cpu/mtrr/mtrr.h    |   2 +
>>>>>>>>      6 files changed, 122 insertions(+), 106 deletions(-)
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>> First of all, what machines are even needing MTRR updates these days?
>>>>>
>>>>> I'm not aware this machine really needed an update.
>>>>>
>>>>>> This isn't a rhetorical question. It is important to understand what the 
>>>>>> underlying problem is.
>>>>>
>>>>> It just took several seconds for all CPUs to check if there is an update
>>>>> needed. It might be an issue with firmware, topology, whatever. It happened
>>>>> in a test doing 300 cold boots in a row after roughly 70 loop iterations,
>>>>> always on one of the last CPUs.
>>>>>
>>>>> The issue shows that there IS a potential problem with doing the MTRR
>>>>> update one CPU after the other, instead just doing it in parallel (which
>>>>> is the "official" recommendation anyway). See the comment in
>>>>> cache_disable(). And it isn't as if the fix would be very complicated.
>>>>>
>>>>>
>>>>> Juergen
>>>>
>>>> You are assuming that it won't break any fragile systems. I'm much more 
>>>> concerned about why this is happening at all.
>>>
>>> I'm having a hard time seeing why my series would break fragile systems.
>>> Its not as if I would change anything regarding the handling on each
>>> cpu.
>>>
>>> My main suspect why this is happening is the topology of the system
>>> (8 socket NUMA machine), causing the uncached memory accesses to have a
>>> rather high latency (multiple hops for accessing some memory), causing
>>> each cpu to need some time for checking all MTRRs.
>>>
>>>
>>> Juergen
>>
>> Please stop avoiding the issue, which is WHY is this happening AT ALL on a 
>> recent production system.
>>
>> The fastest way to do anything is to not do it at all.
>>
>> What do the logs look like, with sufficient verbosity, for one thing?
> 
> I asked for more detailed logs. And indeed in the log with my patches applied
> the following messages could be seen:
> 
> [   72.301321][    T1] mtrr: your CPUs had inconsistent MTRRdefType settings
> [   72.310541][    T1] mtrr: probably your BIOS does not setup all CPUs.
> [   72.318539][    T1] mtrr: corrected configuration.
> 
> While this probably is the reason for the long time MTRR setup on the APs
> needed, I'm still thinking my patches are making a lot of sense.
> 
> I have asked they check their BIOS, of course.

Any further comments?

Any real reason not to take this patch series?


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 --]

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

[Dave Hansen]

On 3/11/26 02:08, Juergen Gross wrote:
> Any real reason not to take this patch series?

Uhh, it has zero review comments on it. That's usually a pretty good reason.

It is a little worrying that this is even happening. Is there just a
single blip during boot when the MTRRs are "corrected" and then it's
never a problem again? Or is it causing latency blips all the time?

I do share some of Peter's concern that this is creaky, fragile,
lightly-used code and this series is mucking with it to work around a
BIOS issue.

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

[Jürgen Groß]

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

On 12.03.26 22:33, Dave Hansen wrote:
> On 3/11/26 02:08, Juergen Gross wrote:
>> Any real reason not to take this patch series?
> 
> Uhh, it has zero review comments on it. That's usually a pretty good reason.
> 
> It is a little worrying that this is even happening. Is there just a
> single blip during boot when the MTRRs are "corrected" and then it's
> never a problem again? Or is it causing latency blips all the time?

The machine having the lockups has been verified to have inconsistent
initial MTRR values, so the APs (at least some of them) need to update
their MTRR registers during boot.

BUT: I believe that the same could happen in case some device driver needs
to set some MTRR registers when adding a device. The code will do the same
as during boot: all CPUs need to do the update of the MTRR(s) using
stop_machine_cpuslocked(), which will serialize the updates across the CPUs.
With enough CPUs on the machine time will sum up again.

> I do share some of Peter's concern that this is creaky, fragile,
> lightly-used code and this series is mucking with it to work around a
> BIOS issue.

The real change on each CPU is just the drop of global variable use and the
drop of the lock. MTRR handling itself on each CPU isn't modified.

Additionally there is even a comment in today's code that doing only one
CPU at a time is not an optimal solution and that doing all CPUs at the same
time would be the preferred way to handle it.

In case you are really worried that updating all CPUs in parallel might be
problematic in rare cases, I could be talked into making the lock optional
(defaulting to not using it) and controllable via a boot parameter.


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 --]