[PATCH v3 0/4] Fix racy and incorrect updates to MSR_K7_HWCR

[PATCH v3 0/4] Fix racy and incorrect updates to MSR_K7_HWCR

[Jim Mattson]

I was backporting commit 65f55a301766 ("x86/CPU/AMD: Add CPUID faulting
support") to a local branch based on Linux v6.12, when our internal Sashiko
asked:

> Can this corrupt MSR_K7_HWCR? disable_cpuid()->set_cpuid_faulting() is
> called with preemption disabled, but interrupts are still enabled. Since
> msr_set_bit() performs a read-modify-write without disabling interrupts,
> if an IPI arrives between the read and write and modifies MSR_K7_HWCR
> (e.g. acpi-cpufreq toggling Core Performance Boost), the IPI's update
> will be lost.

To confirm that this wasn't just AI slop, I set up an empirical test on a
Turin system. First, I replaced the amd-pstate cpufreq driver with
acpi-cpufreq. Then I ran a test program, where one thread repeatedly reads
CPU0's HWCR, toggles /sys/devices/system/cpu/cpufreq/boost, reads CPU0's
HWCR again, and then verifies that the CPB_DIS bit has flipped. A second
thread, pinned to CPU0, repeatedly calls arch_prctl(ARCH_SET_CPUID, <val>),
where <val> alternates between 0 and 1. With the second thread running, the
first thread soon fails the verification step, indicating that the CPB_DIS
bit change is, in fact, lost.

Andrew Cooper raised a valid concern regarding the core-scoped nature of
MSR_K7_HWCR on some AMD CPUs. When the MSR is shared between SMT siblings,
a cross-CPU race is possible if SMT threads concurrently modify the
MSR. Centralizing all runtime updates through the amd_update_hwcr() helper
ensures that if explicit serialization is desired, it can be implemented
transparently in a single place. Note that on recent AMD CPUs, the MSR is
thread-scoped.

Google's internal Sashiko highlighted a separate, pre-existing
architectural issue in acpi-cpufreq that Borislav and I briefly touched on:
during shared policy teardown (e.g., driver unload),
acpi_cpufreq_cpu_exit() is only executed for the final CPU in the
policy. As a result, cpufreq_boost_down_prep() only restores the
boost-disable MSR bit on that single CPU, leaving all other sibling CPUs
that shared the policy with the disable bit still set. On Intel platforms,
where the turbo-disable MSR bit acts as a logical OR across the package,
this leaves the entire package permanently boost-disabled after the driver
is unloaded. This issue out of scope for this series, as it requires a
broader redesign of cpufreq policy teardown coordination, and should be
addressed separately.

v2 -> v3:
 - Add WARN_ON_ONCE(in_nmi()) to the helper to ensure NMI safety [Yosry].
 - Redesign amd_update_hwcr() to be a simple bit-based wrapper [Yosry].
 - Fix cpufreq_boost_down_prep() to target the correct CPU via IPI, resolving
   the wrong-CPU update and Intel preemption/migration races [Sashiko].
 - Drop the Fixes tag from patch 4, since the race is now fixed in patch 3.

v2: https://lore.kernel.org/all/20260612215729.1532175-1-jmattson@google.com/
v1: https://lore.kernel.org/all/20260609211611.466231-1-jmattson@google.com/

Jim Mattson (4):
  x86/CPU/AMD: Avoid racy updates to MSR_K7_HWCR in set_cpuid_faulting()
  x86/mce/inject: Avoid racy updates to MSR_K7_HWCR during MCE injection
  cpufreq: ACPI: Use IPI to update boost MSR in
    cpufreq_boost_down_prep()
  cpufreq: ACPI: Use amd_update_hwcr() for MSR_K7_HWCR updates

 arch/x86/include/asm/msr-index.h |  2 ++
 arch/x86/include/asm/processor.h |  2 ++
 arch/x86/kernel/cpu/amd.c        | 38 ++++++++++++++++++++++++++
 arch/x86/kernel/cpu/mce/inject.c | 46 ++++++++++----------------------
 arch/x86/kernel/process.c        |  5 +---
 drivers/cpufreq/acpi-cpufreq.c   | 11 ++++----
 6 files changed, 62 insertions(+), 42 deletions(-)


base-commit: 83f1454877cc292b88baf13c829c16ce6937d120

[PATCH v3 1/4] x86/CPU/AMD: Avoid racy updates to MSR_K7_HWCR in set_cpuid_faulting()

[Jim Mattson]

Since msr_set_bit() and msr_clear_bit() perform a non-atomic update to an
MSR, they can race with a write to the same MSR from interrupt context.

On AMD CPUs, set_cpuid_faulting() uses these functions to modify
MSR_K7_HWCR from process context, with preemption disabled but interrupts
enabled. The acpi-cpufreq driver's boost_set_msr() modifies HWCR from
interrupt context. If a crosscall IPI arrives between
set_cpuid_faulting()'s read and write of MSR_K7_HWCR and toggles the Core
Performance Boost disable bit (CPB_DIS), the IPI's update is lost.

This race has been observed empirically on a Turin system running the
acpi-cpufreq driver with a synthetic test. One thread repeatedly toggles
/sys/devices/system/cpu/cpufreq/boost and verifies CPB_DIS on CPU0 after
each write. A second thread pinned to CPU0 calls arch_prctl(ARCH_SET_CPUID,
<val>), with alternating <val>s of 0 and 1. CPB_DIS bit changes are
sometimes lost.

Introduce amd_update_hwcr() to perform an interrupt-safe read-modify-write
of MSR_K7_HWCR, and use it in set_cpuid_faulting() to prevent races with
HWCR updates in interrupt context. Note that when set_cpuid_faulting() is
called from __switch_to_xtra(), interrupts are already disabled, so the
race is only possible on the arch_prctl() paths.

Reported-by: Sashiko (gemini/gemini-3.1-pro-preview)
Closes: https://lore.kernel.org/all/20260609211611.466231-1-jmattson@google.com/
Suggested-by: Borislav Petkov <bp@alien8.de>
Fixes: 65f55a301766 ("x86/CPU/AMD: Add CPUID faulting support")
Assisted-by: Gemini:gemini-3.5-pro
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/include/asm/processor.h |  2 ++
 arch/x86/kernel/cpu/amd.c        | 38 ++++++++++++++++++++++++++++++++
 arch/x86/kernel/process.c        |  5 +----
 3 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 87b1d4c0727e..153b621777ae 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -722,9 +722,11 @@ static __always_inline void amd_clear_divider(void)
 }
 
 extern void amd_check_microcode(void);
