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