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

> Jan