+extern int amd_update_hwcr(u8 bit, bool set);
 #else
 static inline void amd_clear_divider(void)		{ }
 static inline void amd_check_microcode(void)		{ }
+static inline int amd_update_hwcr(u8 bit, bool set) { return -ENODEV; }
 #endif
 
 extern unsigned long arch_align_stack(unsigned long sp);
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 487ac147e11f..15e7ca3b815d 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -1320,6 +1320,44 @@ void amd_check_microcode(void)
 		on_each_cpu(zenbleed_check_cpu, NULL, 1);
 }
 
+/**
+ * amd_update_hwcr - Update MSR_K7_HWCR on the executing CPU
+ * @bit: bit number to change
+ * @set: whether to set or clear the bit
+ *
+ * MSR_K7_HWCR is written from both process context (e.g. CPUID faulting
+ * updates via arch_prctl(ARCH_SET_CPUID)) and interrupt context (e.g.
+ * Core Performance Boost updates IPI'd by the acpi-cpufreq driver), so
+ * a read-modify-write of the MSR must be performed with interrupts
+ * disabled to avoid losing an update made by an intervening interrupt.
+ * All runtime (non-initialization) updates of MSR_K7_HWCR should go
+ * through this helper.
+ *
+ * Context: Any context except NMI. Disabling interrupts does not
+ *          serialize against an NMI, so NMI handlers must not write
+ *          MSR_K7_HWCR. Warns if called from NMI context.
+ *
+ * Return: 0 on success, negative error code if an MSR access faults.
+ */
+int amd_update_hwcr(u8 bit, bool set)
+{
+	unsigned long flags;
+	int ret;
+
+	if (WARN_ON_ONCE(in_nmi()))
+		return -EINVAL;
+
+	local_irq_save(flags);
+	if (set)
+		ret = msr_set_bit(MSR_K7_HWCR, bit);
+	else
+		ret = msr_clear_bit(MSR_K7_HWCR, bit);
+	local_irq_restore(flags);
+
+	return ret < 0 ? ret : 0;
+}
+EXPORT_SYMBOL_GPL(amd_update_hwcr);
+
 static const char * const s5_reset_reason_txt[] = {
 	[0]  = "thermal pin BP_THERMTRIP_L was tripped",
 	[1]  = "power button was pressed for 4 seconds",
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index a554f19c9973..8895cc0fa472 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -354,10 +354,7 @@ static void set_cpuid_faulting(bool on)
 		this_cpu_write(msr_misc_features_shadow, msrval);
 		wrmsrq(MSR_MISC_FEATURES_ENABLES, msrval);
 	} else if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
-		if (on)
-			msr_set_bit(MSR_K7_HWCR, MSR_K7_HWCR_CPUID_USER_DIS_BIT);
-		else
-			msr_clear_bit(MSR_K7_HWCR, MSR_K7_HWCR_CPUID_USER_DIS_BIT);
+		amd_update_hwcr(MSR_K7_HWCR_CPUID_USER_DIS_BIT, on);
 	}
 }
 
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v3 2/4] x86/mce/inject: Avoid racy updates to MSR_K7_HWCR during MCE injection

[Jim Mattson]

MCE injection performs a read-modify-write of MSR_K7_HWCR as two
independent crosscalls (via toggle_hw_mce_inject) wrapping the actual MSR
writes (prepare_msrs). Another HWCR update on the target CPU could be lost
if it occurred between these crosscalls.

Introduce ipi_inject_mce() to perform the entire injection sequence (toggle
ON, write MSRs, toggle OFF) in a single IPI callback, ensuring atomicity on
the target CPU.

For the local CPU initialization path in check_hw_inj_possible(), use
amd_update_hwcr() directly to avoid IPI overhead and ensure safe updates.

Remove toggle_hw_mce_inject() as it is no longer used.

Opportunistically, replace the open-coded BIT(18) with a new
MSR_K7_HWCR_MCSTATUSWREN macro.

Fixes: 21690934d934 ("EDAC, mce_amd_inj: Enable direct writes to MCE MSRs")
Link: https://sashiko.dev/#/patchset/20260612215729.1532175-1-jmattson%40google.com?part=2
Assisted-by: Gemini:gemini-3.5-pro
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/include/asm/msr-index.h |  2 ++
 arch/x86/kernel/cpu/mce/inject.c | 46 ++++++++++----------------------
 2 files changed, 16 insertions(+), 32 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 86554de9a3f5..29c4abade594 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -896,6 +896,8 @@
 #define MSR_K7_HWCR			0xc0010015
 #define MSR_K7_HWCR_SMMLOCK_BIT		0
 #define MSR_K7_HWCR_SMMLOCK		BIT_ULL(MSR_K7_HWCR_SMMLOCK_BIT)
+#define MSR_K7_HWCR_MCSTATUSWREN_BIT	18
+#define MSR_K7_HWCR_MCSTATUSWREN	BIT_ULL(MSR_K7_HWCR_MCSTATUSWREN_BIT)
 #define MSR_K7_HWCR_IRPERF_EN_BIT	30
 #define MSR_K7_HWCR_IRPERF_EN		BIT_ULL(MSR_K7_HWCR_IRPERF_EN_BIT)
 #define MSR_K7_HWCR_CPUID_USER_DIS_BIT	35
diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 6f8a49d8baeb..1e017d8e23e4 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -31,6 +31,7 @@
 #include <asm/mce.h>
 #include <asm/msr.h>
 #include <asm/nmi.h>
+#include <asm/processor.h>
 #include <asm/smp.h>
 
 #include "internal.h"
@@ -311,30 +312,6 @@ static struct notifier_block inject_nb = {
 	.notifier_call  = mce_inject_raise,
 };
 
-/*
- * Caller needs to be make sure this cpu doesn't disappear
- * from under us, i.e.: get_cpu/put_cpu.
- */
-static int toggle_hw_mce_inject(unsigned int cpu, bool enable)
-{
-	struct msr val;
-	int err;
-
-	err = rdmsrq_on_cpu(cpu, MSR_K7_HWCR, &val.q);
-	if (err) {
-		pr_err("%s: error reading HWCR\n", __func__);
-		return err;
-	}
-
-	enable ? (val.l |= BIT(18)) : (val.l &= ~BIT(18));
-
-	err = wrmsrq_on_cpu(cpu, MSR_K7_HWCR, val.q);
-	if (err)
-		pr_err("%s: error writing HWCR\n", __func__);
-
-	return err;
-}
-
 static int __set_inj(const char *buf)
 {
 	int i;
@@ -500,6 +477,12 @@ static void prepare_msrs(void *info)
 			wrmsrq(MSR_IA32_MCx_MISC(b), m.misc);
 	}
 }
