Re: [PATCH v4 12/15] tools/xenpm: Print CPPC parameters for amd-cppc driver

[Jan Beulich <jbeulich@xxxxxxxx>]

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