Re: [PATCH 1/9] x86/mwait-idle: arrange for BSP MSR adjustments during S3 resume

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.04.2026 16:34, Roger Pau Monné wrote:
> On Thu, Mar 12, 2026 at 05:54:30PM +0100, Jan Beulich wrote:
>> mwait_idle_cpu_init() is only called for APs, yet MSR writes will
>> typically need re-doing post-S3 even for the BSP. When multiple cores /
>> threads are present (and to come back online) in a package, for package
>> scope MSRs this may be covered by APs doing the writes, but we can't rely
>> on that.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- a/xen/arch/x86/acpi/power.c
>> +++ b/xen/arch/x86/acpi/power.c
>> @@ -28,6 +28,7 @@
>>  #include <asm/io_apic.h>
>>  #include <asm/irq.h>
>>  #include <asm/microcode.h>
>> +#include <asm/mwait.h>
>>  #include <asm/prot-key.h>
>>  #include <asm/spec_ctrl.h>
>>  #include <asm/tboot.h>
>> @@ -299,6 +300,7 @@ static int enter_state(u32 state)
>>      acpi_sleep_post(state);
>>      if ( hvm_cpu_up() )
>>          BUG();
>> +    mwait_idle_resume();
>>      cpufreq_add_cpu(0);
>>  
>>   enable_cpu:
>> --- a/xen/arch/x86/cpu/mwait-idle.c
>> +++ b/xen/arch/x86/cpu/mwait-idle.c
>> @@ -1680,6 +1680,28 @@ static int __init mwait_idle_probe(void)
>>      return 0;
>>  }
>>  
>> +static void mwait_idle_cpu_tweak(unsigned int cpu)
>> +{
>> +    if (icpu->auto_demotion_disable_flags)
>> +            on_selected_cpus(cpumask_of(cpu), auto_demotion_disable, NULL, 
>> 1);
>> +
>> +    if (icpu->byt_auto_demotion_disable_flag)
>> +            on_selected_cpus(cpumask_of(cpu), byt_auto_demotion_disable, 
>> NULL, 1);
>> +
>> +    switch (icpu->c1e_promotion) {
>> +    case C1E_PROMOTION_DISABLE:
>> +            on_selected_cpus(cpumask_of(cpu), c1e_promotion_disable, NULL, 
>> 1);
>> +            break;
>> +
>> +    case C1E_PROMOTION_ENABLE:
>> +            on_selected_cpus(cpumask_of(cpu), c1e_promotion_enable, NULL, 
>> 1);
>> +            break;
>> +
>> +    case C1E_PROMOTION_PRESERVE:
>> +            break;
>> +    }
>> +}
>> +
>>  static int cf_check mwait_idle_cpu_init(
>>      struct notifier_block *nfb, unsigned long action, void *hcpu)
>>  {
>> @@ -1762,24 +1784,7 @@ static int cf_check mwait_idle_cpu_init(
>>              dev->count++;
>>      }
>>  
>> -    if (icpu->auto_demotion_disable_flags)
>> -            on_selected_cpus(cpumask_of(cpu), auto_demotion_disable, NULL, 
>> 1);
>> -
>> -    if (icpu->byt_auto_demotion_disable_flag)
>> -            on_selected_cpus(cpumask_of(cpu), byt_auto_demotion_disable, 
>> NULL, 1);
>> -
>> -    switch (icpu->c1e_promotion) {
>> -    case C1E_PROMOTION_DISABLE:
>> -            on_selected_cpus(cpumask_of(cpu), c1e_promotion_disable, NULL, 
>> 1);
>> -            break;
>> -
>> -    case C1E_PROMOTION_ENABLE:
>> -            on_selected_cpus(cpumask_of(cpu), c1e_promotion_enable, NULL, 
>> 1);
>> -            break;
> 
> I'm possibly missing some context here, but why do we use
> on_selected_cpus and the CPU_ONLINE hook?  Won't it be easier to use
> CPU_STARTING and avoid the use of on_selected_cpus(), as CPU_STARTING
> runs in the context of the CPU being onlined.

CPU_STARTING happens pretty early, e.g. before IRQs are first enabled. I
consider use of that notifier to generally be restricted to pretty
special purposes. But yes, technically switching over may be possible.
However, in any event what you're suggesting is imo an entirely separate
change.

Jan