[PATCH 0/8] Adjustments for preferred core detection

[PATCH 0/8] Adjustments for preferred core detection

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

Preferred core detection is fragile in that any CPU that reports
less than 255 for any core is assumed to be a preferred core design.
This might not always be true, so it's better to check all CPUs and
see that varying values are actually reported.

Furthermore, preferred core detection isn't used by acpi-cpufreq. So
incorrect frequencies are used unless amd-pstate is active.

This series moves preferred core detection out of amd-pstate in a more
robust fashion.  It also removes some tech debt of hardcoded values for
platforms that are actually preferred core platforms.

This branch is based off v6.11-rc5.

Mario Limonciello (8):
  x86/amd: Move amd_get_highest_perf() from amd.c to cppc.c
  x86/amd: Rename amd_get_highest_perf() to
    amd_get_boost_ratio_numerator()
  ACPI: CPPC: Adjust debug messages in amd_set_max_freq_ratio() to warn
  x86/amd: Move amd_get_highest_perf() out of amd-pstate
  x86/amd: Detect preferred cores in amd_get_boost_ratio_numerator()
  cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into
    amd_get_boost_ratio_numerator()
  cpufreq: amd-pstate: Optimize amd_pstate_update_limits()
  cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore

 arch/x86/include/asm/processor.h |  14 +--
 arch/x86/kernel/acpi/cppc.c      | 159 ++++++++++++++++++++++++++++++-
 arch/x86/kernel/cpu/amd.c        |  16 ----
 drivers/cpufreq/acpi-cpufreq.c   |  12 ++-
 drivers/cpufreq/amd-pstate.c     | 132 ++++++-------------------
 include/acpi/cppc_acpi.h         |  10 ++
 6 files changed, 203 insertions(+), 140 deletions(-)

-- 
2.43.0

[PATCH 1/8] x86/amd: Move amd_get_highest_perf() from amd.c to cppc.c

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

To prepare to let amd_get_highest_perf() detect preferred cores
it will require CPPC functions. Move amd_get_highest_perf() to
cppc.c to prepare for 'preferred core detection' rework.

No functional changes intended.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 arch/x86/kernel/acpi/cppc.c | 16 ++++++++++++++++
 arch/x86/kernel/cpu/amd.c   | 16 ----------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
index ff8f25faca3dd..7ec8f2ce859c8 100644
--- a/arch/x86/kernel/acpi/cppc.c
+++ b/arch/x86/kernel/acpi/cppc.c
@@ -116,3 +116,19 @@ void init_freq_invariance_cppc(void)
 	init_done = true;
 	mutex_unlock(&freq_invariance_lock);
 }
+
+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 (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
+			       (c->x86_model >= 0x40 && c->x86_model < 0x70)))
+		return 166;
+
+	return 255;
+}
+EXPORT_SYMBOL_GPL(amd_get_highest_perf);
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 1e0fe5f8ab84e..015971adadfc7 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -1190,22 +1190,6 @@ unsigned long amd_get_dr_addr_mask(unsigned int dr)
 }
 EXPORT_SYMBOL_GPL(amd_get_dr_addr_mask);
 
-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 (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
-			       (c->x86_model >= 0x40 && c->x86_model < 0x70)))
-		return 166;
-
-	return 255;
-}
-EXPORT_SYMBOL_GPL(amd_get_highest_perf);
-
 static void zenbleed_check_cpu(void *unused)
 {
 	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
-- 
2.43.0

RE: [PATCH 1/8] x86/amd: Move amd_get_highest_perf() from amd.c to cppc.c

[Yuan, Perry]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Mario Limonciello <superm1@kernel.org>
> Sent: Tuesday, August 27, 2024 5:14 AM
> To: Borislav Petkov <bp@alien8.de>; Shenoy, Gautham Ranjal
> <gautham.shenoy@amd.com>; Yuan, Perry <Perry.Yuan@amd.com>
> Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) <x86@kernel.org>;
> Rafael J . Wysocki <rafael@kernel.org>; open list:X86 ARCHITECTURE (32-BIT
> AND 64-BIT) <linux-kernel@vger.kernel.org>; open list:ACPI <linux-
> acpi@vger.kernel.org>; open list:CPU FREQUENCY SCALING FRAMEWORK
> <linux-pm@vger.kernel.org>; Limonciello, Mario
> <Mario.Limonciello@amd.com>
> Subject: [PATCH 1/8] x86/amd: Move amd_get_highest_perf() from amd.c to
> cppc.c
>
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> To prepare to let amd_get_highest_perf() detect preferred cores it will require
> CPPC functions. Move amd_get_highest_perf() to cppc.c to prepare for
> 'preferred core detection' rework.
>
> No functional changes intended.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  arch/x86/kernel/acpi/cppc.c | 16 ++++++++++++++++
>  arch/x86/kernel/cpu/amd.c   | 16 ----------------
>  2 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c index
> ff8f25faca3dd..7ec8f2ce859c8 100644
> --- a/arch/x86/kernel/acpi/cppc.c
> +++ b/arch/x86/kernel/acpi/cppc.c
> @@ -116,3 +116,19 @@ void init_freq_invariance_cppc(void)
>       init_done = true;
>       mutex_unlock(&freq_invariance_lock);
>  }
> +
> +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 (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model <
> 0x30) ||
> +                            (c->x86_model >= 0x40 && c->x86_model <
> 0x70)))
> +             return 166;
> +
> +     return 255;
> +}
> +EXPORT_SYMBOL_GPL(amd_get_highest_perf);
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c index
> 1e0fe5f8ab84e..015971adadfc7 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -1190,22 +1190,6 @@ unsigned long amd_get_dr_addr_mask(unsigned
> int dr)  }  EXPORT_SYMBOL_GPL(amd_get_dr_addr_mask);
>
> -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 (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model <
> 0x30) ||
> -                            (c->x86_model >= 0x40 && c->x86_model <
> 0x70)))
> -             return 166;
> -
> -     return 255;
> -}
> -EXPORT_SYMBOL_GPL(amd_get_highest_perf);
> -
>  static void zenbleed_check_cpu(void *unused)  {
>       struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
> --
> 2.43.0
>

LGTM, thanks!

Reviewed-by: Perry Yuan <perry.yuan@amd.com>

Re: [PATCH 1/8] x86/amd: Move amd_get_highest_perf() from amd.c to cppc.c

[Gautham R. Shenoy]

On Mon, Aug 26, 2024 at 04:13:51PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> To prepare to let amd_get_highest_perf() detect preferred cores
> it will require CPPC functions. Move amd_get_highest_perf() to
> cppc.c to prepare for 'preferred core detection' rework.
> 
> No functional changes intended.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.

> ---
>  arch/x86/kernel/acpi/cppc.c | 16 ++++++++++++++++
>  arch/x86/kernel/cpu/amd.c   | 16 ----------------
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
> index ff8f25faca3dd..7ec8f2ce859c8 100644
> --- a/arch/x86/kernel/acpi/cppc.c
> +++ b/arch/x86/kernel/acpi/cppc.c
> @@ -116,3 +116,19 @@ void init_freq_invariance_cppc(void)
>  	init_done = true;
>  	mutex_unlock(&freq_invariance_lock);
>  }
> +
> +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 (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
> +			       (c->x86_model >= 0x40 && c->x86_model < 0x70)))
> +		return 166;
> +
> +	return 255;
> +}
> +EXPORT_SYMBOL_GPL(amd_get_highest_perf);
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 1e0fe5f8ab84e..015971adadfc7 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -1190,22 +1190,6 @@ unsigned long amd_get_dr_addr_mask(unsigned int dr)
>  }
>  EXPORT_SYMBOL_GPL(amd_get_dr_addr_mask);
>  
> -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 (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
> -			       (c->x86_model >= 0x40 && c->x86_model < 0x70)))
> -		return 166;
> -
> -	return 255;
> -}
> -EXPORT_SYMBOL_GPL(amd_get_highest_perf);
> -
>  static void zenbleed_check_cpu(void *unused)
>  {
>  	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
> -- 
> 2.43.0
> 

Re: [PATCH 1/8] x86/amd: Move amd_get_highest_perf() from amd.c to cppc.c

[kernel test robot]

Hi Mario,

kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge tip/x86/core tip/master linus/master v6.11-rc5 next-20240827]
[cannot apply to tip/auto-latest]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/x86-amd-Move-amd_get_highest_perf-from-amd-c-to-cppc-c/20240827-051648
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20240826211358.2694603-2-superm1%40kernel.org
patch subject: [PATCH 1/8] x86/amd: Move amd_get_highest_perf() from amd.c to cppc.c
config: x86_64-randconfig-074-20240828 (https://download.01.org/0day-ci/archive/20240828/202408281228.SdvJBD9o-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240828/202408281228.SdvJBD9o-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408281228.SdvJBD9o-lkp@intel.com/

All errors (new ones prefixed by >>):

>> arch/x86/kernel/acpi/cppc.c:120:5: error: redefinition of 'amd_get_highest_perf'
     120 | u32 amd_get_highest_perf(void)
         |     ^~~~~~~~~~~~~~~~~~~~
   In file included from arch/x86/include/asm/cpufeature.h:5,
                    from arch/x86/include/asm/thread_info.h:59,
                    from include/linux/thread_info.h:60,
                    from include/linux/spinlock.h:60,
                    from include/linux/mmzone.h:8,
                    from include/linux/gfp.h:7,
                    from include/linux/slab.h:16,
                    from include/linux/resource_ext.h:11,
                    from include/linux/acpi.h:13,
                    from include/acpi/cppc_acpi.h:13,
                    from arch/x86/kernel/acpi/cppc.c:7:
   arch/x86/include/asm/processor.h:708:19: note: previous definition of 'amd_get_highest_perf' with type 'u32(void)' {aka 'unsigned int(void)'}
     708 | static inline u32 amd_get_highest_perf(void)            { return 0; }
         |                   ^~~~~~~~~~~~~~~~~~~~


vim +/amd_get_highest_perf +120 arch/x86/kernel/acpi/cppc.c

   119	
 > 120	u32 amd_get_highest_perf(void)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 2/8] x86/amd: Rename amd_get_highest_perf() to amd_get_boost_ratio_numerator()

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

The function name is ambiguous because it returns an intermediate value
for calculating maximum frequency rather than the CPPC 'Highest Perf'
register.

Rename the function to clarify its use and allow the function to return
errors. Adjust the consumer in acpi-cpufreq to catch errors.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 arch/x86/include/asm/processor.h |  3 ---
 arch/x86/kernel/acpi/cppc.c      | 38 +++++++++++++++++++++++---------
 drivers/cpufreq/acpi-cpufreq.c   | 12 +++++++---
 include/acpi/cppc_acpi.h         |  6 +++++
 4 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index a75a07f4931fd..775acbdea1a96 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -691,8 +691,6 @@ static inline u32 per_cpu_l2c_id(unsigned int cpu)
 }
 
 #ifdef CONFIG_CPU_SUP_AMD
-extern u32 amd_get_highest_perf(void);
-
 /*
  * Issue a DIV 0/1 insn to clear any division data from previous DIV
  * operations.
@@ -705,7 +703,6 @@ static __always_inline void amd_clear_divider(void)
 
 extern void amd_check_microcode(void);
 #else
-static inline u32 amd_get_highest_perf(void)		{ return 0; }
 static inline void amd_clear_divider(void)		{ }
 static inline void amd_check_microcode(void)		{ }
 #endif
diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
index 7ec8f2ce859c8..1d631ac5ec328 100644
--- a/arch/x86/kernel/acpi/cppc.c
+++ b/arch/x86/kernel/acpi/cppc.c
@@ -79,11 +79,13 @@ static void amd_set_max_freq_ratio(void)
 		return;
 	}
 
-	highest_perf = amd_get_highest_perf();
+	rc = amd_get_boost_ratio_numerator(0, &highest_perf);
+	if (rc)
+		pr_debug("Could not retrieve highest performance\n");
 	nominal_perf = perf_caps.nominal_perf;
 
-	if (!highest_perf || !nominal_perf) {
-		pr_debug("Could not retrieve highest or nominal performance\n");
+	if (!nominal_perf) {
+		pr_debug("Could not retrieve nominal performance\n");
 		return;
 	}
 
@@ -117,18 +119,34 @@ void init_freq_invariance_cppc(void)
 	mutex_unlock(&freq_invariance_lock);
 }
 
-u32 amd_get_highest_perf(void)
+/**
+ * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation
+ * @cpu: CPU to get numerator for.
+ * @numerator: Output variable for numerator.
+ *
+ * Determine the numerator to use for calculating the boost ratio on
+ * a CPU. On systems that support preferred cores, this will be a hardcoded
+ * value. On other systems this will the highest performance register value.
+ *
+ * Return: 0 for success, negative error code otherwise.
+ */
+int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
 {
 	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;
+			       (c->x86_model >= 0x70 && c->x86_model < 0x80))) {
+		*numerator = 166;
+		return 0;
+	}
 
 	if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
-			       (c->x86_model >= 0x40 && c->x86_model < 0x70)))
-		return 166;
+			       (c->x86_model >= 0x40 && c->x86_model < 0x70))) {
+		*numerator = 166;
+		return 0;
+	}
+	*numerator = 255;
 
-	return 255;
+	return 0;
 }
-EXPORT_SYMBOL_GPL(amd_get_highest_perf);
+EXPORT_SYMBOL_GPL(amd_get_boost_ratio_numerator);
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index a8ca625a98b89..0f04feb6cafaf 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -642,10 +642,16 @@ static u64 get_max_boost_ratio(unsigned int cpu)
 		return 0;
 	}
 
-	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
-		highest_perf = amd_get_highest_perf();
-	else
+	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
+		ret = amd_get_boost_ratio_numerator(cpu, &highest_perf);
+		if (ret) {
+			pr_debug("CPU%d: Unable to get boost ratio numerator (%d)\n",
+				 cpu, ret);
+			return 0;
+		}
+	} else {
 		highest_perf = perf_caps.highest_perf;
+	}
 
 	nominal_perf = perf_caps.nominal_perf;
 
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 930b6afba6f4d..f25a881cd46dd 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -136,6 +136,12 @@ struct cppc_cpudata {
 	cpumask_var_t shared_cpu_map;
 };
 
+#ifdef CONFIG_CPU_SUP_AMD
+extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
+#else /* !CONFIG_CPU_SUP_AMD */
+static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; }
+#endif /* !CONFIG_CPU_SUP_AMD */
+
 #ifdef CONFIG_ACPI_CPPC_LIB
 extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
 extern int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf);
-- 
2.43.0

Re: [PATCH 2/8] x86/amd: Rename amd_get_highest_perf() to amd_get_boost_ratio_numerator()

[Gautham R. Shenoy]

Hello Mario,

On Mon, Aug 26, 2024 at 04:13:52PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> The function name is ambiguous because it returns an intermediate value
> for calculating maximum frequency rather than the CPPC 'Highest Perf'
> register.
> 
> Rename the function to clarify its use and allow the function to return
> errors. Adjust the consumer in acpi-cpufreq to catch errors.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
[..snip..]

> --- a/arch/x86/kernel/acpi/cppc.c
> +++ b/arch/x86/kernel/acpi/cppc.c
> @@ -79,11 +79,13 @@ static void amd_set_max_freq_ratio(void)
>  		return;
>  	}
>  
> -	highest_perf = amd_get_highest_perf();
> +	rc = amd_get_boost_ratio_numerator(0, &highest_perf);

The variable is still named highest_perf, here! I suppose that will
change in a subsequent patch?



> +	if (rc)
> +		pr_debug("Could not retrieve highest performance\n");

I understand that amd_get_boost_ratio_numerator() always returns a 0
in this patch and thus rc == 0, which means we never enter this "if"
condition.

However, when rc is non-zero, shouldn't this function return after
printing the debug message?

