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 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)....


> > 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?
>
> Duplicate information (in distinct commands) isn't a problem either, imo.
>
> Jason, you did the HWP work here - any thoughts?
>
> Jan