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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

>          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.

Unrelated to this patch, but as I've been looking at the code.  What
happens when hwp_init_msrs() fails on an AP?  I can't see anything which
unwinds the initialised HDC state on the prior CPUs...

> @@ -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.

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

> --- 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?

~Andrew