--
Thanks and Regards
gautham.




>  	nominal_perf = perf_caps.nominal_perf;
>  
> -	if (!highest_perf || !nominal_perf) {
> -		pr_debug("Could not retrieve highest or nominal performance\n");
> +	if (!nominal_perf) {
> +		pr_debug("Could not retrieve nominal performance\n");
>  		return;
>  	}
>  
> @@ -117,18 +119,34 @@ void init_freq_invariance_cppc(void)
>  	mutex_unlock(&freq_invariance_lock);
>  }
>  
> -u32 amd_get_highest_perf(void)
> +/**
> + * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation
> + * @cpu: CPU to get numerator for.
> + * @numerator: Output variable for numerator.
> + *
> + * Determine the numerator to use for calculating the boost ratio on
> + * a CPU. On systems that support preferred cores, this will be a hardcoded
> + * value. On other systems this will the highest performance register value.
> + *
> + * Return: 0 for success, negative error code otherwise.
> + */
> +int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
>  {
>  	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;
> +			       (c->x86_model >= 0x70 && c->x86_model < 0x80))) {
> +		*numerator = 166;
> +		return 0;
> +	}
>  
>  	if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
> -			       (c->x86_model >= 0x40 && c->x86_model < 0x70)))
> -		return 166;
> +			       (c->x86_model >= 0x40 && c->x86_model < 0x70))) {
> +		*numerator = 166;
> +		return 0;
> +	}
> +	*numerator = 255;
>  
> -	return 255;
> +	return 0;
>  }
> -EXPORT_SYMBOL_GPL(amd_get_highest_perf);
> +EXPORT_SYMBOL_GPL(amd_get_boost_ratio_numerator);
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index a8ca625a98b89..0f04feb6cafaf 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -642,10 +642,16 @@ static u64 get_max_boost_ratio(unsigned int cpu)
>  		return 0;
>  	}
>  
> -	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
> -		highest_perf = amd_get_highest_perf();
> -	else
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
> +		ret = amd_get_boost_ratio_numerator(cpu, &highest_perf);
> +		if (ret) {
> +			pr_debug("CPU%d: Unable to get boost ratio numerator (%d)\n",
> +				 cpu, ret);
> +			return 0;
> +		}
> +	} else {
>  		highest_perf = perf_caps.highest_perf;
> +	}
>  
>  	nominal_perf = perf_caps.nominal_perf;
>  
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index 930b6afba6f4d..f25a881cd46dd 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -136,6 +136,12 @@ struct cppc_cpudata {
>  	cpumask_var_t shared_cpu_map;
>  };
>  
> +#ifdef CONFIG_CPU_SUP_AMD
> +extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
> +#else /* !CONFIG_CPU_SUP_AMD */
> +static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; }
> +#endif /* !CONFIG_CPU_SUP_AMD */
> +
>  #ifdef CONFIG_ACPI_CPPC_LIB
>  extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
>  extern int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf);
> -- 
> 2.43.0
> 

Re: [PATCH 2/8] x86/amd: Rename amd_get_highest_perf() to amd_get_boost_ratio_numerator()

[Mario Limonciello]

On 8/27/2024 09:42, Gautham R. Shenoy wrote:
> Hello Mario,
> 
> On Mon, Aug 26, 2024 at 04:13:52PM -0500, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> The function name is ambiguous because it returns an intermediate value
>> for calculating maximum frequency rather than the CPPC 'Highest Perf'
>> register.
>>
>> Rename the function to clarify its use and allow the function to return
>> errors. Adjust the consumer in acpi-cpufreq to catch errors.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> [..snip..]
> 
>> --- a/arch/x86/kernel/acpi/cppc.c
>> +++ b/arch/x86/kernel/acpi/cppc.c
>> @@ -79,11 +79,13 @@ static void amd_set_max_freq_ratio(void)
>>   		return;
>>   	}
>>   
>> -	highest_perf = amd_get_highest_perf();
>> +	rc = amd_get_boost_ratio_numerator(0, &highest_perf);
> 
> The variable is still named highest_perf, here! I suppose that will
> change in a subsequent patch?
> 
> 
> 
>> +	if (rc)
>> +		pr_debug("Could not retrieve highest performance\n");
> 
> I understand that amd_get_boost_ratio_numerator() always returns a 0
> in this patch and thus rc == 0, which means we never enter this "if"
> condition.
> 
> However, when rc is non-zero, shouldn't this function return after
> printing the debug message?

Both good points.  Will fix for v2.

> 
> --
> Thanks and Regards
> gautham.
> 
> 
> 
> 
>>   	nominal_perf = perf_caps.nominal_perf;
>>   
>> -	if (!highest_perf || !nominal_perf) {
>> -		pr_debug("Could not retrieve highest or nominal performance\n");
>> +	if (!nominal_perf) {
>> +		pr_debug("Could not retrieve nominal performance\n");
>>   		return;
>>   	}
>>   
>> @@ -117,18 +119,34 @@ void init_freq_invariance_cppc(void)
>>   	mutex_unlock(&freq_invariance_lock);
>>   }
>>   
>> -u32 amd_get_highest_perf(void)
>> +/**
>> + * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation
>> + * @cpu: CPU to get numerator for.
>> + * @numerator: Output variable for numerator.
>> + *
>> + * Determine the numerator to use for calculating the boost ratio on
>> + * a CPU. On systems that support preferred cores, this will be a hardcoded
>> + * value. On other systems this will the highest performance register value.
>> + *
>> + * Return: 0 for success, negative error code otherwise.
>> + */
>> +int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
>>   {
>>   	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;
>> +			       (c->x86_model >= 0x70 && c->x86_model < 0x80))) {
>> +		*numerator = 166;
>> +		return 0;
>> +	}
>>   
>>   	if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
>> -			       (c->x86_model >= 0x40 && c->x86_model < 0x70)))
>> -		return 166;
>> +			       (c->x86_model >= 0x40 && c->x86_model < 0x70))) {
>> +		*numerator = 166;
>> +		return 0;
>> +	}
>> +	*numerator = 255;
>>   
>> -	return 255;
>> +	return 0;
>>   }
>> -EXPORT_SYMBOL_GPL(amd_get_highest_perf);
>> +EXPORT_SYMBOL_GPL(amd_get_boost_ratio_numerator);
>> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
>> index a8ca625a98b89..0f04feb6cafaf 100644
>> --- a/drivers/cpufreq/acpi-cpufreq.c
>> +++ b/drivers/cpufreq/acpi-cpufreq.c
>> @@ -642,10 +642,16 @@ static u64 get_max_boost_ratio(unsigned int cpu)
>>   		return 0;
>>   	}
>>   
>> -	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
>> -		highest_perf = amd_get_highest_perf();
>> -	else
>> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
>> +		ret = amd_get_boost_ratio_numerator(cpu, &highest_perf);
>> +		if (ret) {
>> +			pr_debug("CPU%d: Unable to get boost ratio numerator (%d)\n",
>> +				 cpu, ret);
>> +			return 0;
>> +		}
>> +	} else {
>>   		highest_perf = perf_caps.highest_perf;
>> +	}
>>   
>>   	nominal_perf = perf_caps.nominal_perf;
>>   
>> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
>> index 930b6afba6f4d..f25a881cd46dd 100644
>> --- a/include/acpi/cppc_acpi.h
>> +++ b/include/acpi/cppc_acpi.h
>> @@ -136,6 +136,12 @@ struct cppc_cpudata {
>>   	cpumask_var_t shared_cpu_map;
>>   };
>>   
>> +#ifdef CONFIG_CPU_SUP_AMD
>> +extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
>> +#else /* !CONFIG_CPU_SUP_AMD */
>> +static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; }
>> +#endif /* !CONFIG_CPU_SUP_AMD */
>> +
>>   #ifdef CONFIG_ACPI_CPPC_LIB
>>   extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
>>   extern int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf);
>> -- 
>> 2.43.0
>>

Re: [PATCH 2/8] x86/amd: Rename amd_get_highest_perf() to amd_get_boost_ratio_numerator()

[kernel test robot]

Hi Mario,

kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge tip/x86/core tip/master linus/master v6.11-rc5 next-20240827]
[cannot apply to tip/auto-latest]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/x86-amd-Move-amd_get_highest_perf-from-amd-c-to-cppc-c/20240827-051648
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20240826211358.2694603-3-superm1%40kernel.org
patch subject: [PATCH 2/8] x86/amd: Rename amd_get_highest_perf() to amd_get_boost_ratio_numerator()
config: x86_64-randconfig-074-20240828 (https://download.01.org/0day-ci/archive/20240828/202408281637.ssIOSO7H-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240828/202408281637.ssIOSO7H-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408281637.ssIOSO7H-lkp@intel.com/

All errors (new ones prefixed by >>):

>> arch/x86/kernel/acpi/cppc.c:133:5: error: redefinition of 'amd_get_boost_ratio_numerator'
     133 | int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from arch/x86/kernel/acpi/cppc.c:7:
   include/acpi/cppc_acpi.h:142:19: note: previous definition of 'amd_get_boost_ratio_numerator' with type 'int(unsigned int,  u64 *)' {aka 'int(unsigned int,  long long unsigned int *)'}
     142 | static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; }
         |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/amd_get_boost_ratio_numerator +133 arch/x86/kernel/acpi/cppc.c

   121	
   122	/**
   123	 * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation
   124	 * @cpu: CPU to get numerator for.
   125	 * @numerator: Output variable for numerator.
   126	 *
   127	 * Determine the numerator to use for calculating the boost ratio on
   128	 * a CPU. On systems that support preferred cores, this will be a hardcoded
   129	 * value. On other systems this will the highest performance register value.
   130	 *
   131	 * Return: 0 for success, negative error code otherwise.
   132	 */
 > 133	int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 3/8] ACPI: CPPC: Adjust debug messages in amd_set_max_freq_ratio() to warn

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

If the boost ratio isn't calculated properly for the system for any
reason this can cause other problems that are non-obvious.

Raise all messages to warn instead.

Suggested-by: Perry Yuan <Perry.Yuan@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 arch/x86/kernel/acpi/cppc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
index 1d631ac5ec328..e94507110ca24 100644
--- a/arch/x86/kernel/acpi/cppc.c
+++ b/arch/x86/kernel/acpi/cppc.c
@@ -75,17 +75,17 @@ static void amd_set_max_freq_ratio(void)
 
 	rc = cppc_get_perf_caps(0, &perf_caps);
 	if (rc) {
-		pr_debug("Could not retrieve perf counters (%d)\n", rc);
+		pr_warn("Could not retrieve perf counters (%d)\n", rc);
 		return;
 	}
 
 	rc = amd_get_boost_ratio_numerator(0, &highest_perf);
 	if (rc)
-		pr_debug("Could not retrieve highest performance\n");
+		pr_warn("Could not retrieve highest performance\n");
 	nominal_perf = perf_caps.nominal_perf;
 
 	if (!nominal_perf) {
-		pr_debug("Could not retrieve nominal performance\n");
+		pr_warn("Could not retrieve nominal performance\n");
 		return;
 	}
 
@@ -93,7 +93,7 @@ static void amd_set_max_freq_ratio(void)
 	/* midpoint between max_boost and max_P */
 	perf_ratio = (perf_ratio + SCHED_CAPACITY_SCALE) >> 1;
 	if (!perf_ratio) {
-		pr_debug("Non-zero highest/nominal perf values led to a 0 ratio\n");
+		pr_warn("Non-zero highest/nominal perf values led to a 0 ratio\n");
 		return;
 	}
 
-- 
2.43.0

RE: [PATCH 3/8] ACPI: CPPC: Adjust debug messages in amd_set_max_freq_ratio() to warn

