RE: [PATCH v7 11/13] tools/cpufreq: extract CPPC para from cpufreq para

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

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Monday, August 25, 2025 11:37 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Anthony PERARD
> <anthony.perard@xxxxxxxxxx>; Juergen Gross <jgross@xxxxxxxx>; Andrew
> Cooper <andrew.cooper3@xxxxxxxxxx>; Orzel, Michal <Michal.Orzel@xxxxxxx>;
> Julien Grall <julien@xxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> 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.Zheng@xxxxxxx>
>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx> # 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