On 04.07.2025 10:56, Penny, Zheng wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Friday, July 4, 2025 4:33 PM
>> To: Penny, Zheng <penny.zh...@amd.com>; Andryuk, Jason
>> <jason.andr...@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 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.
>>
>
> 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.)
Jan