+static void ipi_inject_mce(void *info)
+{
+	amd_update_hwcr(MSR_K7_HWCR_MCSTATUSWREN_BIT, true);
+	prepare_msrs(info);
+	amd_update_hwcr(MSR_K7_HWCR_MCSTATUSWREN_BIT, false);
+}
 
 static void do_inject(void)
 {
@@ -556,13 +539,13 @@ static void do_inject(void)
 	if (!cpu_online(cpu))
 		goto err;
 
-	toggle_hw_mce_inject(cpu, true);
-
 	i_mce.mcgstatus = mcg_status;
 	i_mce.inject_flags = inj_type;
-	smp_call_function_single(cpu, prepare_msrs, &i_mce, 0);
 
-	toggle_hw_mce_inject(cpu, false);
+	if (smp_call_function_single(cpu, ipi_inject_mce, &i_mce, 1)) {
+		pr_err("%s: Error injecting MCE on CPU %d\n", __func__, cpu);
+		goto err;
+	}
 
 	switch (inj_type) {
 	case DFR_INT_INJ:
@@ -727,7 +710,6 @@ static void __init debugfs_init(void)
 
 static void check_hw_inj_possible(void)
 {
-	int cpu;
 	u8 bank;
 
 	/*
@@ -737,7 +719,7 @@ static void check_hw_inj_possible(void)
 	if (!cpu_feature_enabled(X86_FEATURE_SMCA))
 		return;
 
-	cpu = get_cpu();
+	get_cpu();
 
 	for (bank = 0; bank < MAX_NR_BANKS; ++bank) {
 		u64 status = MCI_STATUS_VAL, ipid;
@@ -747,7 +729,7 @@ static void check_hw_inj_possible(void)
 		if (!ipid)
 			continue;
 
-		toggle_hw_mce_inject(cpu, true);
+		amd_update_hwcr(MSR_K7_HWCR_MCSTATUSWREN_BIT, true);
 
 		wrmsrq_safe(mca_msr_reg(bank, MCA_STATUS), status);
 		rdmsrq_safe(mca_msr_reg(bank, MCA_STATUS), &status);
@@ -759,7 +741,7 @@ static void check_hw_inj_possible(void)
 				"Try using APEI EINJ instead.\n");
 		}
 
-		toggle_hw_mce_inject(cpu, false);
+		amd_update_hwcr(MSR_K7_HWCR_MCSTATUSWREN_BIT, false);
 
 		break;
 	}
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v3 3/4] cpufreq: ACPI: Use IPI to update boost MSR in cpufreq_boost_down_prep()

[Jim Mattson]

During driver exit or CPU hotplug, acpi_cpufreq_cpu_exit() calls
cpufreq_boost_down_prep() to re-enable boost on the target CPU.  However,
cpufreq_boost_down_prep() ignores the target CPU parameter and calls
boost_set_msr(1) locally. Since this runs in an unbound process context, it
updates the local CPU's MSR while leaving the target CPU's boost state
unchanged.

On Intel platforms, the open-coded read-modify-write of
MSR_IA32_MISC_ENABLE in boost_set_msr() is vulnerable to preemption and
thread migration if executed in process context.

Fix both issues by routing the MSR update through
smp_call_function_single() to execute boost_set_msr() synchronously on the
target CPU. This ensures the correct CPU is updated and that the RMW
sequence on Intel executes safely in IPI context with interrupts disabled.

Fixes: a3605c46e0c0 ("cpufreq: acpi-cpufreq: drop rdmsr_on_cpus() usage")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260612215729.1532175-1-jmattson%40google.com
Assisted-by: Gemini:gemini-3.5-pro
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 drivers/cpufreq/acpi-cpufreq.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 21639d9ac753..48d678812fd6 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -528,10 +528,11 @@ static void free_acpi_perf_data(void)
 static int cpufreq_boost_down_prep(unsigned int cpu)
 {
 	/*
-	 * Clear the boost-disable bit on the CPU_DOWN path so that
-	 * this cpu cannot block the remaining ones from boosting.
+	 * Clear the boost-disable bit on the target CPU so that
+	 * it cannot block the remaining ones from boosting.
 	 */
-	return boost_set_msr(1);
+	return smp_call_function_single(cpu, boost_set_msr_each,
+					(void *)1L, 1);
 }
 
 /*
-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH v3 3/4] cpufreq: ACPI: Use IPI to update boost MSR in cpufreq_boost_down_prep()

[Rafael J. Wysocki]

On Fri, Jun 19, 2026 at 12:45 AM Jim Mattson <jmattson@google.com> wrote:
>
> During driver exit or CPU hotplug, acpi_cpufreq_cpu_exit() calls
> cpufreq_boost_down_prep() to re-enable boost on the target CPU.  However,
> cpufreq_boost_down_prep() ignores the target CPU parameter and calls
> boost_set_msr(1) locally. Since this runs in an unbound process context, it
> updates the local CPU's MSR while leaving the target CPU's boost state
> unchanged.
>
> On Intel platforms, the open-coded read-modify-write of
> MSR_IA32_MISC_ENABLE in boost_set_msr() is vulnerable to preemption and
> thread migration if executed in process context.
>
> Fix both issues by routing the MSR update through
> smp_call_function_single() to execute boost_set_msr() synchronously on the
> target CPU. This ensures the correct CPU is updated and that the RMW
> sequence on Intel executes safely in IPI context with interrupts disabled.
>
> Fixes: a3605c46e0c0 ("cpufreq: acpi-cpufreq: drop rdmsr_on_cpus() usage")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260612215729.1532175-1-jmattson%40google.com
> Assisted-by: Gemini:gemini-3.5-pro
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  drivers/cpufreq/acpi-cpufreq.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 21639d9ac753..48d678812fd6 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -528,10 +528,11 @@ static void free_acpi_perf_data(void)
>  static int cpufreq_boost_down_prep(unsigned int cpu)
>  {
>         /*
> -        * Clear the boost-disable bit on the CPU_DOWN path so that
> -        * this cpu cannot block the remaining ones from boosting.
> +        * Clear the boost-disable bit on the target CPU so that
> +        * it cannot block the remaining ones from boosting.
>          */
> -       return boost_set_msr(1);
> +       return smp_call_function_single(cpu, boost_set_msr_each,
> +                                       (void *)1L, 1);

While I have not investigated the history of this code, I'm wondering
why cpufreq_boost_down_prep()  is a separate function and why it takes
the CPU number argument.

The only caller of it is acpi_cpufreq_cpu_exit() which could just do

set_boost(policy, 1);

instead of calling it (and might check if boost was supported at all
for that matter).

>  }
>
>  /*
> --

Re: [PATCH v3 3/4] cpufreq: ACPI: Use IPI to update boost MSR in cpufreq_boost_down_prep()

[Zhongqiu Han]

On 6/23/2026 8:47 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 19, 2026 at 12:45 AM Jim Mattson <jmattson@google.com> wrote:
>>
>> During driver exit or CPU hotplug, acpi_cpufreq_cpu_exit() calls
>> cpufreq_boost_down_prep() to re-enable boost on the target CPU.  However,
>> cpufreq_boost_down_prep() ignores the target CPU parameter and calls
>> boost_set_msr(1) locally. Since this runs in an unbound process context, it
>> updates the local CPU's MSR while leaving the target CPU's boost state
>> unchanged.
>>
>> On Intel platforms, the open-coded read-modify-write of
>> MSR_IA32_MISC_ENABLE in boost_set_msr() is vulnerable to preemption and
>> thread migration if executed in process context.
>>
>> Fix both issues by routing the MSR update through
>> smp_call_function_single() to execute boost_set_msr() synchronously on the
>> target CPU. This ensures the correct CPU is updated and that the RMW
>> sequence on Intel executes safely in IPI context with interrupts disabled.
>>
>> Fixes: a3605c46e0c0 ("cpufreq: acpi-cpufreq: drop rdmsr_on_cpus() usage")
>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>> Closes: https://sashiko.dev/#/patchset/20260612215729.1532175-1-jmattson%40google.com
>> Assisted-by: Gemini:gemini-3.5-pro
>> Signed-off-by: Jim Mattson <jmattson@google.com>
>> ---
>>   drivers/cpufreq/acpi-cpufreq.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
>> index 21639d9ac753..48d678812fd6 100644
>> --- a/drivers/cpufreq/acpi-cpufreq.c
>> +++ b/drivers/cpufreq/acpi-cpufreq.c
>> @@ -528,10 +528,11 @@ static void free_acpi_perf_data(void)
>>   static int cpufreq_boost_down_prep(unsigned int cpu)
>>   {
>>          /*
>> -        * Clear the boost-disable bit on the CPU_DOWN path so that
>> -        * this cpu cannot block the remaining ones from boosting.
>> +        * Clear the boost-disable bit on the target CPU so that
>> +        * it cannot block the remaining ones from boosting.
>>           */
>> -       return boost_set_msr(1);
>> +       return smp_call_function_single(cpu, boost_set_msr_each,
>> +                                       (void *)1L, 1);
> 
> While I have not investigated the history of this code, I'm wondering
> why cpufreq_boost_down_prep()  is a separate function and why it takes
> the CPU number argument.
> 
> The only caller of it is acpi_cpufreq_cpu_exit() which could just do
> 
> set_boost(policy, 1);
> 
> instead of calling it (and might check if boost was supported at all
> for that matter).


1) It seems that set_boost(policy, 1) would be a no-op here, since
policy->cpus appears to be empty by the time ->exit() is called.

