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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

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 <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.
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