| Message ID | 20240626042043.2410-2-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Mario Limonciello |
| Headers | show |
| Series | Fixes for wrong performance levels in acpi-cpufreq | expand |
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 <=. > + 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.
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.
On Wed, Jun 26, 2024 at 01:18:17PM -0500, Mario Limonciello wrote: > 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. Sounds ok to me. Keep the whole logic in one place. Sure.
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c index 44df3f11e731..8b730193d79e 100644 --- a/arch/x86/kernel/cpu/amd.c +++ b/arch/x86/kernel/cpu/amd.c @@ -29,6 +29,10 @@ #include "cpu.h" +#define CPPC_HIGHEST_PERF_MAX 255 +#define CPPC_HIGHEST_PERF_PERFORMANCE 196 +#define CPPC_HIGHEST_PERF_DEFAULT 166 + 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: + 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: + return CPPC_HIGHEST_PERF_DEFAULT; + default: + return CPPC_HIGHEST_PERF_MAX; + } + } - return 255; + return CPPC_HIGHEST_PERF_MAX; } EXPORT_SYMBOL_GPL(amd_get_highest_perf);
Rather than parsing through families and models as an if/else, use cpu_feature_enabled() and switch/case. Add definitions aligned with the amd-pstate definition for performance levels. No intended functional changes. Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- arch/x86/kernel/cpu/amd.c | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-)