Re: [PATCH v1 09/11] xen/x86: implement EPP support for the AMD processors

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.12.2024 09:11, Penny Zheng wrote:
> @@ -223,14 +227,29 @@ static void amd_pstate_write_request_msrs(void *info)
>  }
>  
>  static int cf_check amd_pstate_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_pstate_drv_data *data = per_cpu(amd_pstate_drv_data, cpu);
> -    uint64_t prev = data->amd_req;
> +    uint64_t prev = data->amd_req, val;
>  
>      data->req.min_perf = min_perf;
>      data->req.max_perf = max_perf;
> -    data->req.des_perf = des_perf;
> +    if ( !epp_mode )
> +        data->req.des_perf = des_perf;
> +    else
> +    {
> +        data->req.des_perf = 0;
> +        /* Get pre-defined BIOS value */
> +        if ( epp < 0 )
> +        {
> +            if ( rdmsr_safe(MSR_AMD_CPPC_REQ, val) )
> +                return -EINVAL;
> +            data->req.epp = (val >> 24) & 0xFF;

This reading may better live in the sole caller where it's relevant. Plus
it might be yet better if this was read just once, assuming "pre-defined
BIOS value" is what it says (and hence doesn't change during the runtime
of the system).

> @@ -257,7 +276,7 @@ static int cf_check amd_pstate_cpufreq_target(struct 
> cpufreq_policy *policy,
>      min_perf = data->hw.lowest_nonlinear_perf;
>      des_perf = amd_pstate_khz_to_perf(data, target_freq);
>  
> -    return amd_pstate_write_request(policy->cpu, min_perf, des_perf, 
> max_perf);
> +    return amd_pstate_write_request(policy->cpu, min_perf, des_perf, 
> max_perf, -1);
>  }
>  
>  static void cf_check amd_pstate_init_msrs(void *info)
> @@ -354,7 +373,7 @@ static void amd_pstate_boost_init(struct cpufreq_policy 
> *policy, struct amd_psta
>      policy->turbo = CPUFREQ_TURBO_ENABLED;
>  }
>  
> -static int cf_check amd_pstate_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
> +static int amd_pstate_cpufreq_init_perf(struct cpufreq_policy *policy)
>  {
>      unsigned int cpu = policy->cpu;
>      struct amd_pstate_drv_data *data;
> @@ -379,10 +398,23 @@ static int cf_check amd_pstate_cpufreq_cpu_init(struct 
> cpufreq_policy *policy)
>          return -ENODEV;
>      }
>  
> -    amd_pstate_boost_init(policy, data);
>      return 0;
>  }
>  
> +static int cf_check amd_pstate_cpufreq_cpu_init(struct cpufreq_policy 
> *policy)
> +{
> +    int ret = 0;

Pointless initializer.

> +    struct amd_pstate_drv_data *data;
> +
> +    ret = amd_pstate_cpufreq_init_perf(policy);
> +    if ( ret )
> +        return ret;
> +
> +    data = per_cpu(amd_pstate_drv_data, policy->cpu);
> +    amd_pstate_boost_init(policy, data);
> +    return ret;
> +}
> +
>  static int cf_check amd_pstate_cpufreq_cpu_exit(struct cpufreq_policy 
> *policy)
>  {
>      struct amd_pstate_drv_data *data = per_cpu(amd_pstate_drv_data, 
> policy->cpu);
> @@ -393,6 +425,70 @@ static int cf_check amd_pstate_cpufreq_cpu_exit(struct 
> cpufreq_policy *policy)
>      return 0;
>  }
>  
> +static void amd_perf_ctl_reset(void *data)
> +{
> +    wrmsr_safe(MSR_K8_PSTATE_CTRL, 0);
> +}
> +
> +static int cf_check amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
> +{
> +    int ret = 0;
> +    struct amd_pstate_drv_data *data;
> +
> +    /*
> +     * Resetting P-State Control register will put the CPU in P0 frequency,
> +     * which is ideal for initialization process.
> +     */
> +    on_selected_cpus(cpumask_of(policy->cpu), amd_perf_ctl_reset, NULL, 1);

How do you/we know what's ideal for initialization? I can think of cases
where it might be better to still conserve power.

> +static int cf_check amd_pstate_epp_update_limit(struct cpufreq_policy 
> *policy)
> +{
> +    struct amd_pstate_drv_data *data = per_cpu(amd_pstate_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->hw.highest_perf;
> +    min_perf = data->hw.lowest_perf;
> +
> +    if ( data->policy == CPUFREQ_POLICY_PERFORMANCE )
> +        min_perf = max_perf;

Why can't this be done ...

> +    /* CPPC EPP feature require to set zero to the desire perf bit */
> +    des_perf = 0;
> +
> +    if ( data->policy == CPUFREQ_POLICY_PERFORMANCE )
> +        /* Force the epp value to be zero for performance policy */
> +        epp = CPPC_ENERGY_PERF_MAX_PERFORMANCE;

... here as well? And why is there nothing respective for ...

> +    else if ( data->policy == CPUFREQ_POLICY_POWERSAVE )
> +        /* Force the epp value to be 0xff for powersave policy */
> +        epp = CPPC_ENERGY_PERF_MAX_POWERSAVE;

... this case (e.g. setting max_perf from min_perf)?

> @@ -402,6 +498,15 @@ static const struct cpufreq_driver __initconstrel 
> amd_pstate_cpufreq_driver =
>      .exit   = amd_pstate_cpufreq_cpu_exit,
>  };
>  
> +static const struct cpufreq_driver __initconstrel amd_pstate_epp_driver =

Again: __initconst_cf_clobber.

> @@ -409,4 +514,9 @@ int __init amd_pstate_register_driver(void)
>  
>      if ( !opt_cpufreq_active )
>          return cpufreq_register_driver(&amd_pstate_cpufreq_driver);
> +    else
> +    {
> +        epp_mode = true;

Why a 2nd global variable? Can't you go from opt_cpufreq_active?

> --- a/xen/arch/x86/acpi/cpufreq/hwp.c
> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
> @@ -21,10 +21,6 @@ static bool __ro_after_init feature_hdc;
>  
>  static bool __ro_after_init opt_cpufreq_hdc = true;
>  
> -#define HWP_ENERGY_PERF_MAX_PERFORMANCE 0
> -#define HWP_ENERGY_PERF_BALANCE         0x80
> -#define HWP_ENERGY_PERF_MAX_POWERSAVE   0xff
> -
>  union hwp_request
>  {
>      struct
> @@ -597,7 +593,7 @@ int set_hwp_para(struct cpufreq_policy *policy,
>          data->minimum = data->hw.lowest;
>          data->maximum = data->hw.lowest;
>          data->activity_window = 0;
> -        data->energy_perf = HWP_ENERGY_PERF_MAX_POWERSAVE;
> +        data->energy_perf = CPPC_ENERGY_PERF_MAX_POWERSAVE;
>          data->desired = 0;
>          break;
>  
> @@ -605,7 +601,7 @@ int set_hwp_para(struct cpufreq_policy *policy,
>          data->minimum = data->hw.highest;
>          data->maximum = data->hw.highest;
>          data->activity_window = 0;
> -        data->energy_perf = HWP_ENERGY_PERF_MAX_PERFORMANCE;
> +        data->energy_perf = CPPC_ENERGY_PERF_MAX_PERFORMANCE;
>          data->desired = 0;
>          break;
>  
> @@ -613,7 +609,7 @@ int set_hwp_para(struct cpufreq_policy *policy,
>          data->minimum = data->hw.lowest;
>          data->maximum = data->hw.highest;
>          data->activity_window = 0;
> -        data->energy_perf = HWP_ENERGY_PERF_BALANCE;
> +        data->energy_perf = CPPC_ENERGY_PERF_BALANCE;
>          data->desired = 0;
>          break;
>  
> diff --git a/xen/include/acpi/cpufreq/cpufreq.h 
> b/xen/include/acpi/cpufreq/cpufreq.h
> index cad27f6811..d2a74d8315 100644
> --- 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;
>  };
>  DECLARE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_policy);
>  
> @@ -264,6 +265,10 @@ void cpufreq_dbs_timer_resume(void);
>  
>  void intel_feature_detect(struct cpufreq_policy *policy);
>  
> +#define CPPC_ENERGY_PERF_MAX_PERFORMANCE 0
> +#define CPPC_ENERGY_PERF_BALANCE         0x80
> +#define CPPC_ENERGY_PERF_MAX_POWERSAVE   0xff

I guess this renaming / movement might better be a separate patch. The more
that the description here doesn't even mention, let alone justify, it.

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -425,7 +425,7 @@ struct xen_set_cppc_para {
>  
>  #define XEN_HWP_DRIVER_NAME "hwp"
>  #define XEN_AMD_PSTATE_DRIVER_NAME "amd-pstate"
> -
> +#define XEN_AMD_PSTATE_EPP_DRIVER_NAME "amd-pstate-epp"
>  /*

Please don't lose blank lines like this.

Jan