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

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

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, May 13, 2025 4:03 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 09.05.2025 08:36, Penny, Zheng wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: Wednesday, April 30, 2025 9:55 PM
> >>
> >> 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.
>
> That's all fine, but completely misses the point of my question: If the 
> governor is
> relevant, why would you bypass respective printing?
>

The only useful info in governor for epp mode, IMO, is its name.
I deduce which performance policy user wants to apply through which governor 
they choose.
If user chooses performance governor, they want maximum performance.
If user chooses powersave governor, they want the least power consumption
I could only provide appropriate epp value in above two scenarios.
ondemand and userspace are forbidden choices, and if users specify such options 
in cmdline,
I shall give warning message to say  that amd-cppc in active mode is 
governor-less, and users could
set epp values on runtime to specify bias towards performance or efficiency.

If above is messy, I could also totally forbid governor thing for active 
mode... wdyt?


> > 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
>
> Medium one may be okay-ish, but it's not really an appropriate fit. We may 
> want to
> at least consider rejecting the use of ondemand with the EPP driver.
> That, however, heavily depends on how hardware would behave when using the
> medium value.
>
> >>> --- 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?
>
> Well, if you set hw_auto first, then you can use that variable plus one more 
> strcmp()
> to set cppc_mode.
>
> Jan