On 11.07.2025 05:51, Penny Zheng wrote:
> 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 need to be printed together via xenpm tool.
>
> If we tried to still put 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.
> So we introduce a new sub-op GET_CPUFREQ_CPPC to specifically print
> CPPC-related para.
>
> Signed-off-by: Penny Zheng <penny.zh...@amd.com>
> ---
> v4 -> v5:
> - new commit
> ---
> v5 -> v6:
> - remove the changes for get-cpufreq-para
> ---
> tools/include/xenctrl.h | 2 ++
> tools/libs/ctrl/xc_pm.c | 27 +++++++++++++++++++++
> tools/misc/xenpm.c | 47 +++++++++++++++++++++++++++++++++++++
> xen/drivers/acpi/pm-op.c | 4 ++++
> xen/include/public/sysctl.h | 2 ++
> 5 files changed, 82 insertions(+)
>
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 965d3b585a..699243c4df 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -1953,6 +1953,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 6fda973f1f..3f72152617 100644
> --- a/tools/libs/ctrl/xc_pm.c
> +++ b/tools/libs/ctrl/xc_pm.c
> @@ -366,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_get_cppc_para *sys_cppc_para = &sysctl.u.pm_op.u.get_cppc;
> +
> + 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 120e9eae22..bdc09f468a 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"
> @@ -996,6 +997,51 @@ void cpufreq_para_func(int argc, char *argv[])
> show_cpufreq_para_by_cpuid(xc_handle, cpuid);
> }
>
> +/* 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 )
> + {
> + printf("cpu id : %d\n", cpuid);
> + print_cppc_para(cpuid, &cppc_para);
> + printf("\n");
> + }
> + 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 )
> + {
> + unsigned int i;
> +
> + /* show cpu cppc information on all cpus */
> + for ( i = 0; i < max_cpu_nr; i++ )
> + /* Bail out only on unsupported platform */
> + 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;
> @@ -1576,6 +1622,7 @@ struct {
> { "get-cpufreq-average", cpufreq_func },
> { "start", start_gather_func },
> { "get-cpufreq-para", cpufreq_para_func },
> + { "get-cpufreq-cppc", cppc_para_func },
Didn't Jason also suggest that we would better not introduce a new command, but
rather make get-cpufreq-para invoke GET_CPUFREQ_CPPC as needed? Considering that
as per patch 15 the same information is already printed, I think I'm a little
lost with the need for this separate operation (and command), and then also with
the need for patch 15.
Jan