acpi-cpufreq only implements ->exit (no ->offline), and ->exit() is
reached only when policy_is_inactive() (i.e. cpumask_empty
(policy->cpus)) is true. set_boost() does on_each_cpu_mask(policy->cpus,
...), which on an empty mask runs nothing.


2) The patch's restore on only policy->cpu
(smp_call_function_single(policy->cpu, ...) will not fully cover shared
policies, and how much it covers seems to depend on the boost-disable
bit's scope (but that seems to be already discussed in the cover letter
and is probably a separate topic).

Please feel free to correct me if I'm misunderstanding the scope of the
boost-disable bit:

On driver unload the policy CPUs are removed from policy->cpus but seem
to stay hardware-online, so a stale disable bit on a non-last CPU might
still matter (those CPUs are presumably still running and may want to
boost). Abstractly:

- if the bit is per-CPU independent: clearing policy->cpu seems to fix
only that CPU, and the other still-online CPUs in the policy may stay
disabled.

- if the bit is shared within a group (e.g. core/package), "disabled if
any member set": clearing policy->cpu maybe only covers its own group,
and other groups spanned by the policy could stay disabled.



> 
>>   }
>>
>>   /*
>> --
> 


-- 
Thx and BRs,
Zhongqiu Han

Re: [PATCH v3 3/4] cpufreq: ACPI: Use IPI to update boost MSR in cpufreq_boost_down_prep()

[Rafael J. Wysocki]

On Wed, Jun 24, 2026 at 1:26 PM Zhongqiu Han
<zhongqiu.han@oss.qualcomm.com> wrote:
>
> On 6/23/2026 8:47 PM, Rafael J. Wysocki wrote:
> > On Fri, Jun 19, 2026 at 12:45 AM Jim Mattson <jmattson@google.com> wrote:
> >>
> >> During driver exit or CPU hotplug, acpi_cpufreq_cpu_exit() calls
> >> cpufreq_boost_down_prep() to re-enable boost on the target CPU.  However,
> >> cpufreq_boost_down_prep() ignores the target CPU parameter and calls
> >> boost_set_msr(1) locally. Since this runs in an unbound process context, it
> >> updates the local CPU's MSR while leaving the target CPU's boost state
> >> unchanged.
> >>
> >> On Intel platforms, the open-coded read-modify-write of
> >> MSR_IA32_MISC_ENABLE in boost_set_msr() is vulnerable to preemption and
> >> thread migration if executed in process context.
> >>
> >> Fix both issues by routing the MSR update through
> >> smp_call_function_single() to execute boost_set_msr() synchronously on the
> >> target CPU. This ensures the correct CPU is updated and that the RMW
> >> sequence on Intel executes safely in IPI context with interrupts disabled.
> >>
> >> Fixes: a3605c46e0c0 ("cpufreq: acpi-cpufreq: drop rdmsr_on_cpus() usage")
> >> Reported-by: Sashiko <sashiko-bot@kernel.org>
> >> Closes: https://sashiko.dev/#/patchset/20260612215729.1532175-1-jmattson%40google.com
> >> Assisted-by: Gemini:gemini-3.5-pro
> >> Signed-off-by: Jim Mattson <jmattson@google.com>
> >> ---
> >>   drivers/cpufreq/acpi-cpufreq.c | 7 ++++---
> >>   1 file changed, 4 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> >> index 21639d9ac753..48d678812fd6 100644
> >> --- a/drivers/cpufreq/acpi-cpufreq.c
> >> +++ b/drivers/cpufreq/acpi-cpufreq.c
> >> @@ -528,10 +528,11 @@ static void free_acpi_perf_data(void)
> >>   static int cpufreq_boost_down_prep(unsigned int cpu)
> >>   {
> >>          /*
> >> -        * Clear the boost-disable bit on the CPU_DOWN path so that
> >> -        * this cpu cannot block the remaining ones from boosting.
> >> +        * Clear the boost-disable bit on the target CPU so that
> >> +        * it cannot block the remaining ones from boosting.
> >>           */
> >> -       return boost_set_msr(1);
> >> +       return smp_call_function_single(cpu, boost_set_msr_each,
> >> +                                       (void *)1L, 1);
> >
> > While I have not investigated the history of this code, I'm wondering
> > why cpufreq_boost_down_prep()  is a separate function and why it takes
> > the CPU number argument.
> >
> > The only caller of it is acpi_cpufreq_cpu_exit() which could just do
> >
> > set_boost(policy, 1);
> >
> > instead of calling it (and might check if boost was supported at all
> > for that matter).
>
>
> 1) It seems that set_boost(policy, 1) would be a no-op here, since
> policy->cpus appears to be empty by the time ->exit() is called.

