Re: [PATCH v4 11/15] xen/x86: implement EPP support for the amd-cppc driver in active mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2025 09:40, Penny Zheng wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -515,7 +515,7 @@ 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-cppc[:[verbose]]`
> +> `= none | {{ <boolean> | xen } { 
> [:[powersave|performance|ondemand|userspace][,[<maxfreq>]][,[<minfreq>]]] } 
> [,verbose]} | dom0-kernel | hwp[:[<hdc>][,verbose]] | 
> amd-cppc[:[active][,verbose]]`
>  
>  > Default: `xen`
>  
> @@ -537,6 +537,12 @@ choice of `dom0-kernel` is deprecated and not supported 
> by all Dom0 kernels.
>  * `amd-cppc` selects ACPI Collaborative Performance and Power Control (CPPC)
>    on supported AMD hardware to provide finer grained frequency control
>    mechanism. The default is disabled.
> +* `active` is to enable amd-cppc driver in active(autonomous) mode. In this
> +  mode, users could write to energy performance preference register(epp) to
> +  tell hardware if they want to bias toward performance or energy efficiency.
> +  Then built-in CPPC power algorithm will calculate the runtime workload and
> +  adjust cores frequency automatically according to the power supply, 
> thermal,
> +  core voltage and some other hardware conditions.

This section of the cmdline doc may not be of the best quality, but may I
nevertheless ask that for this new sub-option you make explicit what the
default is (and that it's of boolean nature)?

> @@ -434,12 +464,82 @@ static int cf_check amd_cppc_cpufreq_cpu_init(struct 
> cpufreq_policy *policy)
>  
>      amd_cppc_boost_init(policy, data);
>  
> +    return 0;
> +}
> +
> +static int cf_check amd_cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
> +{
> +    int ret;
> +
> +    ret = amd_cppc_cpufreq_init_perf(policy);
> +    if ( ret )
> +        return ret;
> +
>      amd_cppc_verbose(policy->cpu,
>                       "CPU initialized with amd-cppc passive mode\n");
>  
>      return 0;
>  }
>  
> +static int cf_check amd_cppc_epp_cpu_init(struct cpufreq_policy *policy)
> +{
> +    int ret;
> +
> +    ret = amd_cppc_cpufreq_init_perf(policy);
> +    if ( ret )
> +        return ret;
> +
> +    policy->policy = cpufreq_policy_from_governor(policy->governor);

This is the init part, which is fine. What if the governor is being changed,
though? The struct field is new, and there's no other place I can spot where
it would be adjusted.

> --- a/xen/arch/x86/include/asm/msr-index.h
> +++ b/xen/arch/x86/include/asm/msr-index.h
> @@ -242,6 +242,7 @@
>  #define MSR_AMD_CPPC_ENABLE                 0xc00102b1U
>  #define  AMD_CPPC_ENABLE                    (_AC(1, ULL) <<  0)
>  #define MSR_AMD_CPPC_REQ                    0xc00102b3U
> +#define  AMD_CPPC_EPP_MASK                  (_AC(0xff, ULL) <<  24)

Nit: Excess blank after << (there's no such issue with AMD_CPPC_ENABLE,
ftaod).

Jan