Re: [PATCH v9 1/8] xen/cpufreq: embed hwp into struct cpufreq_policy{}

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <jbeulich@xxxxxxxx>

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