All right, but this means that ->exit() is not quite suitable for this
type of cleanup.

> acpi-cpufreq only implements ->exit (no ->offline), and ->exit() is
> reached only when policy_is_inactive() (i.e. cpumask_empty
> (policy->cpus)) is true. set_boost() does on_each_cpu_mask(policy->cpus,
> ...), which on an empty mask runs nothing.

Arguably though, .offline() should take care of the boost cleanup for
the CPU going offline.  .exit() is too late because it can't clean up
CPUs that went offline before.

> 2) The patch's restore on only policy->cpu
> (smp_call_function_single(policy->cpu, ...) will not fully cover shared
> policies, and how much it covers seems to depend on the boost-disable
> bit's scope (but that seems to be already discussed in the cover letter
> and is probably a separate topic).
>
> Please feel free to correct me if I'm misunderstanding the scope of the
> boost-disable bit:
>
> On driver unload the policy CPUs are removed from policy->cpus but seem
> to stay hardware-online, so a stale disable bit on a non-last CPU might
> still matter (those CPUs are presumably still running and may want to
> boost). Abstractly:
>
> - if the bit is per-CPU independent: clearing policy->cpu seems to fix
> only that CPU, and the other still-online CPUs in the policy may stay
> disabled.

Yes.

> - if the bit is shared within a group (e.g. core/package), "disabled if
> any member set": clearing policy->cpu maybe only covers its own group,
> and other groups spanned by the policy could stay disabled.

Yes.

Thus it is better to clear it at offline time for the given CPU.

Re: [PATCH v3 3/4] cpufreq: ACPI: Use IPI to update boost MSR in cpufreq_boost_down_prep()

[Zhongqiu Han]

On 6/24/2026 9:35 PM, Rafael J. Wysocki wrote:
> On Wed, Jun 24, 2026 at 1:26 PM Zhongqiu Han
> <zhongqiu.han@oss.qualcomm.com> wrote:
>>
>> On 6/23/2026 8:47 PM, Rafael J. Wysocki wrote:
>>> On Fri, Jun 19, 2026 at 12:45 AM Jim Mattson <jmattson@google.com> wrote:
>>>>
>>>> During driver exit or CPU hotplug, acpi_cpufreq_cpu_exit() calls
>>>> cpufreq_boost_down_prep() to re-enable boost on the target CPU.  However,
>>>> cpufreq_boost_down_prep() ignores the target CPU parameter and calls
>>>> boost_set_msr(1) locally. Since this runs in an unbound process context, it
>>>> updates the local CPU's MSR while leaving the target CPU's boost state
>>>> unchanged.
>>>>
>>>> On Intel platforms, the open-coded read-modify-write of
>>>> MSR_IA32_MISC_ENABLE in boost_set_msr() is vulnerable to preemption and
>>>> thread migration if executed in process context.
>>>>
>>>> Fix both issues by routing the MSR update through
>>>> smp_call_function_single() to execute boost_set_msr() synchronously on the
>>>> target CPU. This ensures the correct CPU is updated and that the RMW
>>>> sequence on Intel executes safely in IPI context with interrupts disabled.
>>>>
>>>> Fixes: a3605c46e0c0 ("cpufreq: acpi-cpufreq: drop rdmsr_on_cpus() usage")
>>>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>>>> Closes: https://sashiko.dev/#/patchset/20260612215729.1532175-1-jmattson%40google.com
>>>> Assisted-by: Gemini:gemini-3.5-pro
>>>> Signed-off-by: Jim Mattson <jmattson@google.com>
>>>> ---
>>>>    drivers/cpufreq/acpi-cpufreq.c | 7 ++++---
>>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
>>>> index 21639d9ac753..48d678812fd6 100644
>>>> --- a/drivers/cpufreq/acpi-cpufreq.c
>>>> +++ b/drivers/cpufreq/acpi-cpufreq.c
>>>> @@ -528,10 +528,11 @@ static void free_acpi_perf_data(void)
>>>>    static int cpufreq_boost_down_prep(unsigned int cpu)
>>>>    {
>>>>           /*
>>>> -        * Clear the boost-disable bit on the CPU_DOWN path so that
>>>> -        * this cpu cannot block the remaining ones from boosting.
>>>> +        * Clear the boost-disable bit on the target CPU so that
>>>> +        * it cannot block the remaining ones from boosting.
>>>>            */
>>>> -       return boost_set_msr(1);
>>>> +       return smp_call_function_single(cpu, boost_set_msr_each,
>>>> +                                       (void *)1L, 1);
>>>
>>> While I have not investigated the history of this code, I'm wondering
>>> why cpufreq_boost_down_prep()  is a separate function and why it takes
>>> the CPU number argument.
>>>
>>> The only caller of it is acpi_cpufreq_cpu_exit() which could just do
>>>
>>> set_boost(policy, 1);
>>>
>>> instead of calling it (and might check if boost was supported at all
>>> for that matter).
>>
>>
>> 1) It seems that set_boost(policy, 1) would be a no-op here, since
>> policy->cpus appears to be empty by the time ->exit() is called.
> 
> All right, but this means that ->exit() is not quite suitable for this
> type of cleanup.
> 
>> acpi-cpufreq only implements ->exit (no ->offline), and ->exit() is
>> reached only when policy_is_inactive() (i.e. cpumask_empty
>> (policy->cpus)) is true. set_boost() does on_each_cpu_mask(policy->cpus,
>> ...), which on an empty mask runs nothing.
> 
> Arguably though, .offline() should take care of the boost cleanup for
> the CPU going offline.  .exit() is too late because it can't clean up
> CPUs that went offline before.
> 
>> 2) The patch's restore on only policy->cpu
>> (smp_call_function_single(policy->cpu, ...) will not fully cover shared
>> policies, and how much it covers seems to depend on the boost-disable
>> bit's scope (but that seems to be already discussed in the cover letter
>> and is probably a separate topic).
>>
>> Please feel free to correct me if I'm misunderstanding the scope of the
>> boost-disable bit:
>>
>> On driver unload the policy CPUs are removed from policy->cpus but seem
>> to stay hardware-online, so a stale disable bit on a non-last CPU might
>> still matter (those CPUs are presumably still running and may want to
>> boost). Abstractly:
>>
>> - if the bit is per-CPU independent: clearing policy->cpu seems to fix
>> only that CPU, and the other still-online CPUs in the policy may stay
>> disabled.
> 
> Yes.
> 
>> - if the bit is shared within a group (e.g. core/package), "disabled if
>> any member set": clearing policy->cpu maybe only covers its own group,
>> and other groups spanned by the policy could stay disabled.
> 
> Yes.
> 
> Thus it is better to clear it at offline time for the given CPU.


