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

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[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);
                ...
        }
```

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