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

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, June 17, 2025 6:08 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Anthony PERARD
> <anthony.perard@xxxxxxxxxx>; Juergen Gross <jgross@xxxxxxxx>; Andrew
> Cooper <andrew.cooper3@xxxxxxxxxx>; Orzel, Michal <Michal.Orzel@xxxxxxx>;
> Julien Grall <julien@xxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 14/18] xen/cpufreq: introduce GET_CPUFREQ_CPPC sub-
> cmd
>
> 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.

Hwp is more like amd-cppc in active mode. It also does not rely on Xen governor 
to do performance tuning, so in previous design, we could borrow governor filed 
to output cppc info
However after introducing amd-cppc passive mode, we have request to output both 
governor and CPPC info. And if continuing expanding get-cpufreq-para to include 
CPPC info, it will make the parent stuct xen_sysctl.u exceed exceed 128 bytes. 
So I'm left to create a new subcmd to specifically deal with CPPC info
I could leave above output for get-cpufreq-para unchanged. Then we will have 
duplicate CPPC info in two commands. Or is it fine to do that?

>
>
> Jan