linux-kernel - Re: [PATCH 1/2] x86/cpu/amd: Clarify amd_get_highest

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <681732d3-76ba-47ba-9cce-362c6fe094cb@amd.com>
Date: Wed, 26 Jun 2024 13:18:17 -0500
From: Mario Limonciello <mario.limonciello@....com>
To: Borislav Petkov <bp@...en8.de>
Cc: "Gautham R . Shenoy" <gautham.shenoy@....com>,
 Perry Yuan <perry.yuan@....com>, Thomas Gleixner <tglx@...utronix.de>,
 Ingo Molnar <mingo@...hat.com>, Dave Hansen <dave.hansen@...ux.intel.com>,
 "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
 "H . Peter Anvin" <hpa@...or.com>, Huang Rui <ray.huang@....com>,
 "Rafael J . Wysocki" <rafael@...nel.org>,
 Viresh Kumar <viresh.kumar@...aro.org>,
 Nikolay Borisov <nik.borisov@...e.com>, Peter Zijlstra
 <peterz@...radead.org>,
 "open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)"
 <linux-kernel@...r.kernel.org>,
 "open list:AMD PSTATE DRIVER" <linux-pm@...r.kernel.org>
Subject: Re: [PATCH 1/2] x86/cpu/amd: Clarify amd_get_highest_perf()

On 6/26/2024 12:14, Borislav Petkov wrote:
> On Tue, Jun 25, 2024 at 11:20:42PM -0500, Mario Limonciello wrote:
>>   static inline int rdmsrl_amd_safe(unsigned msr, unsigned long long *p)
>>   {
>>   	u32 gprs[8] = { 0 };
>> @@ -1194,15 +1198,27 @@ u32 amd_get_highest_perf(void)
>>   {
>>   	struct cpuinfo_x86 *c = &boot_cpu_data;
>>   
>> -	if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) ||
>> -			       (c->x86_model >= 0x70 && c->x86_model < 0x80)))
>> -		return 166;
>> +	if (cpu_feature_enabled(X86_FEATURE_ZEN2)) {
>> +		switch (c->x86_model) {
>> +		case 0x30 ... 0x40:
>> +		case 0x70 ... 0x80:
> 
> Well, it was < 0x40 and < 0x80
> 
> You're making it <=.
> 

Good catch, I'll adjust to 0x3f and 0x7f.

>> +			return CPPC_HIGHEST_PERF_DEFAULT;
>> +		default:
>> +			return CPPC_HIGHEST_PERF_MAX;
>> +		}
>> +	}
>>   
>> -	if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
>> -			       (c->x86_model >= 0x40 && c->x86_model < 0x70)))
>> -		return 166;
>> +	if (cpu_feature_enabled(X86_FEATURE_ZEN3)) {
>> +		switch (c->x86_model) {
>> +		case 0x20 ... 0x30:
>> +		case 0x40 ... 0x70:
> 
> Ditto.
> 
> Also, ontop:
> 
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 73559db78433..5d496de4e141 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -1204,7 +1204,7 @@ u32 amd_get_highest_perf(void)
>   		case 0x70 ... 0x80:
>   			return CPPC_HIGHEST_PERF_DEFAULT;
>   		default:
> -			return CPPC_HIGHEST_PERF_MAX;
> +			break;
>   		}
>   	}
>   
> @@ -1214,7 +1214,7 @@ u32 amd_get_highest_perf(void)
>   		case 0x40 ... 0x70:
>   			return CPPC_HIGHEST_PERF_DEFAULT;
>   		default:
> -			return CPPC_HIGHEST_PERF_MAX;
> +			break;
>   		}
>   	}
>   
> so that you don't have so many redundant returns in the function.
> 

This uncovers a case that I'm not really sure what to do about.

Right now acpi-cpufreq uses this function and if the CPU isn't special 
cased you'll get the value as 255.  This is totally wrong for newer SoCs 
though.  That's what prompted this series in the first place that I saw 
different behavior in amd-pstate and acpi-cpufreq.

So I think we need to have something that is:

switch (zen1) {
case ...
default)
	return 255;
}
switch (zen2) {
case ...
default)
	return 255;
}
switch (zen3) {
case ...
default)
	break;
}
switch (zen4) {
case ...
default)
	break;
}

return 255;

(This is no functional changes)

And then patch 2 or patch 3 change the "default" return to 166 and if 
there is functional issues then they need to be special cased.