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: Tuesday, August 26, 2025 4:33 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 26.08.2025 10:21, Penny, Zheng wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: Monday, August 25, 2025 11:37 PM
> >>
> >> On 22.08.2025 12:52, Penny Zheng wrote:
> >>> --- 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;
>
> Oh. Then ...
>
> > 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));
>
> ... why is this here, when ...
>
> >>> +    memcpy(cppc_para, sys_cppc_para, sizeof(*sys_cppc_para));
> >>
> >> ... you minimally want to apply as much checking here.
>
> ... a better effect can be had by
>
>     cppc_para = sys_cppc_para;
>
> ?
>

True, no need to do memory copy then if it is an alias

> Jan