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@suse.com>
---
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
/* CPUID level 0x00000006.ecx */
#define cpu_has_aperfmperf host_cpu_policy.basic.aperfmperfOn 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@suse.com>
> ---
> 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
On 10.12.2025 15:11, Andrew Cooper wrote: >> --- 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. To be honest, I see only one of two options: Either we stick to what we had settled on when the HWP driver went in (switching to acronyms where helpful), or we strictly follow the SDM. In the latter case I think I will need to split this patch, for every of the renames to be separate (to be easier to verify). And the naming decisions then want applying in patch 1 as well. Unless I hear otherwise (soon), I'll go the "strictly per SDM" route. Still, the union-or-not question in patch 1 also needs sorting, so I'm dependent anyway upon you replying in a somewhat timely manner. Jan
On 2025-11-24 07:25, 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@suse.com>
> ---
> v2: Introduce cpu_has_*.
>
> --- a/xen/arch/x86/acpi/cpufreq/hwp.c
> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
> @@ -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 )
This ...
> {
> hwp_verbose("HWP activity window not supported\n");
>
> @@ -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);
...and this can still be on one line.
Preferably with that fixed:
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Thanks,
Jason
> cppc_para->lowest = data->hw.lowest;
> cppc_para->lowest_nonlinear = data->hw.most_efficient;
> cppc_para->nominal = data->hw.guaranteed;
On 24.11.2025 15:35, Jason Andryuk wrote:
> On 2025-11-24 07:25, Jan Beulich wrote:
>> --- a/xen/arch/x86/acpi/cpufreq/hwp.c
>> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
>
>> @@ -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 )
>
> This ...
>> {
>> hwp_verbose("HWP activity window not supported\n");
>>
>
>> @@ -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);
>
> ...and this can still be on one line.
Oh, yes, the lines now became shorter again.
> Preferably with that fixed:
>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Thanks.
Jan
© 2016 - 2025 Red Hat, Inc.