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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> 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