[Yuan, Perry]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Mario Limonciello <superm1@kernel.org>
> Sent: Tuesday, August 27, 2024 5:14 AM
> To: Borislav Petkov <bp@alien8.de>; Shenoy, Gautham Ranjal
> <gautham.shenoy@amd.com>; Yuan, Perry <Perry.Yuan@amd.com>
> Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) <x86@kernel.org>;
> Rafael J . Wysocki <rafael@kernel.org>; open list:X86 ARCHITECTURE (32-BIT
> AND 64-BIT) <linux-kernel@vger.kernel.org>; open list:ACPI <linux-
> acpi@vger.kernel.org>; open list:CPU FREQUENCY SCALING FRAMEWORK
> <linux-pm@vger.kernel.org>; Limonciello, Mario
> <Mario.Limonciello@amd.com>; Yuan, Perry <Perry.Yuan@amd.com>
> Subject: [PATCH 3/8] ACPI: CPPC: Adjust debug messages in
> amd_set_max_freq_ratio() to warn
>
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> If the boost ratio isn't calculated properly for the system for any reason this
> can cause other problems that are non-obvious.
>
> Raise all messages to warn instead.
>
> Suggested-by: Perry Yuan <Perry.Yuan@amd.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  arch/x86/kernel/acpi/cppc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c index
> 1d631ac5ec328..e94507110ca24 100644
> --- a/arch/x86/kernel/acpi/cppc.c
> +++ b/arch/x86/kernel/acpi/cppc.c
> @@ -75,17 +75,17 @@ static void amd_set_max_freq_ratio(void)
>
>       rc = cppc_get_perf_caps(0, &perf_caps);
>       if (rc) {
> -             pr_debug("Could not retrieve perf counters (%d)\n", rc);
> +             pr_warn("Could not retrieve perf counters (%d)\n", rc);
>               return;
>       }
>
>       rc = amd_get_boost_ratio_numerator(0, &highest_perf);
>       if (rc)
> -             pr_debug("Could not retrieve highest performance\n");
> +             pr_warn("Could not retrieve highest performance\n");
>       nominal_perf = perf_caps.nominal_perf;
>
>       if (!nominal_perf) {
> -             pr_debug("Could not retrieve nominal performance\n");
> +             pr_warn("Could not retrieve nominal performance\n");
>               return;
>       }
>
> @@ -93,7 +93,7 @@ static void amd_set_max_freq_ratio(void)
>       /* midpoint between max_boost and max_P */
>       perf_ratio = (perf_ratio + SCHED_CAPACITY_SCALE) >> 1;
>       if (!perf_ratio) {
> -             pr_debug("Non-zero highest/nominal perf values led to a 0
> ratio\n");
> +             pr_warn("Non-zero highest/nominal perf values led to a 0
> ratio\n");
>               return;
>       }
>
> --
> 2.43.0
>

LGTM, it is good to show the warning message once there are some perf values are invalid.
That will help to debug the issue from customer report log.

Reviewed-by: Perry Yuan <perry.yuan@amd.com>


Best Regards.

Perry.

Re: [PATCH 3/8] ACPI: CPPC: Adjust debug messages in amd_set_max_freq_ratio() to warn

[Gautham R. Shenoy]

On Mon, Aug 26, 2024 at 04:13:53PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> If the boost ratio isn't calculated properly for the system for any
> reason this can cause other problems that are non-obvious.
> 
> Raise all messages to warn instead.
> 
> Suggested-by: Perry Yuan <Perry.Yuan@amd.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  arch/x86/kernel/acpi/cppc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
> index 1d631ac5ec328..e94507110ca24 100644
> --- a/arch/x86/kernel/acpi/cppc.c
> +++ b/arch/x86/kernel/acpi/cppc.c
> @@ -75,17 +75,17 @@ static void amd_set_max_freq_ratio(void)
>  
>  	rc = cppc_get_perf_caps(0, &perf_caps);
>  	if (rc) {
> -		pr_debug("Could not retrieve perf counters (%d)\n", rc);
> +		pr_warn("Could not retrieve perf counters (%d)\n", rc);
>  		return;
>  	}
>  
>  	rc = amd_get_boost_ratio_numerator(0, &highest_perf);
>  	if (rc)
> -		pr_debug("Could not retrieve highest performance\n");
> +		pr_warn("Could not retrieve highest performance\n");
>  	nominal_perf = perf_caps.nominal_perf;
>  
>  	if (!nominal_perf) {
> -		pr_debug("Could not retrieve nominal performance\n");
> +		pr_warn("Could not retrieve nominal performance\n");
>  		return;
>  	}
>  
> @@ -93,7 +93,7 @@ static void amd_set_max_freq_ratio(void)
>  	/* midpoint between max_boost and max_P */
>  	perf_ratio = (perf_ratio + SCHED_CAPACITY_SCALE) >> 1;
>  	if (!perf_ratio) {
> -		pr_debug("Non-zero highest/nominal perf values led to a 0 ratio\n");
> +		pr_warn("Non-zero highest/nominal perf values led to a 0 ratio\n");
>  		return;

Aside:
perf_ratio is a u64, and SCHED_CAPACITY_SCALE is (1L << 10). Thus, is
it even possible to have !perf_ratio?

Otherwise, I am ok with this promotion of pr_debug to pr_warn.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.

Re: [PATCH 3/8] ACPI: CPPC: Adjust debug messages in amd_set_max_freq_ratio() to warn

[Mario Limonciello]

On 8/27/2024 09:50, Gautham R. Shenoy wrote:
> On Mon, Aug 26, 2024 at 04:13:53PM -0500, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> If the boost ratio isn't calculated properly for the system for any
>> reason this can cause other problems that are non-obvious.
>>
>> Raise all messages to warn instead.
>>
>> Suggested-by: Perry Yuan <Perry.Yuan@amd.com>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   arch/x86/kernel/acpi/cppc.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
>> index 1d631ac5ec328..e94507110ca24 100644
>> --- a/arch/x86/kernel/acpi/cppc.c
>> +++ b/arch/x86/kernel/acpi/cppc.c
>> @@ -75,17 +75,17 @@ static void amd_set_max_freq_ratio(void)
>>   
>>   	rc = cppc_get_perf_caps(0, &perf_caps);
>>   	if (rc) {
>> -		pr_debug("Could not retrieve perf counters (%d)\n", rc);
>> +		pr_warn("Could not retrieve perf counters (%d)\n", rc);
>>   		return;
>>   	}
>>   
>>   	rc = amd_get_boost_ratio_numerator(0, &highest_perf);
>>   	if (rc)
>> -		pr_debug("Could not retrieve highest performance\n");
>> +		pr_warn("Could not retrieve highest performance\n");
>>   	nominal_perf = perf_caps.nominal_perf;
>>   
>>   	if (!nominal_perf) {
>> -		pr_debug("Could not retrieve nominal performance\n");
>> +		pr_warn("Could not retrieve nominal performance\n");
>>   		return;
>>   	}
>>   
>> @@ -93,7 +93,7 @@ static void amd_set_max_freq_ratio(void)
>>   	/* midpoint between max_boost and max_P */
>>   	perf_ratio = (perf_ratio + SCHED_CAPACITY_SCALE) >> 1;
>>   	if (!perf_ratio) {
>> -		pr_debug("Non-zero highest/nominal perf values led to a 0 ratio\n");
>> +		pr_warn("Non-zero highest/nominal perf values led to a 0 ratio\n");
>>   		return;
> 
> Aside:
> perf_ratio is a u64, and SCHED_CAPACITY_SCALE is (1L << 10). Thus, is
> it even possible to have !perf_ratio?
> 
> Otherwise, I am ok with this promotion of pr_debug to pr_warn.

You're right; I don't see this is possible.  I'll tear it out in a 
prerequisite patch in v2.

> 
> Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>
> 
> --
> Thanks and Regards
> gautham.

[PATCH 4/8] x86/amd: Move amd_get_highest_perf() out of amd-pstate

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

amd_get_highest_perf() is a helper used to get the highest perf
value on AMD systems.  It's used in amd-pstate as part of preferred
core handling, but applicable for acpi-cpufreq as well.

Move it out to cppc handling code.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 arch/x86/kernel/acpi/cppc.c  | 30 ++++++++++++++++++++++++++++++
 drivers/cpufreq/amd-pstate.c | 34 ++--------------------------------
 include/acpi/cppc_acpi.h     |  2 ++
 3 files changed, 34 insertions(+), 32 deletions(-)

diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
index e94507110ca24..5a6c01a1b0d95 100644
--- a/arch/x86/kernel/acpi/cppc.c
+++ b/arch/x86/kernel/acpi/cppc.c
@@ -119,6 +119,36 @@ void init_freq_invariance_cppc(void)
 	mutex_unlock(&freq_invariance_lock);
 }
 
+/*
+ * Get the highest performance register value.
+ * @cpu: CPU from which to get highest performance.
+ * @highest_perf: Return address for highest performance value.
+ *
+ * Return: 0 for success, negative error code otherwise.
+ */
+int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf)
+{
+	u64 val;
+	int ret;
+
+	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
+		ret = rdmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_CAP1, &val);
+		if (ret)
+			goto out;
+
+		val = AMD_CPPC_HIGHEST_PERF(val);
+	} else {
+		ret = cppc_get_highest_perf(cpu, &val);
+		if (ret)
+			goto out;
+	}
+
+	WRITE_ONCE(*highest_perf, (u32)val);
+out:
+	return ret;
+}
+EXPORT_SYMBOL_GPL(amd_get_highest_perf);
+
 /**
  * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation
  * @cpu: CPU to get numerator for.
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 89bda7a2bb8d1..f470b5700db58 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -803,36 +803,6 @@ static void amd_pstste_sched_prefcore_workfn(struct work_struct *work)
 }
 static DECLARE_WORK(sched_prefcore_work, amd_pstste_sched_prefcore_workfn);
 
-/*
- * Get the highest performance register value.
- * @cpu: CPU from which to get highest performance.
- * @highest_perf: Return address.
- *
- * Return: 0 for success, -EIO otherwise.
- */
-static int amd_pstate_get_highest_perf(int cpu, u32 *highest_perf)
-{
-	int ret;
-
-	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
-		u64 cap1;
-
-		ret = rdmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_CAP1, &cap1);
-		if (ret)
-			return ret;
-		WRITE_ONCE(*highest_perf, AMD_CPPC_HIGHEST_PERF(cap1));
-	} else {
-		u64 cppc_highest_perf;
-
-		ret = cppc_get_highest_perf(cpu, &cppc_highest_perf);
-		if (ret)
-			return ret;
-		WRITE_ONCE(*highest_perf, cppc_highest_perf);
-	}
-
-	return (ret);
-}
-
 #define CPPC_MAX_PERF	U8_MAX
 
 static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
@@ -840,7 +810,7 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
 	int ret, prio;
 	u32 highest_perf;
 
-	ret = amd_pstate_get_highest_perf(cpudata->cpu, &highest_perf);
+	ret = amd_get_highest_perf(cpudata->cpu, &highest_perf);
 	if (ret)
 		return;
 
@@ -879,7 +849,7 @@ static void amd_pstate_update_limits(unsigned int cpu)
 	if ((!amd_pstate_prefcore) || (!cpudata->hw_prefcore))
 		goto free_cpufreq_put;
 
-	ret = amd_pstate_get_highest_perf(cpu, &cur_high);
+	ret = amd_get_highest_perf(cpu, &cur_high);
 	if (ret)
 		goto free_cpufreq_put;
 
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index f25a881cd46dd..2246ce0630362 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -137,8 +137,10 @@ struct cppc_cpudata {
 };
 
 #ifdef CONFIG_CPU_SUP_AMD
+extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
 extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
 #else /* !CONFIG_CPU_SUP_AMD */
+static inline int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf) { return -ENODEV; }
 static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; }
 #endif /* !CONFIG_CPU_SUP_AMD */
 
-- 
2.43.0

RE: [PATCH 4/8] x86/amd: Move amd_get_highest_perf() out of amd-pstate

[Yuan, Perry]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Mario Limonciello <superm1@kernel.org>
> Sent: Tuesday, August 27, 2024 5:14 AM
> To: Borislav Petkov <bp@alien8.de>; Shenoy, Gautham Ranjal
> <gautham.shenoy@amd.com>; Yuan, Perry <Perry.Yuan@amd.com>
> Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) <x86@kernel.org>;
> Rafael J . Wysocki <rafael@kernel.org>; open list:X86 ARCHITECTURE (32-BIT
> AND 64-BIT) <linux-kernel@vger.kernel.org>; open list:ACPI <linux-
> acpi@vger.kernel.org>; open list:CPU FREQUENCY SCALING FRAMEWORK
> <linux-pm@vger.kernel.org>; Limonciello, Mario
> <Mario.Limonciello@amd.com>
> Subject: [PATCH 4/8] x86/amd: Move amd_get_highest_perf() out of amd-
> pstate
>
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> amd_get_highest_perf() is a helper used to get the highest perf value on AMD
> systems.  It's used in amd-pstate as part of preferred core handling, but
> applicable for acpi-cpufreq as well.
>
> Move it out to cppc handling code.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  arch/x86/kernel/acpi/cppc.c  | 30 ++++++++++++++++++++++++++++++
> drivers/cpufreq/amd-pstate.c | 34 ++--------------------------------
>  include/acpi/cppc_acpi.h     |  2 ++
>  3 files changed, 34 insertions(+), 32 deletions(-)
>
> diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c index
> e94507110ca24..5a6c01a1b0d95 100644
> --- a/arch/x86/kernel/acpi/cppc.c
> +++ b/arch/x86/kernel/acpi/cppc.c
> @@ -119,6 +119,36 @@ void init_freq_invariance_cppc(void)
>       mutex_unlock(&freq_invariance_lock);
>  }
>
> +/*
> + * Get the highest performance register value.
> + * @cpu: CPU from which to get highest performance.
> + * @highest_perf: Return address for highest performance value.
> + *
> + * Return: 0 for success, negative error code otherwise.
> + */
> +int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf) {
> +     u64 val;
> +     int ret;
> +
> +     if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
> +             ret = rdmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_CAP1, &val);
> +             if (ret)
> +                     goto out;
> +
> +             val = AMD_CPPC_HIGHEST_PERF(val);
> +     } else {
> +             ret = cppc_get_highest_perf(cpu, &val);
> +             if (ret)
> +                     goto out;
> +     }
> +
> +     WRITE_ONCE(*highest_perf, (u32)val);
> +out:
> +     return ret;
> +}
> +EXPORT_SYMBOL_GPL(amd_get_highest_perf);
> +
>  /**
>   * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio
> calculation
>   * @cpu: CPU to get numerator for.
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 89bda7a2bb8d1..f470b5700db58 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -803,36 +803,6 @@ static void
> amd_pstste_sched_prefcore_workfn(struct work_struct *work)  }  static
> DECLARE_WORK(sched_prefcore_work,
> amd_pstste_sched_prefcore_workfn);
>
> -/*
> - * Get the highest performance register value.
> - * @cpu: CPU from which to get highest performance.
> - * @highest_perf: Return address.
> - *
> - * Return: 0 for success, -EIO otherwise.
> - */
> -static int amd_pstate_get_highest_perf(int cpu, u32 *highest_perf) -{
> -     int ret;
> -
> -     if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
> -             u64 cap1;
> -
> -             ret = rdmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_CAP1,
> &cap1);
> -             if (ret)
> -                     return ret;
> -             WRITE_ONCE(*highest_perf,
> AMD_CPPC_HIGHEST_PERF(cap1));
> -     } else {
> -             u64 cppc_highest_perf;
> -
> -             ret = cppc_get_highest_perf(cpu, &cppc_highest_perf);
> -             if (ret)
> -                     return ret;
> -             WRITE_ONCE(*highest_perf, cppc_highest_perf);
> -     }
> -
> -     return (ret);
> -}
> -
>  #define CPPC_MAX_PERF        U8_MAX
>
>  static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata) @@ -
> 840,7 +810,7 @@ static void amd_pstate_init_prefcore(struct amd_cpudata
> *cpudata)
>       int ret, prio;
>       u32 highest_perf;
>
> -     ret = amd_pstate_get_highest_perf(cpudata->cpu, &highest_perf);
> +     ret = amd_get_highest_perf(cpudata->cpu, &highest_perf);
>       if (ret)
>               return;
>
> @@ -879,7 +849,7 @@ static void amd_pstate_update_limits(unsigned int
> cpu)
>       if ((!amd_pstate_prefcore) || (!cpudata->hw_prefcore))
>               goto free_cpufreq_put;
>
> -     ret = amd_pstate_get_highest_perf(cpu, &cur_high);
> +     ret = amd_get_highest_perf(cpu, &cur_high);
>       if (ret)
>               goto free_cpufreq_put;
>
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h index
> f25a881cd46dd..2246ce0630362 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -137,8 +137,10 @@ struct cppc_cpudata {  };
>
>  #ifdef CONFIG_CPU_SUP_AMD
> +extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
>  extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64
> *numerator);  #else /* !CONFIG_CPU_SUP_AMD */
> +static inline int amd_get_highest_perf(unsigned int cpu, u32
> +*highest_perf) { return -ENODEV; }
>  static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64
> *numerator) { return -ENODEV; }  #endif /* !CONFIG_CPU_SUP_AMD */
>
> --
> 2.43.0
>

LGTM, thank you.

Reviewed-by: Perry Yuan <perry.yuan@amd.com>


Best Regards.

Perry.

Re: [PATCH 4/8] x86/amd: Move amd_get_highest_perf() out of amd-pstate

[Gautham R. Shenoy]

On Mon, Aug 26, 2024 at 04:13:54PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> amd_get_highest_perf() is a helper used to get the highest perf

Nit: amd_pstate_get_highest_perf() is the existing helper, that is
being moved to the cppc handling code and being renamed as
amd_get_highest_perf().


> value on AMD systems.  It's used in amd-pstate as part of preferred
> core handling, but applicable for acpi-cpufreq as well.
> 
> Move it out to cppc handling code.

Otherwise, looks good to me.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.



> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  arch/x86/kernel/acpi/cppc.c  | 30 ++++++++++++++++++++++++++++++
>  drivers/cpufreq/amd-pstate.c | 34 ++--------------------------------
>  include/acpi/cppc_acpi.h     |  2 ++
>  3 files changed, 34 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
> index e94507110ca24..5a6c01a1b0d95 100644
> --- a/arch/x86/kernel/acpi/cppc.c
> +++ b/arch/x86/kernel/acpi/cppc.c
> @@ -119,6 +119,36 @@ void init_freq_invariance_cppc(void)
>  	mutex_unlock(&freq_invariance_lock);
>  }
>  
> +/*
> + * Get the highest performance register value.
> + * @cpu: CPU from which to get highest performance.
> + * @highest_perf: Return address for highest performance value.
> + *
> + * Return: 0 for success, negative error code otherwise.
> + */
> +int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf)
> +{
> +	u64 val;
> +	int ret;
> +
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
> +		ret = rdmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_CAP1, &val);
> +		if (ret)
> +			goto out;
> +
> +		val = AMD_CPPC_HIGHEST_PERF(val);
> +	} else {
> +		ret = cppc_get_highest_perf(cpu, &val);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	WRITE_ONCE(*highest_perf, (u32)val);
> +out:
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(amd_get_highest_perf);
> +
>  /**
>   * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation
>   * @cpu: CPU to get numerator for.
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 89bda7a2bb8d1..f470b5700db58 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -803,36 +803,6 @@ static void amd_pstste_sched_prefcore_workfn(struct work_struct *work)
>  }
>  static DECLARE_WORK(sched_prefcore_work, amd_pstste_sched_prefcore_workfn);
>  
> -/*
> - * Get the highest performance register value.
> - * @cpu: CPU from which to get highest performance.
> - * @highest_perf: Return address.
> - *
> - * Return: 0 for success, -EIO otherwise.
> - */
> -static int amd_pstate_get_highest_perf(int cpu, u32 *highest_perf)
> -{
> -	int ret;
> -
> -	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
> -		u64 cap1;
> -
> -		ret = rdmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_CAP1, &cap1);
> -		if (ret)
> -			return ret;
> -		WRITE_ONCE(*highest_perf, AMD_CPPC_HIGHEST_PERF(cap1));
> -	} else {
> -		u64 cppc_highest_perf;
> -
> -		ret = cppc_get_highest_perf(cpu, &cppc_highest_perf);
> -		if (ret)
> -			return ret;
> -		WRITE_ONCE(*highest_perf, cppc_highest_perf);
> -	}
> -
> -	return (ret);
> -}
> -
>  #define CPPC_MAX_PERF	U8_MAX
>  
>  static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
> @@ -840,7 +810,7 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
>  	int ret, prio;
>  	u32 highest_perf;
>  
> -	ret = amd_pstate_get_highest_perf(cpudata->cpu, &highest_perf);
> +	ret = amd_get_highest_perf(cpudata->cpu, &highest_perf);
>  	if (ret)
>  		return;
>  
> @@ -879,7 +849,7 @@ static void amd_pstate_update_limits(unsigned int cpu)
>  	if ((!amd_pstate_prefcore) || (!cpudata->hw_prefcore))
>  		goto free_cpufreq_put;
>  
> -	ret = amd_pstate_get_highest_perf(cpu, &cur_high);
> +	ret = amd_get_highest_perf(cpu, &cur_high);
>  	if (ret)
>  		goto free_cpufreq_put;
>  
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index f25a881cd46dd..2246ce0630362 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -137,8 +137,10 @@ struct cppc_cpudata {
>  };
>  
>  #ifdef CONFIG_CPU_SUP_AMD
> +extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
>  extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
>  #else /* !CONFIG_CPU_SUP_AMD */
> +static inline int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf) { return -ENODEV; }
>  static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; }
>  #endif /* !CONFIG_CPU_SUP_AMD */
>  
> -- 
> 2.43.0
> 

[PATCH 5/8] x86/amd: Detect preferred cores in amd_get_boost_ratio_numerator()

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

AMD systems that support preferred cores will use "166" as their
numerator for max frequency calculations instead of "255".

Add a function for detecting preferred cores by looking at the
highest perf value on all cores.

If preferred cores are enabled return 166 and if disabled the
value in the highest perf register. As the function will be called
multiple times, cache the values for the boost numerator and if
preferred cores will be enabled in global variables.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 arch/x86/kernel/acpi/cppc.c  | 89 ++++++++++++++++++++++++++++++++----
 drivers/cpufreq/amd-pstate.c | 34 ++++++--------
 include/acpi/cppc_acpi.h     |  2 +
 3 files changed, 94 insertions(+), 31 deletions(-)

diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
index 5a6c01a1b0d95..729b35e84f5eb 100644
--- a/arch/x86/kernel/acpi/cppc.c
+++ b/arch/x86/kernel/acpi/cppc.c
@@ -9,6 +9,16 @@
 #include <asm/processor.h>
 #include <asm/topology.h>
 
+#define CPPC_HIGHEST_PERF_PREFCORE	166
+
+enum amd_pref_core {
+	AMD_PREF_CORE_UNKNOWN = 0,
+	AMD_PREF_CORE_SUPPORTED,
+	AMD_PREF_CORE_UNSUPPORTED,
+};
+static enum amd_pref_core amd_pref_core_detected;
+static u64 boost_numerator;
+
 /* Refer to drivers/acpi/cppc_acpi.c for the description of functions */
 
 bool cpc_supported_by_cpu(void)
@@ -149,6 +159,66 @@ int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf)
 }
 EXPORT_SYMBOL_GPL(amd_get_highest_perf);
 
+/**
+ * amd_detect_prefcore: Detect if CPUs in the system support preferred cores
+ * @detected: Output variable for the result of the detection.
+ *
+ * Determine whether CPUs in the system support preferred cores. On systems
+ * that support preferred cores, different highest perf values will be found
+ * on different cores. On other systems, the highest perf value will be the
+ * same on all cores.
+ *
+ * The result of the detection will be stored in the 'detected' parameter.
+ *
+ * Return: 0 for success, negative error code otherwise
+ */
+int amd_detect_prefcore(bool *detected)
+{
+	int cpu, count = 0;
+	u64 highest_perf[2] = {0};
+
+	if (WARN_ON(!detected))
+		return -EINVAL;
+
+	switch (amd_pref_core_detected) {
+	case AMD_PREF_CORE_SUPPORTED:
+		*detected = true;
+		return 0;
+	case AMD_PREF_CORE_UNSUPPORTED:
+		*detected = false;
+		return 0;
+	default:
+		break;
+	}
+
+	for_each_present_cpu(cpu) {
+		u32 tmp;
+		int ret;
+
+		ret = amd_get_highest_perf(cpu, &tmp);
+		if (ret)
+			return ret;
+
+		if (!count || (count == 1 && tmp != highest_perf[0]))
+			highest_perf[count++] = tmp;
+
+		if (count == 2)
+			break;
+	}
+
+	*detected = (count == 2);
+	boost_numerator = highest_perf[0];
+
+	amd_pref_core_detected = *detected ? AMD_PREF_CORE_SUPPORTED :
+					     AMD_PREF_CORE_UNSUPPORTED;
+
+	pr_debug("AMD CPPC preferred core is %ssupported (highest perf: 0x%llx)\n",
+		 *detected ? "" : "un", highest_perf[0]);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(amd_detect_prefcore);
+
 /**
  * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation
  * @cpu: CPU to get numerator for.
@@ -162,20 +232,19 @@ EXPORT_SYMBOL_GPL(amd_get_highest_perf);
  */
 int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
 {
-	struct cpuinfo_x86 *c = &boot_cpu_data;
+	bool prefcore;
+	int ret;
 
-	if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) ||
-			       (c->x86_model >= 0x70 && c->x86_model < 0x80))) {
-		*numerator = 166;
-		return 0;
-	}
+	ret = amd_detect_prefcore(&prefcore);
+	if (ret)
+		return ret;
 
-	if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
-			       (c->x86_model >= 0x40 && c->x86_model < 0x70))) {
-		*numerator = 166;
+	/* without preferred cores, return the highest perf register value */
+	if (!prefcore) {
+		*numerator = boost_numerator;
 		return 0;
 	}
-	*numerator = 255;
+	*numerator = CPPC_HIGHEST_PERF_PREFCORE;
 
 	return 0;
 }
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index f470b5700db58..ec32c830abc1d 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -807,32 +807,18 @@ static DECLARE_WORK(sched_prefcore_work, amd_pstste_sched_prefcore_workfn);
 
 static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
 {
-	int ret, prio;
-	u32 highest_perf;
-
-	ret = amd_get_highest_perf(cpudata->cpu, &highest_perf);
-	if (ret)
+	/* user disabled or not detected */
+	if (!amd_pstate_prefcore)
 		return;
 
 	cpudata->hw_prefcore = true;
-	/* check if CPPC preferred core feature is enabled*/
-	if (highest_perf < CPPC_MAX_PERF)
-		prio = (int)highest_perf;
-	else {
-		pr_debug("AMD CPPC preferred core is unsupported!\n");
-		cpudata->hw_prefcore = false;
-		return;
-	}
-
-	if (!amd_pstate_prefcore)
-		return;
 
 	/*
 	 * The priorities can be set regardless of whether or not
 	 * sched_set_itmt_support(true) has been called and it is valid to
 	 * update them at any time after it has been called.
 	 */
-	sched_set_itmt_core_prio(prio, cpudata->cpu);
+	sched_set_itmt_core_prio((int)READ_ONCE(cpudata->highest_perf), cpudata->cpu);
 
 	schedule_work(&sched_prefcore_work);
 }
@@ -998,12 +984,12 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 
 	cpudata->cpu = policy->cpu;
 
-	amd_pstate_init_prefcore(cpudata);
-
 	ret = amd_pstate_init_perf(cpudata);
 	if (ret)
 		goto free_cpudata1;
 
+	amd_pstate_init_prefcore(cpudata);
+
 	ret = amd_pstate_init_freq(cpudata);
 	if (ret)
 		goto free_cpudata1;
@@ -1453,12 +1439,12 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
 	cpudata->cpu = policy->cpu;
 	cpudata->epp_policy = 0;
 
-	amd_pstate_init_prefcore(cpudata);
-
 	ret = amd_pstate_init_perf(cpudata);
 	if (ret)
 		goto free_cpudata1;
 
+	amd_pstate_init_prefcore(cpudata);
+
 	ret = amd_pstate_init_freq(cpudata);
 	if (ret)
 		goto free_cpudata1;
@@ -1903,6 +1889,12 @@ static int __init amd_pstate_init(void)
 		static_call_update(amd_pstate_update_perf, cppc_update_perf);
 	}
 
+	if (amd_pstate_prefcore) {
+		ret = amd_detect_prefcore(&amd_pstate_prefcore);
+		if (ret)
+			return ret;
+	}
+
 	/* enable amd pstate feature */
 	ret = amd_pstate_enable(true);
 	if (ret) {
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 2246ce0630362..1d79320a23490 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -137,10 +137,12 @@ struct cppc_cpudata {
 };
 
 #ifdef CONFIG_CPU_SUP_AMD
+extern int amd_detect_prefcore(bool *detected);
 extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
 extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
 #else /* !CONFIG_CPU_SUP_AMD */
 static inline int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf) { return -ENODEV; }
+static inline int amd_detect_prefcore(bool *detected) { return -ENODEV; }
 static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; }
 #endif /* !CONFIG_CPU_SUP_AMD */
 
-- 
2.43.0

Re: [PATCH 5/8] x86/amd: Detect preferred cores in amd_get_boost_ratio_numerator()

[Gautham R. Shenoy]

On Mon, Aug 26, 2024 at 04:13:55PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> AMD systems that support preferred cores will use "166" as their
> numerator for max frequency calculations instead of "255".
> 
> Add a function for detecting preferred cores by looking at the
> highest perf value on all cores.
> 
> If preferred cores are enabled return 166 and if disabled the
> value in the highest perf register. As the function will be called
> multiple times, cache the values for the boost numerator and if
> preferred cores will be enabled in global variables.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---

[..snip..]

>  /**
>   * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation
>   * @cpu: CPU to get numerator for.
> @@ -162,20 +232,19 @@ EXPORT_SYMBOL_GPL(amd_get_highest_perf);
>   */
>  int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
>  {
> -	struct cpuinfo_x86 *c = &boot_cpu_data;
> +	bool prefcore;
> +	int ret;
>  
> -	if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) ||
> -			       (c->x86_model >= 0x70 && c->x86_model < 0x80))) {
> -		*numerator = 166;
> -		return 0;
> -	}
> +	ret = amd_detect_prefcore(&prefcore);
> +	if (ret)
> +		return ret;
>  
> -	if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
> -			       (c->x86_model >= 0x40 && c->x86_model < 0x70))) {
> -		*numerator = 166;
> +	/* without preferred cores, return the highest perf register value */
> +	if (!prefcore) {
> +		*numerator = boost_numerator;
>  		return 0;
>  	}
> -	*numerator = 255;
> +	*numerator = CPPC_HIGHEST_PERF_PREFCORE;


Interesting. So even when the user boots a system that supports
preferred-cores with "amd_preferred=disable",
amd_get_boost_ratio_numerator() will return CPPC_HIGHEST_PERF_PREFCORE
as the call prefcore == true here.

I suppose that is as intended, since even though the user may not want
to use the preferred core logic for task-scheduling/load-balancing,
the numerator for the boost-ratio is purely dependent on the h/w
capability.

Is this understanding correct? If so, can this be included as a
comment in the code ?

The rest of the patch looks good to me.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

--
Thanks and Regards
gautham.



>  
>  	return 0;
>  }
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index f470b5700db58..ec32c830abc1d 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -807,32 +807,18 @@ static DECLARE_WORK(sched_prefcore_work, amd_pstste_sched_prefcore_workfn);
>  
>  static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
>  {
> -	int ret, prio;
> -	u32 highest_perf;
> -
> -	ret = amd_get_highest_perf(cpudata->cpu, &highest_perf);
> -	if (ret)
> +	/* user disabled or not detected */
> +	if (!amd_pstate_prefcore)
>  		return;
>  
>  	cpudata->hw_prefcore = true;
> -	/* check if CPPC preferred core feature is enabled*/
> -	if (highest_perf < CPPC_MAX_PERF)
> -		prio = (int)highest_perf;
> -	else {
> -		pr_debug("AMD CPPC preferred core is unsupported!\n");
> -		cpudata->hw_prefcore = false;
> -		return;
> -	}
> -
> -	if (!amd_pstate_prefcore)
> -		return;
>  
>  	/*
>  	 * The priorities can be set regardless of whether or not
>  	 * sched_set_itmt_support(true) has been called and it is valid to
>  	 * update them at any time after it has been called.
>  	 */
> -	sched_set_itmt_core_prio(prio, cpudata->cpu);
> +	sched_set_itmt_core_prio((int)READ_ONCE(cpudata->highest_perf), cpudata->cpu);
>  
>  	schedule_work(&sched_prefcore_work);
>  }
> @@ -998,12 +984,12 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>  
>  	cpudata->cpu = policy->cpu;
>  
> -	amd_pstate_init_prefcore(cpudata);
> -
>  	ret = amd_pstate_init_perf(cpudata);
>  	if (ret)
>  		goto free_cpudata1;
>  
> +	amd_pstate_init_prefcore(cpudata);
> +
>  	ret = amd_pstate_init_freq(cpudata);
>  	if (ret)
>  		goto free_cpudata1;
> @@ -1453,12 +1439,12 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>  	cpudata->cpu = policy->cpu;
>  	cpudata->epp_policy = 0;
>  
> -	amd_pstate_init_prefcore(cpudata);
> -
>  	ret = amd_pstate_init_perf(cpudata);
>  	if (ret)
>  		goto free_cpudata1;
>  
> +	amd_pstate_init_prefcore(cpudata);
> +
>  	ret = amd_pstate_init_freq(cpudata);
>  	if (ret)
>  		goto free_cpudata1;
> @@ -1903,6 +1889,12 @@ static int __init amd_pstate_init(void)
>  		static_call_update(amd_pstate_update_perf, cppc_update_perf);
>  	}
>  
> +	if (amd_pstate_prefcore) {
> +		ret = amd_detect_prefcore(&amd_pstate_prefcore);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/* enable amd pstate feature */
>  	ret = amd_pstate_enable(true);
>  	if (ret) {
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index 2246ce0630362..1d79320a23490 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -137,10 +137,12 @@ struct cppc_cpudata {
>  };
>  
>  #ifdef CONFIG_CPU_SUP_AMD
> +extern int amd_detect_prefcore(bool *detected);
>  extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
>  extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
>  #else /* !CONFIG_CPU_SUP_AMD */
>  static inline int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf) { return -ENODEV; }
> +static inline int amd_detect_prefcore(bool *detected) { return -ENODEV; }
>  static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; }
>  #endif /* !CONFIG_CPU_SUP_AMD */
>  
> -- 
> 2.43.0
> 

Re: [PATCH 5/8] x86/amd: Detect preferred cores in amd_get_boost_ratio_numerator()

[Mario Limonciello]

