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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.02.2025 09:32, Penny Zheng wrote:
> @@ -258,14 +259,27 @@ static void amd_cppc_write_request_msrs(void *info)
>  }
>  
>  static int cf_check amd_cppc_write_request(int cpu, uint8_t min_perf,
> -                                           uint8_t des_perf, uint8_t 
> max_perf)
> +                                           uint8_t des_perf, uint8_t 
> max_perf,
> +                                           int epp)
>  {
>      struct amd_cppc_drv_data *data = per_cpu(amd_cppc_drv_data, cpu);
>      uint64_t prev = data->req.raw;
>  
>      data->req.min_perf = min_perf;
>      data->req.max_perf = max_perf;
> -    data->req.des_perf = des_perf;
> +    if ( !opt_cpufreq_active )
> +        data->req.des_perf = des_perf;
> +    else
> +    {
> +        data->req.des_perf = 0;
> +        /* Get pre-defined BIOS value */
> +        if ( epp < 0 )
> +            data->req.epp = epp_init;

Instead of passing in a negative value, could't the caller pass epp_init,
allowing the function parameter to be of an unsigned type?

> @@ -292,7 +306,25 @@ static int cf_check amd_cppc_cpufreq_target(struct 
> cpufreq_policy *policy,
>          return res;
>  
>      return amd_cppc_write_request(policy->cpu, 
> data->caps.lowest_nonlinear_perf,
> -                                  des_perf, data->caps.highest_perf);
> +                                  des_perf, data->caps.highest_perf, -1);
> +}
> +
> +static int read_epp_init_once(void)
> +{
> +    uint64_t val;
> +    static bool read_once = false;
> +
> +    if ( !read_once )
> +    {
> +        if ( rdmsr_safe(MSR_AMD_CPPC_REQ, val) )
> +            return -EINVAL;
> +        epp_init = (val >> 24) & 0xFF;
> +
> +        /* Read pre-defined BIOS value once */
> +        read_once = true;
> +    }
> +
> +    return 0;
>  }

And all processors are (silently) assumed to have been configured the same?

> @@ -381,7 +413,8 @@ static void cf_check amd_cppc_init_msrs(void *info)
>      data->nominal_freq = nominal_freq;
>      data->max_freq = max_freq;
>  
> -    return;
> +    if ( !read_epp_init_once() )
> +        return;

With this kind of a caller the function ought to return bool. If the function
returns an error code, it should not be lost here.

> @@ -443,6 +487,52 @@ static int cf_check amd_cppc_cpufreq_cpu_exit(struct 
> cpufreq_policy *policy)
>      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_parse_policy(policy->governor);
> +
> +    amd_cppc_verbose("CPU %u initialized with amd-cppc active mode\n", 
> policy->cpu);
> +
> +    return 0;
> +}
> +
> +static int amd_cppc_epp_update_limit(const 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, des_perf;
> +    int epp = -1;
> +
> +    /* Initial min/max values for CPPC Performance Controls Register */
> +    max_perf = data->caps.highest_perf;
> +    min_perf = data->caps.lowest_perf;
> +
> +    /* CPPC EPP feature require to set zero to the desire perf bit */
> +    des_perf = 0;

Then why the local variable? Can't you pass ...

> +    if ( policy->policy == CPUFREQ_POLICY_PERFORMANCE )
> +    {
> +        /* Force the epp value to be zero for performance policy */
> +        epp = CPPC_ENERGY_PERF_MAX_PERFORMANCE;
> +        min_perf = max_perf;
> +    }
> +    else if ( policy->policy == CPUFREQ_POLICY_POWERSAVE )
> +        /* Force the epp value to be 0xff for powersave policy */
> +        epp = CPPC_ENERGY_PERF_MAX_POWERSAVE;
> +
> +    return amd_cppc_write_request(policy->cpu, min_perf, des_perf, max_perf, 
> epp);

... 0 here (if need be with a /* des_perf */ comment)?

The line (nit) is too long anyway, and hence needs wrapping no matter what.

> @@ -452,10 +542,22 @@ static const struct cpufreq_driver 
> __initconst_cf_clobber amd_cppc_cpufreq_drive
>      .exit   = amd_cppc_cpufreq_cpu_exit,
>  };
>  
> +static const struct cpufreq_driver  __initconst_cf_clobber 
> amd_cppc_epp_driver =
> +{
> +    .name       = XEN_AMD_CPPC_EPP_DRIVER_NAME,
> +    .verify     = amd_cppc_cpufreq_verify,
> +    .setpolicy  = amd_cppc_epp_set_policy,
> +    .init       = amd_cppc_epp_cpu_init,
> +    .exit       = amd_cppc_cpufreq_cpu_exit,
> +};
> +
>  int __init amd_cppc_register_driver(void)
>  {
>      if ( !cpu_has_cppc )
>          return -ENODEV;
>  
> -    return cpufreq_register_driver(&amd_cppc_cpufreq_driver);
> +    if ( !opt_cpufreq_active )
> +        return cpufreq_register_driver(&amd_cppc_cpufreq_driver);
> +    else
> +        return cpufreq_register_driver(&amd_cppc_epp_driver);
>  }

I'm personally opposed to this style of coding, and you ...

> --- a/xen/drivers/cpufreq/utility.c
> +++ b/xen/drivers/cpufreq/utility.c
> @@ -484,3 +484,14 @@ int __cpufreq_set_policy(struct cpufreq_policy *data,
>  
>      return __cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
>  }
> +
> +unsigned int cpufreq_parse_policy(const struct cpufreq_governor *gov)
> +{
> +    if ( !strncasecmp(gov->name, "performance", CPUFREQ_NAME_LEN) )
> +        return CPUFREQ_POLICY_PERFORMANCE;
> +
> +    if ( !strncasecmp(gov->name, "powersave", CPUFREQ_NAME_LEN) )
> +        return CPUFREQ_POLICY_POWERSAVE;
> +
> +    return CPUFREQ_POLICY_UNKNOWN;
> +}

... aren't doing that consistently anyway. May I ask that you drop the "else"
there, or perhaps switch to using a conditional operator?

> --- a/xen/include/acpi/cpufreq/cpufreq.h
> +++ b/xen/include/acpi/cpufreq/cpufreq.h
> @@ -83,6 +83,7 @@ struct cpufreq_policy {
>      int8_t              turbo;  /* tristate flag: 0 for unsupported
>                                   * -1 for disable, 1 for enabled
>                                   * See CPUFREQ_TURBO_* below for defines */
> +    unsigned int        policy;

This new field wants connecting, by way of a comment perhaps, to ...

> @@ -133,6 +134,17 @@ extern int cpufreq_register_governor(struct 
> cpufreq_governor *governor);
>  extern struct cpufreq_governor *__find_governor(const char *governor);
>  #define CPUFREQ_DEFAULT_GOVERNOR &cpufreq_gov_dbs
>  
> +#define CPUFREQ_POLICY_UNKNOWN      0
> +/*
> + * If cpufreq_driver->target() exists, the ->governor decides what frequency
> + * within the limits is used. If cpufreq_driver->setpolicy() exists, these
> + * two generic policies are available:
> + */
> +#define CPUFREQ_POLICY_POWERSAVE    1
> +#define CPUFREQ_POLICY_PERFORMANCE  2

... the values to be put there, which - yes - ...

> +unsigned int cpufreq_parse_policy(const struct cpufreq_governor *gov);

... are returned by this function. Such a comment could be as simple as
/* CPUFREQ_POLICY_* */

Jan