[Public]
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Tuesday, August 26, 2025 4:33 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 26.08.2025 10:21, Penny, Zheng wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeul...@suse.com>
> >> 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
