On 2025-09-04 07:50, Jan Beulich wrote:
On 04.09.2025 08:35, Penny Zheng wrote:
For cpus sharing one cpufreq domain, cpufreq_driver.init() is
only invoked on the firstcpu, so current per-CPU hwp driver data
struct hwp_drv_data{} actually fails to be allocated for cpus other than the
first one.
>> There is no need to make it per-CPU.>> We embed struct
hwp_drv_data{} into struct cpufreq_policy{}, then cpus could
share the hwp driver data allocated for the firstcpu, like the way they share
struct cpufreq_policy{}. We also make it a union, with "hwp", and later
"amd-cppc" as a sub-struct.
And ACPI, as per my patch (which then will need re-basing).
Suggested-by: Jan Beulich <jbeul...@suse.com>
Not quite, this really is Reported-by: as it's a bug you fix, and in turn it
also wants to gain a Fixes: tag. This also will need backporting.
It would also have been nice if you had Cc-ed Jason right away, seeing that
this code was all written by him.
@@ -259,7 +258,7 @@ static int cf_check hwp_cpufreq_target(struct
cpufreq_policy *policy,
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->u.hwp;
/* Zero everything to ensure reserved bits are zero... */
union hwp_request hwp_req = { .raw = 0 };
Further down in this same function we have
on_selected_cpus(cpumask_of(cpu), hwp_write_request, policy, 1);
That's similarly problematic when the CPU denoted by policy->cpu isn't
online anymore. (It's not quite clear whether all related issues would
want fixing together, or in multiple patches.)
@@ -350,7 +349,7 @@ static void hwp_get_cpu_speeds(struct cpufreq_policy
*policy)
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->u.hwp;
uint64_t val;
/*
@@ -426,15 +425,14 @@ static int cf_check hwp_cpufreq_cpu_init(struct
cpufreq_policy *policy)
policy->governor = &cpufreq_gov_hwp;
- per_cpu(hwp_drv_data, cpu) = data;
+ policy->u.hwp = data;
on_selected_cpus(cpumask_of(cpu), hwp_init_msrs, policy, 1);
If multiple CPUs are in a domain, not all of them will make it here. By
implication the MSRs accessed by hwp_init_msrs() would need to have wider
than thread scope. The SDM, afaics, says nothing either way in this regard
in the Architectural MSRs section. Later model-specific tables have some
data.
When I wrote the HWP driver, I expected there to be per-cpu
hwp_drv_data. policy->cpu looked like the correct way to identify each
CPU. I was unaware of the idea of cpufreq_domains, and didn't intend
there to be any sharing.
Which gets me back to my original question: Is "sharing" actually possible
for HWP? Note further how there are both HWP_REQUEST and HWP_REQUEST_PKG
MSRs, for example. Which one is (to be) used looks to be controlled by
HWP_CTL.PKG_CTL_POLARITY.
I was aware of the Package Level MSRs, but chose not to support them.
Topology information didn't seem readily available to the driver, and
using non-Package Level MSRs is needed for backwards compatibility anyway.
I don't have access to an HWP system, so I cannot check if processors
share a domain. I'd feel a little silly if I only ever wrote to CPU 0 :/
I have no proof, but I want to say that at some point I had debug
statements and saw hwp_cpufreq_target() called for each CPU.
Maybe forcing hw_all=1 in cpufreq_add_cpu()/cpufreq_del_cpu() would
ensure per-cpu policies?
Regards,
Jason
