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