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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.09.2025 20:53, Jason Andryuk wrote:
> On 2025-09-04 07:50, Jan Beulich wrote:
>> On 04.09.2025 08:35, Penny Zheng wrote:
>>> @@ -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?

No, domain info is handed to us from ACPI (_PSD). That's what
cpufreq_add_cpu() evaluates. Therefore if there was evidence that HWP (and
CPPC) can only ever have single-CPU domains, we could refuse such _PSD
being handed to us (ideally already in set_px_pminfo()). But I don't think
we can just go and override what we were told. I fear though that the mere
existence of a package-level (alternative) MSR suggests that such
configurations might be possible.

Jan