In amd-cppc passive mode, it's Xen governor which is responsible for
performance tuning, so governor and CPPC could co-exist. That is, both
governor-info and CPPC-info could be printed together via xenpm tool.
To achieve that, "struct xen_cppc_para" needs to be moved out of the union
and also "struct xen_get_cpufreq_para". Also if still putting it in
"struct xen_get_cpufreq_para" (e.g. just move out of union),
"struct xen_get_cpufreq_para" will enlarge too much to further make
xen_sysctl.u exceed 128 bytes.
We introduce a new sub-cmd GET_CPUFREQ_CPPC to specifically print
CPPC-related para and clear cppc print in GET_CPUFREQ_PARA.
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v4 -> v5:
- new commit
---
tools/include/xenctrl.h | 3 +-
tools/libs/ctrl/xc_pm.c | 28 ++++++++++-
tools/misc/xenpm.c | 97 ++++++++++++++++++++++++++-----------
xen/drivers/acpi/pmstat.c | 18 +++++--
xen/include/public/sysctl.h | 3 +-
5 files changed, 116 insertions(+), 33 deletions(-)
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 4955981231..79523d2d73 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1938,7 +1938,6 @@ struct xc_get_cpufreq_para {
xc_ondemand_t ondemand;
} u;
} s;
- xc_cppc_para_t cppc_para;
} u;
int32_t turbo_enabled;
@@ -1953,6 +1952,8 @@ int xc_set_cpufreq_para(xc_interface *xch, int cpuid,
int ctrl_type, int ctrl_value);
int xc_set_cpufreq_cppc(xc_interface *xch, int cpuid,
xc_set_cppc_para_t *set_cppc);
+int xc_get_cppc_para(xc_interface *xch, unsigned int cpuid,
+ xc_cppc_para_t *cppc_para);
int xc_get_cpufreq_avgfreq(xc_interface *xch, int cpuid, int *avg_freq);
int xc_set_sched_opt_smt(xc_interface *xch, uint32_t value);
diff --git a/tools/libs/ctrl/xc_pm.c b/tools/libs/ctrl/xc_pm.c
index ff7b5ada05..3c9e272aee 100644
--- a/tools/libs/ctrl/xc_pm.c
+++ b/tools/libs/ctrl/xc_pm.c
@@ -289,7 +289,6 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
CHK_FIELD(s.scaling_min_freq);
CHK_FIELD(s.u.userspace);
CHK_FIELD(s.u.ondemand);
- CHK_FIELD(cppc_para);
#undef CHK_FIELD
@@ -367,6 +366,33 @@ int xc_set_cpufreq_cppc(xc_interface *xch, int cpuid,
return ret;
}
+int xc_get_cppc_para(xc_interface *xch, unsigned int cpuid,
+ xc_cppc_para_t *cppc_para)
+{
+ int ret;
+ struct xen_sysctl sysctl = {};
+ struct xen_cppc_para *sys_cppc_para = &sysctl.u.pm_op.u.cppc_para;
+
+ if ( !xch || !cppc_para )
+ {
+ errno = EINVAL;
+ return -1;
+ }
+
+ sysctl.cmd = XEN_SYSCTL_pm_op;
+ sysctl.u.pm_op.cmd = GET_CPUFREQ_CPPC;
+ sysctl.u.pm_op.cpuid = cpuid;
+
+ ret = xc_sysctl(xch, &sysctl);
+ if ( ret )
+ return ret;
+
+ BUILD_BUG_ON(sizeof(*cppc_para) != sizeof(*sys_cppc_para));
+ memcpy(cppc_para, sys_cppc_para, sizeof(*sys_cppc_para));
+
+ return ret;
+}
+
int xc_get_cpufreq_avgfreq(xc_interface *xch, int cpuid, int *avg_freq)
{
int ret = 0;
diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
index db658ebadd..2a87f7ae8a 100644
--- 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",
@@ -981,6 +956,73 @@ void cpufreq_para_func(int argc, char *argv[])
show_cpufreq_para_by_cpuid(xc_handle, cpuid);
}
+/* print out parameters about cpu cppc */
+static void print_cppc_para(unsigned int cpuid,
+ const xc_cppc_para_t *cppc)
+{
+ printf("cpu id : %u\n", cpuid);
+ 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");
+ printf("\n");
+}
+
+/* show cpu cppc parameters information on CPU cpuid */
+static int show_cppc_para_by_cpuid(xc_interface *xc_handle, unsigned int cpuid)
+{
+ int ret;
+ xc_cppc_para_t cppc_para;
+
+ ret = xc_get_cppc_para(xc_handle, cpuid, &cppc_para);
+ if ( ret == 0 )
+ print_cppc_para(cpuid, &cppc_para);
+ else if ( errno == ENODEV )
+ {
+ ret = -ENODEV;
+ fprintf(stderr, "CPPC is not available!\n");
+ }
+ else
+ fprintf(stderr, "[CPU%u] failed to get cppc parameter\n", cpuid);
+
+ return ret;
+}
+
+static void cppc_para_func(int argc, char *argv[])
+{
+ int cpuid = -1;
+
+ if ( argc > 0 )
+ parse_cpuid(argv[0], &cpuid);
+
+ if ( cpuid < 0 )
+ {
+ /* show cpu cppc information on all cpus */
+ for ( unsigned int i = 0; i < max_cpu_nr; i++ )
+ if ( show_cppc_para_by_cpuid(xc_handle, i) == -ENODEV )
+ break;
+ }
+ else
+ show_cppc_para_by_cpuid(xc_handle, cpuid);
+}
+
void scaling_max_freq_func(int argc, char *argv[])
{
int cpuid = -1, freq = -1;
@@ -1561,6 +1603,7 @@ struct {
{ "get-cpufreq-average", cpufreq_func },
{ "start", start_gather_func },
{ "get-cpufreq-para", cpufreq_para_func },
+ { "get-cpufreq-cppc", cppc_para_func },
{ "set-cpufreq-cppc", cppc_set_func },
{ "set-scaling-maxfreq", scaling_max_freq_func },
{ "set-scaling-minfreq", scaling_min_freq_func },
diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c
index c09e001ec3..6e9178ade1 100644
--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -253,9 +253,7 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
else
strlcpy(op->u.get_para.scaling_driver, "Unknown", CPUFREQ_NAME_LEN);
- if ( hwp_active() )
- ret = get_hwp_para(policy->cpu, &op->u.get_para.u.cppc_para);
- else
+ if ( !hwp_active() )
{
if ( !(scaling_available_governors =
xzalloc_array(char, gov_num * CPUFREQ_NAME_LEN)) )
@@ -328,6 +326,16 @@ static int set_cpufreq_gov(struct xen_sysctl_pm_op *op)
return __cpufreq_set_policy(old_policy, &new_policy);
}
+static int get_cpufreq_cppc(struct xen_sysctl_pm_op *op)
+{
+ int ret = -ENODEV;
+
+ if ( hwp_active() )
+ ret = get_hwp_para(op->cpuid, &op->u.cppc_para);
+
+ return ret;
+}
+
static int set_cpufreq_para(struct xen_sysctl_pm_op *op)
{
int ret = 0;
@@ -498,6 +506,10 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
break;
}
+ case GET_CPUFREQ_CPPC:
+ ret = get_cpufreq_cppc(op);
+ break;
+
case SET_CPUFREQ_CPPC:
ret = set_cpufreq_cppc(op);
break;
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index fa431fd983..29872fe508 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -462,7 +462,6 @@ struct xen_get_cpufreq_para {
struct xen_ondemand ondemand;
} u;
} s;
- struct xen_cppc_para cppc_para;
} u;
int32_t turbo_enabled;
@@ -493,6 +492,7 @@ struct xen_sysctl_pm_op {
#define SET_CPUFREQ_PARA (CPUFREQ_PARA | 0x03)
#define GET_CPUFREQ_AVGFREQ (CPUFREQ_PARA | 0x04)
#define SET_CPUFREQ_CPPC (CPUFREQ_PARA | 0x05)
+ #define GET_CPUFREQ_CPPC (CPUFREQ_PARA | 0x06)
/* set/reset scheduler power saving option */
#define XEN_SYSCTL_pm_op_set_sched_opt_smt 0x21
@@ -517,6 +517,7 @@ struct xen_sysctl_pm_op {
uint32_t cpuid;
union {
struct xen_get_cpufreq_para get_para;
+ struct xen_cppc_para cppc_para;
struct xen_set_cpufreq_gov set_gov;
struct xen_set_cpufreq_para set_para;
struct xen_set_cppc_para set_cppc;
--
2.34.1On 27.05.2025 10:48, Penny Zheng wrote:
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -462,7 +462,6 @@ struct xen_get_cpufreq_para {
> struct xen_ondemand ondemand;
> } u;
> } s;
> - struct xen_cppc_para cppc_para;
> } u;
>
> int32_t turbo_enabled;
> @@ -493,6 +492,7 @@ struct xen_sysctl_pm_op {
> #define SET_CPUFREQ_PARA (CPUFREQ_PARA | 0x03)
> #define GET_CPUFREQ_AVGFREQ (CPUFREQ_PARA | 0x04)
> #define SET_CPUFREQ_CPPC (CPUFREQ_PARA | 0x05)
> + #define GET_CPUFREQ_CPPC (CPUFREQ_PARA | 0x06)
>
> /* set/reset scheduler power saving option */
> #define XEN_SYSCTL_pm_op_set_sched_opt_smt 0x21
> @@ -517,6 +517,7 @@ struct xen_sysctl_pm_op {
> uint32_t cpuid;
> union {
> struct xen_get_cpufreq_para get_para;
> + struct xen_cppc_para cppc_para;
> struct xen_set_cpufreq_gov set_gov;
> struct xen_set_cpufreq_para set_para;
> struct xen_set_cppc_para set_cppc;
Imo with the move and new use, the struct wants to become xen_get_cppc_para.
Jan
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.
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -462,7 +462,6 @@ struct xen_get_cpufreq_para {
> struct xen_ondemand ondemand;
> } u;
> } s;
> - struct xen_cppc_para cppc_para;
> } u;
>
> int32_t turbo_enabled;
> @@ -493,6 +492,7 @@ struct xen_sysctl_pm_op {
> #define SET_CPUFREQ_PARA (CPUFREQ_PARA | 0x03)
> #define GET_CPUFREQ_AVGFREQ (CPUFREQ_PARA | 0x04)
> #define SET_CPUFREQ_CPPC (CPUFREQ_PARA | 0x05)
> + #define GET_CPUFREQ_CPPC (CPUFREQ_PARA | 0x06)
>
> /* set/reset scheduler power saving option */
> #define XEN_SYSCTL_pm_op_set_sched_opt_smt 0x21
> @@ -517,6 +517,7 @@ struct xen_sysctl_pm_op {
> uint32_t cpuid;
> union {
> struct xen_get_cpufreq_para get_para;
> + struct xen_cppc_para cppc_para;
> struct xen_set_cpufreq_gov set_gov;
> struct xen_set_cpufreq_para set_para;
> struct xen_set_cppc_para set_cppc;
This (tools-only) public interface change, otoh, may be okay to do.
Jan
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, June 17, 2025 6:08 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; Anthony PERARD
> <anthony.perard@vates.tech>; Juergen Gross <jgross@suse.com>; Andrew
> Cooper <andrew.cooper3@citrix.com>; Orzel, Michal <Michal.Orzel@amd.com>;
> Julien Grall <julien@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; Stefano
> Stabellini <sstabellini@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. 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?
>
>
> Jan
On 04.07.2025 10:13, Penny, Zheng wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Tuesday, June 17, 2025 6:08 PM
>> To: Penny, Zheng <penny.zheng@amd.com>
>> Cc: Huang, Ray <Ray.Huang@amd.com>; Anthony PERARD
>> <anthony.perard@vates.tech>; Juergen Gross <jgross@suse.com>; Andrew
>> Cooper <andrew.cooper3@citrix.com>; Orzel, Michal <Michal.Orzel@amd.com>;
>> Julien Grall <julien@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; Stefano
>> Stabellini <sstabellini@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
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, July 4, 2025 4:33 PM
> To: Penny, Zheng <penny.zheng@amd.com>; Andryuk, Jason
> <Jason.Andryuk@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; Anthony PERARD
> <anthony.perard@vates.tech>; Juergen Gross <jgross@suse.com>; Andrew
> Cooper <andrew.cooper3@citrix.com>; Orzel, Michal <Michal.Orzel@amd.com>;
> Julien Grall <julien@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; Stefano
> Stabellini <sstabellini@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 <jbeulich@suse.com>
> >> Sent: Tuesday, June 17, 2025 6:08 PM
> >> To: Penny, Zheng <penny.zheng@amd.com>
> >> Cc: Huang, Ray <Ray.Huang@amd.com>; Anthony PERARD
> >> <anthony.perard@vates.tech>; Juergen Gross <jgross@suse.com>; Andrew
> >> Cooper <andrew.cooper3@citrix.com>; Orzel, Michal
> >> <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau
> >> Monné <roger.pau@citrix.com>; Stefano Stabellini
> >> <sstabellini@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
On 04.07.2025 10:56, Penny, Zheng wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Friday, July 4, 2025 4:33 PM
>> To: Penny, Zheng <penny.zheng@amd.com>; Andryuk, Jason
>> <Jason.Andryuk@amd.com>
>> Cc: Huang, Ray <Ray.Huang@amd.com>; Anthony PERARD
>> <anthony.perard@vates.tech>; Juergen Gross <jgross@suse.com>; Andrew
>> Cooper <andrew.cooper3@citrix.com>; Orzel, Michal <Michal.Orzel@amd.com>;
>> Julien Grall <julien@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; Stefano
>> Stabellini <sstabellini@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 <jbeulich@suse.com>
>>>> Sent: Tuesday, June 17, 2025 6:08 PM
>>>> To: Penny, Zheng <penny.zheng@amd.com>
>>>> Cc: Huang, Ray <Ray.Huang@amd.com>; Anthony PERARD
>>>> <anthony.perard@vates.tech>; Juergen Gross <jgross@suse.com>; Andrew
>>>> Cooper <andrew.cooper3@citrix.com>; Orzel, Michal
>>>> <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau
>>>> Monné <roger.pau@citrix.com>; Stefano Stabellini
>>>> <sstabellini@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
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, July 4, 2025 5:46 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; Anthony PERARD
> <anthony.perard@vates.tech>; Juergen Gross <jgross@suse.com>; Andrew
> Cooper <andrew.cooper3@citrix.com>; Orzel, Michal <Michal.Orzel@amd.com>;
> Julien Grall <julien@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; Stefano
> Stabellini <sstabellini@kernel.org>; xen-devel@lists.xenproject.org; Andryuk, Jason
> <Jason.Andryuk@amd.com>
> Subject: Re: [PATCH v5 14/18] xen/cpufreq: introduce GET_CPUFREQ_CPPC sub-
> cmd
>
> On 04.07.2025 10:56, Penny, Zheng wrote:
> > [Public]
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Friday, July 4, 2025 4:33 PM
> >> To: Penny, Zheng <penny.zheng@amd.com>; Andryuk, Jason
> >> <Jason.Andryuk@amd.com>
> >> Cc: Huang, Ray <Ray.Huang@amd.com>; Anthony PERARD
> >> <anthony.perard@vates.tech>; Juergen Gross <jgross@suse.com>; Andrew
> >> Cooper <andrew.cooper3@citrix.com>; Orzel, Michal
> >> <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau
> >> Monné <roger.pau@citrix.com>; Stefano Stabellini
> >> <sstabellini@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 <jbeulich@suse.com>
> >>>> Sent: Tuesday, June 17, 2025 6:08 PM
> >>>> To: Penny, Zheng <penny.zheng@amd.com>
> >>>> Cc: Huang, Ray <Ray.Huang@amd.com>; Anthony PERARD
> >>>> <anthony.perard@vates.tech>; Juergen Gross <jgross@suse.com>;
> >>>> Andrew Cooper <andrew.cooper3@citrix.com>; Orzel, Michal
> >>>> <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau
> >>>> Monné <roger.pau@citrix.com>; Stefano Stabellini
> >>>> <sstabellini@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.)
>
Okay, I only explained why I couldn't use one command "get-cpufreq-para" to output all info
> Jan
On 2025-07-06 23:31, Penny, Zheng wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Friday, July 4, 2025 5:46 PM
>> To: Penny, Zheng <penny.zheng@amd.com>
>> Cc: Huang, Ray <Ray.Huang@amd.com>; Anthony PERARD
>> <anthony.perard@vates.tech>; Juergen Gross <jgross@suse.com>; Andrew
>> Cooper <andrew.cooper3@citrix.com>; Orzel, Michal <Michal.Orzel@amd.com>;
>> Julien Grall <julien@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; Stefano
>> Stabellini <sstabellini@kernel.org>; xen-devel@lists.xenproject.org; Andryuk, Jason
>> <Jason.Andryuk@amd.com>
>> Subject: Re: [PATCH v5 14/18] xen/cpufreq: introduce GET_CPUFREQ_CPPC sub-
>> cmd
>>
>> On 04.07.2025 10:56, Penny, Zheng wrote:
>>> [Public]
>>>
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: Friday, July 4, 2025 4:33 PM
>>>> To: Penny, Zheng <penny.zheng@amd.com>; Andryuk, Jason
>>>> <Jason.Andryuk@amd.com>
>>>> Cc: Huang, Ray <Ray.Huang@amd.com>; Anthony PERARD
>>>> <anthony.perard@vates.tech>; Juergen Gross <jgross@suse.com>; Andrew
>>>> Cooper <andrew.cooper3@citrix.com>; Orzel, Michal
>>>> <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau
>>>> Monné <roger.pau@citrix.com>; Stefano Stabellini
>>>> <sstabellini@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 <jbeulich@suse.com>
>>>>>> Sent: Tuesday, June 17, 2025 6:08 PM
>>>>>> To: Penny, Zheng <penny.zheng@amd.com>
>>>>>> Cc: Huang, Ray <Ray.Huang@amd.com>; Anthony PERARD
>>>>>> <anthony.perard@vates.tech>; Juergen Gross <jgross@suse.com>;
>>>>>> Andrew Cooper <andrew.cooper3@citrix.com>; Orzel, Michal
>>>>>> <Michal.Orzel@amd.com>; Julien Grall <julien@xen.org>; Roger Pau
>>>>>> Monné <roger.pau@citrix.com>; Stefano Stabellini
>>>>>> <sstabellini@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.)
>>
>
> Okay, I only explained why I couldn't use one command "get-cpufreq-para" to output all info
I think Jan's suggestion to make `xenpm get-cpufreq-para` keep
outputting cppc/hwp information is good. That way users get the cpufreq
information without having to know what driver is running. So you will
have to issue the additional hypercall as necessary to retrieve the data.
Regards,
Jason
On 04.07.2025 10:32, Jan Beulich wrote:
> On 04.07.2025 10:13, Penny, Zheng wrote:
>>> -----Original Message-----
>>> From: Jan Beulich <jbeulich@suse.com>
>>> Sent: Tuesday, June 17, 2025 6:08 PM
>>>
>>> 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.
But, ftaod, duplicate code producing the same information is. Such code would
want breaking out into a helper function then. (And yet further ftaod, if the
code only produces identical information, but from different input structures,
such breaking out of course wouldn't normally be an option.)
Jan
© 2016 - 2025 Red Hat, Inc.