On 8/27/2024 10:43, Gautham R. Shenoy wrote:
> 
> On Mon, Aug 26, 2024 at 04:13:55PM -0500, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> AMD systems that support preferred cores will use "166" as their
>> numerator for max frequency calculations instead of "255".
>>
>> Add a function for detecting preferred cores by looking at the
>> highest perf value on all cores.
>>
>> If preferred cores are enabled return 166 and if disabled the
>> value in the highest perf register. As the function will be called
>> multiple times, cache the values for the boost numerator and if
>> preferred cores will be enabled in global variables.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
> 
> [..snip..]
> 
>>   /**
>>    * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation
>>    * @cpu: CPU to get numerator for.
>> @@ -162,20 +232,19 @@ EXPORT_SYMBOL_GPL(amd_get_highest_perf);
>>    */
>>   int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
>>   {
>> -	struct cpuinfo_x86 *c = &boot_cpu_data;
>> +	bool prefcore;
>> +	int ret;
>>   
>> -	if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) ||
>> -			       (c->x86_model >= 0x70 && c->x86_model < 0x80))) {
>> -		*numerator = 166;
>> -		return 0;
>> -	}
>> +	ret = amd_detect_prefcore(&prefcore);
>> +	if (ret)
>> +		return ret;
>>   
>> -	if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) ||
>> -			       (c->x86_model >= 0x40 && c->x86_model < 0x70))) {
>> -		*numerator = 166;
>> +	/* without preferred cores, return the highest perf register value */
>> +	if (!prefcore) {
>> +		*numerator = boost_numerator;
>>   		return 0;
>>   	}
>> -	*numerator = 255;
>> +	*numerator = CPPC_HIGHEST_PERF_PREFCORE;
> 
> 
> Interesting. So even when the user boots a system that supports
> preferred-cores with "amd_preferred=disable",
> amd_get_boost_ratio_numerator() will return CPPC_HIGHEST_PERF_PREFCORE
> as the call prefcore == true here.
> 
> I suppose that is as intended, since even though the user may not want
> to use the preferred core logic for task-scheduling/load-balancing,
> the numerator for the boost-ratio is purely dependent on the h/w
> capability.
> 
> Is this understanding correct? If so, can this be included as a
> comment in the code ?
> 

Yup, you got it all right.  I'll fold some of this into the function 
comment for v2.

> The rest of the patch looks good to me.
> 
> Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>
> 
> --
> Thanks and Regards
> gautham.
> 
> 
> 
>>   
>>   	return 0;
>>   }
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index f470b5700db58..ec32c830abc1d 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -807,32 +807,18 @@ static DECLARE_WORK(sched_prefcore_work, amd_pstste_sched_prefcore_workfn);
>>   
>>   static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
>>   {
>> -	int ret, prio;
>> -	u32 highest_perf;
>> -
>> -	ret = amd_get_highest_perf(cpudata->cpu, &highest_perf);
>> -	if (ret)
>> +	/* user disabled or not detected */
>> +	if (!amd_pstate_prefcore)
>>   		return;
>>   
>>   	cpudata->hw_prefcore = true;
>> -	/* check if CPPC preferred core feature is enabled*/
>> -	if (highest_perf < CPPC_MAX_PERF)
>> -		prio = (int)highest_perf;
>> -	else {
>> -		pr_debug("AMD CPPC preferred core is unsupported!\n");
>> -		cpudata->hw_prefcore = false;
>> -		return;
>> -	}
>> -
>> -	if (!amd_pstate_prefcore)
>> -		return;
>>   
>>   	/*
>>   	 * The priorities can be set regardless of whether or not
>>   	 * sched_set_itmt_support(true) has been called and it is valid to
>>   	 * update them at any time after it has been called.
>>   	 */
>> -	sched_set_itmt_core_prio(prio, cpudata->cpu);
>> +	sched_set_itmt_core_prio((int)READ_ONCE(cpudata->highest_perf), cpudata->cpu);
>>   
>>   	schedule_work(&sched_prefcore_work);
>>   }
>> @@ -998,12 +984,12 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>>   
>>   	cpudata->cpu = policy->cpu;
>>   
>> -	amd_pstate_init_prefcore(cpudata);
>> -
>>   	ret = amd_pstate_init_perf(cpudata);
>>   	if (ret)
>>   		goto free_cpudata1;
>>   
>> +	amd_pstate_init_prefcore(cpudata);
>> +
>>   	ret = amd_pstate_init_freq(cpudata);
>>   	if (ret)
>>   		goto free_cpudata1;
>> @@ -1453,12 +1439,12 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>>   	cpudata->cpu = policy->cpu;
>>   	cpudata->epp_policy = 0;
>>   
>> -	amd_pstate_init_prefcore(cpudata);
>> -
>>   	ret = amd_pstate_init_perf(cpudata);
>>   	if (ret)
>>   		goto free_cpudata1;
>>   
>> +	amd_pstate_init_prefcore(cpudata);
>> +
>>   	ret = amd_pstate_init_freq(cpudata);
>>   	if (ret)
>>   		goto free_cpudata1;
>> @@ -1903,6 +1889,12 @@ static int __init amd_pstate_init(void)
>>   		static_call_update(amd_pstate_update_perf, cppc_update_perf);
>>   	}
>>   
>> +	if (amd_pstate_prefcore) {
>> +		ret = amd_detect_prefcore(&amd_pstate_prefcore);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>   	/* enable amd pstate feature */
>>   	ret = amd_pstate_enable(true);
>>   	if (ret) {
>> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
>> index 2246ce0630362..1d79320a23490 100644
>> --- a/include/acpi/cppc_acpi.h
>> +++ b/include/acpi/cppc_acpi.h
>> @@ -137,10 +137,12 @@ struct cppc_cpudata {
>>   };
>>   
>>   #ifdef CONFIG_CPU_SUP_AMD
>> +extern int amd_detect_prefcore(bool *detected);
>>   extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
>>   extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
>>   #else /* !CONFIG_CPU_SUP_AMD */
>>   static inline int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf) { return -ENODEV; }
>> +static inline int amd_detect_prefcore(bool *detected) { return -ENODEV; }
>>   static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; }
>>   #endif /* !CONFIG_CPU_SUP_AMD */
>>   
>> -- 
>> 2.43.0
>>

[PATCH 6/8] cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into amd_get_boost_ratio_numerator()

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

The special case in amd_pstate_highest_perf_set() is the value used
for calculating the boost numerator.  Merge this into
amd_get_boost_ratio_numerator() and then use that to calculate boost
ratio.

This allows dropping more special casing of the highest perf value.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 arch/x86/kernel/acpi/cppc.c  | 16 ++++++++++++
 drivers/cpufreq/amd-pstate.c | 49 +++++++-----------------------------
 2 files changed, 25 insertions(+), 40 deletions(-)

diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
index 729b35e84f5eb..44b13a4e28740 100644
--- a/arch/x86/kernel/acpi/cppc.c
+++ b/arch/x86/kernel/acpi/cppc.c
@@ -9,6 +9,7 @@
 #include <asm/processor.h>
 #include <asm/topology.h>
 
+#define CPPC_HIGHEST_PERF_PERFORMANCE	196
 #define CPPC_HIGHEST_PERF_PREFCORE	166
 
 enum amd_pref_core {
@@ -244,6 +245,21 @@ int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
 		*numerator = boost_numerator;
 		return 0;
 	}
+
+	/*
+	 * For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7f,
+	 * the highest performance level is set to 196.
+	 * https://bugzilla.kernel.org/show_bug.cgi?id=218759
+	 */
+	if (cpu_feature_enabled(X86_FEATURE_ZEN4)) {
+		switch (boot_cpu_data.x86_model) {
+		case 0x70 ... 0x7f:
+			*numerator = CPPC_HIGHEST_PERF_PERFORMANCE;
+			return 0;
+		default:
+			break;
+		}
+	}
 	*numerator = CPPC_HIGHEST_PERF_PREFCORE;
 
 	return 0;
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index ec32c830abc1d..75568d0f84623 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -52,8 +52,6 @@
 #define AMD_PSTATE_TRANSITION_LATENCY	20000
 #define AMD_PSTATE_TRANSITION_DELAY	1000
 #define AMD_PSTATE_FAST_CPPC_TRANSITION_DELAY 600
-#define CPPC_HIGHEST_PERF_PERFORMANCE	196
-#define CPPC_HIGHEST_PERF_DEFAULT	166
 
 #define AMD_CPPC_EPP_PERFORMANCE		0x00
 #define AMD_CPPC_EPP_BALANCE_PERFORMANCE	0x80
@@ -372,43 +370,17 @@ static inline int amd_pstate_enable(bool enable)
 	return static_call(amd_pstate_enable)(enable);
 }
 
-static u32 amd_pstate_highest_perf_set(struct amd_cpudata *cpudata)
-{
-	struct cpuinfo_x86 *c = &cpu_data(0);
-
-	/*
-	 * For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7f,
-	 * the highest performance level is set to 196.
-	 * https://bugzilla.kernel.org/show_bug.cgi?id=218759
-	 */
-	if (c->x86 == 0x19 && (c->x86_model >= 0x70 && c->x86_model <= 0x7f))
-		return CPPC_HIGHEST_PERF_PERFORMANCE;
-
-	return CPPC_HIGHEST_PERF_DEFAULT;
-}
-
 static int pstate_init_perf(struct amd_cpudata *cpudata)
 {
 	u64 cap1;
-	u32 highest_perf;
 
 	int ret = rdmsrl_safe_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1,
 				     &cap1);
 	if (ret)
 		return ret;
 
-	/* For platforms that do not support the preferred core feature, the
-	 * highest_pef may be configured with 166 or 255, to avoid max frequency
-	 * calculated wrongly. we take the AMD_CPPC_HIGHEST_PERF(cap1) value as
-	 * the default max perf.
-	 */
-	if (cpudata->hw_prefcore)
-		highest_perf = amd_pstate_highest_perf_set(cpudata);
-	else
-		highest_perf = AMD_CPPC_HIGHEST_PERF(cap1);
-
-	WRITE_ONCE(cpudata->highest_perf, highest_perf);
-	WRITE_ONCE(cpudata->max_limit_perf, highest_perf);
+	WRITE_ONCE(cpudata->highest_perf, AMD_CPPC_HIGHEST_PERF(cap1));
+	WRITE_ONCE(cpudata->max_limit_perf, AMD_CPPC_HIGHEST_PERF(cap1));
 	WRITE_ONCE(cpudata->nominal_perf, AMD_CPPC_NOMINAL_PERF(cap1));
 	WRITE_ONCE(cpudata->lowest_nonlinear_perf, AMD_CPPC_LOWNONLIN_PERF(cap1));
 	WRITE_ONCE(cpudata->lowest_perf, AMD_CPPC_LOWEST_PERF(cap1));
@@ -426,12 +398,7 @@ static int cppc_init_perf(struct amd_cpudata *cpudata)
 	if (ret)
 		return ret;
 
-	if (cpudata->hw_prefcore)
-		highest_perf = amd_pstate_highest_perf_set(cpudata);
-	else
-		highest_perf = cppc_perf.highest_perf;
-
-	WRITE_ONCE(cpudata->highest_perf, highest_perf);
+	WRITE_ONCE(cpudata->highest_perf, cppc_perf.highest_perf);
 	WRITE_ONCE(cpudata->max_limit_perf, highest_perf);
 	WRITE_ONCE(cpudata->nominal_perf, cppc_perf.nominal_perf);
 	WRITE_ONCE(cpudata->lowest_nonlinear_perf,
@@ -905,8 +872,8 @@ static u32 amd_pstate_get_transition_latency(unsigned int cpu)
 static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 {
 	int ret;
-	u32 min_freq;
-	u32 highest_perf, max_freq;
+	u32 min_freq, max_freq;
+	u64 numerator;
 	u32 nominal_perf, nominal_freq;
 	u32 lowest_nonlinear_perf, lowest_nonlinear_freq;
 	u32 boost_ratio, lowest_nonlinear_ratio;
@@ -928,8 +895,10 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 
 	nominal_perf = READ_ONCE(cpudata->nominal_perf);
 
-	highest_perf = READ_ONCE(cpudata->highest_perf);
-	boost_ratio = div_u64(highest_perf << SCHED_CAPACITY_SHIFT, nominal_perf);
+	ret = amd_get_boost_ratio_numerator(cpudata->cpu, &numerator);
+	if (ret)
+		return ret;
+	boost_ratio = div_u64(numerator << SCHED_CAPACITY_SHIFT, nominal_perf);
 	max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT) * 1000;
 
 	lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
-- 
2.43.0

Re: [PATCH 6/8] cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into amd_get_boost_ratio_numerator()

[Gautham R. Shenoy]

Hello Mario,


On Mon, Aug 26, 2024 at 04:13:56PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> The special case in amd_pstate_highest_perf_set() is the value used
> for calculating the boost numerator.  Merge this into
> amd_get_boost_ratio_numerator() and then use that to calculate boost
> ratio.
> 
> This allows dropping more special casing of the highest perf value.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  arch/x86/kernel/acpi/cppc.c  | 16 ++++++++++++
>  drivers/cpufreq/amd-pstate.c | 49 +++++++-----------------------------
>  2 files changed, 25 insertions(+), 40 deletions(-)
> 
> diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
> index 729b35e84f5eb..44b13a4e28740 100644
> --- a/arch/x86/kernel/acpi/cppc.c
> +++ b/arch/x86/kernel/acpi/cppc.c
> @@ -9,6 +9,7 @@
>  #include <asm/processor.h>
>  #include <asm/topology.h>
>  
> +#define CPPC_HIGHEST_PERF_PERFORMANCE	196
>  #define CPPC_HIGHEST_PERF_PREFCORE	166
>  
>  enum amd_pref_core {
> @@ -244,6 +245,21 @@ int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
>  		*numerator = boost_numerator;
>  		return 0;
>  	}
> +
> +	/*
> +	 * For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7f,
> +	 * the highest performance level is set to 196.
> +	 * https://bugzilla.kernel.org/show_bug.cgi?id=218759
> +	 */
> +	if (cpu_feature_enabled(X86_FEATURE_ZEN4)) {
> +		switch (boot_cpu_data.x86_model) {
> +		case 0x70 ... 0x7f:
> +			*numerator = CPPC_HIGHEST_PERF_PERFORMANCE;
> +			return 0;
> +		default:
> +			break;
> +		}
> +	}
>  	*numerator = CPPC_HIGHEST_PERF_PREFCORE;
>  
>  	return 0;
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index ec32c830abc1d..75568d0f84623 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -52,8 +52,6 @@
>  #define AMD_PSTATE_TRANSITION_LATENCY	20000
>  #define AMD_PSTATE_TRANSITION_DELAY	1000
>  #define AMD_PSTATE_FAST_CPPC_TRANSITION_DELAY 600
> -#define CPPC_HIGHEST_PERF_PERFORMANCE	196
> -#define CPPC_HIGHEST_PERF_DEFAULT	166
>  
>  #define AMD_CPPC_EPP_PERFORMANCE		0x00
>  #define AMD_CPPC_EPP_BALANCE_PERFORMANCE	0x80
> @@ -372,43 +370,17 @@ static inline int amd_pstate_enable(bool enable)
>  	return static_call(amd_pstate_enable)(enable);
>  }
>  
> -static u32 amd_pstate_highest_perf_set(struct amd_cpudata *cpudata)
> -{
> -	struct cpuinfo_x86 *c = &cpu_data(0);
> -
> -	/*
> -	 * For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7f,
> -	 * the highest performance level is set to 196.
> -	 * https://bugzilla.kernel.org/show_bug.cgi?id=218759
> -	 */
> -	if (c->x86 == 0x19 && (c->x86_model >= 0x70 && c->x86_model <= 0x7f))
> -		return CPPC_HIGHEST_PERF_PERFORMANCE;
> -
> -	return CPPC_HIGHEST_PERF_DEFAULT;
> -}
> -
>  static int pstate_init_perf(struct amd_cpudata *cpudata)
>  {
>  	u64 cap1;
> -	u32 highest_perf;
>  
>  	int ret = rdmsrl_safe_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1,
>  				     &cap1);
>  	if (ret)
>  		return ret;
>  
> -	/* For platforms that do not support the preferred core feature, the
> -	 * highest_pef may be configured with 166 or 255, to avoid max frequency
> -	 * calculated wrongly. we take the AMD_CPPC_HIGHEST_PERF(cap1) value as
> -	 * the default max perf.
> -	 */
> -	if (cpudata->hw_prefcore)
> -		highest_perf = amd_pstate_highest_perf_set(cpudata);
> -	else
> -		highest_perf = AMD_CPPC_HIGHEST_PERF(cap1);
> -
> -	WRITE_ONCE(cpudata->highest_perf, highest_perf);
> -	WRITE_ONCE(cpudata->max_limit_perf, highest_perf);
> +	WRITE_ONCE(cpudata->highest_perf, AMD_CPPC_HIGHEST_PERF(cap1));
> +	WRITE_ONCE(cpudata->max_limit_perf, AMD_CPPC_HIGHEST_PERF(cap1));


