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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.08.2025 02:16, Jason Andryuk wrote:
> On 2025-08-28 07:22, Jan Beulich wrote:
>> 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. 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.
> 
> Sorry, I'm confused by your use of "domain" here.

I agree it's confusing, but that's the terminology used in cpufreq.c (see
e.g. "struct cpufreq_dom" or "const struct xen_psd_package *domain_info").

>  Do you mean a 
> per_cpu(..., policy->cpu) access racing with a cpu offline?

Yes (I wouldn't call it "racing" though, as it's not a timing issue).

>  I'm not 
> away of anything preventing that, though I'm not particularly familiar 
> with it.



> do_pm_op() has:
>      if ( op->cpuid >= nr_cpu_ids || !cpu_online(op->cpuid) )
>          return -EINVAL;
>      pmpt = processor_pminfo[op->cpuid];
> 
> and do_get_pm_info() has:
>      if ( !op || (op->cpuid >= nr_cpu_ids) || !cpu_online(op->cpuid) )
>          return -EINVAL;
>      pmpt = processor_pminfo[op->cpuid];
> 
> But those are only at entry.

That's not accessing struct cpufreq_policy, though. Per-CPU accesses
using policy->cpu are the problematic ones, as - from all I can tell -
the CPU named there can have gone offline, with the policy surviving
when some other CPU is also part of the same "domain".

As said in the reply to Penny, main question is whether the data
controlling what a "domain" covers may be constrained in the HWP case,
demanding that no two CPUs (threads) can be in the same "domain". Then
adding merely a sanity check somewhere would suffice.

Jan