Hi Rafael,
Thanks for the clarification.

I agree. Just for discussion, two possible approaches come to mind:

Option 1 (structurally clean):
Introduce an optional per-CPU cpufreq_driver->set_boost_cpu(cpu, enable)
hook and call it from __cpufreq_offline() before cpumask_clear_cpu(),
but only on the unbind path (not on plain CPU hotplug-offline - clearing
it there maybe wrongly re-enable boost on a still-online sibling on
shared-scope HW). Roughly (rough sketch only):

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 21639d9ac753..5f298fbb0d8c 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -525,13 +525,10 @@ static void free_acpi_perf_data(void)
         free_percpu(acpi_perf_data);
  }

-static int cpufreq_boost_down_prep(unsigned int cpu)
+static void acpi_cpufreq_set_boost_cpu(unsigned int cpu, bool enable)
  {
-       /*
-        * Clear the boost-disable bit on the CPU_DOWN path so that
-        * this cpu cannot block the remaining ones from boosting.
-        */
-       return boost_set_msr(1);
+       smp_call_function_single(cpu, boost_set_msr_each,
+                                (void *)(long)enable, 1);
  }

  /*
@@ -951,7 +948,6 @@ static void acpi_cpufreq_cpu_exit(struct 
cpufreq_policy *policy)

         pr_debug("%s\n", __func__);

-       cpufreq_boost_down_prep(policy->cpu);
         policy->fast_switch_possible = false;
         policy->driver_data = NULL;
         acpi_processor_unregister_performance(data->acpi_perf_cpu);
@@ -986,6 +982,7 @@ static struct cpufreq_driver acpi_cpufreq_driver = {
         .bios_limit     = acpi_processor_get_bios_limit,
         .init           = acpi_cpufreq_cpu_init,
         .exit           = acpi_cpufreq_cpu_exit,
+       .set_boost_cpu  = acpi_cpufreq_set_boost_cpu,
         .resume         = acpi_cpufreq_resume,
         .name           = "acpi-cpufreq",
         .attr           = acpi_cpufreq_attr,
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 507224c9ecd3..c4952ca3e8a5 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1669,13 +1669,18 @@ static int cpufreq_add_dev(struct device *dev, 
struct subsys_interface *sif)
         return 0;
  }

-static void __cpufreq_offline(unsigned int cpu, struct cpufreq_policy 
*policy)
+static void __cpufreq_offline(unsigned int cpu, struct cpufreq_policy 
*policy,
+                             bool unbinding)
  {
         int ret;

         if (has_target())
                 cpufreq_stop_governor(policy);

+       if (unbinding && cpufreq_driver->set_boost_cpu &&
+           policy->boost_supported)
+               cpufreq_driver->set_boost_cpu(cpu, true);
+
         cpumask_clear_cpu(cpu, policy->cpus);

         if (!policy_is_inactive(policy)) {
@@ -1730,7 +1735,7 @@ static int cpufreq_offline(unsigned int cpu)

         guard(cpufreq_policy_write)(policy);

-       __cpufreq_offline(cpu, policy);
+       __cpufreq_offline(cpu, policy, false);

         return 0;
  }
@@ -1750,7 +1755,7 @@ static void cpufreq_remove_dev(struct device *dev, 
struct subsys_interface *sif)

         scoped_guard(cpufreq_policy_write, policy) {
                 if (cpu_online(cpu))
-                       __cpufreq_offline(cpu, policy);
+                       __cpufreq_offline(cpu, policy, true);

                 remove_cpu_dev_symlink(policy, cpu, dev);

diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index ae9d1ce4f49c..c174b013dd76 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -413,6 +413,8 @@ struct cpufreq_driver {
         int             (*online)(struct cpufreq_policy *policy);
         int             (*offline)(struct cpufreq_policy *policy);
         void            (*exit)(struct cpufreq_policy *policy);
+       /* Optional */
+       void            (*set_boost_cpu)(unsigned int cpu, bool enable);
         int             (*suspend)(struct cpufreq_policy *policy);
         int             (*resume)(struct cpufreq_policy *policy);



Option 2 (lighter, but it is at odds with the direction discussed
above): stay in acpi-cpufreq's ->exit() and restore over
policy->related_cpus instead.




-- 
Thx and BRs,
Zhongqiu Han

Re: [PATCH v3 3/4] cpufreq: ACPI: Use IPI to update boost MSR in cpufreq_boost_down_prep()

[Jim Mattson]

On Thu, Jun 25, 2026 at 8:50 AM Zhongqiu Han
<zhongqiu.han@oss.qualcomm.com> wrote:

