| Message ID | 20210421023807.1540290-1-ray.huang@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] x86, sched: Fix the AMD CPPC maximum perf on some specific generations | expand |
On Wed, Apr 21, 2021 at 09:42:15PM +0800, Fontenot, Nathan wrote: > On 4/20/2021 9:38 PM, Huang Rui wrote: > > Some AMD Ryzen generations has different calculation method on maximum > > perf. 255 is not for all asics, some specific generations should use 166 > > as the maximum perf. Otherwise, it will report incorrect frequency value > > like below: > > > > ~ → lscpu | grep MHz > > CPU MHz: 3400.000 > > CPU max MHz: 7228.3198 > > CPU min MHz: 2200.0000 > > > > Fixes: 41ea667227ba ("x86, sched: Calculate frequency invariance for AMD systems") > > Fixes: 3c55e94c0ade ("cpufreq: ACPI: Extend frequency tables to cover boost frequencies") > > > > Reported-by: Jason Bagavatsingham <jason.bagavatsingham@gmail.com> > > Tested-by: Jason Bagavatsingham <jason.bagavatsingham@gmail.com> > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=211791 > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > Cc: Alex Deucher <alexander.deucher@amd.com> > > Cc: Nathan Fontenot <nathan.fontenot@amd.com> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Cc: Borislav Petkov <bp@suse.de> > > Cc: x86@kernel.org > > Cc: stable@vger.kernel.org > > --- > > > > Changes from V1 -> V2: > > - Enhance the commit message. > > - Move amd_get_highest_perf() into amd.c. > > - Refine the implementation of switch-case. > > - Cc stable mail list. > > > > --- > > arch/x86/include/asm/processor.h | 2 ++ > > arch/x86/kernel/cpu/amd.c | 22 ++++++++++++++++++++++ > > arch/x86/kernel/smpboot.c | 2 +- > > drivers/cpufreq/acpi-cpufreq.c | 19 +++++++++++++++++++ > > 4 files changed, 44 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h > > index f1b9ed5efaa9..908bcaea1361 100644 > > --- a/arch/x86/include/asm/processor.h > > +++ b/arch/x86/include/asm/processor.h > > @@ -804,8 +804,10 @@ DECLARE_PER_CPU(u64, msr_misc_features_shadow); > > > > #ifdef CONFIG_CPU_SUP_AMD > > extern u32 amd_get_nodes_per_socket(void); > > +extern u32 amd_get_highest_perf(void); > > #else > > static inline u32 amd_get_nodes_per_socket(void) { return 0; } > > +static inline u32 amd_get_highest_perf(void) { return 0; } > > #endif > > > > static inline uint32_t hypervisor_cpuid_base(const char *sig, uint32_t leaves) > > diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c > > index 347a956f71ca..aadb691d9357 100644 > > --- a/arch/x86/kernel/cpu/amd.c > > +++ b/arch/x86/kernel/cpu/amd.c > > @@ -1170,3 +1170,25 @@ void set_dr_addr_mask(unsigned long mask, int dr) > > break; > > } > > } > > + > > +u32 amd_get_highest_perf(void) > > +{ > > + struct cpuinfo_x86 *c = &boot_cpu_data; > > + u32 cppc_max_perf = 225; > > + > > + switch (c->x86) { > > + case 0x17: > > + if ((c->x86_model >= 0x30 && c->x86_model < 0x40) || > > + (c->x86_model >= 0x70 && c->x86_model < 0x80)) > > + cppc_max_perf = 166; > > + break; > > + case 0x19: > > + if ((c->x86_model >= 0x20 && c->x86_model < 0x30) || > > + (c->x86_model >= 0x40 && c->x86_model < 0x70)) > > + cppc_max_perf = 166; > > + break; > > + } > > + > > + return cppc_max_perf; > > +} > > +EXPORT_SYMBOL_GPL(amd_get_highest_perf); > > Should this be an update to cpp_get_perf_caps()? > > This approach would ensure that all callers have the correct value > and remove the need to fix up individual callers to use this new > routine to get the correct value. > It's a good idea to modify cppc_get_perf_caps() to correct the right highest perf. I would like to keep amd_get_highest_perf() function in amd.c as well because it can be called in other spaces without querying an ACPI register in case only wants to get highest perf value. --- diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index 69057fcd2c04..58e72b6e222f 100644 --- a/drivers/acpi/cppc_acpi.c +++ b/drivers/acpi/cppc_acpi.c @@ -1107,8 +1107,12 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps) } } - cpc_read(cpunum, highest_reg, &high); - perf_caps->highest_perf = high; + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) { + perf_caps->highest_perf = amd_get_highest_perf(); + } else { + cpc_read(cpunum, highest_reg, &high); + perf_caps->highest_perf = high; + } cpc_read(cpunum, lowest_reg, &low); perf_caps->lowest_perf = low; --- Thanks, Ray
On Wed, Apr 21, 2021 at 4:38 AM Huang Rui <ray.huang@amd.com> wrote: > > Some AMD Ryzen generations has different calculation method on maximum > perf. 255 is not for all asics, some specific generations should use 166 > as the maximum perf. Otherwise, it will report incorrect frequency value > like below: > > ~ → lscpu | grep MHz > CPU MHz: 3400.000 > CPU max MHz: 7228.3198 > CPU min MHz: 2200.0000 > > Fixes: 41ea667227ba ("x86, sched: Calculate frequency invariance for AMD systems") > Fixes: 3c55e94c0ade ("cpufreq: ACPI: Extend frequency tables to cover boost frequencies") > > Reported-by: Jason Bagavatsingham <jason.bagavatsingham@gmail.com> > Tested-by: Jason Bagavatsingham <jason.bagavatsingham@gmail.com> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=211791 > Signed-off-by: Huang Rui <ray.huang@amd.com> > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Nathan Fontenot <nathan.fontenot@amd.com> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: Borislav Petkov <bp@suse.de> > Cc: x86@kernel.org > Cc: stable@vger.kernel.org > --- > > Changes from V1 -> V2: > - Enhance the commit message. > - Move amd_get_highest_perf() into amd.c. > - Refine the implementation of switch-case. > - Cc stable mail list. > > --- > arch/x86/include/asm/processor.h | 2 ++ > arch/x86/kernel/cpu/amd.c | 22 ++++++++++++++++++++++ > arch/x86/kernel/smpboot.c | 2 +- > drivers/cpufreq/acpi-cpufreq.c | 19 +++++++++++++++++++ > 4 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h > index f1b9ed5efaa9..908bcaea1361 100644 > --- a/arch/x86/include/asm/processor.h > +++ b/arch/x86/include/asm/processor.h > @@ -804,8 +804,10 @@ DECLARE_PER_CPU(u64, msr_misc_features_shadow); > > #ifdef CONFIG_CPU_SUP_AMD > extern u32 amd_get_nodes_per_socket(void); > +extern u32 amd_get_highest_perf(void); > #else > static inline u32 amd_get_nodes_per_socket(void) { return 0; } > +static inline u32 amd_get_highest_perf(void) { return 0; } > #endif > > static inline uint32_t hypervisor_cpuid_base(const char *sig, uint32_t leaves) > diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c > index 347a956f71ca..aadb691d9357 100644 > --- a/arch/x86/kernel/cpu/amd.c > +++ b/arch/x86/kernel/cpu/amd.c > @@ -1170,3 +1170,25 @@ void set_dr_addr_mask(unsigned long mask, int dr) > break; > } > } > + > +u32 amd_get_highest_perf(void) > +{ > + struct cpuinfo_x86 *c = &boot_cpu_data; > + u32 cppc_max_perf = 225; The extra local variable is redundant. > + > + switch (c->x86) { > + case 0x17: > + if ((c->x86_model >= 0x30 && c->x86_model < 0x40) || > + (c->x86_model >= 0x70 && c->x86_model < 0x80)) > + cppc_max_perf = 166; > + break; Also it would be cleaner to write this as if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) || (c->x86_model >= 0x70 && c->x86_model < 0x80)) return 166; And analogously below. > + case 0x19: > + if ((c->x86_model >= 0x20 && c->x86_model < 0x30) || > + (c->x86_model >= 0x40 && c->x86_model < 0x70)) > + cppc_max_perf = 166; > + break; > + } > + > + return cppc_max_perf; And here return 225; > +} > +EXPORT_SYMBOL_GPL(amd_get_highest_perf); > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > index 02813a7f3a7c..7bec57d04a87 100644 > --- a/arch/x86/kernel/smpboot.c > +++ b/arch/x86/kernel/smpboot.c > @@ -2046,7 +2046,7 @@ static bool amd_set_max_freq_ratio(void) > return false; > } > > - highest_perf = perf_caps.highest_perf; > + highest_perf = amd_get_highest_perf(); > nominal_perf = perf_caps.nominal_perf; > > if (!highest_perf || !nominal_perf) { > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c > index d1bbc16fba4b..3f0a19dd658c 100644 > --- a/drivers/cpufreq/acpi-cpufreq.c > +++ b/drivers/cpufreq/acpi-cpufreq.c > @@ -630,6 +630,22 @@ static int acpi_cpufreq_blacklist(struct cpuinfo_x86 *c) > #endif > > #ifdef CONFIG_ACPI_CPPC_LIB > + > +static u64 get_amd_max_boost_ratio(unsigned int cpu, u64 nominal_perf) > +{ > + u64 boost_ratio, cppc_max_perf; > + > + if (!nominal_perf) > + return 0; > + > + cppc_max_perf = amd_get_highest_perf(); > + > + boost_ratio = div_u64(cppc_max_perf << SCHED_CAPACITY_SHIFT, > + nominal_perf); > + > + return boost_ratio; > +} The function above is not necessary if I'm not mistaken. > + > static u64 get_max_boost_ratio(unsigned int cpu) > { > struct cppc_perf_caps perf_caps; > @@ -646,6 +662,9 @@ static u64 get_max_boost_ratio(unsigned int cpu) > return 0; > } > > + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) > + return get_amd_max_boost_ratio(cpu, perf_caps.nominal_perf); > + > highest_perf = perf_caps.highest_perf; The above can be written as if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) highest_perf = amd_get_highest_perf(); else highest_perf = perf_caps.highest_perf; > nominal_perf = perf_caps.nominal_perf; > > --
On Fri, Apr 23, 2021 at 11:19:46PM +0800, Rafael J. Wysocki wrote: > On Wed, Apr 21, 2021 at 4:38 AM Huang Rui <ray.huang@amd.com> wrote: > > > > Some AMD Ryzen generations has different calculation method on maximum > > perf. 255 is not for all asics, some specific generations should use 166 > > as the maximum perf. Otherwise, it will report incorrect frequency value > > like below: > > > > ~ $B"*(B lscpu | grep MHz > > CPU MHz: 3400.000 > > CPU max MHz: 7228.3198 > > CPU min MHz: 2200.0000 > > > > Fixes: 41ea667227ba ("x86, sched: Calculate frequency invariance for AMD systems") > > Fixes: 3c55e94c0ade ("cpufreq: ACPI: Extend frequency tables to cover boost frequencies") > > > > Reported-by: Jason Bagavatsingham <jason.bagavatsingham@gmail.com> > > Tested-by: Jason Bagavatsingham <jason.bagavatsingham@gmail.com> > > Bugzilla: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D211791&data=04%7C01%7Cray.huang%40amd.com%7C5069cfd46dfe4f0c504208d9066b41be%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637547880005034494%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=YSgziLlmyJlAxMQceGlx%2FB1EgN50h512ai1F4ypXoD8%3D&reserved=0 > > Signed-off-by: Huang Rui <ray.huang@amd.com> > > Cc: Alex Deucher <alexander.deucher@amd.com> > > Cc: Nathan Fontenot <nathan.fontenot@amd.com> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Cc: Borislav Petkov <bp@suse.de> > > Cc: x86@kernel.org > > Cc: stable@vger.kernel.org > > --- > > > > Changes from V1 -> V2: > > - Enhance the commit message. > > - Move amd_get_highest_perf() into amd.c. > > - Refine the implementation of switch-case. > > - Cc stable mail list. > > > > --- > > arch/x86/include/asm/processor.h | 2 ++ > > arch/x86/kernel/cpu/amd.c | 22 ++++++++++++++++++++++ > > arch/x86/kernel/smpboot.c | 2 +- > > drivers/cpufreq/acpi-cpufreq.c | 19 +++++++++++++++++++ > > 4 files changed, 44 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h > > index f1b9ed5efaa9..908bcaea1361 100644 > > --- a/arch/x86/include/asm/processor.h > > +++ b/arch/x86/include/asm/processor.h > > @@ -804,8 +804,10 @@ DECLARE_PER_CPU(u64, msr_misc_features_shadow); > > > > #ifdef CONFIG_CPU_SUP_AMD > > extern u32 amd_get_nodes_per_socket(void); > > +extern u32 amd_get_highest_perf(void); > > #else > > static inline u32 amd_get_nodes_per_socket(void) { return 0; } > > +static inline u32 amd_get_highest_perf(void) { return 0; } > > #endif > > > > static inline uint32_t hypervisor_cpuid_base(const char *sig, uint32_t leaves) > > diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c > > index 347a956f71ca..aadb691d9357 100644 > > --- a/arch/x86/kernel/cpu/amd.c > > +++ b/arch/x86/kernel/cpu/amd.c > > @@ -1170,3 +1170,25 @@ void set_dr_addr_mask(unsigned long mask, int dr) > > break; > > } > > } > > + > > +u32 amd_get_highest_perf(void) > > +{ > > + struct cpuinfo_x86 *c = &boot_cpu_data; > > + u32 cppc_max_perf = 225; > > The extra local variable is redundant. > > > + > > + switch (c->x86) { > > + case 0x17: > > + if ((c->x86_model >= 0x30 && c->x86_model < 0x40) || > > + (c->x86_model >= 0x70 && c->x86_model < 0x80)) > > + cppc_max_perf = 166; > > + break; > > Also it would be cleaner to write this as > > if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) || > (c->x86_model >= 0x70 && c->x86_model < 0x80)) > return 166; > > And analogously below. > > > + case 0x19: > > + if ((c->x86_model >= 0x20 && c->x86_model < 0x30) || > > + (c->x86_model >= 0x40 && c->x86_model < 0x70)) > > + cppc_max_perf = 166; > > + break; > > + } > > + > > + return cppc_max_perf; > > And here > > return 225; > > > +} > > +EXPORT_SYMBOL_GPL(amd_get_highest_perf); > > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > > index 02813a7f3a7c..7bec57d04a87 100644 > > --- a/arch/x86/kernel/smpboot.c > > +++ b/arch/x86/kernel/smpboot.c > > @@ -2046,7 +2046,7 @@ static bool amd_set_max_freq_ratio(void) > > return false; > > } > > > > - highest_perf = perf_caps.highest_perf; > > + highest_perf = amd_get_highest_perf(); > > nominal_perf = perf_caps.nominal_perf; > > > > if (!highest_perf || !nominal_perf) { > > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c > > index d1bbc16fba4b..3f0a19dd658c 100644 > > --- a/drivers/cpufreq/acpi-cpufreq.c > > +++ b/drivers/cpufreq/acpi-cpufreq.c > > @@ -630,6 +630,22 @@ static int acpi_cpufreq_blacklist(struct cpuinfo_x86 *c) > > #endif > > > > #ifdef CONFIG_ACPI_CPPC_LIB > > + > > +static u64 get_amd_max_boost_ratio(unsigned int cpu, u64 nominal_perf) > > +{ > > + u64 boost_ratio, cppc_max_perf; > > + > > + if (!nominal_perf) > > + return 0; > > + > > + cppc_max_perf = amd_get_highest_perf(); > > + > > + boost_ratio = div_u64(cppc_max_perf << SCHED_CAPACITY_SHIFT, > > + nominal_perf); > > + > > + return boost_ratio; > > +} > > The function above is not necessary if I'm not mistaken. > Yes, right. > > + > > static u64 get_max_boost_ratio(unsigned int cpu) > > { > > struct cppc_perf_caps perf_caps; > > @@ -646,6 +662,9 @@ static u64 get_max_boost_ratio(unsigned int cpu) > > return 0; > > } > > > > + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) > > + return get_amd_max_boost_ratio(cpu, perf_caps.nominal_perf); > > + > > highest_perf = perf_caps.highest_perf; > > The above can be written as > > if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) > highest_perf = amd_get_highest_perf(); > else > highest_perf = perf_caps.highest_perf; > Thanks to simplify the implementation. Will update it in V4. Best Regards, Ray
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index f1b9ed5efaa9..908bcaea1361 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -804,8 +804,10 @@ DECLARE_PER_CPU(u64, msr_misc_features_shadow); #ifdef CONFIG_CPU_SUP_AMD extern u32 amd_get_nodes_per_socket(void); +extern u32 amd_get_highest_perf(void); #else static inline u32 amd_get_nodes_per_socket(void) { return 0; } +static inline u32 amd_get_highest_perf(void) { return 0; } #endif static inline uint32_t hypervisor_cpuid_base(const char *sig, uint32_t leaves) diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c index 347a956f71ca..aadb691d9357 100644 --- a/arch/x86/kernel/cpu/amd.c +++ b/arch/x86/kernel/cpu/amd.c @@ -1170,3 +1170,25 @@ void set_dr_addr_mask(unsigned long mask, int dr) break; } } + +u32 amd_get_highest_perf(void) +{ + struct cpuinfo_x86 *c = &boot_cpu_data; + u32 cppc_max_perf = 225; + + switch (c->x86) { + case 0x17: + if ((c->x86_model >= 0x30 && c->x86_model < 0x40) || + (c->x86_model >= 0x70 && c->x86_model < 0x80)) + cppc_max_perf = 166; + break; + case 0x19: + if ((c->x86_model >= 0x20 && c->x86_model < 0x30) || + (c->x86_model >= 0x40 && c->x86_model < 0x70)) + cppc_max_perf = 166; + break; + } + + return cppc_max_perf; +} +EXPORT_SYMBOL_GPL(amd_get_highest_perf); diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c index 02813a7f3a7c..7bec57d04a87 100644 --- a/arch/x86/kernel/smpboot.c +++ b/arch/x86/kernel/smpboot.c @@ -2046,7 +2046,7 @@ static bool amd_set_max_freq_ratio(void) return false; } - highest_perf = perf_caps.highest_perf; + highest_perf = amd_get_highest_perf(); nominal_perf = perf_caps.nominal_perf; if (!highest_perf || !nominal_perf) { diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index d1bbc16fba4b..3f0a19dd658c 100644 --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -630,6 +630,22 @@ static int acpi_cpufreq_blacklist(struct cpuinfo_x86 *c) #endif #ifdef CONFIG_ACPI_CPPC_LIB + +static u64 get_amd_max_boost_ratio(unsigned int cpu, u64 nominal_perf) +{ + u64 boost_ratio, cppc_max_perf; + + if (!nominal_perf) + return 0; + + cppc_max_perf = amd_get_highest_perf(); + + boost_ratio = div_u64(cppc_max_perf << SCHED_CAPACITY_SHIFT, + nominal_perf); + + return boost_ratio; +} + static u64 get_max_boost_ratio(unsigned int cpu) { struct cppc_perf_caps perf_caps; @@ -646,6 +662,9 @@ static u64 get_max_boost_ratio(unsigned int cpu) return 0; } + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) + return get_amd_max_boost_ratio(cpu, perf_caps.nominal_perf); + highest_perf = perf_caps.highest_perf; nominal_perf = perf_caps.nominal_perf;
Some AMD Ryzen generations has different calculation method on maximum perf. 255 is not for all asics, some specific generations should use 166 as the maximum perf. Otherwise, it will report incorrect frequency value like below: ~ → lscpu | grep MHz CPU MHz: 3400.000 CPU max MHz: 7228.3198 CPU min MHz: 2200.0000 Fixes: 41ea667227ba ("x86, sched: Calculate frequency invariance for AMD systems") Fixes: 3c55e94c0ade ("cpufreq: ACPI: Extend frequency tables to cover boost frequencies") Reported-by: Jason Bagavatsingham <jason.bagavatsingham@gmail.com> Tested-by: Jason Bagavatsingham <jason.bagavatsingham@gmail.com> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=211791 Signed-off-by: Huang Rui <ray.huang@amd.com> Cc: Alex Deucher <alexander.deucher@amd.com> Cc: Nathan Fontenot <nathan.fontenot@amd.com> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Cc: Borislav Petkov <bp@suse.de> Cc: x86@kernel.org Cc: stable@vger.kernel.org --- Changes from V1 -> V2: - Enhance the commit message. - Move amd_get_highest_perf() into amd.c. - Refine the implementation of switch-case. - Cc stable mail list. --- arch/x86/include/asm/processor.h | 2 ++ arch/x86/kernel/cpu/amd.c | 22 ++++++++++++++++++++++ arch/x86/kernel/smpboot.c | 2 +- drivers/cpufreq/acpi-cpufreq.c | 19 +++++++++++++++++++ 4 files changed, 44 insertions(+), 1 deletion(-)