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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/11/2025 12:25 pm, Jan Beulich wrote:
> There's no need to invoke CPUID yet another time. This way two of the
> static booleans can also go away.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v2: Introduce cpu_has_*.
>
> --- a/xen/arch/x86/acpi/cpufreq/hwp.c
> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
> @@ -18,9 +18,6 @@
>  
>  static bool __ro_after_init hwp_in_use;
>  
> -static bool __ro_after_init feature_hwp_notification;
> -static bool __ro_after_init feature_hwp_activity_window;
> -
>  static bool __read_mostly feature_hdc;
>  
>  static bool __ro_after_init opt_cpufreq_hdc = true;
> @@ -165,8 +162,6 @@ bool hwp_active(void)
>  
>  static bool __init hwp_available(void)
>  {
> -    unsigned int eax;
> -
>      if ( boot_cpu_data.cpuid_level < CPUID_PM_LEAF )
>      {
>          hwp_verbose("cpuid_level (%#x) lacks HWP support\n",
> @@ -183,29 +178,22 @@ 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));
> +                cpu_has_hwp, cpu_has_hwp_notification,
> +                cpu_has_hwp_activity_window, cpu_has_hwp_epp,
> +                cpu_has_hwp_plr, cpu_has_hwp_peci);
>  
> -    if ( !(eax & CPUID6_EAX_HWP) )
> +    if ( !cpu_has_hwp )
>          return false;
>  
> -    if ( !(eax & CPUID6_EAX_HWP_ENERGY_PERFORMANCE_PREFERENCE) )
> +    if ( !cpu_has_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 = cpu_has_hdc;
>  
>      hwp_verbose("Hardware Duty Cycling (HDC) %ssupported%s\n",
>                  feature_hdc ? "" : "not ",
> @@ -213,7 +201,7 @@ static bool __init hwp_available(void)
>                              : "");
>  
>      hwp_verbose("HW_FEEDBACK %ssupported\n",
> -                (eax & CPUID6_EAX_HW_FEEDBACK) ? "" : "not ");
> +                cpu_has_hw_feedback ? "" : "not ");
>  
>      hwp_in_use = true;
>  
> @@ -226,7 +214,8 @@ static int cf_check hwp_cpufreq_verify(s
>  {
>      struct hwp_drv_data *data = per_cpu(hwp_drv_data, policy->cpu);
>  
> -    if ( !feature_hwp_activity_window && data->activity_window )
> +    if ( !cpu_has_hwp_activity_window &&
> +         data->activity_window )
>      {
>          hwp_verbose("HWP activity window not supported\n");
>  
> @@ -268,7 +257,7 @@ static int cf_check hwp_cpufreq_target(s
>      hwp_req.max_perf = data->maximum;
>      hwp_req.desired = data->desired;
>      hwp_req.energy_perf = data->energy_perf;
> -    if ( feature_hwp_activity_window )
> +    if ( cpu_has_hwp_activity_window )
>          hwp_req.activity_window = data->activity_window;
>  
>      if ( hwp_req.raw == data->curr_req.raw )
> @@ -365,7 +354,7 @@ static void cf_check hwp_init_msrs(void
>      }
>  
>      /* Ensure we don't generate interrupts */
> -    if ( feature_hwp_notification )
> +    if ( cpu_has_hwp_notification )
>          wrmsr_safe(MSR_HWP_INTERRUPT, 0);
>  
>      if ( !(val & PM_ENABLE_HWP_ENABLE) )
> @@ -537,7 +526,8 @@ int get_hwp_para(unsigned int cpu,
>          return -ENODATA;
>  
>      cppc_para->features         =
> -        (feature_hwp_activity_window ? XEN_SYSCTL_CPPC_FEAT_ACT_WINDOW : 0);
> +        (cpu_has_hwp_activity_window
> +         ? XEN_SYSCTL_CPPC_FEAT_ACT_WINDOW : 0);
>      cppc_para->lowest           = data->hw.lowest;
>      cppc_para->lowest_nonlinear = data->hw.most_efficient;
>      cppc_para->nominal          = data->hw.guaranteed;
> @@ -585,7 +575,7 @@ int set_hwp_para(struct cpufreq_policy *
>  
>      /* Clear out activity window if lacking HW supported. */
>      if ( (set_cppc->set_params & XEN_SYSCTL_CPPC_SET_ACT_WINDOW) &&
> -         !feature_hwp_activity_window )
> +         !cpu_has_hwp_activity_window )
>      {
>          set_cppc->set_params &= ~XEN_SYSCTL_CPPC_SET_ACT_WINDOW;
>          cleared_act_window = true;
> --- 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
> -#define CPUID6_EAX_HWP                               BIT(7, U)
> -#define CPUID6_EAX_HWP_NOTIFICATION                  BIT(8, U)
> -#define CPUID6_EAX_HWP_ACTIVITY_WINDOW               BIT(9, U)
> -#define CPUID6_EAX_HWP_ENERGY_PERFORMANCE_PREFERENCE BIT(10, U)
> -#define CPUID6_EAX_HWP_PACKAGE_LEVEL_REQUEST         BIT(11, U)
> -#define CPUID6_EAX_HDC                               BIT(13, U)
> -#define CPUID6_EAX_HWP_PECI                          BIT(16, U)
> -#define CPUID6_EAX_HW_FEEDBACK                       BIT(19, U)
>  
>  /* CPUID level 0x00000001.edx */
>  #define cpu_has_fpu             1
> @@ -179,6 +171,14 @@ static inline bool boot_cpu_has(unsigned
>  /* CPUID level 0x00000006.eax */
>  #define cpu_has_turbo           host_cpu_policy.basic.turbo
>  #define cpu_has_arat            host_cpu_policy.basic.arat
> +#define cpu_has_hwp             host_cpu_policy.basic.hwp
> +#define cpu_has_hwp_notification host_cpu_policy.basic.hwp_notification
> +#define cpu_has_hwp_activity_window host_cpu_policy.basic.hwp_activity_window
> +#define cpu_has_hwp_epp        host_cpu_policy.basic.hwp_epp
> +#define cpu_has_hwp_plr        host_cpu_policy.basic.hwp_plr
> +#define cpu_has_hdc            host_cpu_policy.basic.hdc
> +#define cpu_has_hwp_peci       host_cpu_policy.basic.hwp_peci
> +#define cpu_has_hw_feedback    host_cpu_policy.basic.hw_feedback
>  

The indentation of these final 5 is one-too-few spaces.

I can't help but feel that notification could be shortened to notify. 
Except upon looking in the SDM, it's named HWP_INTERRUPT because it
enumerates MSR_HWP_INTERRUPT.

Similarly, HWP_PLR is really HWP_REQUEST_PKG because it enumerates
MSR_HWP_REQUEST_PKG.

ACTIVITY_WINDOW and EPP are wonky because they're out of order WRT
PLR/REQUEST_PKG.  It clearly means they all came in together, but have
SKU controls.

But I digress.  ACTIVITY_WINDOW can probably be shortened to just
WINDOW, and that fixes the two egregiously long ones.

~Andrew