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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

> --- 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 ...

> @@ -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));
> +    memcpy(cppc_para, sys_cppc_para, sizeof(*sys_cppc_para));

... you minimally want to apply as much checking here.

Jan