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

[Jan Beulich <jbeulich@xxxxxxxx>]

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;

?

Jan