On 14.04.2025 09:40, Penny Zheng wrote:
> HWP, amd-cppc, amd-cppc-epp are all the implementation
> of ACPI CPPC (Collaborative Processor Performace Control),
> so we introduce cppc_mode flag to print CPPC-related para.
>
> And HWP and amd-cppc-epp are both governor-less driver,
> so we introduce hw_auto flag to bypass governor-related print.
But in the EPP driver you use the information which governor is active.
> --- a/tools/misc/xenpm.c
> +++ b/tools/misc/xenpm.c
> @@ -790,9 +790,18 @@ static unsigned int calculate_activity_window(const
> xc_cppc_para_t *cppc,
> /* print out parameters about cpu frequency */
> static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para
> *p_cpufreq)
> {
> - bool hwp = strcmp(p_cpufreq->scaling_driver, XEN_HWP_DRIVER_NAME) == 0;
> + bool cppc_mode = false, hw_auto = false;
> int i;
>
> + if ( !strcmp(p_cpufreq->scaling_driver, XEN_HWP_DRIVER_NAME) ||
> + !strcmp(p_cpufreq->scaling_driver, XEN_AMD_CPPC_DRIVER_NAME) ||
> + !strcmp(p_cpufreq->scaling_driver, XEN_AMD_CPPC_EPP_DRIVER_NAME) )
> + cppc_mode = true;
> +
> + if ( !strcmp(p_cpufreq->scaling_driver, XEN_HWP_DRIVER_NAME) ||
> + !strcmp(p_cpufreq->scaling_driver, XEN_AMD_CPPC_EPP_DRIVER_NAME) )
> + hw_auto = true;
Please avoid doing the same strcmp()s twice. There are several ways how to, so
I'm not going to make a particular suggestion.
> @@ -800,7 +809,7 @@ static void print_cpufreq_para(int cpuid, struct
> xc_get_cpufreq_para *p_cpufreq)
> printf(" %d", p_cpufreq->affected_cpus[i]);
> printf("\n");
>
> - if ( hwp )
> + if ( hw_auto )
> printf("cpuinfo frequency : base [%"PRIu32"] max [%"PRIu32"]\n",
> p_cpufreq->cpuinfo_min_freq,
> p_cpufreq->cpuinfo_max_freq);
> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -201,7 +201,7 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
> pmpt = processor_pminfo[op->cpuid];
> policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
>
> - if ( !pmpt || !pmpt->perf.states ||
> + if ( !pmpt || ((pmpt->init & XEN_PX_INIT) && !pmpt->perf.states) ||
> !policy || !policy->governor )
> return -EINVAL;
This looks questionable all on its own. Where is it that ->perf.states
allocation
is being avoided? I first thought it might be patch 06 which is related, but
that
doesn't look to be it. In any event further down from here there is
for ( i = 0; i < op->u.get_para.freq_num; i++ )
data[i] = pmpt->perf.states[i].core_frequency * 1000;
i.e. an access to the array solely based on hypercall input.
Both this and ...
> @@ -461,9 +461,10 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
> switch ( op->cmd & PM_PARA_CATEGORY_MASK )
> {
> case CPUFREQ_PARA:
> - if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_PX) )
> + if ( !(xen_processor_pmbits & (XEN_PROCESSOR_PM_PX |
> + XEN_PROCESSOR_PM_CPPC)) )
> return -ENODEV;
> - if ( !pmpt || !(pmpt->init & XEN_PX_INIT) )
> + if ( !pmpt || !(pmpt->init & (XEN_PX_INIT | XEN_CPPC_INIT)) )
> return -EINVAL;
> break;
> }
... this hunk also look as if they would belong (partly?) in maybe patch 03?
Even more so as per the title this is solely a tool stack (xenpm) change.
Jan
