Re: [PATCH v5 14/18] xen/cpufreq: introduce GET_CPUFREQ_CPPC sub-cmd

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.05.2025 10:48, Penny Zheng wrote:
> --- 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"
> @@ -812,33 +813,7 @@ static void print_cpufreq_para(int cpuid, struct 
> xc_get_cpufreq_para *p_cpufreq)
>  
>      printf("scaling_driver       : %s\n", p_cpufreq->scaling_driver);
>  
> -    if ( hwp )
> -    {
> -        const xc_cppc_para_t *cppc = &p_cpufreq->u.cppc_para;
> -
> -        printf("cppc variables       :\n");
> -        printf("  hardware limits    : lowest [%"PRIu32"] lowest nonlinear 
> [%"PRIu32"]\n",
> -               cppc->lowest, cppc->lowest_nonlinear);
> -        printf("                     : nominal [%"PRIu32"] highest 
> [%"PRIu32"]\n",
> -               cppc->nominal, cppc->highest);
> -        printf("  configured limits  : min [%"PRIu32"] max [%"PRIu32"] 
> energy perf [%"PRIu32"]\n",
> -               cppc->minimum, cppc->maximum, cppc->energy_perf);
> -
> -        if ( cppc->features & XEN_SYSCTL_CPPC_FEAT_ACT_WINDOW )
> -        {
> -            unsigned int activity_window;
> -            const char *units;
> -
> -            activity_window = calculate_activity_window(cppc, &units);
> -            printf("                     : activity_window [%"PRIu32" %s]\n",
> -                   activity_window, units);
> -        }
> -
> -        printf("                     : desired [%"PRIu32"%s]\n",
> -               cppc->desired,
> -               cppc->desired ? "" : " hw autonomous");
> -    }
> -    else
> +    if ( !hwp )
>      {
>          if ( p_cpufreq->gov_num )
>              printf("scaling_avail_gov    : %s\n",

I'm not sure it is a good idea to alter what is being output for
get-cpufreq-para. People may simply miss that output then, without having
any indication where it went.

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -462,7 +462,6 @@ struct xen_get_cpufreq_para {
>                  struct  xen_ondemand ondemand;
>              } u;
>          } s;
> -        struct xen_cppc_para cppc_para;
>      } u;
>  
>      int32_t turbo_enabled;
> @@ -493,6 +492,7 @@ struct xen_sysctl_pm_op {
>      #define SET_CPUFREQ_PARA           (CPUFREQ_PARA | 0x03)
>      #define GET_CPUFREQ_AVGFREQ        (CPUFREQ_PARA | 0x04)
>      #define SET_CPUFREQ_CPPC           (CPUFREQ_PARA | 0x05)
> +    #define GET_CPUFREQ_CPPC           (CPUFREQ_PARA | 0x06)
>  
>      /* set/reset scheduler power saving option */
>      #define XEN_SYSCTL_pm_op_set_sched_opt_smt    0x21
> @@ -517,6 +517,7 @@ struct xen_sysctl_pm_op {
>      uint32_t cpuid;
>      union {
>          struct xen_get_cpufreq_para get_para;
> +        struct xen_cppc_para        cppc_para;
>          struct xen_set_cpufreq_gov  set_gov;
>          struct xen_set_cpufreq_para set_para;
>          struct xen_set_cppc_para    set_cppc;

This (tools-only) public interface change, otoh, may be okay to do.

Jan