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

[Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>]

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