| 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: <c9fc8ee2-db38-48e3-86ee-69a33eab1473@amd.com>
Date: Mon, 9 Dec 2024 10:49:31 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: "Gautham R. Shenoy" <gautham.shenoy@....com>
Cc: Perry Yuan <perry.yuan@....com>, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org, Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>
Subject: Re: [PATCH v2 12/16] cpufreq/amd-pstate: Always write EPP value when
updating perf
On 12/9/2024 02:42, Gautham R. Shenoy wrote:
> Hello Mario,
>
> On Sun, Dec 08, 2024 at 12:30:27AM -0600, Mario Limonciello wrote:
>> For MSR systems the EPP value is in the same register as perf targets
>> and so divding them into two separate MSR writes is wasteful.
>>
>> In msr_update_perf(), update both EPP and perf values in one write to
>> MSR_AMD_CPPC_REQ, and cache them if successful.
>>
>> To accomplish this plumb the EPP value into the update_perf call and modify
>> all its callers to check the return value.
>>
>> Reviewed-and-tested-by: Dhananjay Ugwekar <dhananjay.ugwekar@....com>
>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>> ---
>> drivers/cpufreq/amd-pstate.c | 71 ++++++++++++++++++++++--------------
>> 1 file changed, 43 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index d21acd961edcd..dd11ba6c00cc3 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -222,25 +222,36 @@ static s16 shmem_get_epp(struct amd_cpudata *cpudata)
>> }
>>
>> static int msr_update_perf(struct amd_cpudata *cpudata, u32 min_perf,
>> - u32 des_perf, u32 max_perf, bool fast_switch)
>> + u32 des_perf, u32 max_perf, u32 epp, bool fast_switch)
>> {
>> + u64 value;
>> +
>> + value = READ_ONCE(cpudata->cppc_req_cached);
>
>
> There seems to be a mismatch here between what the API is passing and
> parameters and how this function is *not* using them, and instead
> using cpudata->cppc_req_cached.
>
> The expectation seems to be that the max_perf, min_perf, des_perf and
> epp fields in cpudata->cppc_req_cached would be the same as @des_perf,
> @max_perf, @min_perf and @ep, no ?
>
> Or is it that for the MSR update, the value in
> cpudata->cppc_req_cached take precedence over the arguments passed ?
>
> Ideally, the "value" should be recomputed here using (@min_perf |
> @max_perf | @des_perf | @epp) and that value should be cached as you
> are doing below.
>
Yeah - that's what the next patch does (which I think you probably saw
after you reviewed it).
Do you think maybe I should just squash the two? Or would you be
happier if I re-ordered the two?
>
>> if (fast_switch) {
>> wrmsrl(MSR_AMD_CPPC_REQ, READ_ONCE(cpudata->cppc_req_cached));
>> return 0;
>> + } else {
>> + int ret = wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
>> + READ_ONCE(cpudata->cppc_req_cached));
>> + if (ret)
>> + return ret;
>> }
>>
>> - return wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
>> - READ_ONCE(cpudata->cppc_req_cached));
>> + WRITE_ONCE(cpudata->cppc_req_cached, value);
>
> Since cppc_req_cached is not changed, why write it again ?
Because of the next patch. It will look at cpudata->cppc_req_cached and
determine if anything changed in it - including EPP.
>
>> + WRITE_ONCE(cpudata->epp_cached, epp);
>> +
>> + return 0;
>> }
>>
>> DEFINE_STATIC_CALL(amd_pstate_update_perf, msr_update_perf);
>>
>> static inline int amd_pstate_update_perf(struct amd_cpudata *cpudata,
>> u32 min_perf, u32 des_perf,
>> - u32 max_perf, bool fast_switch)
>> + u32 max_perf, u32 epp,
>> + bool fast_switch)
>> {
>> return static_call(amd_pstate_update_perf)(cpudata, min_perf, des_perf,
>> - max_perf, fast_switch);
>> + max_perf, epp, fast_switch);
>> }
>>
>> static int msr_set_epp(struct amd_cpudata *cpudata, u32 epp)
>> @@ -459,12 +470,19 @@ static inline int amd_pstate_init_perf(struct amd_cpudata *cpudata)
>> return static_call(amd_pstate_init_perf)(cpudata);
>> }
>>
>> -static int shmem_update_perf(struct amd_cpudata *cpudata,
>> - u32 min_perf, u32 des_perf,
>> - u32 max_perf, bool fast_switch)
>> +static int shmem_update_perf(struct amd_cpudata *cpudata, u32 min_perf,
>> + u32 des_perf, u32 max_perf, u32 epp, bool fast_switch)
>> {
>> struct cppc_perf_ctrls perf_ctrls;
>>
>> + if (cppc_state == AMD_PSTATE_ACTIVE) {
>> + int ret = shmem_set_epp(cpudata, epp);
>> +
>> + if (ret)
>> + return ret;
>> + WRITE_ONCE(cpudata->epp_cached, epp);
>> + }
>> +
>> perf_ctrls.max_perf = max_perf;
>> perf_ctrls.min_perf = min_perf;
>> perf_ctrls.desired_perf = des_perf;
>> @@ -545,10 +563,10 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
>>
>> WRITE_ONCE(cpudata->cppc_req_cached, value);
>>
>> - amd_pstate_update_perf(cpudata, min_perf, des_perf,
>> - max_perf, fast_switch);
>> + amd_pstate_update_perf(cpudata, min_perf, des_perf, max_perf, 0, fast_switch);
>>
>> cpufreq_policy_put:
>> +
>> cpufreq_cpu_put(policy);
>> }
>>
>> @@ -1545,6 +1563,7 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>> {
>> struct amd_cpudata *cpudata = policy->driver_data;
>> u64 value;
>> + u32 epp;
>>
>> amd_pstate_update_min_max_limit(policy);
>>
>> @@ -1557,23 +1576,19 @@ static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>> value |= FIELD_PREP(AMD_CPPC_MIN_PERF_MASK, cpudata->min_limit_perf);
>>
>> if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
>> - WRITE_ONCE(cpudata->epp_cached, 0);
>> - value |= FIELD_PREP(AMD_CPPC_EPP_PERF_MASK, cpudata->epp_cached);
>> -
>> - WRITE_ONCE(cpudata->cppc_req_cached, value);
>> + epp = 0;
>> + else
>> + epp = READ_ONCE(cpudata->epp_cached);
>>
>> if (trace_amd_pstate_epp_perf_enabled()) {
>> - trace_amd_pstate_epp_perf(cpudata->cpu, cpudata->highest_perf,
>> - cpudata->epp_cached,
>> + trace_amd_pstate_epp_perf(cpudata->cpu, cpudata->highest_perf, epp,
>> cpudata->min_limit_perf,
>> cpudata->max_limit_perf,
>> policy->boost_enabled);
>> }
>>
>> - amd_pstate_update_perf(cpudata, cpudata->min_limit_perf, 0U,
>> - cpudata->max_limit_perf, false);
>> -
>> - return amd_pstate_set_epp(cpudata, READ_ONCE(cpudata->epp_cached));
>> + return amd_pstate_update_perf(cpudata, cpudata->min_limit_perf, 0U,
>> + cpudata->max_limit_perf, epp, false);
>> }
>>
>> static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
>> @@ -1602,7 +1617,7 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
>> return 0;
>> }
>>
>> -static void amd_pstate_epp_reenable(struct amd_cpudata *cpudata)
>> +static int amd_pstate_epp_reenable(struct amd_cpudata *cpudata)
>> {
>> u64 max_perf;
>> int ret;
>> @@ -1620,17 +1635,19 @@ static void amd_pstate_epp_reenable(struct amd_cpudata *cpudata)
>> max_perf, cpudata->boost_state);
>> }
>>
>> - amd_pstate_update_perf(cpudata, 0, 0, max_perf, false);
>> - amd_pstate_set_epp(cpudata, cpudata->epp_cached);
>> + return amd_pstate_update_perf(cpudata, 0, 0, max_perf, cpudata->epp_cached, false);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> On an MSR based system, none of the values passed here will be used,
> and instead the value in cpudata->cppc_req_cached will be used, no?
Currently; yes. After the next patch that changes.
>
>> }
>>
>> static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
>> {
>> struct amd_cpudata *cpudata = policy->driver_data;
>> + int ret;
>>
>> pr_debug("AMD CPU Core %d going online\n", cpudata->cpu);
>>
>> - amd_pstate_epp_reenable(cpudata);
>> + ret = amd_pstate_epp_reenable(cpudata);
>> + if (ret)
>> + return ret;
>> cpudata->suspended = false;
>>
>> return 0;
>> @@ -1654,10 +1671,8 @@ static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
>> min_perf, min_perf, policy->boost_enabled);
>> }
>>
>> - amd_pstate_update_perf(cpudata, min_perf, 0, min_perf, false);
>> - amd_pstate_set_epp(cpudata, AMD_CPPC_EPP_BALANCE_POWERSAVE);
>> -
>> - return 0;
>> + return amd_pstate_update_perf(cpudata, min_perf, 0, min_perf,
>> + AMD_CPPC_EPP_BALANCE_POWERSAVE, false);
>> }
>>
>> static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
>> --
>> 2.43.0
>>
>
> --
> Thanks and Regards
> gautham.
Powered by blists - more mailing lists