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>
--- 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,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 )
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;
hwp_verbose("Hardware Duty Cycling (HDC) %ssupported%s\n",
feature_hdc ? "" : "not ",
@@ -213,7 +204,7 @@ static bool __init hwp_available(void)
: "");
hwp_verbose("HW_FEEDBACK %ssupported\n",
- (eax & CPUID6_EAX_HW_FEEDBACK) ? "" : "not ");
+ host_cpu_policy.basic.pm.hw_feedback ? "" : "not ");
hwp_in_use = true;
@@ -226,7 +217,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 ( !host_cpu_policy.basic.pm.hwp_activity_window &&
+ data->activity_window )
{
hwp_verbose("HWP activity window not supported\n");
@@ -268,7 +260,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 ( host_cpu_policy.basic.pm.hwp_activity_window )
hwp_req.activity_window = data->activity_window;
if ( hwp_req.raw == data->curr_req.raw )
@@ -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);
if ( !(val & PM_ENABLE_HWP_ENABLE) )
@@ -537,7 +529,8 @@ int get_hwp_para(unsigned int cpu,
return -ENODATA;
cppc_para->features =
- (feature_hwp_activity_window ? XEN_SYSCTL_CPPC_FEAT_ACT_WINDOW : 0);
+ (host_cpu_policy.basic.pm.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 +578,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 )
+ !host_cpu_policy.basic.pm.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 1On 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
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
On 2025-11-18 15: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;
>>
>> - 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...
Assuming symmetry, it'd fail on the BSP and never get to an AP. Yes, I
didn't consider unwinding.
>> @@ -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() ?
It seemed safer ;)
> If the feature is enumerated, the MSR had better work.
Yes, but I wasn't sure how much to trust it. I only tested with 2
processor models, so I wanted to be safer. IIRC, I saw a #GP or two
during development. Those might have been from writing a reserved bit
and fixed during development.
Given my small testing size, I wanted to keep it safe and not take down Xen.
Regards,
Jason
On 2025-11-18 10:09, 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> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> Thanks, Jason
© 2016 - 2026 Red Hat, Inc.