linux-kernel - Re: [PATCH v2 02/14] cpufreq: amd-pstate: enable AMD Precision Boost mode switch

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 12 Jul 2022 12:08:43 -0500
From:   Nathan Fontenot <nafonten@....com>
To:     "Yuan, Perry" <Perry.Yuan@....com>,
        "Fontenot, Nathan" <Nathan.Fontenot@....com>,
        "rafael.j.wysocki@...el.com" <rafael.j.wysocki@...el.com>,
        "viresh.kumar@...aro.org" <viresh.kumar@...aro.org>,
        "Huang, Ray" <Ray.Huang@....com>
Cc:     "Sharma, Deepak" <Deepak.Sharma@....com>,
        "Limonciello, Mario" <Mario.Limonciello@....com>,
        "Deucher, Alexander" <Alexander.Deucher@....com>,
        "Su, Jinzhou (Joe)" <Jinzhou.Su@....com>,
        "Huang, Shimmer" <Shimmer.Huang@....com>,
        "Du, Xiaojian" <Xiaojian.Du@....com>,
        "Meng, Li (Jassmine)" <Li.Meng@....com>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 02/14] cpufreq: amd-pstate: enable AMD Precision Boost
 mode switch

On 7/11/22 23:15, Yuan, Perry wrote:>>> --- a/drivers/cpufreq/amd-pstate.c
>>> +++ b/drivers/cpufreq/amd-pstate.c
>>> @@ -122,6 +122,7 @@ struct amd_cpudata {
>>>
>>>  	u64 freq;
>>>  	bool	boost_supported;
>>> +	u64 	cppc_hw_conf_cached;
>>>  };
>>>
>>>  static inline int pstate_enable(bool enable) @@ -438,18 +439,27 @@
>>> static int amd_pstate_set_boost(struct cpufreq_policy *policy, int
>>> state)  {
>>>  	struct amd_cpudata *cpudata = policy->driver_data;
>>>  	int ret;
>>> +	u64 value;
>>>
>>>  	if (!cpudata->boost_supported) {
>>>  		pr_err("Boost mode is not supported by this processor or
>> SBIOS\n");
>>>  		return -EINVAL;
>>>  	}
>>>
>>> -	if (state)
>>> +	ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_HW_CTL,
>> &value);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (state) {
>>> +		value |= AMD_CPPC_PRECISION_BOOST_ENABLED;
>>>  		policy->cpuinfo.max_freq = cpudata->max_freq;
>>> -	else
>>> +	} else {
>>> +		value &= ~AMD_CPPC_PRECISION_BOOST_ENABLED;
>>>  		policy->cpuinfo.max_freq = cpudata->nominal_freq;
>>> -
>>> +	}
>>>  	policy->max = policy->cpuinfo.max_freq;
>>> +	WRITE_ONCE(cpudata->cppc_hw_conf_cached, value);
>>
>> Does the entire MSR value need to be cached? We only care about the
>> boost enabled bit so it may be better to just cache that.
>>
>> -Nathan
> 
> I think the whole MSR value should be cached, because it has no bad impact to the hardware or driver .
> And it is simple to do that, when we need to check other bits in the hardware configuration MSR in future, we can still use this cached value as well.
> Dose it make sense ?
> 
> Perry.
> 

The MSR controls much more than boost enabling, my concern is someone else starting to update
bits in the MSR and now we have a stale MSR value. Just caching the boost enabled bit is really
aimed at possible future bug prevention.

-Nathan