On 01.09.2025 05:21, Penny, Zheng wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Friday, August 29, 2025 2:12 PM
>> To: Penny, Zheng <penny.zh...@amd.com>
>> Cc: Huang, Ray <ray.hu...@amd.com>; Andrew Cooper
>> <andrew.coop...@citrix.com>; Roger Pau Monné <roger....@citrix.com>;
>> Anthony PERARD <anthony.per...@vates.tech>; Orzel, Michal
>> <michal.or...@amd.com>; Julien Grall <jul...@xen.org>; Stefano Stabellini
>> <sstabell...@kernel.org>; xen-devel@lists.xenproject.org; Andryuk, Jason
>> <jason.andr...@amd.com>
>> 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 <jbeul...@suse.com>
>>>> 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
