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

[Nathan Fontenot]

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