| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z1KA82G2rajkCHLx@BLRRASHENOY1.amd.com>
Date: Fri, 6 Dec 2024 10:13:31 +0530
From: "Gautham R. Shenoy" <gautham.shenoy@....com>
To: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>
Cc: mario.limonciello@....com, perry.yuan@....com, rafael@...nel.org,
viresh.kumar@...aro.org, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] cpufreq/amd-pstate: Convert the
amd_pstate_get/set_epp() to static calls
Hello Dhananjay,
On Wed, Dec 04, 2024 at 02:48:38PM +0000, Dhananjay Ugwekar wrote:
> MSR and shared memory based systems have different mechanisms to get and
> set the epp value. Split those mechanisms into different functions and
> assign them appropriately to the static calls at boot time. This eliminates
> the need for the "if(cpu_feature_enabled(X86_FEATURE_CPPC))" checks at
> runtime.
>
> Also, propagate the error code from rdmsrl_on_cpu() and cppc_get_epp_perf()
> to *_get_epp()'s caller, instead of returning -EIO unconditionally.
>
> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>
Thanks for taking this up.
This patch looks good to me.
Reviewed-by: Gautham R. Shenoy <gautham.shenoy@....com>
--
Thanks and Regards
gautham.
> ---
> drivers/cpufreq/amd-pstate.c | 92 +++++++++++++++++++++++-------------
> 1 file changed, 60 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index d7630bab2516..d391e8cafeca 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -180,26 +180,40 @@ static inline int get_mode_idx_from_str(const char *str, size_t size)
> static DEFINE_MUTEX(amd_pstate_limits_lock);
> static DEFINE_MUTEX(amd_pstate_driver_lock);
>
> -static s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
> +static s16 msr_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
> {
> u64 epp;
> int ret;
>
> - if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
> - if (!cppc_req_cached) {
> - epp = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
> - &cppc_req_cached);
> - if (epp)
> - return epp;
> - }
> - epp = (cppc_req_cached >> 24) & 0xFF;
> - } else {
> - ret = cppc_get_epp_perf(cpudata->cpu, &epp);
> + if (!cppc_req_cached) {
> + ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, &cppc_req_cached);
> if (ret < 0) {
> pr_debug("Could not retrieve energy perf value (%d)\n", ret);
> - return -EIO;
> + return ret;
> }
> }
> + epp = (cppc_req_cached >> 24) & 0xFF;
> +
> + return (s16)epp;
> +}
> +
> +DEFINE_STATIC_CALL(amd_pstate_get_epp, msr_get_epp);
> +
> +static inline s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
> +{
> + return static_call(amd_pstate_get_epp)(cpudata, cppc_req_cached);
> +}
> +
> +static s16 shmem_get_epp(struct amd_cpudata *cpudata, u64 dummy)
> +{
> + u64 epp;
> + int ret;
> +
> + ret = cppc_get_epp_perf(cpudata->cpu, &epp);
> + if (ret < 0) {
> + pr_debug("Could not retrieve energy perf value (%d)\n", ret);
> + return ret;
> + }
>
> return (s16)(epp & 0xff);
> }
> @@ -253,33 +267,45 @@ static inline void amd_pstate_update_perf(struct amd_cpudata *cpudata,
> max_perf, fast_switch);
> }
>
> -static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
> +static int msr_set_epp(struct amd_cpudata *cpudata, u32 epp)
> {
> int ret;
> - struct cppc_perf_ctrls perf_ctrls;
> -
> - if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
> - u64 value = READ_ONCE(cpudata->cppc_req_cached);
>
> - value &= ~GENMASK_ULL(31, 24);
> - value |= (u64)epp << 24;
> - WRITE_ONCE(cpudata->cppc_req_cached, value);
> + u64 value = READ_ONCE(cpudata->cppc_req_cached);
>
> - ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
> - if (!ret)
> - cpudata->epp_cached = epp;
> - } else {
> - amd_pstate_update_perf(cpudata, cpudata->min_limit_perf, 0U,
> - cpudata->max_limit_perf, false);
> + value &= ~GENMASK_ULL(31, 24);
> + value |= (u64)epp << 24;
> + WRITE_ONCE(cpudata->cppc_req_cached, value);
>
> - perf_ctrls.energy_perf = epp;
> - ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
> - if (ret) {
> - pr_debug("failed to set energy perf value (%d)\n", ret);
> - return ret;
> - }
> + ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
> + if (!ret)
> cpudata->epp_cached = epp;
> +
> + return ret;
> +}
> +
> +DEFINE_STATIC_CALL(amd_pstate_set_epp, msr_set_epp);
> +
> +static inline int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
> +{
> + return static_call(amd_pstate_set_epp)(cpudata, epp);
> +}
> +
> +static int shmem_set_epp(struct amd_cpudata *cpudata, u32 epp)
> +{
> + int ret;
> + struct cppc_perf_ctrls perf_ctrls;
> +
> + amd_pstate_update_perf(cpudata, cpudata->min_limit_perf, 0U,
> + cpudata->max_limit_perf, false);
> +
> + perf_ctrls.energy_perf = epp;
> + ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1);
> + if (ret) {
> + pr_debug("failed to set energy perf value (%d)\n", ret);
> + return ret;
> }
> + cpudata->epp_cached = epp;
>
> return ret;
> }
> @@ -1867,6 +1893,8 @@ static int __init amd_pstate_init(void)
> static_call_update(amd_pstate_cppc_enable, shmem_cppc_enable);
> static_call_update(amd_pstate_init_perf, shmem_init_perf);
> static_call_update(amd_pstate_update_perf, shmem_update_perf);
> + static_call_update(amd_pstate_get_epp, shmem_get_epp);
> + static_call_update(amd_pstate_set_epp, shmem_set_epp);
> }
>
> ret = amd_pstate_register_driver(cppc_state);
> --
> 2.34.1
>
Powered by blists - more mailing lists