Re: [PATCH v6 13/19] xen/x86: implement amd-cppc-epp driver for CPPC in active mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.07.2025 05:51, Penny Zheng wrote:
> --- a/xen/arch/x86/acpi/cpufreq/amd-cppc.c
> +++ b/xen/arch/x86/acpi/cpufreq/amd-cppc.c
> @@ -67,7 +67,14 @@
>   * max_perf.
>   * Field des_perf conveys performance level Xen governor is requesting. And 
> it
>   * may be set to any performance value in the range [min_perf, max_perf],
> - * inclusive.
> + * inclusive. In active mode, desf_perf must be zero.

Nit (typo): des_perf

> @@ -259,11 +276,18 @@ static void cf_check amd_cppc_write_request_msrs(void 
> *info)
>  }
>  
>  static void amd_cppc_write_request(unsigned int cpu, uint8_t min_perf,
> -                                   uint8_t des_perf, uint8_t max_perf)
> +                                   uint8_t des_perf, uint8_t max_perf,
> +                                   uint8_t epp)
>  {
>      struct amd_cppc_drv_data *data = per_cpu(amd_cppc_drv_data, cpu);
>      uint64_t prev = data->req.raw;
>  
> +    if ( !opt_active_mode )
> +        data->req.des_perf = des_perf;
> +    else
> +        data->req.des_perf = 0;

In amd_cppc_epp_set_policy() you pass 0 anyway. Why is this needed? With this
change dropped, opt_active_mode can become __initdata. (But of course you may
want to add an assertion instead, in which case the variable needs to stay
where it is at least in debug builds.)

> +    data->req.epp = epp;

Ahead of this patch, aren't you mis-handling this field then, in that you
clear it (as you never read the MSR)?

>      data->req.min_perf = min_perf;
>      data->req.max_perf = max_perf;
>      data->req.des_perf = des_perf;

Don't you need to delete this line with the addition above, or alternatively
change the above to

    if ( opt_active_mode )
        data->req.des_perf = 0;

?

> @@ -274,6 +298,14 @@ static void amd_cppc_write_request(unsigned int cpu, 
> uint8_t min_perf,
>      on_selected_cpus(cpumask_of(cpu), amd_cppc_write_request_msrs, data, 1);
>  }
>  
> +static void read_epp_init(void)
> +{
> +    uint64_t val;
> +
> +    rdmsrl(MSR_AMD_CPPC_REQ, val);
> +    this_cpu(epp_init) = MASK_EXTR(val, AMD_CPPC_EPP_MASK);
> +}

I'm unconvinced this is worth a separate function.

> +static int cf_check amd_cppc_epp_set_policy(struct cpufreq_policy *policy)
> +{
> +    const struct amd_cppc_drv_data *data = per_cpu(amd_cppc_drv_data,
> +                                                   policy->cpu);
> +    uint8_t max_perf, min_perf, epp;
> +
> +    /*
> +     * On default, set min_perf with lowest_nonlinear_perf, and max_perf
> +     * with the highest, to ensure performance scaling in P-states range.
> +     */
> +    max_perf = data->caps.highest_perf;
> +    min_perf = data->caps.lowest_nonlinear_perf;
> +
> +    /*
> +     * In policy CPUFREQ_POLICY_PERFORMANCE, increase min_perf to
> +     * highest_perf to achieve ultmost performance.
> +     * In policy CPUFREQ_POLICY_POWERSAVE, decrease max_perf to
> +     * lowest_nonlinear_perf to achieve ultmost power saving.
> +     */
> +    switch ( policy->policy )
> +    {
> +    case CPUFREQ_POLICY_PERFORMANCE:
> +        /* Force the epp value to be zero for performance policy */
> +        epp = CPPC_ENERGY_PERF_MAX_PERFORMANCE;
> +        min_perf = data->caps.highest_perf;

Use the local variable you have, i.e. max_perf?

> +        break;
> +
> +    case CPUFREQ_POLICY_POWERSAVE:
> +        /* Force the epp value to be 0xff for powersave policy */
> +        epp = CPPC_ENERGY_PERF_MAX_POWERSAVE;
> +        max_perf = data->caps.lowest_nonlinear_perf;

Use the local variable you have, i.e. min_perf?

> --- a/xen/arch/x86/include/asm/msr-index.h
> +++ b/xen/arch/x86/include/asm/msr-index.h
> @@ -245,6 +245,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)

The reason I noticed the EPP issue in amd_cppc_write_request() is because
I wondered why you would need this, when you have the fields defined in
struct amd_cppc_drv_data.

Jan