Re: [PATCH 8/8] x86/cpufreq: use host CPU policy in HWP driver

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.11.2025 21:04, Andrew Cooper wrote:
> On 18/11/2025 3:09 pm, Jan Beulich wrote:
>> --- a/xen/arch/x86/acpi/cpufreq/hwp.c
>> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
>> @@ -183,29 +178,25 @@ static bool __init hwp_available(void)
>>          return false;
>>      }
>>  
>> -    eax = cpuid_eax(CPUID_PM_LEAF);
>> -
>>      hwp_verbose("%d notify: %d act-window: %d energy-perf: %d pkg-level: %d 
>> peci: %d\n",
>> -                !!(eax & CPUID6_EAX_HWP),
>> -                !!(eax & CPUID6_EAX_HWP_NOTIFICATION),
>> -                !!(eax & CPUID6_EAX_HWP_ACTIVITY_WINDOW),
>> -                !!(eax & CPUID6_EAX_HWP_ENERGY_PERFORMANCE_PREFERENCE),
>> -                !!(eax & CPUID6_EAX_HWP_PACKAGE_LEVEL_REQUEST),
>> -                !!(eax & CPUID6_EAX_HWP_PECI));
>> +                host_cpu_policy.basic.pm.hwp,
>> +                host_cpu_policy.basic.pm.hwp_notification,
>> +                host_cpu_policy.basic.pm.hwp_activity_window,
>> +                host_cpu_policy.basic.pm.hwp_epp,
>> +                host_cpu_policy.basic.pm.hwp_plr,
>> +                host_cpu_policy.basic.pm.hwp_peci);
>>  
>> -    if ( !(eax & CPUID6_EAX_HWP) )
>> +    if ( !host_cpu_policy.basic.pm.hwp )
> 
> I think this justifies a cpu_has_hwp like we have for turbo/arat/etc. 
> Similarly for the other features.

Hmm, okay, I can do that. The difference between using boot_cpu_data vs
host_cpu_policy in cpu_has_* is completely that way, which in fact made
me uncertain even for the introduction of the APERFMPERF, ARAT, and
TURBO shorthands.

>>          return false;
>>  
>> -    if ( !(eax & CPUID6_EAX_HWP_ENERGY_PERFORMANCE_PREFERENCE) )
>> +    if ( !host_cpu_policy.basic.pm.hwp_epp )
>>      {
>>          hwp_verbose("disabled: No energy/performance preference available");
>>  
>>          return false;
>>      }
>>  
>> -    feature_hwp_notification    = eax & CPUID6_EAX_HWP_NOTIFICATION;
>> -    feature_hwp_activity_window = eax & CPUID6_EAX_HWP_ACTIVITY_WINDOW;
>> -    feature_hdc                 = eax & CPUID6_EAX_HDC;
>> +    feature_hdc                 = host_cpu_policy.basic.pm.hdc;
> 
> Looking at how feature_hdc is used, I think it should be the bit within
> the host policy, rather than a separate boolean.
> 
> The host policy "is" what Xen is using, so if the HWP code decides it
> doesn't like HDC, then that does want reflecting.

I'm not sure about this. Yes as long as the host policy bits don't propagate
to guest policies. But if they did (and as said earlier, sooner or later we
may want / need to do that for some of the leaf 6 ones), why would the
driver's choice affect what guests get to see? (That's applicable to a fair
degree for the host policy as well: What the driver chooses doesn't need to
match Xen's global view of the world.

>> @@ -365,7 +357,7 @@ static void cf_check hwp_init_msrs(void
>>      }
>>  
>>      /* Ensure we don't generate interrupts */
>> -    if ( feature_hwp_notification )
>> +    if ( host_cpu_policy.basic.pm.hwp_notification )
>>          wrmsr_safe(MSR_HWP_INTERRUPT, 0);
> 
> Again, unrelated to the patch, but why is this a wrmsr_safe() ?
> 
> If the feature is enumerated, the MSR had better work.

Yet as we know when running virtualized ourselves, we can't always rely on
CPUID bits and MSR accessibility to be fully in sync. Yes, using in fact any
cpufreq driver is pretty meaningless when running virtualized, but we don't
prevent that scenario afaict.

> Things like this start to matter more when we consider the code
> generation for wrmsr_safe() using MSR_IMM.

Your patch from August only altered wrmsrns() iirc, hence even if we switched
to wrmsr() here there would be no difference (yet). If wrmsrns() was provably
usable in this case, wouldn't wrmsrns_safe() (yet to be invented) ultimately
be not significantly different in terms of code gen, wrt MSR_IMM? The
difference would continue to be whether there's recovery code; the recovery
code, however, isn't affected by MSR_IMM. Finally, for a purpose like the one
here (infrequently executed code), would the code size increase from trying
to use the immediate form really be justified by the to be expected perf
gain?

>> --- a/xen/arch/x86/include/asm/cpufeature.h
>> +++ b/xen/arch/x86/include/asm/cpufeature.h
>> @@ -115,14 +115,6 @@ static inline bool boot_cpu_has(unsigned
>>  }
>>  
>>  #define CPUID_PM_LEAF                                6
> 
> Doesn't this patch drop the final user?

No, there's one use left in the HWP driver.

Jan