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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.09.2025 05:49, Penny, Zheng wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Monday, September 8, 2025 6:02 PM
>> To: Penny, Zheng <penny.zheng@xxxxxxx>
>> Cc: Andryuk, Jason <Jason.Andryuk@xxxxxxx>; xen-
>> devel@xxxxxxxxxxxxxxxxxxxx
>> Subject: Re: [PATCH v9 1/8] xen/cpufreq: embed hwp into struct 
>> cpufreq_policy{}
>>
>> (re-adding the list)
>>
>> On 05.09.2025 06:58, Penny, Zheng wrote:
>>> [Public]
>>>
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Sent: Thursday, September 4, 2025 7:51 PM
>>>> To: Penny, Zheng <penny.zheng@xxxxxxx>; Andryuk, Jason
>>>> <Jason.Andryuk@xxxxxxx>
>>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné
>>>> <roger.pau@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
>>>> Subject: Re: [PATCH v9 1/8] xen/cpufreq: embed hwp into struct
>>>> cpufreq_policy{}
>>>>
>>>> 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.)
>>>
>>> Checking the logic in cpufreq_del_cpu(), once any processor in the
>>> domain gets offline, the governor will stop.
>>
>> Yet with HWP and CPPC drivers being governor-less, how would that matter?
>>
>>> That is to say, only all processors in the domain are online, cpufreq 
>>> driver could
>> still be effective. Which is also complies to the description in _PSD ACPI 
>> SPEC
>> for "Num Processors" [1]:
>>> ```
>>> The number of processors belonging to the domain for this logical 
>>> processor’s
>> P-states. OSPM will not start performing power state transitions to a 
>> particular P-
>> state until this number of processors belonging to the same domain have been
>> detected and started.
>>> ```
>>> [1]
>>> https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control
>>> .html?highlight=cppc#pstatedependency-package-values
>>>
>>> I know that AMD CPPC is obeying the _PSD dependency relation too, even if
>> both CPPC Request register (ccd[15:0]_lthree0_core[7:0]_thread[1:0];
>> MSRC001_02B3) and CPPC Capability Register
>> (_ccd[15:0]_lthree0_core[7:0]_thread[1:0]; MSRC001_02B0) is Per-thread MSR.
>>> I don't have the hardware to test "sharing" logic. All my platform says
>> "HW_ALL" in _PSD.
>>
>> Aiui that's not mandated by the CPU spec, though. Plus HW_ALL still doesn't 
>> say
>> anything about the scope/size of each domain.
> 
> Sorry for the late reply.

No worries. From feedback from Stefano I was fearing much of a delay.

> I have discussed this with hardware team now, they said that we shall not 
> expect any value other than "HW_ALL" from _PSD, if we have _CPC table, aka, 
> CPPC enabled. Maybe except for some initial implementations, which may or may 
> have not reached product release, this may still need a few time to look for 
> conclusion
> And if it is HW_ALL, it means, in codes, we are invoking per-cpu 
> cpufreq_driver.init, allocating per-cpu copufreq_policy, and etc. In HW_ALL, 
> OSPM can make different state requests for processors in the domain, while 
> hardware determines the resulting state for all processors in the domain.

Okay, so going back to v8 (and doing some unrelated adjustments there)
looks (to me) to be the best option. Eventually (I wouldn't insist on
this happening right away) we may want to actually reject non-HW_ALL
configurations when using this new driver.

Jan