So henceforth, cpudata->highest_perf is expected to cache the value of
CPPC.highest_perf and not the boost_ratio_numerator. There are couple
of user-visible changes due to this.


1.  On platforms where preferred-core is supported, previously the
    sysfs file
    /sys/devices/system/cpu/cpuX/cpufreq/amd_pstate_highest_perf would
    report the boost_ratio_numerator. Henceforth it will report
    CPPC.highest_perf.

    I hope there are no userspace tools that compute the boost_ratio
    using the syfs amd_pstate_highest_perf/amd_pstate_nominal_perf.

2. The amd_pstate_prefcore_ranking and amd_pstate_highest_perf will
   show the same values on all platforms, and henceforth are
   redundant.


Shouldn't this be documented?

The rest of the patch looks good to me.



--
Thanks and Regards
gautham.

Re: [PATCH 6/8] cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into amd_get_boost_ratio_numerator()

[Mario Limonciello]

On 8/27/2024 11:52, Gautham R. Shenoy wrote:
> Hello Mario,
> 
> 
> On Mon, Aug 26, 2024 at 04:13:56PM -0500, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> The special case in amd_pstate_highest_perf_set() is the value used
>> for calculating the boost numerator.  Merge this into
>> amd_get_boost_ratio_numerator() and then use that to calculate boost
>> ratio.
>>
>> This allows dropping more special casing of the highest perf value.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   arch/x86/kernel/acpi/cppc.c  | 16 ++++++++++++
>>   drivers/cpufreq/amd-pstate.c | 49 +++++++-----------------------------
>>   2 files changed, 25 insertions(+), 40 deletions(-)
>>
>> diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
>> index 729b35e84f5eb..44b13a4e28740 100644
>> --- a/arch/x86/kernel/acpi/cppc.c
>> +++ b/arch/x86/kernel/acpi/cppc.c
>> @@ -9,6 +9,7 @@
>>   #include <asm/processor.h>
>>   #include <asm/topology.h>
>>   
>> +#define CPPC_HIGHEST_PERF_PERFORMANCE	196
>>   #define CPPC_HIGHEST_PERF_PREFCORE	166
>>   
>>   enum amd_pref_core {
>> @@ -244,6 +245,21 @@ int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
>>   		*numerator = boost_numerator;
>>   		return 0;
>>   	}
>> +
>> +	/*
>> +	 * For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7f,
>> +	 * the highest performance level is set to 196.
>> +	 * https://bugzilla.kernel.org/show_bug.cgi?id=218759
>> +	 */
>> +	if (cpu_feature_enabled(X86_FEATURE_ZEN4)) {
>> +		switch (boot_cpu_data.x86_model) {
>> +		case 0x70 ... 0x7f:
>> +			*numerator = CPPC_HIGHEST_PERF_PERFORMANCE;
>> +			return 0;
>> +		default:
>> +			break;
>> +		}
>> +	}
>>   	*numerator = CPPC_HIGHEST_PERF_PREFCORE;
>>   
>>   	return 0;
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index ec32c830abc1d..75568d0f84623 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -52,8 +52,6 @@
>>   #define AMD_PSTATE_TRANSITION_LATENCY	20000
>>   #define AMD_PSTATE_TRANSITION_DELAY	1000
>>   #define AMD_PSTATE_FAST_CPPC_TRANSITION_DELAY 600
>> -#define CPPC_HIGHEST_PERF_PERFORMANCE	196
>> -#define CPPC_HIGHEST_PERF_DEFAULT	166
>>   
>>   #define AMD_CPPC_EPP_PERFORMANCE		0x00
>>   #define AMD_CPPC_EPP_BALANCE_PERFORMANCE	0x80
>> @@ -372,43 +370,17 @@ static inline int amd_pstate_enable(bool enable)
>>   	return static_call(amd_pstate_enable)(enable);
>>   }
>>   
>> -static u32 amd_pstate_highest_perf_set(struct amd_cpudata *cpudata)
>> -{
>> -	struct cpuinfo_x86 *c = &cpu_data(0);
>> -
>> -	/*
>> -	 * For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7f,
>> -	 * the highest performance level is set to 196.
>> -	 * https://bugzilla.kernel.org/show_bug.cgi?id=218759
>> -	 */
>> -	if (c->x86 == 0x19 && (c->x86_model >= 0x70 && c->x86_model <= 0x7f))
>> -		return CPPC_HIGHEST_PERF_PERFORMANCE;
>> -
>> -	return CPPC_HIGHEST_PERF_DEFAULT;
>> -}
>> -
>>   static int pstate_init_perf(struct amd_cpudata *cpudata)
>>   {
>>   	u64 cap1;
>> -	u32 highest_perf;
>>   
>>   	int ret = rdmsrl_safe_on_cpu(cpudata->cpu, MSR_AMD_CPPC_CAP1,
>>   				     &cap1);
>>   	if (ret)
>>   		return ret;
>>   
>> -	/* For platforms that do not support the preferred core feature, the
>> -	 * highest_pef may be configured with 166 or 255, to avoid max frequency
>> -	 * calculated wrongly. we take the AMD_CPPC_HIGHEST_PERF(cap1) value as
>> -	 * the default max perf.
>> -	 */
>> -	if (cpudata->hw_prefcore)
>> -		highest_perf = amd_pstate_highest_perf_set(cpudata);
>> -	else
>> -		highest_perf = AMD_CPPC_HIGHEST_PERF(cap1);
>> -
>> -	WRITE_ONCE(cpudata->highest_perf, highest_perf);
>> -	WRITE_ONCE(cpudata->max_limit_perf, highest_perf);
>> +	WRITE_ONCE(cpudata->highest_perf, AMD_CPPC_HIGHEST_PERF(cap1));
>> +	WRITE_ONCE(cpudata->max_limit_perf, AMD_CPPC_HIGHEST_PERF(cap1));
> 
> 
> So henceforth, cpudata->highest_perf is expected to cache the value of
> CPPC.highest_perf and not the boost_ratio_numerator. There are couple
> of user-visible changes due to this.
> 
> 
> 1.  On platforms where preferred-core is supported, previously the
>      sysfs file
>      /sys/devices/system/cpu/cpuX/cpufreq/amd_pstate_highest_perf would
>      report the boost_ratio_numerator. Henceforth it will report
>      CPPC.highest_perf.
> 
>      I hope there are no userspace tools that compute the boost_ratio
>      using the syfs amd_pstate_highest_perf/amd_pstate_nominal_perf.
> 
> 2. The amd_pstate_prefcore_ranking and amd_pstate_highest_perf will
>     show the same values on all platforms, and henceforth are
>     redundant.
> 

Good observations here.  I'm not aware of any tools trying to replicate 
this calculation.
With the redundancy I would actually argue we should just drop the sysfs 
file 'amd_pstate_prefcore_ranking'.

Thoughts?

> 
> Shouldn't this be documented?

I noticed amd_pstate_prefcore_ranking wasn't properly documented in 
amd-pstate.rst in the first place.  If the decision is not to drop the 
sysfs file, then I'll add a section for it.

> 
> The rest of the patch looks good to me.
> 
> 
> 
> --
> Thanks and Regards
> gautham.
> 

Re: [PATCH 6/8] cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into amd_get_boost_ratio_numerator()

[Gautham R. Shenoy]

On Tue, Aug 27, 2024 at 01:36:47PM -0500, Mario Limonciello wrote:
> On 8/27/2024 11:52, Gautham R. Shenoy wrote:

[..snip..]

> > 
> > 
> > So henceforth, cpudata->highest_perf is expected to cache the value of
> > CPPC.highest_perf and not the boost_ratio_numerator. There are couple
> > of user-visible changes due to this.
> > 
> > 
> > 1.  On platforms where preferred-core is supported, previously the
> >      sysfs file
> >      /sys/devices/system/cpu/cpuX/cpufreq/amd_pstate_highest_perf would
> >      report the boost_ratio_numerator. Henceforth it will report
> >      CPPC.highest_perf.

One other side effect is that the highest_perf sysfs file will now
reveal the differential highest_perf, even when "amd_prefcore=false",
while earlier all the cores would report CPPC_HIGHEST_PERF_DEFAULT.

I think we may be better off reporting the boost-numerator here, but
that's really not the highest_perf :(

> > 
> >      I hope there are no userspace tools that compute the boost_ratio
> >      using the syfs amd_pstate_highest_perf/amd_pstate_nominal_perf.
> > 
> > 2. The amd_pstate_prefcore_ranking and amd_pstate_highest_perf will
> >     show the same values on all platforms, and henceforth are
> >     redundant.
> > 
> 
> Good observations here.  I'm not aware of any tools trying to replicate this
> calculation.
> With the redundancy I would actually argue we should just drop the sysfs
> file 'amd_pstate_prefcore_ranking'.
> 
> Thoughts?

Looking at the code again, I realize that I was
wrong. cpudata->prefcore_ranking also gets updated in
amd_pstate_update_min_max_limits() and reflects the dynamic
preference.

While cpudata->highest_perf value indicates the initial boot-time
preference.

Hence it makes sense to amd_pstate_prefcore_ranking.

> 
> > 
> > Shouldn't this be documented?
> 
> I noticed amd_pstate_prefcore_ranking wasn't properly documented in
> amd-pstate.rst in the first place.  If the decision is not to drop the sysfs
> file, then I'll add a section for it.

Thanks.

> 
> > 
> > The rest of the patch looks good to me.
> > 
> > 
> > 
--
Thanks and Regards
gautham.

Re: [PATCH 6/8] cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into amd_get_boost_ratio_numerator()

[kernel test robot]

Hi Mario,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/bleeding-edge tip/x86/core tip/master linus/master v6.11-rc5 next-20240827]
[cannot apply to tip/auto-latest]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/x86-amd-Move-amd_get_highest_perf-from-amd-c-to-cppc-c/20240827-051648
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20240826211358.2694603-7-superm1%40kernel.org
patch subject: [PATCH 6/8] cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into amd_get_boost_ratio_numerator()
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20240828/202408280553.k4hXRtZy-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240828/202408280553.k4hXRtZy-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408280553.k4hXRtZy-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/cpufreq/amd-pstate.c:402:38: warning: variable 'highest_perf' is uninitialized when used here [-Wuninitialized]
     402 |         WRITE_ONCE(cpudata->max_limit_perf, highest_perf);
         |                                             ^~~~~~~~~~~~
   include/asm-generic/rwonce.h:61:18: note: expanded from macro 'WRITE_ONCE'
      61 |         __WRITE_ONCE(x, val);                                           \
         |                         ^~~
   include/asm-generic/rwonce.h:55:33: note: expanded from macro '__WRITE_ONCE'
      55 |         *(volatile typeof(x) *)&(x) = (val);                            \
         |                                        ^~~
   drivers/cpufreq/amd-pstate.c:395:18: note: initialize the variable 'highest_perf' to silence this warning
     395 |         u32 highest_perf;
         |                         ^
         |                          = 0
   1 warning generated.


vim +/highest_perf +402 drivers/cpufreq/amd-pstate.c

ec437d71db77a1 Huang Rui         2021-12-24  391  
e059c184da47e9 Huang Rui         2021-12-24  392  static int cppc_init_perf(struct amd_cpudata *cpudata)
e059c184da47e9 Huang Rui         2021-12-24  393  {
e059c184da47e9 Huang Rui         2021-12-24  394  	struct cppc_perf_caps cppc_perf;
bedadcfb011fef Perry Yuan        2022-08-30  395  	u32 highest_perf;
e059c184da47e9 Huang Rui         2021-12-24  396  
e059c184da47e9 Huang Rui         2021-12-24  397  	int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
e059c184da47e9 Huang Rui         2021-12-24  398  	if (ret)
e059c184da47e9 Huang Rui         2021-12-24  399  		return ret;
e059c184da47e9 Huang Rui         2021-12-24  400  
347b3754cc9780 Mario Limonciello 2024-08-26  401  	WRITE_ONCE(cpudata->highest_perf, cppc_perf.highest_perf);
febab20caebac9 Wyes Karny        2023-11-17 @402  	WRITE_ONCE(cpudata->max_limit_perf, highest_perf);
e059c184da47e9 Huang Rui         2021-12-24  403  	WRITE_ONCE(cpudata->nominal_perf, cppc_perf.nominal_perf);
e059c184da47e9 Huang Rui         2021-12-24  404  	WRITE_ONCE(cpudata->lowest_nonlinear_perf,
e059c184da47e9 Huang Rui         2021-12-24  405  		   cppc_perf.lowest_nonlinear_perf);
e059c184da47e9 Huang Rui         2021-12-24  406  	WRITE_ONCE(cpudata->lowest_perf, cppc_perf.lowest_perf);
e571a5e2068ef5 Meng Li           2024-01-19  407  	WRITE_ONCE(cpudata->prefcore_ranking, cppc_perf.highest_perf);
febab20caebac9 Wyes Karny        2023-11-17  408  	WRITE_ONCE(cpudata->min_limit_perf, cppc_perf.lowest_perf);
e059c184da47e9 Huang Rui         2021-12-24  409  
2dd6d0ebf74049 Wyes Karny        2023-03-07  410  	if (cppc_state == AMD_PSTATE_ACTIVE)
2dd6d0ebf74049 Wyes Karny        2023-03-07  411  		return 0;
2dd6d0ebf74049 Wyes Karny        2023-03-07  412  
2dd6d0ebf74049 Wyes Karny        2023-03-07  413  	ret = cppc_get_auto_sel_caps(cpudata->cpu, &cppc_perf);
2dd6d0ebf74049 Wyes Karny        2023-03-07  414  	if (ret) {
2dd6d0ebf74049 Wyes Karny        2023-03-07  415  		pr_warn("failed to get auto_sel, ret: %d\n", ret);
e059c184da47e9 Huang Rui         2021-12-24  416  		return 0;
e059c184da47e9 Huang Rui         2021-12-24  417  	}
e059c184da47e9 Huang Rui         2021-12-24  418  
2dd6d0ebf74049 Wyes Karny        2023-03-07  419  	ret = cppc_set_auto_sel(cpudata->cpu,
2dd6d0ebf74049 Wyes Karny        2023-03-07  420  			(cppc_state == AMD_PSTATE_PASSIVE) ? 0 : 1);
2dd6d0ebf74049 Wyes Karny        2023-03-07  421  
2dd6d0ebf74049 Wyes Karny        2023-03-07  422  	if (ret)
2dd6d0ebf74049 Wyes Karny        2023-03-07  423  		pr_warn("failed to set auto_sel, ret: %d\n", ret);
2dd6d0ebf74049 Wyes Karny        2023-03-07  424  
2dd6d0ebf74049 Wyes Karny        2023-03-07  425  	return ret;
2dd6d0ebf74049 Wyes Karny        2023-03-07  426  }
2dd6d0ebf74049 Wyes Karny        2023-03-07  427  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 7/8] cpufreq: amd-pstate: Optimize amd_pstate_update_limits()

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

