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

[Jan Beulich <jbeulich@xxxxxxxx>]

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>]][,[<minfreq>]]] } 
> [,verbose]} | dom0-kernel | hwp[:[<hdc>][,verbose]] | amd-pstate[:[verbose]]`
> +> `= none | {{ <boolean> | xen } { 
> [:[powersave|performance|ondemand|userspace][,[<maxfreq>]][,[<minfreq>]]] }
> +[,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?

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

Jan