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
