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

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Wednesday, April 30, 2025 9:55 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Anthony PERARD
> <anthony.perard@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4 12/15] tools/xenpm: Print CPPC parameters for amd-cppc
> driver
>
> 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.
>

We want to have a one-one mapping between governor and epp value, such as,
If users choose performance governor, no matter via "xenpm" or cmdline, users 
want maximum performance,
We set epp with 0 to meet the expectation.
And if users choose powersave governor, users want the least power consumption, 
then we shall set
epp with 255 to meet the expectation.
Ondemand is a tricky part, hmmmm, I don't know which value is suitable for it, 
the medium one? So I neglect it in the first place
I'll add above explanation in commit which introduces 
CPUFREQ_POLICY_POWERSAVE/PERFORMANCE

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

Maybe we shall use switch-case() to replace the same strcmp()s
Since it's not easy to switch-case() string value, I had a draft idea to 
include an new entry in "struct xen_cppc_para",
See:
```
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index fa431fd983..b872f1b66a 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -308,6 +308,10 @@ struct xen_ondemand {

 struct xen_cppc_para {
     /* OUT */
+#define XEN_SYSCTL_CPPC_VENDOR_HWP      1
+#define XEN_SYSCTL_CPPC_VENDOR_AMD      2
+#define XEN_SYSCTL_CPPC_VENDOR_AMD_EPP  3
+    uint8_t vendor;
     /* activity_window supported if set */
 #define XEN_SYSCTL_CPPC_FEAT_ACT_WINDOW  (1 << 0)
     uint32_t features; /* bit flags for features */

```
A new vendor filed in struct xen_cppc_para could help us differ the underlying 
implementation.
Or any better suggestions?

> > @@ -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
>

->perf.states is allocated in set_px_pminfo()
It is a px-specific function.

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

I'll guard it with pmpt->init & XEN_PX_INIT too

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

True, I shall move them to 03, to let this commit being solely a tool stack 
(xenpm) change
> Jan