RE: [PATCH v1 08/11] x86/cpufreq: add "cpufreq=amd-pstate,active" para

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

[AMD Official Use Only - AMD Internal Distribution Only]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Thursday, January 9, 2025 7:24 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Stabellini, Stefano <stefano.stabellini@xxxxxxx>; Huang, Ray
> <Ray.Huang@xxxxxxx>; Ragiadakou, Xenia <Xenia.Ragiadakou@xxxxxxx>;
> Andryuk, Jason <Jason.Andryuk@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v1 08/11] x86/cpufreq: add "cpufreq=amd-pstate,active" 
> para
>
> On 03.12.2024 09:11, Penny Zheng wrote:
> > From: Penny Zheng <penny.zheng@xxxxxxx>
> >
> > The amd-pstate driver may support multiple working modes, passive and 
> > active.
> >
> > Introduce a new variable to keep track of which mode is currently enabled.
> > This variable will also help to choose which cpufreq driver to be 
> > registered.
> >
> > Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
> > ---
> >  docs/misc/xen-command-line.pandoc      |  9 ++++++++-
> >  xen/arch/x86/acpi/cpufreq/amd-pstate.c | 12 +++++++++++-
> >  2 files changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/docs/misc/xen-command-line.pandoc
> > b/docs/misc/xen-command-line.pandoc
> > index 30f855fa18..a9a3e0ef79 100644
> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -499,7 +499,8 @@ If set, force use of the performance counters for
> > oprofile, rather than detectin  available support.
> >
> >  ### cpufreq
> > -> `= none | {{ <boolean> | xen } {
> > -> [:[powersave|performance|ondemand|userspace][,[<maxfreq>]][,[<minfr
> > -> eq>]]] } [,verbose]} | dom0-kernel | hwp[:[<hdc>][,verbose]] |
> > -> amd-pstate[:[verbose]]`
> > +> `= none | {{ <boolean> | xen } {
> > +> [:[powersave|performance|ondemand|userspace][,[<maxfreq>]][,[<minfr
> > +> eq>]]] }
> > +[,verbose]} | dom0-kernel | hwp[:[<hdc>][,verbose]] |
> > +amd-pstate[:[active][,verbose]]`
> >
> >  > Default: `xen`
> >
> > @@ -522,6 +523,12 @@ choice of `dom0-kernel` is deprecated and not
> supported by all Dom0 kernels.
> >    on supported AMD hardware to provide a finer grained frequency control
> mechanism.
> >    The default is disabled. If `amd-pstate` is selected, but hardware 
> > support
> >    is not available, Xen will fallback to cpufreq=xen.
> > +* `active` is a boolean to enable amd-pstate driver in active(autonomous) 
> > mode.
> > +  In this mode, users could provide a hint with energy performance
> > +preference
> > +  register to the hardware if they want to bias toward
> > +performance(0x0) or
> > +  energy efficiency(0xff), then CPPC power algorithm will calculate
> > +the runtime
> > +  workload and adjust the realtime cores frequency according to the
> > +power supply
> > +  and themal, core voltage and some other hardware conditions.
>
> Nit: thermal
>
> > --- a/xen/arch/x86/acpi/cpufreq/amd-pstate.c
> > +++ b/xen/arch/x86/acpi/cpufreq/amd-pstate.c
> > @@ -27,6 +27,8 @@
> >  #define amd_pstate_warn(fmt, args...) \
> >      printk(XENLOG_WARNING "AMD_PSTATE: CPU%u warning: " fmt, cpu, ##
> > args)
> >
> > +static bool __ro_after_init opt_cpufreq_active = false;
>
> Pointless initializer.
>
> > @@ -398,5 +407,6 @@ int __init amd_pstate_register_driver(void)
> >      if ( !cpu_has_cppc )
> >          return -ENODEV;
> >
> > -    return cpufreq_register_driver(&amd_pstate_cpufreq_driver);
> > +    if ( !opt_cpufreq_active )
> > +        return cpufreq_register_driver(&amd_pstate_cpufreq_driver);
> >  }
>
> I'm afraid the description is of no help in determining why this is a correct 
> change to
> make (here). How would the user provided hint (see cmdline option 
> description) be
> communicated to hardware when the driver isn't even registered?
>

Maybe I shall combine this commit into the next one about implementing epp 
driver for active mode,
and the changes will be like:
  -    return cpufreq_register_driver(&amd_pstate_cpufreq_driver);
 +    if ( opt_cpufreq_active )
 +        return cpufreq_register_driver(&&amd_cppc_epp_driver);
 +    else
 +        return cpufreq_register_driver(&amd_cppc_cpufreq_driver);
Now, the description makes more sense.

> Finally I don't think the change above would build, as it leaves a return 
> from the
> function without return value.
>
> Jan

Many thanks
Penny