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

[Jan Beulich <jbeulich@xxxxxxxx>]

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, ...

>> Since initially I was thinking of using per-CPU data also in my patch, I'm
>> reproducing this in raw form below, for your reference. It's generally only
>> 4.22 material now, of course. Yet in turn for your driver the new drv_data 
>> field
>> may want to become a union, with an "acpi" and a "cppc" sub-struct.
> 
> How about I embed my new driver data " struct amd_cppc_drv_data * " into 
> cpufreq policy, maybe pointer is enough?
> Later, maybe, all "cppc", "acpi" and "hwp" could constitute an union in 
> policy.

... I'd prefer to go the union approach right away. Whether then to take my
patch as a prereq is tbd; that largely depends on what (if anything) is
needed on the HWP side. If HWP needs fixing, that wants to to come first, as
it would want backporting.

Jan