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.
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.
> @@ -462,10 +460,8 @@ static int cf_check hwp_cpufreq_cpu_init(struct
> cpufreq_policy *policy)
>
> 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);
> + if ( policy->u.hwp )
> + XFREE(policy->u.hwp);
No if() needed here.
> @@ -480,7 +476,7 @@ static int cf_check hwp_cpufreq_cpu_exit(struct
> cpufreq_policy *policy)
> 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 hwp_drv_data *data = policy->u.hwp;
> uint64_t msr;
>
> data->ret = 0;
> @@ -511,7 +507,7 @@ static int cf_check hwp_cpufreq_update(unsigned int cpu,
> struct cpufreq_policy *
> {
> on_selected_cpus(cpumask_of(cpu), hwp_set_misc_turbo, policy, 1);
>
> - return per_cpu(hwp_drv_data, cpu)->ret;
> + return policy->u.hwp->ret;
> }
> #endif /* CONFIG_PM_OP */
Same concern here wrt MSR scope. MISC_ENABLE.TURBO_DISENGAGE's scope is
package as per the few tables which have the bit explicitly explained;
whether that extends to all models is unclear.
> --- a/xen/include/acpi/cpufreq/cpufreq.h
> +++ b/xen/include/acpi/cpufreq/cpufreq.h
> @@ -62,6 +62,7 @@ struct perf_limits {
> uint32_t min_policy_pct;
> };
>
> +struct hwp_drv_data;
This shouldn't be needed.
> @@ -81,6 +82,11 @@ struct cpufreq_policy {
> int8_t turbo; /* tristate flag: 0 for unsupported
> * -1 for disable, 1 for enabled
> * See CPUFREQ_TURBO_* below for defines */
> + union {
> +#ifdef CONFIG_INTEL
> + struct hwp_drv_data *hwp; /* Driver data for Intel HWP */
> +#endif
While it may make for a smaller diff, ultimately I think we don't want
this to be a pointer, much like I've done in my patch for the ACPI driver.
> + } u;
This wants to either not have a name at all, or be named e.g. drv_data.
Jan
