Re: [PATCH v8 3/8] xen/cpufreq: implement amd-cppc driver for CPPC in passive mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.09.2025 05:21, Penny, Zheng wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Friday, August 29, 2025 2:12 PM
>> To: Penny, Zheng <penny.zheng@xxxxxxx>
>> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
>> <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>;
>> Anthony PERARD <anthony.perard@xxxxxxxxxx>; Orzel, Michal
>> <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano Stabellini
>> <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; Andryuk, Jason
>> <Jason.Andryuk@xxxxxxx>
>> Subject: Re: [PATCH v8 3/8] xen/cpufreq: implement amd-cppc driver for CPPC 
>> in
>> passive mode
>>
>> On 29.08.2025 05:30, Penny, Zheng wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Sent: Thursday, August 28, 2025 7:23 PM
>>>>
>>>> On 28.08.2025 12:03, Penny Zheng wrote:
>>>>> +static int cf_check amd_cppc_cpufreq_target(struct cpufreq_policy 
>>>>> *policy,
>>>>> +                                            unsigned int target_freq,
>>>>> +                                            unsigned int relation) {
>>>>> +    unsigned int cpu = policy->cpu;
>>>>> +    const struct amd_cppc_drv_data *data =
>>>>> +per_cpu(amd_cppc_drv_data, cpu);
>>>>
>>>> I fear there's a problem here that I so far overlooked. As it
>>>> happens, just yesterday I made a patch to eliminate
>>>> cpufreq_drv_data[] global. In the course of doing so it became clear
>>>> that in principle the CPU denoted by
>>>> policy->cpu can be offline. Hence its per-CPU data is also unavailable.
>>>> policy->See
>>>> cpufreq_add_cpu()'s invocation of .init() and cpufreq_del_cpu()'s
>>>> invocation of .exit(). Is there anything well-hidden (and likely
>>>> lacking some suitable
>>>> comment) which guarantees that no two CPUs (threads) will be in the
>>>> same domain? If not, I fear you simply can't use per-CPU data here.
>>>>
>>>
>>> Correct me if I understand you wrongly:
>>> No, my env is always per pcpu per cpufreq domain. So it never occurred to me
>> that cpus, other than the first one in domain, will never call .init(), and 
>> of course, no
>> per_cpu(amd_cppc_drv_data) ever gets allocated then.
>>
>> Well, the question is how domains are organized when using the CPPC driver.
>> Aiui that's still driven by data passed in by Dom0, so in turn the question 
>> is whether
>> there are any constraints on what ACPI may surface. If there are, all that 
>> may be
>> necessary is adding a check. If there aren't, ...
>>
> 
> According to ACPI spec, _PSD controls both P-state or CPPC, so in my 
> implementation of getting CPPC data passed by Dom0(set_cppc_pminfo()), I 
> demand both entry exist, _PSD and _CPC.
> ```
>         if ( cppc_data->flags == (XEN_CPPC_PSD | XEN_CPPC_CPC) )
>         {
>                 ...
>                 pm_info->init = XEN_CPPC_INIT;
>                 ret = cpufreq_cpu_init(cpuid);
>                 ...
>         }
> ```

That's only about presence of the data though. My remark, otoh, was about its
content. It could in principle be specified somewhere that no domain may cover
more than a single CPU / thread. In the absence of such, the case needs
handling correctly.

Jan