> Hi Rafael,
> Thanks for the clarification.
>
> I agree. Just for discussion, two possible approaches come to mind:
>
> Option 1 (structurally clean):
> Introduce an optional per-CPU cpufreq_driver->set_boost_cpu(cpu, enable)
> hook and call it from __cpufreq_offline() before cpumask_clear_cpu(),
> but only on the unbind path (not on plain CPU hotplug-offline - clearing
> it there maybe wrongly re-enable boost on a still-online sibling on
> shared-scope HW). Roughly (rough sketch only):
>
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 21639d9ac753..5f298fbb0d8c 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -525,13 +525,10 @@ static void free_acpi_perf_data(void)
>          free_percpu(acpi_perf_data);
>   }
>
> -static int cpufreq_boost_down_prep(unsigned int cpu)
> +static void acpi_cpufreq_set_boost_cpu(unsigned int cpu, bool enable)
>   {
> -       /*
> -        * Clear the boost-disable bit on the CPU_DOWN path so that
> -        * this cpu cannot block the remaining ones from boosting.
> -        */
> -       return boost_set_msr(1);
> +       smp_call_function_single(cpu, boost_set_msr_each,
> +                                (void *)(long)enable, 1);
>   }
>
>   /*
> @@ -951,7 +948,6 @@ static void acpi_cpufreq_cpu_exit(struct
> cpufreq_policy *policy)
>
>          pr_debug("%s\n", __func__);
>
> -       cpufreq_boost_down_prep(policy->cpu);
>          policy->fast_switch_possible = false;
>          policy->driver_data = NULL;
>          acpi_processor_unregister_performance(data->acpi_perf_cpu);
> @@ -986,6 +982,7 @@ static struct cpufreq_driver acpi_cpufreq_driver = {
>          .bios_limit     = acpi_processor_get_bios_limit,
>          .init           = acpi_cpufreq_cpu_init,
>          .exit           = acpi_cpufreq_cpu_exit,
> +       .set_boost_cpu  = acpi_cpufreq_set_boost_cpu,
>          .resume         = acpi_cpufreq_resume,
>          .name           = "acpi-cpufreq",
>          .attr           = acpi_cpufreq_attr,
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 507224c9ecd3..c4952ca3e8a5 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1669,13 +1669,18 @@ static int cpufreq_add_dev(struct device *dev,
> struct subsys_interface *sif)
>          return 0;
>   }
>
> -static void __cpufreq_offline(unsigned int cpu, struct cpufreq_policy
> *policy)
> +static void __cpufreq_offline(unsigned int cpu, struct cpufreq_policy
> *policy,
> +                             bool unbinding)
>   {
>          int ret;
>
>          if (has_target())
>                  cpufreq_stop_governor(policy);
>
> +       if (unbinding && cpufreq_driver->set_boost_cpu &&
> +           policy->boost_supported)
> +               cpufreq_driver->set_boost_cpu(cpu, true);
> +
>          cpumask_clear_cpu(cpu, policy->cpus);
>
>          if (!policy_is_inactive(policy)) {
> @@ -1730,7 +1735,7 @@ static int cpufreq_offline(unsigned int cpu)
>
>          guard(cpufreq_policy_write)(policy);
>
> -       __cpufreq_offline(cpu, policy);
> +       __cpufreq_offline(cpu, policy, false);
>
>          return 0;
>   }
> @@ -1750,7 +1755,7 @@ static void cpufreq_remove_dev(struct device *dev,
> struct subsys_interface *sif)
>
>          scoped_guard(cpufreq_policy_write, policy) {
>                  if (cpu_online(cpu))
> -                       __cpufreq_offline(cpu, policy);
> +                       __cpufreq_offline(cpu, policy, true);
>
>                  remove_cpu_dev_symlink(policy, cpu, dev);
>
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index ae9d1ce4f49c..c174b013dd76 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -413,6 +413,8 @@ struct cpufreq_driver {
>          int             (*online)(struct cpufreq_policy *policy);
>          int             (*offline)(struct cpufreq_policy *policy);
>          void            (*exit)(struct cpufreq_policy *policy);
> +       /* Optional */
> +       void            (*set_boost_cpu)(unsigned int cpu, bool enable);
>          int             (*suspend)(struct cpufreq_policy *policy);
>          int             (*resume)(struct cpufreq_policy *policy);
>
>
>
> Option 2 (lighter, but it is at odds with the direction discussed
> above): stay in acpi-cpufreq's ->exit() and restore over
> policy->related_cpus instead.
>
>
>
>
> --
> Thx and BRs,
> Zhongqiu Han

While you negotiate the cleanup, should I remove this change from the series?

Re: [PATCH v3 3/4] cpufreq: ACPI: Use IPI to update boost MSR in cpufreq_boost_down_prep()

[Zhongqiu Han]

