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
