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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2025-07-06 23:31, Penny, Zheng wrote:
> [Public]
>
> > -----Original Message-----
> > From: Jan Beulich <jbeulich@xxxxxxxx>
> > Sent: Friday, July 4, 2025 5:46 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; Andryuk, 
> > Jason
> > <Jason.Andryuk@xxxxxxx>
> > Subject: Re: [PATCH v5 14/18] xen/cpufreq: introduce GET_CPUFREQ_CPPC sub-
> > cmd
> >
> > On 04.07.2025 10:56, Penny, Zheng wrote:
> > > [Public]
> > >
> > > > -----Original Message-----
> > > > From: Jan Beulich <jbeulich@xxxxxxxx>
> > > > Sent: Friday, July 4, 2025 4:33 PM
> > > > To: Penny, Zheng <penny.zheng@xxxxxxx>; Andryuk, Jason
> > > > <Jason.Andryuk@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 04.07.2025 10:13, Penny, Zheng wrote:
> > > > > [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.
> > > >
> > > > How is the xenpm command "get-cpufreq-para" related to the sysctl
> > > > interface struct size? If you need to invoke two sysctl sub-ops to
> > > > produce all relevant "get-cpufreq- para" output, so be it I would say.
> > > Because we are limiting each sysctl-subcmd-struct, such as " struct
> > xen_sysctl_pm_op ", 128 bytes in "struct xen_sysctl"，They are all combined as a
> > union.
> > > Also, in "struct xen_sysctl_pm_op", its descending sub-op structs,
> > > including "struct xen_get_cpufreq_para", are all combined as union too
> > > ``` struct xen_sysctl_pm_op {
> > >      ... ...
> > >      union {
> > >          struct xen_get_cpufreq_para get_para;
> > >          struct xen_set_cpufreq_gov  set_gov;
> > >          struct xen_set_cpufreq_para set_para;
> > >          struct xen_set_cppc_para    set_cppc;
> > >          uint64_aligned_t get_avgfreq;
> > >          uint32_t                    set_sched_opt_smt;
> > > #define XEN_SYSCTL_CX_UNLIMITED 0xffffffffU
> > >          uint32_t                    get_max_cstate;
> > >          uint32_t                    set_max_cstate;
> > >      } u;
> > > }
> > > ```
> > > It could deduce that "struct xen_get_cpufreq_para" is limited to 128 bytes (I 
> > > think
> > actual limit is smaller than 128)....
> >
> > And that implies what? In my earlier reply I already said that you may then 
> > simply
> > need to invoke more than one sysctl to get all the data you need. (As one of the
> > options, that is.)
> Okay, I only explained why I couldn't use one command "get-cpufreq-para" to 
> output all info
I think Jan's suggestion to make `xenpm get-cpufreq-para` keep 
outputting cppc/hwp information is good.  That way users get the cpufreq 
information without having to know what driver is running.  So you will 
have to issue the additional hypercall as necessary to retrieve the data.
Regards,
Jason