Re: [PATCH 1/2] x86/cpu/amd: Clarify amd_get_highest_perf()

[Mario Limonciello]

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.