On 04.07.2025 10:13, Penny, Zheng wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Tuesday, June 17, 2025 6:08 PM
>> To: Penny, Zheng <penny.zh...@amd.com>
>> Cc: Huang, Ray <ray.hu...@amd.com>; Anthony PERARD
>> <anthony.per...@vates.tech>; Juergen Gross <jgr...@suse.com>; Andrew
>> Cooper <andrew.coop...@citrix.com>; Orzel, Michal <michal.or...@amd.com>;
>> Julien Grall <jul...@xen.org>; Roger Pau Monné <roger....@citrix.com>;
>> Stefano
>> Stabellini <sstabell...@kernel.org>; xen-devel@lists.xenproject.org
>> 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