On 7/17/2026 3:11 AM, Jim Mattson wrote:
> On Thu, Jun 25, 2026 at 8:50 AM Zhongqiu Han
> <zhongqiu.han@oss.qualcomm.com> wrote:
> 
>> Hi Rafael,
>> Thanks for the clarification.
>>
>> I agree. Just for discussion, two possible approaches come to mind:
>>
>> Option 1 (structurally clean):
>> Introduce an optional per-CPU cpufreq_driver->set_boost_cpu(cpu, enable)
>> hook and call it from __cpufreq_offline() before cpumask_clear_cpu(),
>> but only on the unbind path (not on plain CPU hotplug-offline - clearing
>> it there maybe wrongly re-enable boost on a still-online sibling on
>> shared-scope HW). Roughly (rough sketch only):
>>
>> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
>> index 21639d9ac753..5f298fbb0d8c 100644
>> --- a/drivers/cpufreq/acpi-cpufreq.c
>> +++ b/drivers/cpufreq/acpi-cpufreq.c
>> @@ -525,13 +525,10 @@ static void free_acpi_perf_data(void)
>>           free_percpu(acpi_perf_data);
>>    }
>>
>> -static int cpufreq_boost_down_prep(unsigned int cpu)
>> +static void acpi_cpufreq_set_boost_cpu(unsigned int cpu, bool enable)
>>    {
>> -       /*
>> -        * Clear the boost-disable bit on the CPU_DOWN path so that
>> -        * this cpu cannot block the remaining ones from boosting.
>> -        */
>> -       return boost_set_msr(1);
>> +       smp_call_function_single(cpu, boost_set_msr_each,
>> +                                (void *)(long)enable, 1);
>>    }
>>
>>    /*
>> @@ -951,7 +948,6 @@ static void acpi_cpufreq_cpu_exit(struct
>> cpufreq_policy *policy)
>>
>>           pr_debug("%s\n", __func__);
>>
>> -       cpufreq_boost_down_prep(policy->cpu);
>>           policy->fast_switch_possible = false;
>>           policy->driver_data = NULL;
>>           acpi_processor_unregister_performance(data->acpi_perf_cpu);
>> @@ -986,6 +982,7 @@ static struct cpufreq_driver acpi_cpufreq_driver = {
>>           .bios_limit     = acpi_processor_get_bios_limit,
>>           .init           = acpi_cpufreq_cpu_init,
>>           .exit           = acpi_cpufreq_cpu_exit,
>> +       .set_boost_cpu  = acpi_cpufreq_set_boost_cpu,
>>           .resume         = acpi_cpufreq_resume,
>>           .name           = "acpi-cpufreq",
>>           .attr           = acpi_cpufreq_attr,
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 507224c9ecd3..c4952ca3e8a5 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1669,13 +1669,18 @@ static int cpufreq_add_dev(struct device *dev,
>> struct subsys_interface *sif)
>>           return 0;
>>    }
>>
>> -static void __cpufreq_offline(unsigned int cpu, struct cpufreq_policy
>> *policy)
>> +static void __cpufreq_offline(unsigned int cpu, struct cpufreq_policy
>> *policy,
>> +                             bool unbinding)
>>    {
>>           int ret;
>>
>>           if (has_target())
>>                   cpufreq_stop_governor(policy);
>>
>> +       if (unbinding && cpufreq_driver->set_boost_cpu &&
>> +           policy->boost_supported)
>> +               cpufreq_driver->set_boost_cpu(cpu, true);
>> +
>>           cpumask_clear_cpu(cpu, policy->cpus);
>>
>>           if (!policy_is_inactive(policy)) {
>> @@ -1730,7 +1735,7 @@ static int cpufreq_offline(unsigned int cpu)
>>
>>           guard(cpufreq_policy_write)(policy);
>>
>> -       __cpufreq_offline(cpu, policy);
>> +       __cpufreq_offline(cpu, policy, false);
>>
>>           return 0;
>>    }
>> @@ -1750,7 +1755,7 @@ static void cpufreq_remove_dev(struct device *dev,
>> struct subsys_interface *sif)
>>
>>           scoped_guard(cpufreq_policy_write, policy) {
>>                   if (cpu_online(cpu))
>> -                       __cpufreq_offline(cpu, policy);
>> +                       __cpufreq_offline(cpu, policy, true);
>>
>>                   remove_cpu_dev_symlink(policy, cpu, dev);
>>
>> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
>> index ae9d1ce4f49c..c174b013dd76 100644
>> --- a/include/linux/cpufreq.h
>> +++ b/include/linux/cpufreq.h
>> @@ -413,6 +413,8 @@ struct cpufreq_driver {
>>           int             (*online)(struct cpufreq_policy *policy);
>>           int             (*offline)(struct cpufreq_policy *policy);
>>           void            (*exit)(struct cpufreq_policy *policy);
>> +       /* Optional */
>> +       void            (*set_boost_cpu)(unsigned int cpu, bool enable);
>>           int             (*suspend)(struct cpufreq_policy *policy);
>>           int             (*resume)(struct cpufreq_policy *policy);
>>
>>
>>
>> Option 2 (lighter, but it is at odds with the direction discussed
>> above): stay in acpi-cpufreq's ->exit() and restore over
>> policy->related_cpus instead.
>>
>>
>>
>>
>> --
>> Thx and BRs,
>> Zhongqiu Han
> 
> While you negotiate the cleanup, should I remove this change from the series?

Thanks a lot Jim for the checking. Maybe Rafael or Viresh can help
comment on this.


Besides, one possible improvement over Option 1 is to leave the
`cpufreq_offline()` API unchanged, and instead move the set boost-bit
callback into `cpufreq_remove_dev()`. To avoid redundant IPIs, place it
after

     if (!cpumask_empty(policy->real_cpus))
         return;

and use `related_cpus` instead of `policy->cpus` (because `policy->cpus`
is emptied after `__cpufreq_offline()` is called).

Pseudo-code:

     scoped_guard(cpufreq_policy_write, policy) {
         if (cpu_online(cpu))
             __cpufreq_offline(cpu, policy);

             remove_cpu_dev_symlink(policy, cpu, dev);

             if (!cpumask_empty(policy->real_cpus))
                 return;

+           if (cpufreq_driver->set_boost_cpu
                 && policy->boost_supported)
+               cpufreq_driver->set_boost_cpu(policy->related_cpus);
+
         /*
          * Unregister cpufreq cooling once all the CPUs of the policy
          * are removed.
          */


-- 
Thx and BRs,
Zhongqiu Han

[PATCH v3 4/4] cpufreq: ACPI: Use amd_update_hwcr() for MSR_K7_HWCR updates

[Jim Mattson]

For consistency and maintainability, update boost_set_msr() to use
the centralized amd_update_hwcr() helper when modifying MSR_K7_HWCR.

Suggested-by: Borislav Petkov <bp@alien8.de>
Assisted-by: Gemini:gemini-3.5-pro
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 drivers/cpufreq/acpi-cpufreq.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 48d678812fd6..b3e058dd6b4b 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -103,9 +103,7 @@ static int boost_set_msr(bool enable)
 		break;
 	case X86_VENDOR_HYGON:
 	case X86_VENDOR_AMD:
-		msr_addr = MSR_K7_HWCR;
-		msr_mask = MSR_K7_HWCR_CPB_DIS;
-		break;
+		return amd_update_hwcr(MSR_K7_HWCR_CPB_DIS_BIT, !enable);
 	default:
 		return -EINVAL;
 	}
-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH v3 4/4] cpufreq: ACPI: Use amd_update_hwcr() for MSR_K7_HWCR updates

[Rafael J. Wysocki]

On Fri, Jun 19, 2026 at 12:45 AM Jim Mattson <jmattson@google.com> wrote:
>
> For consistency and maintainability, update boost_set_msr() to use
> the centralized amd_update_hwcr() helper when modifying MSR_K7_HWCR.
>
> Suggested-by: Borislav Petkov <bp@alien8.de>
> Assisted-by: Gemini:gemini-3.5-pro
> Signed-off-by: Jim Mattson <jmattson@google.com>

Acked-by: Rafael J. Wysocki (Intel) <rafael@kernel.org>

> ---
>  drivers/cpufreq/acpi-cpufreq.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 48d678812fd6..b3e058dd6b4b 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -103,9 +103,7 @@ static int boost_set_msr(bool enable)
>                 break;
>         case X86_VENDOR_HYGON:
>         case X86_VENDOR_AMD:
> -               msr_addr = MSR_K7_HWCR;
> -               msr_mask = MSR_K7_HWCR_CPB_DIS;
> -               break;
> +               return amd_update_hwcr(MSR_K7_HWCR_CPB_DIS_BIT, !enable);
>         default:
>                 return -EINVAL;
>         }
> --
> 2.55.0.rc0.799.gd6f94ed593-goog
>