Re: [PATCH v6 16/19] xen/cpufreq: introduce GET_CPUFREQ_CPPC sub-op

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.07.2025 05:51, Penny Zheng wrote:
> In amd-cppc passive mode, it's Xen governor which is responsible for
> performance tuning, so governor and CPPC could co-exist. That is, both
> governor-info and CPPC-info 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-op GET_CPUFREQ_CPPC to specifically print
> CPPC-related para.
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
> ---
> v4 -> v5:
> - new commit
> ---
> v5 -> v6:
> - remove the changes for get-cpufreq-para
> ---
>  tools/include/xenctrl.h     |  2 ++
>  tools/libs/ctrl/xc_pm.c     | 27 +++++++++++++++++++++
>  tools/misc/xenpm.c          | 47 +++++++++++++++++++++++++++++++++++++
>  xen/drivers/acpi/pm-op.c    |  4 ++++
>  xen/include/public/sysctl.h |  2 ++
>  5 files changed, 82 insertions(+)
> 
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 965d3b585a..699243c4df 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -1953,6 +1953,8 @@ int xc_set_cpufreq_para(xc_interface *xch, int cpuid,
>                          int ctrl_type, int ctrl_value);
>  int xc_set_cpufreq_cppc(xc_interface *xch, int cpuid,
>                          xc_set_cppc_para_t *set_cppc);
> +int xc_get_cppc_para(xc_interface *xch, unsigned int cpuid,
> +                     xc_cppc_para_t *cppc_para);
>  int xc_get_cpufreq_avgfreq(xc_interface *xch, int cpuid, int *avg_freq);
>  
>  int xc_set_sched_opt_smt(xc_interface *xch, uint32_t value);
> diff --git a/tools/libs/ctrl/xc_pm.c b/tools/libs/ctrl/xc_pm.c
> index 6fda973f1f..3f72152617 100644
> --- a/tools/libs/ctrl/xc_pm.c
> +++ b/tools/libs/ctrl/xc_pm.c
> @@ -366,6 +366,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));
> +
> +    return ret;
> +}
> +
>  int xc_get_cpufreq_avgfreq(xc_interface *xch, int cpuid, int *avg_freq)
>  {
>      int ret = 0;
> diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
> index 120e9eae22..bdc09f468a 100644
> --- a/tools/misc/xenpm.c
> +++ b/tools/misc/xenpm.c
> @@ -69,6 +69,7 @@ void show_help(void)
>              " set-max-cstate        <num>|'unlimited' [<num2>|'unlimited']\n"
>              "                                     set the C-State limitation 
> (<num> >= 0) and\n"
>              "                                     optionally the C-sub-state 
> limitation (<num2> >= 0)\n"
> +            " get-cpufreq-cppc      [cpuid]       list cpu cppc parameter of 
> CPU <cpuid> or all\n"
>              " set-cpufreq-cppc      [cpuid] [balance|performance|powersave] 
> <param:val>*\n"
>              "                                     set Hardware P-State (HWP) 
> parameters\n"
>              "                                     on CPU <cpuid> or all if 
> omitted.\n"
> @@ -996,6 +997,51 @@ void cpufreq_para_func(int argc, char *argv[])
>          show_cpufreq_para_by_cpuid(xc_handle, cpuid);
>  }
>  
> +/* show cpu cppc parameters information on CPU cpuid */
> +static int show_cppc_para_by_cpuid(xc_interface *xc_handle, unsigned int 
> cpuid)
> +{
> +    int ret;
> +    xc_cppc_para_t cppc_para;
> +
> +    ret = xc_get_cppc_para(xc_handle, cpuid, &cppc_para);
> +    if ( !ret )
> +    {
> +        printf("cpu id               : %d\n", cpuid);
> +        print_cppc_para(cpuid, &cppc_para);
> +        printf("\n");
> +    }
> +    else if ( errno == ENODEV )
> +    {
> +        ret = -ENODEV;
> +        fprintf(stderr, "CPPC is not available!\n");
> +    }
> +    else
> +        fprintf(stderr, "[CPU%u] failed to get cppc parameter\n", cpuid);
> +
> +    return ret;
> +}
> +
> +static void cppc_para_func(int argc, char *argv[])
> +{
> +    int cpuid = -1;
> +
> +    if ( argc > 0 )
> +        parse_cpuid(argv[0], &cpuid);
> +
> +    if ( cpuid < 0 )
> +    {
> +        unsigned int i;
> +
> +        /* show cpu cppc information on all cpus */
> +        for ( i = 0; i < max_cpu_nr; i++ )
> +            /* Bail out only on unsupported platform */
> +            if ( show_cppc_para_by_cpuid(xc_handle, i) == -ENODEV )
> +                break;
> +    }
> +    else
> +        show_cppc_para_by_cpuid(xc_handle, cpuid);
> +}
> +
>  void scaling_max_freq_func(int argc, char *argv[])
>  {
>      int cpuid = -1, freq = -1;
> @@ -1576,6 +1622,7 @@ struct {
>      { "get-cpufreq-average", cpufreq_func },
>      { "start", start_gather_func },
>      { "get-cpufreq-para", cpufreq_para_func },
> +    { "get-cpufreq-cppc", cppc_para_func },

Didn't Jason also suggest that we would better not introduce a new command, but
rather make get-cpufreq-para invoke GET_CPUFREQ_CPPC as needed? Considering that
as per patch 15 the same information is already printed, I think I'm a little
lost with the need for this separate operation (and command), and then also with
the need for patch 15.

Jan