[Public]
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Monday, August 25, 2025 11:37 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Huang, Ray <ray.hu...@amd.com>; Anthony PERARD
> <anthony.per...@vates.tech>; Juergen Gross <jgr...@suse.com>; Andrew
> Cooper <andrew.coop...@citrix.com>; Orzel, Michal <michal.or...@amd.com>;
> Julien Grall <jul...@xen.org>; Roger Pau Monné <roger....@citrix.com>; Stefano
> Stabellini <sstabell...@kernel.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v7 11/13] tools/cpufreq: extract CPPC para from cpufreq
> para
>
> On 22.08.2025 12:52, Penny Zheng wrote:
> > We extract cppc info from "struct xen_get_cpufreq_para", where it acts
> > as a member of union, and share the space with governor info.
> > However, it may fail in amd-cppc passive mode, in which governor info
> > and CPPC info could co-exist, and both need to be printed together via xenpm
> tool.
> > If we tried to still put it in "struct xen_get_cpufreq_para" (e.g.
> > just move out of union), "struct xen_get_cpufreq_para" will enlarge
> > too much to further make xen_sysctl.u exceed 128 bytes.
> >
> > So we introduce a new sub-field GET_CPUFREQ_CPPC to dedicatedly
> > acquire CPPC-related para, and make get-cpufreq-para invoke
> > GET_CPUFREQ_CPPC if available.
> > New helpers print_cppc_para() and get_cpufreq_cppc() are introduced to
> > extract CPPC-related parameters process from cpufreq para.
> >
> > Signed-off-by: Penny Zheng <penny.zh...@amd.com>
>
> Acked-by: Jan Beulich <jbeul...@suse.com> # hypervisor
>
Thx
> > --- a/tools/libs/ctrl/xc_pm.c
> > +++ b/tools/libs/ctrl/xc_pm.c
> > @@ -288,7 +288,6 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
> > CHK_FIELD(s.scaling_min_freq);
> > CHK_FIELD(s.u.userspace);
> > CHK_FIELD(s.u.ondemand);
> > - CHK_FIELD(cppc_para);
> >
> > #undef CHK_FIELD
>
> What is done here is already less than what could be done; I think ...
>
Emm, maybe because we define two different cpufreq para structures for user
space and sysctl, struct xc_get_cpufreq_para and struct xen_get_cppc_para.
But for cppc para, it is an alias:
typedef struct xen_get_cppc_para xc_cppc_para_t;
So ...
> > @@ -366,6 +365,33 @@ int xc_set_cpufreq_cppc(xc_interface *xch, int cpuid,
> > return ret;
> > }
> >
> > +int xc_get_cppc_para(xc_interface *xch, unsigned int cpuid,
> > + xc_cppc_para_t *cppc_para) {
> > + int ret;
> > + struct xen_sysctl sysctl = {};
> > + struct xen_get_cppc_para *sys_cppc_para =
> > +&sysctl.u.pm_op.u.get_cppc;
> > +
> > + if ( !xch || !cppc_para )
> > + {
> > + errno = EINVAL;
> > + return -1;
> > + }
> > +
> > + sysctl.cmd = XEN_SYSCTL_pm_op;
> > + sysctl.u.pm_op.cmd = GET_CPUFREQ_CPPC;
> > + sysctl.u.pm_op.cpuid = cpuid;
> > +
> > + ret = xc_sysctl(xch, &sysctl);
> > + if ( ret )
> > + return ret;
> > +
> > + BUILD_BUG_ON(sizeof(*cppc_para) != sizeof(*sys_cppc_para));
... maybe whole structure size checking is enough?
> > + memcpy(cppc_para, sys_cppc_para, sizeof(*sys_cppc_para));
>
> ... you minimally want to apply as much checking here.
>
> Jan