Don't take and release the mutex when prefcore isn't present and
avoid initialization of variables that will be initially set
in the function.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 75568d0f84623..ed05d7a0add10 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -798,17 +798,17 @@ static void amd_pstate_update_limits(unsigned int cpu)
 	int ret;
 	bool highest_perf_changed = false;
 
-	mutex_lock(&amd_pstate_driver_lock);
-	if ((!amd_pstate_prefcore) || (!cpudata->hw_prefcore))
-		goto free_cpufreq_put;
+	if (!amd_pstate_prefcore)
+		return;
 
+	mutex_lock(&amd_pstate_driver_lock);
 	ret = amd_get_highest_perf(cpu, &cur_high);
 	if (ret)
 		goto free_cpufreq_put;
 
 	prev_high = READ_ONCE(cpudata->prefcore_ranking);
-	if (prev_high != cur_high) {
-		highest_perf_changed = true;
+	highest_perf_changed = (prev_high != cur_high);
+	if (highest_perf_changed) {
 		WRITE_ONCE(cpudata->prefcore_ranking, cur_high);
 
 		if (cur_high < CPPC_MAX_PERF)
-- 
2.43.0

RE: [PATCH 7/8] cpufreq: amd-pstate: Optimize amd_pstate_update_limits()

[Yuan, Perry]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Mario Limonciello <superm1@kernel.org>
> Sent: Tuesday, August 27, 2024 5:14 AM
> To: Borislav Petkov <bp@alien8.de>; Shenoy, Gautham Ranjal
> <gautham.shenoy@amd.com>; Yuan, Perry <Perry.Yuan@amd.com>
> Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) <x86@kernel.org>;
> Rafael J . Wysocki <rafael@kernel.org>; open list:X86 ARCHITECTURE (32-BIT
> AND 64-BIT) <linux-kernel@vger.kernel.org>; open list:ACPI <linux-
> acpi@vger.kernel.org>; open list:CPU FREQUENCY SCALING FRAMEWORK
> <linux-pm@vger.kernel.org>; Limonciello, Mario
> <Mario.Limonciello@amd.com>
> Subject: [PATCH 7/8] cpufreq: amd-pstate: Optimize
> amd_pstate_update_limits()
>
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> Don't take and release the mutex when prefcore isn't present and avoid
> initialization of variables that will be initially set in the function.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 75568d0f84623..ed05d7a0add10 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -798,17 +798,17 @@ static void amd_pstate_update_limits(unsigned int
> cpu)
>       int ret;
>       bool highest_perf_changed = false;
>
> -     mutex_lock(&amd_pstate_driver_lock);
> -     if ((!amd_pstate_prefcore) || (!cpudata->hw_prefcore))
> -             goto free_cpufreq_put;
> +     if (!amd_pstate_prefcore)
> +             return;
>
> +     mutex_lock(&amd_pstate_driver_lock);
>       ret = amd_get_highest_perf(cpu, &cur_high);
>       if (ret)
>               goto free_cpufreq_put;
>
>       prev_high = READ_ONCE(cpudata->prefcore_ranking);
> -     if (prev_high != cur_high) {
> -             highest_perf_changed = true;
> +     highest_perf_changed = (prev_high != cur_high);
> +     if (highest_perf_changed) {
>               WRITE_ONCE(cpudata->prefcore_ranking, cur_high);
>
>               if (cur_high < CPPC_MAX_PERF)
> --
> 2.43.0
>

Reviewed-by: Perry Yuan <perry.yuan@amd.com>


Best Regards.

Perry.

Re: [PATCH 7/8] cpufreq: amd-pstate: Optimize amd_pstate_update_limits()

[Gautham R. Shenoy]

On Mon, Aug 26, 2024 at 04:13:57PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> Don't take and release the mutex when prefcore isn't present and
> avoid initialization of variables that will be initially set
> in the function.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>  drivers/cpufreq/amd-pstate.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 75568d0f84623..ed05d7a0add10 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -798,17 +798,17 @@ static void amd_pstate_update_limits(unsigned int cpu)
>  	int ret;
>  	bool highest_perf_changed = false;
>  
> -	mutex_lock(&amd_pstate_driver_lock);
> -	if ((!amd_pstate_prefcore) || (!cpudata->hw_prefcore))
> -		goto free_cpufreq_put;
> +	if (!amd_pstate_prefcore)
> +		return;

Looks good to me.

Wondering if it is worth maintaining a static key for
amd_pstate_prefcore. Anyway it doesn't change after boot.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>


--
Thanks and Regards
gautham.

Re: [PATCH 7/8] cpufreq: amd-pstate: Optimize amd_pstate_update_limits()

[Mario Limonciello]

On 8/27/2024 11:57, Gautham R. Shenoy wrote:
> On Mon, Aug 26, 2024 at 04:13:57PM -0500, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> Don't take and release the mutex when prefcore isn't present and
>> avoid initialization of variables that will be initially set
>> in the function.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> 
>> ---
>>   drivers/cpufreq/amd-pstate.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index 75568d0f84623..ed05d7a0add10 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -798,17 +798,17 @@ static void amd_pstate_update_limits(unsigned int cpu)
>>   	int ret;
>>   	bool highest_perf_changed = false;
>>   
>> -	mutex_lock(&amd_pstate_driver_lock);
>> -	if ((!amd_pstate_prefcore) || (!cpudata->hw_prefcore))
>> -		goto free_cpufreq_put;
>> +	if (!amd_pstate_prefcore)
>> +		return;
> 
> Looks good to me.
> 
> Wondering if it is worth maintaining a static key for
> amd_pstate_prefcore. Anyway it doesn't change after boot.

As there is a kernel command line option how would you pass the early 
param parsing result over without a static variable?

> 
> Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>
> 
Thanks!

> 
> --
> Thanks and Regards
> gautham.

[PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

As the global variable is cleared when preferred cores is not present
the local variable use isn't needed in many functions.
Drop it where possible.  No intended functional changes.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index ed05d7a0add10..257e28e549bd1 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1112,12 +1112,7 @@ static ssize_t show_amd_pstate_prefcore_ranking(struct cpufreq_policy *policy,
 static ssize_t show_amd_pstate_hw_prefcore(struct cpufreq_policy *policy,
 					   char *buf)
 {
-	bool hw_prefcore;
-	struct amd_cpudata *cpudata = policy->driver_data;
-
-	hw_prefcore = READ_ONCE(cpudata->hw_prefcore);
-
-	return sysfs_emit(buf, "%s\n", str_enabled_disabled(hw_prefcore));
+	return sysfs_emit(buf, "%s\n", str_enabled_disabled(amd_pstate_prefcore));
 }
 
 static ssize_t show_energy_performance_available_preferences(
-- 
2.43.0

RE: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore

[Yuan, Perry]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Mario Limonciello <superm1@kernel.org>
> Sent: Tuesday, August 27, 2024 5:14 AM
> To: Borislav Petkov <bp@alien8.de>; Shenoy, Gautham Ranjal
> <gautham.shenoy@amd.com>; Yuan, Perry <Perry.Yuan@amd.com>
> Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) <x86@kernel.org>;
> Rafael J . Wysocki <rafael@kernel.org>; open list:X86 ARCHITECTURE (32-BIT
> AND 64-BIT) <linux-kernel@vger.kernel.org>; open list:ACPI <linux-
> acpi@vger.kernel.org>; open list:CPU FREQUENCY SCALING FRAMEWORK
> <linux-pm@vger.kernel.org>; Limonciello, Mario
> <Mario.Limonciello@amd.com>
> Subject: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata-
> >hw_prefcore
>
> From: Mario Limonciello <mario.limonciello@amd.com>
>
> As the global variable is cleared when preferred cores is not present the local
> variable use isn't needed in many functions.
> Drop it where possible.  No intended functional changes.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index ed05d7a0add10..257e28e549bd1 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1112,12 +1112,7 @@ static ssize_t
> show_amd_pstate_prefcore_ranking(struct cpufreq_policy *policy,  static
> ssize_t show_amd_pstate_hw_prefcore(struct cpufreq_policy *policy,
>                                          char *buf)
>  {
> -     bool hw_prefcore;
> -     struct amd_cpudata *cpudata = policy->driver_data;
> -
> -     hw_prefcore = READ_ONCE(cpudata->hw_prefcore);
> -
> -     return sysfs_emit(buf, "%s\n", str_enabled_disabled(hw_prefcore));
> +     return sysfs_emit(buf, "%s\n",
> +str_enabled_disabled(amd_pstate_prefcore));
>  }
>
>  static ssize_t show_energy_performance_available_preferences(
> --
> 2.43.0
>

Reviewed-by: Perry Yuan <perry.yuan@amd.com>


Best Regards.

Perry.

Re: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore

[Gautham R. Shenoy]

On Mon, Aug 26, 2024 at 04:13:58PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> As the global variable is cleared when preferred cores is not present
> the local variable use isn't needed in many functions.
> Drop it where possible.  No intended functional changes.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index ed05d7a0add10..257e28e549bd1 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1112,12 +1112,7 @@ static ssize_t show_amd_pstate_prefcore_ranking(struct cpufreq_policy *policy,
>  static ssize_t show_amd_pstate_hw_prefcore(struct cpufreq_policy *policy,
>  					   char *buf)
>  {
> -	bool hw_prefcore;
> -	struct amd_cpudata *cpudata = policy->driver_data;
> -
> -	hw_prefcore = READ_ONCE(cpudata->hw_prefcore);
> -
> -	return sysfs_emit(buf, "%s\n", str_enabled_disabled(hw_prefcore));
> +	return sysfs_emit(buf, "%s\n", str_enabled_disabled(amd_pstate_prefcore));


If the user boots with "amd_prefcore=disable" on a system that
supports preferred-core, pror to this patchset, cpudata->hw_prefcore
would be true and thus, amd_pstate_hw_prefcore would show "enabled".

With this patchset, it would show "disabled". Or am I missing something?



>  }
>  
>  static ssize_t show_energy_performance_available_preferences(
> -- 
> 2.43.0
> 

--
Thanks and Regards
gautham.

Re: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore

[Mario Limonciello]

On 8/27/2024 12:03, Gautham R. Shenoy wrote:
> On Mon, Aug 26, 2024 at 04:13:58PM -0500, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> As the global variable is cleared when preferred cores is not present
>> the local variable use isn't needed in many functions.
>> Drop it where possible.  No intended functional changes.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/cpufreq/amd-pstate.c | 7 +------
>>   1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index ed05d7a0add10..257e28e549bd1 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -1112,12 +1112,7 @@ static ssize_t show_amd_pstate_prefcore_ranking(struct cpufreq_policy *policy,
>>   static ssize_t show_amd_pstate_hw_prefcore(struct cpufreq_policy *policy,
>>   					   char *buf)
>>   {
>> -	bool hw_prefcore;
>> -	struct amd_cpudata *cpudata = policy->driver_data;
>> -
>> -	hw_prefcore = READ_ONCE(cpudata->hw_prefcore);
>> -
>> -	return sysfs_emit(buf, "%s\n", str_enabled_disabled(hw_prefcore));
>> +	return sysfs_emit(buf, "%s\n", str_enabled_disabled(amd_pstate_prefcore));
> 
> 
> If the user boots with "amd_prefcore=disable" on a system that
> supports preferred-core, pror to this patchset, cpudata->hw_prefcore
> would be true and thus, amd_pstate_hw_prefcore would show "enabled".
> 

Yes; you're right about the previous behavior.

> With this patchset, it would show "disabled". Or am I missing something?

This appears to be another case we don't have documentation for the 
sysfs file `amd_pstate_hw_prefcore`.

I had thought this was a malfunction in the behavior that it reflected 
the current status, not the hardware /capability/.

Which one makes more sense for userspace?  In my mind the most likely 
consumer of this information would be something a sched_ext based 
userspace scheduler.  They would need to know whether the scheduler was 
using preferred cores; not whether the hardware supported it.

Whichever direction we agree to go; I'll add documentation for v2.

> 
> 
> 
>>   }
>>   
>>   static ssize_t show_energy_performance_available_preferences(
>> -- 
>> 2.43.0
>>
> 
> --
> Thanks and Regards
> gautham.

Re: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore

[Gautham R. Shenoy]

On Tue, Aug 27, 2024 at 02:16:51PM -0500, Mario Limonciello wrote:
> On 8/27/2024 12:03, Gautham R. Shenoy wrote:
> > On Mon, Aug 26, 2024 at 04:13:58PM -0500, Mario Limonciello wrote:
> > > From: Mario Limonciello <mario.limonciello@amd.com>
> > > 
> > > As the global variable is cleared when preferred cores is not present
> > > the local variable use isn't needed in many functions.
> > > Drop it where possible.  No intended functional changes.
> > > 
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > >   drivers/cpufreq/amd-pstate.c | 7 +------
> > >   1 file changed, 1 insertion(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> > > index ed05d7a0add10..257e28e549bd1 100644
> > > --- a/drivers/cpufreq/amd-pstate.c
> > > +++ b/drivers/cpufreq/amd-pstate.c
> > > @@ -1112,12 +1112,7 @@ static ssize_t show_amd_pstate_prefcore_ranking(struct cpufreq_policy *policy,
> > >   static ssize_t show_amd_pstate_hw_prefcore(struct cpufreq_policy *policy,
> > >   					   char *buf)
> > >   {
> > > -	bool hw_prefcore;
> > > -	struct amd_cpudata *cpudata = policy->driver_data;
> > > -
> > > -	hw_prefcore = READ_ONCE(cpudata->hw_prefcore);
> > > -
> > > -	return sysfs_emit(buf, "%s\n", str_enabled_disabled(hw_prefcore));
> > > +	return sysfs_emit(buf, "%s\n", str_enabled_disabled(amd_pstate_prefcore));
> > 
> > 
> > If the user boots with "amd_prefcore=disable" on a system that
> > supports preferred-core, pror to this patchset, cpudata->hw_prefcore
> > would be true and thus, amd_pstate_hw_prefcore would show "enabled".
> > 
> 
> Yes; you're right about the previous behavior.
> 
> > With this patchset, it would show "disabled". Or am I missing something?
> 
> This appears to be another case we don't have documentation for the sysfs
> file `amd_pstate_hw_prefcore`.

:(

> 
> I had thought this was a malfunction in the behavior that it reflected the
> current status, not the hardware /capability/.
> 
> Which one makes more sense for userspace?  In my mind the most likely
> consumer of this information would be something a sched_ext based userspace
> scheduler.  They would need to know whether the scheduler was using
> preferred cores; not whether the hardware supported it.

The commandline parameter currently impacts only the fair sched-class
tasks since the preference information gets used only during
load-balancing.

IMO, the same should continue with sched-ext, i.e. if the user has
explicitly disabled prefcore support via commandline, the no sched-ext
scheduler should use the preference information to make task placement
decisions. However, I would like to see what the sched-ext folks have
to say. Adding some of them to the Cc list.

> 
> Whichever direction we agree to go; I'll add documentation for v2.

Yes please.

--
Thanks and Regards
gautham.

Re: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore

[Andrea Righi]

On Wed, Aug 28, 2024 at 10:38:45AM +0530, Gautham R. Shenoy wrote:
...
> > I had thought this was a malfunction in the behavior that it reflected the
> > current status, not the hardware /capability/.
> > 
> > Which one makes more sense for userspace?  In my mind the most likely
> > consumer of this information would be something a sched_ext based userspace
> > scheduler.  They would need to know whether the scheduler was using
> > preferred cores; not whether the hardware supported it.
> 
> The commandline parameter currently impacts only the fair sched-class
> tasks since the preference information gets used only during
> load-balancing.
> 
> IMO, the same should continue with sched-ext, i.e. if the user has
> explicitly disabled prefcore support via commandline, the no sched-ext
> scheduler should use the preference information to make task placement
> decisions. However, I would like to see what the sched-ext folks have
> to say. Adding some of them to the Cc list.

IMHO it makes more sense to reflect the real state of prefcore support
from a "system" perspective, more than a "hardware" perspective, so if
it's disabled via boot command line it should show disabled.

From a user-space scheduler perspective we should be fine either way, as
long as the ABI is clearly documented, since we also have access to
/proc/cmdline and we would be able to figure out if the user has
disabled it via cmdline (however, the preference is still to report the
actual system status).

Question: having prefcore enabled affects also the value of
scaling_max_freq? Like an `lscpu -e`, for example, would show a higher
max frequency for the specific preferred cores? (this is another useful
information from a sched_ext scheduler perspective).

Thanks,
-Andrea

Re: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore

[Gautham R. Shenoy]

Hello Andrea,

On Wed, Aug 28, 2024 at 08:20:50AM +0200, Andrea Righi wrote:
> On Wed, Aug 28, 2024 at 10:38:45AM +0530, Gautham R. Shenoy wrote:
> ...
> > > I had thought this was a malfunction in the behavior that it reflected the
> > > current status, not the hardware /capability/.
> > > 
> > > Which one makes more sense for userspace?  In my mind the most likely
> > > consumer of this information would be something a sched_ext based userspace
> > > scheduler.  They would need to know whether the scheduler was using
> > > preferred cores; not whether the hardware supported it.
> > 
> > The commandline parameter currently impacts only the fair sched-class
> > tasks since the preference information gets used only during
> > load-balancing.
> > 
> > IMO, the same should continue with sched-ext, i.e. if the user has
> > explicitly disabled prefcore support via commandline, the no sched-ext
> > scheduler should use the preference information to make task placement
> > decisions. However, I would like to see what the sched-ext folks have
> > to say. Adding some of them to the Cc list.
> 
> IMHO it makes more sense to reflect the real state of prefcore support
> from a "system" perspective, more than a "hardware" perspective, so if
> it's disabled via boot command line it should show disabled.
> 
> From a user-space scheduler perspective we should be fine either way, as
> long as the ABI is clearly documented, since we also have access to
> /proc/cmdline and we would be able to figure out if the user has
> disabled it via cmdline (however, the preference is still to report the
> actual system status).

Thank you for confirming this.

> 
> Question: having prefcore enabled affects also the value of
> scaling_max_freq? Like an `lscpu -e`, for example, would show a higher
> max frequency for the specific preferred cores? (this is another useful
> information from a sched_ext scheduler perspective).

Since the scaling_max_freq is computed based on the boost-numerator,
at least from this patchset, the numerator would be the same across
all kinds of cores, and thus the scaling_max_freq reported will be the
same across all the cores.

> 
> Thanks,
> -Andrea

--
Thanks and Regards
gautham.

Re: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore

[Andrea Righi]

On Wed, Aug 28, 2024 at 08:27:44PM +0530, Gautham R. Shenoy wrote:
> Hello Andrea,
> 
> On Wed, Aug 28, 2024 at 08:20:50AM +0200, Andrea Righi wrote:
> > On Wed, Aug 28, 2024 at 10:38:45AM +0530, Gautham R. Shenoy wrote:
> > ...
> > > > I had thought this was a malfunction in the behavior that it reflected the
> > > > current status, not the hardware /capability/.
> > > > 
> > > > Which one makes more sense for userspace?  In my mind the most likely
> > > > consumer of this information would be something a sched_ext based userspace
> > > > scheduler.  They would need to know whether the scheduler was using
> > > > preferred cores; not whether the hardware supported it.
> > > 
> > > The commandline parameter currently impacts only the fair sched-class
> > > tasks since the preference information gets used only during
> > > load-balancing.
> > > 
> > > IMO, the same should continue with sched-ext, i.e. if the user has
> > > explicitly disabled prefcore support via commandline, the no sched-ext
> > > scheduler should use the preference information to make task placement
> > > decisions. However, I would like to see what the sched-ext folks have
> > > to say. Adding some of them to the Cc list.
> > 
> > IMHO it makes more sense to reflect the real state of prefcore support
> > from a "system" perspective, more than a "hardware" perspective, so if
> > it's disabled via boot command line it should show disabled.
> > 
> > From a user-space scheduler perspective we should be fine either way, as
> > long as the ABI is clearly documented, since we also have access to
> > /proc/cmdline and we would be able to figure out if the user has
> > disabled it via cmdline (however, the preference is still to report the
> > actual system status).
> 
> Thank you for confirming this.
> 
> > 
> > Question: having prefcore enabled affects also the value of
> > scaling_max_freq? Like an `lscpu -e`, for example, would show a higher
> > max frequency for the specific preferred cores? (this is another useful
> > information from a sched_ext scheduler perspective).
> 
> Since the scaling_max_freq is computed based on the boost-numerator,
> at least from this patchset, the numerator would be the same across
> all kinds of cores, and thus the scaling_max_freq reported will be the
> same across all the cores.

I see, so IIUC from user-space the most reliable way to detect the
fastest cores is to check amd_pstate_highest_perf / amd_pstate_max_freq,
right? I'm trying to figure out a way to abstract and generalize the
concept of "fast cores" in sched_ext.

Also, is this something that has changed recently? I see this on an
AMD Ryzen Threadripper PRO 7975WX 32-Cores running a 6.8 kernel:

$ uname -r
6.8.0-40-generic

$ grep . /sys/devices/system/cpu/cpu*/cpufreq/cpuinfo_max_freq
/sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu1/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu2/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu3/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu4/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu5/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu6/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu7/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu8/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu9/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu10/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu11/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu12/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu13/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu14/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu15/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu16/cpufreq/cpuinfo_max_freq:6161000
/sys/devices/system/cpu/cpu17/cpufreq/cpuinfo_max_freq:6321000
/sys/devices/system/cpu/cpu18/cpufreq/cpuinfo_max_freq:6001000
/sys/devices/system/cpu/cpu19/cpufreq/cpuinfo_max_freq:6646000
/sys/devices/system/cpu/cpu20/cpufreq/cpuinfo_max_freq:5837000
/sys/devices/system/cpu/cpu21/cpufreq/cpuinfo_max_freq:6482000
/sys/devices/system/cpu/cpu22/cpufreq/cpuinfo_max_freq:5517000
/sys/devices/system/cpu/cpu23/cpufreq/cpuinfo_max_freq:5677000
/sys/devices/system/cpu/cpu24/cpufreq/cpuinfo_max_freq:6966000
/sys/devices/system/cpu/cpu25/cpufreq/cpuinfo_max_freq:7775000
/sys/devices/system/cpu/cpu26/cpufreq/cpuinfo_max_freq:6806000
/sys/devices/system/cpu/cpu27/cpufreq/cpuinfo_max_freq:7775000
/sys/devices/system/cpu/cpu28/cpufreq/cpuinfo_max_freq:7130000
/sys/devices/system/cpu/cpu29/cpufreq/cpuinfo_max_freq:7451000
/sys/devices/system/cpu/cpu30/cpufreq/cpuinfo_max_freq:7290000
/sys/devices/system/cpu/cpu31/cpufreq/cpuinfo_max_freq:7611000
/sys/devices/system/cpu/cpu32/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu33/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu34/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu35/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu36/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu37/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu38/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu39/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu40/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu41/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu42/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu43/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu44/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu45/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu46/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu47/cpufreq/cpuinfo_max_freq:5352000
/sys/devices/system/cpu/cpu48/cpufreq/cpuinfo_max_freq:6161000
/sys/devices/system/cpu/cpu49/cpufreq/cpuinfo_max_freq:6321000
/sys/devices/system/cpu/cpu50/cpufreq/cpuinfo_max_freq:6001000
/sys/devices/system/cpu/cpu51/cpufreq/cpuinfo_max_freq:6646000
/sys/devices/system/cpu/cpu52/cpufreq/cpuinfo_max_freq:5837000
/sys/devices/system/cpu/cpu53/cpufreq/cpuinfo_max_freq:6482000
/sys/devices/system/cpu/cpu54/cpufreq/cpuinfo_max_freq:5517000
/sys/devices/system/cpu/cpu55/cpufreq/cpuinfo_max_freq:5677000
/sys/devices/system/cpu/cpu56/cpufreq/cpuinfo_max_freq:6966000
/sys/devices/system/cpu/cpu57/cpufreq/cpuinfo_max_freq:7775000
/sys/devices/system/cpu/cpu58/cpufreq/cpuinfo_max_freq:6806000
/sys/devices/system/cpu/cpu59/cpufreq/cpuinfo_max_freq:7775000
/sys/devices/system/cpu/cpu60/cpufreq/cpuinfo_max_freq:7130000
/sys/devices/system/cpu/cpu61/cpufreq/cpuinfo_max_freq:7451000
/sys/devices/system/cpu/cpu62/cpufreq/cpuinfo_max_freq:7290000
/sys/devices/system/cpu/cpu63/cpufreq/cpuinfo_max_freq:7611000

Thanks,
-Andrea

Re: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore

[Mario Limonciello]

On 8/29/2024 07:52, Andrea Righi wrote:
> On Wed, Aug 28, 2024 at 08:27:44PM +0530, Gautham R. Shenoy wrote:
>> Hello Andrea,
>>
>> On Wed, Aug 28, 2024 at 08:20:50AM +0200, Andrea Righi wrote:
>>> On Wed, Aug 28, 2024 at 10:38:45AM +0530, Gautham R. Shenoy wrote:
>>> ...
>>>>> I had thought this was a malfunction in the behavior that it reflected the
>>>>> current status, not the hardware /capability/.
>>>>>
>>>>> Which one makes more sense for userspace?  In my mind the most likely
>>>>> consumer of this information would be something a sched_ext based userspace
>>>>> scheduler.  They would need to know whether the scheduler was using
>>>>> preferred cores; not whether the hardware supported it.
>>>>
>>>> The commandline parameter currently impacts only the fair sched-class
>>>> tasks since the preference information gets used only during
>>>> load-balancing.
>>>>
>>>> IMO, the same should continue with sched-ext, i.e. if the user has
>>>> explicitly disabled prefcore support via commandline, the no sched-ext
>>>> scheduler should use the preference information to make task placement
>>>> decisions. However, I would like to see what the sched-ext folks have
>>>> to say. Adding some of them to the Cc list.
>>>
>>> IMHO it makes more sense to reflect the real state of prefcore support
>>> from a "system" perspective, more than a "hardware" perspective, so if
>>> it's disabled via boot command line it should show disabled.
>>>
>>>  From a user-space scheduler perspective we should be fine either way, as
>>> long as the ABI is clearly documented, since we also have access to
>>> /proc/cmdline and we would be able to figure out if the user has
>>> disabled it via cmdline (however, the preference is still to report the
>>> actual system status).
>>
>> Thank you for confirming this.
>>
>>>
>>> Question: having prefcore enabled affects also the value of
>>> scaling_max_freq? Like an `lscpu -e`, for example, would show a higher
>>> max frequency for the specific preferred cores? (this is another useful
>>> information from a sched_ext scheduler perspective).
>>
>> Since the scaling_max_freq is computed based on the boost-numerator,
>> at least from this patchset, the numerator would be the same across
>> all kinds of cores, and thus the scaling_max_freq reported will be the
>> same across all the cores.
> 
> I see, so IIUC from user-space the most reliable way to detect the
> fastest cores is to check amd_pstate_highest_perf / amd_pstate_max_freq,
> right? I'm trying to figure out a way to abstract and generalize the
> concept of "fast cores" in sched_ext.

Right now the best way to do this is to look at the 
amd_pstate_precore_ranking file.

In this series there has been some discussion of dropping it though in 
favor of looking at the highest perf file.  I don't believe we're 
concluded one way or another on it yet though.

> 
> Also, is this something that has changed recently? I see this on an
> AMD Ryzen Threadripper PRO 7975WX 32-Cores running a 6.8 kernel:
> 
> $ uname -r
> 6.8.0-40-generic

You're missing the preferred core patches on this kernel.  They landed 
in 6.9, it's better to upgrade to 6.10.y or 6.11-rc.

Re: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore

[Andrea Righi]

On Thu, Aug 29, 2024 at 08:01:48AM -0500, Mario Limonciello wrote:
> On 8/29/2024 07:52, Andrea Righi wrote:
> > On Wed, Aug 28, 2024 at 08:27:44PM +0530, Gautham R. Shenoy wrote:
> > > Hello Andrea,
> > > 
> > > On Wed, Aug 28, 2024 at 08:20:50AM +0200, Andrea Righi wrote:
> > > > On Wed, Aug 28, 2024 at 10:38:45AM +0530, Gautham R. Shenoy wrote:
> > > > ...
> > > > > > I had thought this was a malfunction in the behavior that it reflected the
> > > > > > current status, not the hardware /capability/.
> > > > > > 
> > > > > > Which one makes more sense for userspace?  In my mind the most likely
> > > > > > consumer of this information would be something a sched_ext based userspace
> > > > > > scheduler.  They would need to know whether the scheduler was using
> > > > > > preferred cores; not whether the hardware supported it.
> > > > > 
> > > > > The commandline parameter currently impacts only the fair sched-class
> > > > > tasks since the preference information gets used only during
> > > > > load-balancing.
> > > > > 
> > > > > IMO, the same should continue with sched-ext, i.e. if the user has
> > > > > explicitly disabled prefcore support via commandline, the no sched-ext
> > > > > scheduler should use the preference information to make task placement
> > > > > decisions. However, I would like to see what the sched-ext folks have
> > > > > to say. Adding some of them to the Cc list.
> > > > 
> > > > IMHO it makes more sense to reflect the real state of prefcore support
> > > > from a "system" perspective, more than a "hardware" perspective, so if
> > > > it's disabled via boot command line it should show disabled.
> > > > 
> > > >  From a user-space scheduler perspective we should be fine either way, as
> > > > long as the ABI is clearly documented, since we also have access to
> > > > /proc/cmdline and we would be able to figure out if the user has
> > > > disabled it via cmdline (however, the preference is still to report the
> > > > actual system status).
> > > 
> > > Thank you for confirming this.
> > > 
> > > > 
> > > > Question: having prefcore enabled affects also the value of
> > > > scaling_max_freq? Like an `lscpu -e`, for example, would show a higher
> > > > max frequency for the specific preferred cores? (this is another useful
> > > > information from a sched_ext scheduler perspective).
> > > 
> > > Since the scaling_max_freq is computed based on the boost-numerator,
> > > at least from this patchset, the numerator would be the same across
> > > all kinds of cores, and thus the scaling_max_freq reported will be the
> > > same across all the cores.
> > 
> > I see, so IIUC from user-space the most reliable way to detect the
> > fastest cores is to check amd_pstate_highest_perf / amd_pstate_max_freq,
> > right? I'm trying to figure out a way to abstract and generalize the
> > concept of "fast cores" in sched_ext.
> 
> Right now the best way to do this is to look at the
> amd_pstate_precore_ranking file.

Ok.

> 
> In this series there has been some discussion of dropping it though in favor
> of looking at the highest perf file.  I don't believe we're concluded one
> way or another on it yet though.
> 
> > 
> > Also, is this something that has changed recently? I see this on an
> > AMD Ryzen Threadripper PRO 7975WX 32-Cores running a 6.8 kernel:
> > 
> > $ uname -r
> > 6.8.0-40-generic
> 
> You're missing the preferred core patches on this kernel.  They landed in
> 6.9, it's better to upgrade to 6.10.y or 6.11-rc.

So, if I move to 6.9+ I should see the same max frequency across all the
CPUs and I can use amd_pstate_precore_ranking to determine the subset of
fast cores.

Thanks for the clarification.

-Andrea