Share space with the ACPI and powernow drivers, avoiding a separate
allocation for each CPU. Except for get_hwp_para() all use sites already
have the policy available, and this one function can simply be brought
closer to its sibling set_hwp_para().
This then also eliminates the concern over hwp_cpufreq_cpu_init() being
called for all CPUs, or a CPU going offline that's recorded in
policy->cpu (which would result in accesses of per-CPU data of offline
CPUs).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
hwp_cpufreq_target() still requires policy->cpu to be online, though.
--- a/xen/arch/x86/acpi/cpufreq/hwp.c
+++ b/xen/arch/x86/acpi/cpufreq/hwp.c
@@ -22,50 +22,6 @@ static bool __read_mostly feature_hdc;
static bool __ro_after_init opt_cpufreq_hdc = true;
-union hwp_request
-{
- struct
- {
- unsigned int min_perf:8;
- unsigned int max_perf:8;
- unsigned int desired:8;
- unsigned int energy_perf:8;
- unsigned int activity_window:10;
- bool package_control:1;
- unsigned int :16;
- bool activity_window_valid:1;
- bool energy_perf_valid:1;
- bool desired_valid:1;
- bool max_perf_valid:1;
- bool min_perf_valid:1;
- };
- uint64_t raw;
-};
-
-struct hwp_drv_data
-{
- union
- {
- uint64_t hwp_caps;
- struct
- {
- unsigned int highest:8;
- unsigned int guaranteed:8;
- unsigned int most_efficient:8;
- unsigned int lowest:8;
- unsigned int :32;
- } hw;
- };
- union hwp_request curr_req;
- int ret;
- uint16_t activity_window;
- uint8_t minimum;
- uint8_t maximum;
- uint8_t desired;
- uint8_t energy_perf;
-};
-static DEFINE_PER_CPU_READ_MOSTLY(struct hwp_drv_data *, hwp_drv_data);
-
#define hwp_err(cpu, fmt, args...) \
printk(XENLOG_ERR "HWP: CPU%u error: " fmt, cpu, ## args)
#define hwp_info(fmt, args...) printk(XENLOG_INFO "HWP: " fmt, ## args)
@@ -212,7 +168,7 @@ static bool __init hwp_available(void)
static int cf_check hwp_cpufreq_verify(struct cpufreq_policy *policy)
{
- struct hwp_drv_data *data = per_cpu(hwp_drv_data, policy->cpu);
+ struct hwp_drv_data *data = &policy->drv_data.hwp;
if ( !cpu_has_hwp_activity_window && data->activity_window )
{
@@ -226,8 +182,8 @@ static int cf_check hwp_cpufreq_verify(s
static void cf_check hwp_write_request(void *info)
{
- const struct cpufreq_policy *policy = info;
- struct hwp_drv_data *data = this_cpu(hwp_drv_data);
+ struct cpufreq_policy *policy = info;
+ struct hwp_drv_data *data = &policy->drv_data.hwp;
union hwp_request hwp_req = data->curr_req;
data->ret = 0;
@@ -247,7 +203,7 @@ static int cf_check hwp_cpufreq_target(s
unsigned int relation)
{
unsigned int cpu = policy->cpu;
- struct hwp_drv_data *data = per_cpu(hwp_drv_data, cpu);
+ struct hwp_drv_data *data = &policy->drv_data.hwp;
/* Zero everything to ensure reserved bits are zero... */
union hwp_request hwp_req = { .raw = 0 };
@@ -338,7 +294,7 @@ static void hwp_get_cpu_speeds(struct cp
static void cf_check hwp_init_msrs(void *info)
{
struct cpufreq_policy *policy = info;
- struct hwp_drv_data *data = this_cpu(hwp_drv_data);
+ struct hwp_drv_data *data = &policy->drv_data.hwp;
uint64_t val;
/*
@@ -406,23 +362,15 @@ static int cf_check hwp_cpufreq_cpu_init
static bool __read_mostly first_run = true;
static union hwp_request initial_req;
unsigned int cpu = policy->cpu;
- struct hwp_drv_data *data;
-
- data = xzalloc(struct hwp_drv_data);
- if ( !data )
- return -ENOMEM;
+ struct hwp_drv_data *data = &policy->drv_data.hwp;
policy->governor = &cpufreq_gov_hwp;
- per_cpu(hwp_drv_data, cpu) = data;
-
on_selected_cpus(cpumask_of(cpu), hwp_init_msrs, policy, 1);
if ( data->curr_req.raw == -1 )
{
hwp_err(cpu, "Could not initialize HWP properly\n");
- per_cpu(hwp_drv_data, cpu) = NULL;
- xfree(data);
return -ENODEV;
}
@@ -450,11 +398,6 @@ static int cf_check hwp_cpufreq_cpu_init
static int cf_check hwp_cpufreq_cpu_exit(struct cpufreq_policy *policy)
{
- struct hwp_drv_data *data = per_cpu(hwp_drv_data, policy->cpu);
-
- per_cpu(hwp_drv_data, policy->cpu) = NULL;
- xfree(data);
-
return 0;
}
@@ -467,8 +410,8 @@ static int cf_check hwp_cpufreq_cpu_exit
*/
static void cf_check hwp_set_misc_turbo(void *info)
{
- const struct cpufreq_policy *policy = info;
- struct hwp_drv_data *data = per_cpu(hwp_drv_data, policy->cpu);
+ struct cpufreq_policy *policy = info;
+ struct hwp_drv_data *data = &policy->drv_data.hwp;
uint64_t msr;
data->ret = 0;
@@ -499,7 +442,7 @@ static int cf_check hwp_cpufreq_update(u
{
on_selected_cpus(cpumask_of(cpu), hwp_set_misc_turbo, policy, 1);
- return per_cpu(hwp_drv_data, cpu)->ret;
+ return policy->drv_data.hwp.ret;
}
#endif /* CONFIG_PM_OP */
@@ -516,12 +459,12 @@ hwp_cpufreq_driver = {
};
#ifdef CONFIG_PM_OP
-int get_hwp_para(unsigned int cpu,
+int get_hwp_para(const struct cpufreq_policy *policy,
struct xen_get_cppc_para *cppc_para)
{
- const struct hwp_drv_data *data = per_cpu(hwp_drv_data, cpu);
+ const struct hwp_drv_data *data = &policy->drv_data.hwp;
- if ( data == NULL )
+ if ( !data->maximum )
return -ENODATA;
cppc_para->features =
@@ -542,11 +485,10 @@ int get_hwp_para(unsigned int cpu,
int set_hwp_para(struct cpufreq_policy *policy,
struct xen_set_cppc_para *set_cppc)
{
- unsigned int cpu = policy->cpu;
- struct hwp_drv_data *data = per_cpu(hwp_drv_data, cpu);
+ struct hwp_drv_data *data = &policy->drv_data.hwp;
bool cleared_act_window = false;
- if ( data == NULL )
+ if ( !data->maximum )
return -ENOENT;
/* Validate all parameters - Disallow reserved bits. */
--- a/xen/drivers/acpi/pm-op.c
+++ b/xen/drivers/acpi/pm-op.c
@@ -80,10 +80,12 @@ static int read_scaling_available_govern
static int get_cpufreq_cppc(unsigned int cpu,
struct xen_get_cppc_para *cppc_para)
{
+ const struct cpufreq_policy *policy =
+ per_cpu(cpufreq_cpu_policy, cpu);
int ret = -ENODEV;
- if ( hwp_active() )
- ret = get_hwp_para(cpu, cppc_para);
+ if ( policy && hwp_active() )
+ ret = get_hwp_para(policy, cppc_para);
return ret;
}
--- a/xen/include/acpi/cpufreq/cpufreq.h
+++ b/xen/include/acpi/cpufreq/cpufreq.h
@@ -38,6 +38,42 @@ struct acpi_cpufreq_data {
unsigned int arch_cpu_flags;
};
+struct hwp_drv_data {
+ union {
+ uint64_t hwp_caps;
+ struct {
+ unsigned int highest:8;
+ unsigned int guaranteed:8;
+ unsigned int most_efficient:8;
+ unsigned int lowest:8;
+ unsigned int :32;
+ } hw;
+ };
+ union hwp_request {
+ struct {
+ unsigned int min_perf:8;
+ unsigned int max_perf:8;
+ unsigned int desired:8;
+ unsigned int energy_perf:8;
+ unsigned int activity_window:10;
+ bool package_control:1;
+ unsigned int :16;
+ bool activity_window_valid:1;
+ bool energy_perf_valid:1;
+ bool desired_valid:1;
+ bool max_perf_valid:1;
+ bool min_perf_valid:1;
+ };
+ uint64_t raw;
+ } curr_req;
+ int ret;
+ uint16_t activity_window;
+ uint8_t minimum;
+ uint8_t maximum;
+ uint8_t desired;
+ uint8_t energy_perf;
+};
+
struct cpufreq_cpuinfo {
unsigned int max_freq;
unsigned int second_max_freq; /* P1 if Turbo Mode is on */
@@ -83,6 +119,7 @@ struct cpufreq_policy {
union {
struct acpi_cpufreq_data acpi;
+ struct hwp_drv_data hwp;
} drv_data;
};
DECLARE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_policy);
@@ -286,7 +323,7 @@ bool hwp_active(void);
static inline bool hwp_active(void) { return false; }
#endif
-int get_hwp_para(unsigned int cpu,
+int get_hwp_para(const struct cpufreq_policy *policy,
struct xen_get_cppc_para *cppc_para);
int set_hwp_para(struct cpufreq_policy *policy,
struct xen_set_cppc_para *set_cppc);On 2026-01-22 04:42, Jan Beulich wrote: > Share space with the ACPI and powernow drivers, avoiding a separate > allocation for each CPU. Except for get_hwp_para() all use sites already > have the policy available, and this one function can simply be brought > closer to its sibling set_hwp_para(). Minor, but maybe 's/function/function's signature/'. The original phrasing made me think it was code movement. > > This then also eliminates the concern over hwp_cpufreq_cpu_init() being > called for all CPUs We expect hwp_cpufreq_cpu_init() to be called for each CPU, so I am confused by this statement. The data... >, or a CPU going offline that's recorded in> policy->cpu (which would result in accesses of per-CPU data of offline > CPUs). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > hwp_cpufreq_target() still requires policy->cpu to be online, though. > > --- a/xen/arch/x86/acpi/cpufreq/hwp.c > +++ b/xen/arch/x86/acpi/cpufreq/hwp.c > -static DEFINE_PER_CPU_READ_MOSTLY(struct hwp_drv_data *, hwp_drv_data); ... here is tracked and filled per-CPU. Do we need cpufreq_add_cpu() to force hw_all = 1 for HWP (and maybe AMD-CPPC) to ensure that policy is allocated per-CPU? Are we implicitly relying on shared_type == CPUFREQ_SHARED_TYPE_HW to do that for us? The code here is okay, fwiw. Thanks, Jason
On 23.01.2026 23:35, Jason Andryuk wrote: > On 2026-01-22 04:42, Jan Beulich wrote: >> Share space with the ACPI and powernow drivers, avoiding a separate >> allocation for each CPU. Except for get_hwp_para() all use sites already >> have the policy available, and this one function can simply be brought >> closer to its sibling set_hwp_para(). > > Minor, but maybe 's/function/function's signature/'. The original > phrasing made me think it was code movement. I don't see an issue there, but I've adjusted as you asked for. >> This then also eliminates the concern over hwp_cpufreq_cpu_init() being >> called for all CPUs > > We expect hwp_cpufreq_cpu_init() to be called for each CPU, so I am > confused by this statement. The data... Well, "expect" is the problem. Recall my pointing out of the problem when having noticed the same pattern in the amd-cppc driver patches. My recollection from the discussion is that there's no formal statement of ... > >, or a CPU going offline that's recorded in> policy->cpu (which would > result in accesses of per-CPU data of offline >> CPUs). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> hwp_cpufreq_target() still requires policy->cpu to be online, though. >> >> --- a/xen/arch/x86/acpi/cpufreq/hwp.c >> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c > >> -static DEFINE_PER_CPU_READ_MOSTLY(struct hwp_drv_data *, hwp_drv_data); > > ... here is tracked and filled per-CPU. > > Do we need cpufreq_add_cpu() to force hw_all = 1 for HWP (and maybe > AMD-CPPC) to ensure that policy is allocated per-CPU? ... this being a correct thing to do, hence our code imo would better be resilient to it being different somewhere. > Are we implicitly relying on shared_type == CPUFREQ_SHARED_TYPE_HW to do > that for us? Right now we do, I believe, without - as said above - this being actually mandated by the spec. Jan
On 2026-01-26 04:08, Jan Beulich wrote: > On 23.01.2026 23:35, Jason Andryuk wrote: >> On 2026-01-22 04:42, Jan Beulich wrote: >>> Share space with the ACPI and powernow drivers, avoiding a separate >>> allocation for each CPU. Except for get_hwp_para() all use sites already >>> have the policy available, and this one function can simply be brought >>> closer to its sibling set_hwp_para(). >> >> Minor, but maybe 's/function/function's signature/'. The original >> phrasing made me think it was code movement. > > I don't see an issue there, but I've adjusted as you asked for. Thank you. >>> This then also eliminates the concern over hwp_cpufreq_cpu_init() being >>> called for all CPUs >> >> We expect hwp_cpufreq_cpu_init() to be called for each CPU, so I am >> confused by this statement. The data... > > Well, "expect" is the problem. Recall my pointing out of the problem when > having noticed the same pattern in the amd-cppc driver patches. My > recollection from the discussion is that there's no formal statement of > ... > >> >, or a CPU going offline that's recorded in> policy->cpu (which would >> result in accesses of per-CPU data of offline >>> CPUs). >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> hwp_cpufreq_target() still requires policy->cpu to be online, though. >>> >>> --- a/xen/arch/x86/acpi/cpufreq/hwp.c >>> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c >> >>> -static DEFINE_PER_CPU_READ_MOSTLY(struct hwp_drv_data *, hwp_drv_data); >> >> ... here is tracked and filled per-CPU. >> >> Do we need cpufreq_add_cpu() to force hw_all = 1 for HWP (and maybe >> AMD-CPPC) to ensure that policy is allocated per-CPU? > > ... this being a correct thing to do, hence our code imo would better be > resilient to it being different somewhere. > >> Are we implicitly relying on shared_type == CPUFREQ_SHARED_TYPE_HW to do >> that for us? > > Right now we do, I believe, without - as said above - this being actually > mandated by the spec. HWP doesn't need ACPI data. I wrote the driver according to the SDM, which is just MSRs. It's Xen that needs ACPI data to initialize and use cpufreq. Regardless of that, it looks like the checks for cpu_online() and performance_pminfo[] would constrain CPU accesses, so: Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> Thanks, Jason
On 26.01.2026 21:17, Jason Andryuk wrote: > On 2026-01-26 04:08, Jan Beulich wrote: >> On 23.01.2026 23:35, Jason Andryuk wrote: >>> On 2026-01-22 04:42, Jan Beulich wrote: >>>> --- a/xen/arch/x86/acpi/cpufreq/hwp.c >>>> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c >>> >>>> -static DEFINE_PER_CPU_READ_MOSTLY(struct hwp_drv_data *, hwp_drv_data); >>> >>> ... here is tracked and filled per-CPU. >>> >>> Do we need cpufreq_add_cpu() to force hw_all = 1 for HWP (and maybe >>> AMD-CPPC) to ensure that policy is allocated per-CPU? >> >> ... this being a correct thing to do, hence our code imo would better be >> resilient to it being different somewhere. >> >>> Are we implicitly relying on shared_type == CPUFREQ_SHARED_TYPE_HW to do >>> that for us? >> >> Right now we do, I believe, without - as said above - this being actually >> mandated by the spec. > > HWP doesn't need ACPI data. I wrote the driver according to the SDM, > which is just MSRs. It's Xen that needs ACPI data to initialize and use > cpufreq. Maybe we should see about lifting that restriction then? Becoming independent of Dom0's xen-acpi-processor driver would be quite a meaningful gain, I suppose. See e.g. the thread rooted at https://lists.xen.org/archives/html/xen-devel/2025-12/msg01114.html. > Regardless of that, it looks like the checks for cpu_online() and > performance_pminfo[] would constrain CPU accesses, so: > > Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> Thanks. Jan
© 2016 - 2026 Red Hat, Inc.