AMD Pstate Driver Fixes and Improvements

If CPPC feature is supported by the CPU however the CPUID flag bit is not set by SBIOS, the amd_pstate will be failed to load while system booting.
So adding one more debug message to inform user to check the SBIOS setting, The change also can help maintainers to debug why amd_pstate driver failed to be loaded at system booting if the processor support CPPC.

Closes https://bugzilla.kernel.org/show_bug.cgi?id=218686

Signed-off-by Perry Yuan <[email protected]>

Reviewed-by Gautham R. Shenoy <[email protected]>

---
  drivers/cpufreq/amd-pstate.c | 26 ++++++++++++++++++++++++++
  1 file changed, 26 insertions(+)

drivers/cpufreq/amd-pstate.c +26 -0

index 76419762c04f..9aa220a0e3fe 100644
@@ -1749,11 +1749,37 @@ static int __init amd_pstate_set_driver(int mode_idx)
   */
  static bool amd_cppc_supported(void)
  {
+	struct cpuinfo_x86 *c = &cpu_data(0);
+	bool warn = false;
+
  	if ((boot_cpu_data.x86 == 0x17) && (boot_cpu_data.x86_model < 0x30)) {
  		pr_debug_once("CPPC feature is not supported by the processor\n");
  		return false;
  	}
 
+	/*
+	 * If the CPPC feature is disabled in the BIOS for processors that support MSR-based CPPC,
+	 * the AMD Pstate driver may not function correctly.
+	 * Check the CPPC flag and display a warning message if the platform supports CPPC.
+	 * Note: below checking code will not abort the driver registeration process because of
+	 * the code is added for debugging purposes.
+	 */
+	if (!cpu_feature_enabled(X86_FEATURE_CPPC)) {
+		if (cpu_feature_enabled(X86_FEATURE_ZEN1) || cpu_feature_enabled(X86_FEATURE_ZEN2)) {
+			if (c->x86_model > 0x60 && c->x86_model < 0xaf)
+				warn = true;
+		} else if (cpu_feature_enabled(X86_FEATURE_ZEN3) || cpu_feature_enabled(X86_FEATURE_ZEN4)) {
+			if ((c->x86_model > 0x10 && c->x86_model < 0x1F) ||
+					(c->x86_model > 0x40 && c->x86_model < 0xaf))
+				warn = true;
+		} else if (cpu_feature_enabled(X86_FEATURE_ZEN5)) {
+			warn = true;
+		}
+	}
+
+	if (warn)
+		pr_warn_once("The CPPC feature is supported but currently disabled by the BIOS.\n"
+					"Please enable it if your BIOS has the CPPC option.\n");
  	return true;
  }
 

To address issues with the loading of the amd-pstate driver on certain platforms, It needs to enable dynamic debugging to capture debug messages during the driver loading process. By adding "amd_pstate.dyndbg=+p cppc_acpi.dyndbg=+p debug amd_pstate=active" and loglevel to the kernel command line, then driver debug logging is enabled.

Signed-off-by Perry Yuan <[email protected]>

---
  Documentation/admin-guide/pm/amd-pstate.rst | 14 ++++++++++++++
  1 file changed, 14 insertions(+)

Documentation/admin-guide/pm/amd-pstate.rst +0 -0

index 1e0d101b020a..ceeb073c9ada 100644

Documentation/admin-guide/pm/amd-pstate.rst +7 -0

+to the kernel command line. This will enable dynamic debugging and allow better
+analysis and troubleshooting of the driver loading process.
+
+Please note that adding this parameter will only enable debug logging during the
+driver loading phase and may affect system behavior. Use this option with caution
+and only for debugging purposes.
+
  Trace Events
  --------------
 

CPUID leaf 0x80000026 advertises core types with different efficiency rankings

Bit 30 indicates the heterogeneous core topology feature, if the bit set, it means not all instances at the current hierarchical level have the same core topology.

For better utilization of feature words and help to identify core type, X86_FEATURE_HETERO_CORE_TOPOLOGY is added as a few scattered feature bits.

Reference:
See the page 119 of PPR for AMD Family 19h Model 61h B1, docID 56713

Signed-off-by Perry Yuan <[email protected]>

---
 arch/x86/include/asm/cpufeatures.h | 1 +
 arch/x86/kernel/cpu/scattered.c    | 1 +
 2 files changed, 2 insertions(+)

arch/x86/include/asm/cpufeatures.h +1 -0

index 6c128d463a14..eceaa0df0137 100644
@@ -471,6 +471,7 @@
 #define X86_FEATURE_CLEAR_BHB_HW	(21*32+ 3) /* "" BHI_DIS_S HW control enabled */
 #define X86_FEATURE_CLEAR_BHB_LOOP_ON_VMEXIT (21*32+ 4) /* "" Clear branch history at vmexit using SW loop */
 #define X86_FEATURE_FAST_CPPC		(21*32 + 5) /* "" AMD Fast CPPC */
+#define X86_FEATURE_HETERO_CORE_TOPOLOGY       (21*32+ 6) /* "" Heterogeneous Core Topology */
 
 /*
  * BUG word(s)

arch/x86/kernel/cpu/scattered.c +0 -0

index c84c30188fdf..6b3477503dd0 100644

-----Original Message-----
From: Borislav Petkov <[email protected]>
Sent: Monday, June 17, 2024 4:39 PM
To: Yuan, Perry <[email protected]>
Cc: [email protected]; Limonciello, Mario <[email protected]>; [email protected]; Huang, Ray <[email protected]>; Shenoy, Gautham Ranjal <[email protected]>; Deucher, Alexander <[email protected]>; Huang, Shimmer <[email protected]>; Du, Xiaojian <[email protected]>; Meng, Li (Jassmine) <[email protected]>; [email protected]; linux- [email protected]
Subject: Re: [PATCH v4 08/11] x86/cpufeatures: Add feature bits for AMD heterogeneous processor

Quoted 9 lines · Perry Yuan

CPUID leaf 0x80000026 advertises core types with different efficiency rankings

Bit 30 indicates the heterogeneous core topology feature, if the bit set, it means not all instances at the current hierarchical level have the same core topology.

For better utilization of feature words and help to identify core type, X86_FEATURE_HETERO_CORE_TOPOLOGY is added as a few scattered

feature bits.

Quoted 16 lines

Reference:
See the page 119 of PPR for AMD Family 19h Model 61h B1, docID 56713

Signed-off-by Perry Yuan <[email protected]>

---
 arch/x86/include/asm/cpufeatures.h | 1 +
 arch/x86/kernel/cpu/scattered.c    | 1 +
 2 files changed, 2 insertions(+)

arch/x86/include/asm/cpufeatures.h +0 -0

index 6c128d463a14..eceaa0df0137 100644

arch/x86/include/asm/cpufeatures.h line 471

Quoted 1 line

arch/x86/include/asm/cpufeatures.h +0 -0

@@ -471,6 +471,7 @@
 #define X86_FEATURE_CLEAR_BHB_HW   (21*32+ 3) /* "" BHI_DIS_S

HW control enabled */

arch/x86/include/asm/cpufeatures.h line 472

Quoted 1 line

arch/x86/include/asm/cpufeatures.h +0 -0

 #define X86_FEATURE_CLEAR_BHB_LOOP_ON_VMEXIT (21*32+ 4) /* ""

Clear branch history at vmexit using SW loop */

arch/x86/include/asm/cpufeatures.h line 473

Quoted 1 line

arch/x86/include/asm/cpufeatures.h +0 -0

 #define X86_FEATURE_FAST_CPPC              (21*32 + 5) /* "" AMD Fast

CPPC */

arch/x86/include/asm/cpufeatures.h line 474

Quoted 1 line

arch/x86/include/asm/cpufeatures.h +1 -0

+#define X86_FEATURE_HETERO_CORE_TOPOLOGY       (21*32+ 6) /* ""

Heterogeneous Core Topology */

arch/x86/kernel/cpu/scattered.c static const struct cpuid_bit cpuid_bits[] = { · line 54

Quoted 12 lines

arch/x86/include/asm/cpufeatures.h +0 -0

 /*
  * BUG word(s)

(quoted diff) +0 -0

b/arch/x86/kernel/cpu/scattered.c index c84c30188fdf..6b3477503dd0 100644

arch/x86/kernel/cpu/scattered.c +0 -0

@@ -52,6 +52,7 @@ static const struct cpuid_bit cpuid_bits[] = {
    { X86_FEATURE_PERFMON_V2,       CPUID_EAX,  0, 0x80000022, 0 },
    { X86_FEATURE_AMD_LBR_V2,       CPUID_EAX,  1, 0x80000022, 0 },
    { X86_FEATURE_AMD_LBR_PMC_FREEZE,       CPUID_EAX,  2,

0x80000022, 0 },

arch/x86/kernel/cpu/scattered.c static const struct cpuid_bit cpuid_bits[] = { · line 55

Quoted 1 line

arch/x86/kernel/cpu/scattered.c +1 -0

+   { X86_FEATURE_HETERO_CORE_TOPOLOGY,     CPUID_EAX,  30,

0x80000026, 0 },

arch/x86/kernel/cpu/scattered.c static const struct cpuid_bit cpuid_bits[] = {

Quoted 4 lines

arch/x86/kernel/cpu/scattered.c +0 -1

    { 0, 0, 0, 0, 0 }
 };
 
--

Nacked-by Borislav Petkov (AMD) <[email protected]>

Until all review comments have been addressed:

https://lore.kernel.org/r/20240611105216.GAZmgsYC-
J_yLfdupF@fat_crate.local

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Introduces an optimization to the AMD-Pstate driver by implementing a heterogeneous core topology for the initialization of the highest performance value while driver loading.
The two core types supported are "performance" and "efficiency". Each core type has different highest performance and frequency values configured by the platform. The amd_pstate driver needs to identify the type of core to correctly set an appropriate highest perf value.

X86_FEATURE_HETERO_CORE_TOPOLOGY is used to identify whether the processor support heterogeneous core type by reading CPUID leaf Fn_0x80000026_EAX and bit 30. if the bit is set as one, then amd_pstate driver will check EBX 30:28 bits to get the core type.

Reference:
See the page 119 of PPR for AMD Family 19h Model 61h B1, docID 56713

Signed-off-by Perry Yuan <[email protected]>

---
  arch/x86/include/asm/processor.h |  2 ++
  arch/x86/kernel/cpu/amd.c        | 19 ++++++++++++
  drivers/cpufreq/amd-pstate.c     | 53 ++++++++++++++++++++++++++++++--
  3 files changed, 71 insertions(+), 3 deletions(-)

arch/x86/include/asm/processor.h +2 -0

index cb4f6c513c48..223aa58e2d5c 100644
@@ -694,10 +694,12 @@ static inline u32 per_cpu_l2c_id(unsigned int cpu)
  extern u32 amd_get_highest_perf(void);
  extern void amd_clear_divider(void);
  extern void amd_check_microcode(void);
+extern int amd_get_this_core_type(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)		{ }
+static inline int amd_get_this_core_type(void)		{ return -1; }
  #endif
 
  extern unsigned long arch_align_stack(unsigned long sp);

arch/x86/kernel/cpu/amd.c +2 -0

index 44df3f11e731..62a4ef21ef79 100644
@@ -1231,3 +1231,22 @@ void noinstr amd_clear_divider(void)
  		     :: "a" (0), "d" (0), "r" (1));
  }
  EXPORT_SYMBOL_GPL(amd_clear_divider);
+
+#define X86_CPU_TYPE_ID_SHIFT	28

arch/x86/kernel/cpu/amd.c +7 -0

+{
+	if (!cpu_feature_enabled(X86_FEATURE_HETERO_CORE_TOPOLOGY))
+		return -1;
+
+	return cpuid_ebx(0x80000026) >> X86_CPU_TYPE_ID_SHIFT;
+}
+EXPORT_SYMBOL_GPL(amd_get_this_core_type);

drivers/cpufreq/amd-pstate.c +50 -3

index cb750ef305fe..cf68343219d1 100644
@@ -52,8 +52,10 @@
  #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 CPPC_HIGHEST_PERF_EFFICIENT		132
+#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

@@ -86,6 +88,14 @@ struct quirk_entry {
  	u32 lowest_freq;
  };
 
+/* defined by CPUID_Fn80000026_EBX BIT [31:28] */
+enum amd_core_type {
+	CPU_CORE_TYPE_NO_HETERO_SUP = -1,
+	CPU_CORE_TYPE_PERFORMANCE = 0,
+	CPU_CORE_TYPE_EFFICIENCY = 1,
+	CPU_CORE_TYPE_UNDEFINED = 2,
+};
+
  /*
   * TODO: We need more time to fine tune processors with shared memory solution
   * with community together.

@@ -358,9 +368,27 @@ static inline int amd_pstate_enable(bool enable)
  	return static_call(amd_pstate_enable)(enable);
  }
 
+static void get_this_core_type(void *data)
+{
+	enum amd_core_type *cpu_type = data;
+
+	*cpu_type = amd_get_this_core_type();
+}
+
+static enum amd_core_type  amd_pstate_get_cpu_type(int cpu)
+{
+	enum amd_core_type cpu_type;
+
+	smp_call_function_single(cpu, get_this_core_type, &cpu_type, 1);
+
+	return cpu_type;
+}
+
  static u32 amd_pstate_highest_perf_set(struct amd_cpudata *cpudata)
  {
  	struct cpuinfo_x86 *c = &cpu_data(0);
+	u32 highest_perf;
+	enum amd_core_type core_type;
 
  	/*
  	 * For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7f,

@@ -370,7 +398,26 @@ static u32 amd_pstate_highest_perf_set(struct amd_cpudata *cpudata)
  	if (c->x86 == 0x19 && (c->x86_model >= 0x70 && c->x86_model <= 0x7f))
  		return CPPC_HIGHEST_PERF_PERFORMANCE;
 
-	return CPPC_HIGHEST_PERF_DEFAULT;
+	core_type = amd_pstate_get_cpu_type(cpudata->cpu);
+	pr_debug("core_type %d found\n", core_type);
+
+	switch (core_type) {
+	case CPU_CORE_TYPE_NO_HETERO_SUP:
+		highest_perf = CPPC_HIGHEST_PERF_DEFAULT;
+		break;
+	case CPU_CORE_TYPE_PERFORMANCE:
+		highest_perf = CPPC_HIGHEST_PERF_PERFORMANCE;
+		break;
+	case CPU_CORE_TYPE_EFFICIENCY:
+		highest_perf = CPPC_HIGHEST_PERF_EFFICIENT;
+		break;
+	default:
+		highest_perf = CPPC_HIGHEST_PERF_DEFAULT;
+		WARN_ONCE(true, "WARNING: Undefined core type found");
+		break;
+	}
+
+    return highest_perf;
  }
 
  static int pstate_init_perf(struct amd_cpudata *cpudata)

-----Original Message-----
From: Limonciello, Mario <[email protected]>
Sent: Wednesday, June 19, 2024 3:23 AM
To: Yuan, Perry <[email protected]>; [email protected]; [email protected]; Huang, Ray <[email protected]>; Shenoy, Gautham Ranjal <[email protected]>; Petkov, Borislav <[email protected]>
Cc: Deucher, Alexander <[email protected]>; Huang, Shimmer <[email protected]>; Du, Xiaojian <[email protected]>; Meng, Li (Jassmine) <[email protected]>; [email protected]; linux- [email protected]
Subject: Re: [PATCH v4 09/11] cpufreq: amd-pstate: implement heterogeneous core topology for highest performance initialization

Quoted 13 lines · Perry Yuan

Introduces an optimization to the AMD-Pstate driver by implementing a heterogeneous core topology for the initialization of the highest performance value while driver loading.
The two core types supported are "performance" and "efficiency". Each core type has different highest performance and frequency values configured by the platform. The amd_pstate driver needs to identify the type of core to correctly set an appropriate highest perf value.

X86_FEATURE_HETERO_CORE_TOPOLOGY is used to identify whether the processor support heterogeneous core type by reading CPUID leaf Fn_0x80000026_EAX and bit 30. if the bit is set as one, then amd_pstate driver will check EBX 30:28 bits to get the core type.

Quoted 8 lines

Reference:
See the page 119 of PPR for AMD Family 19h Model 61h B1, docID 56713

Signed-off-by Perry Yuan <[email protected]>

---
  arch/x86/include/asm/processor.h |  2 ++
  arch/x86/kernel/cpu/amd.c        | 19 ++++++++++++
  drivers/cpufreq/amd-pstate.c     | 53 ++++++++++++++++++++++++++++++-

-

arch/x86/include/asm/processor.h static inline u32 per_cpu_l2c_id(unsigned int

Quoted 8 lines

  3 files changed, 71 insertions(+), 3 deletions(-)

arch/x86/include/asm/processor.h +0 -0

index cb4f6c513c48..223aa58e2d5c 100644
@@ -694,10 +694,12 @@ static inline u32 per_cpu_l2c_id(unsigned int

cpu)

Quoted 28 lines

arch/x86/include/asm/processor.h +2 -0

  extern u32 amd_get_highest_perf(void);
  extern void amd_clear_divider(void);
  extern void amd_check_microcode(void);
+extern int amd_get_this_core_type(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)              { }
+static inline int amd_get_this_core_type(void)             { return -1; }
  #endif
 
  extern unsigned long arch_align_stack(unsigned long sp); diff --git

a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c index 44df3f11e731..62a4ef21ef79 100644

arch/x86/kernel/cpu/amd.c +8 -0

@@ -1231,3 +1231,22 @@ void noinstr amd_clear_divider(void)
                 :: "a" (0), "d" (0), "r" (1));
  }
  EXPORT_SYMBOL_GPL(amd_clear_divider);
+
+#define X86_CPU_TYPE_ID_SHIFT      28
+
+/**
+ * amd_get_this_core_type - Get the type of this heterogeneous CPU
+ *
+ * Returns the CPU type [31:28] (i.e., performance or efficient) of
+ * a CPU in the processor.

arch/x86/kernel/cpu/amd.c void noinstr amd_clear_divider(void) · line 1245

Quoted 4 lines

arch/x86/kernel/cpu/amd.c +4 -0

+ * If the processor has no core type support, returns -1.
+ */
+
+int amd_get_this_core_type(void)

Did you miss my feedback from v3? I don't see changes for the return type or for returning CPU_CORE_TYPE_NO_HETERO_SUP instead of -1.

Quoted 30 lines

arch/x86/kernel/cpu/amd.c +7 -0

+{
+   if (!cpu_feature_enabled(X86_FEATURE_HETERO_CORE_TOPOLOGY))
+           return -1;
+
+   return cpuid_ebx(0x80000026) >> X86_CPU_TYPE_ID_SHIFT;
+}
+EXPORT_SYMBOL_GPL(amd_get_this_core_type);

drivers/cpufreq/amd-pstate.c +4 -2

index cb750ef305fe..cf68343219d1 100644
@@ -52,8 +52,10 @@
  #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 CPPC_HIGHEST_PERF_EFFICIENT                132
+#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

@@ -86,6 +88,14 @@ struct quirk_entry {
    u32 lowest_freq;
  };
 

drivers/cpufreq/amd-pstate.c struct quirk_entry { · line 100

Quoted 10 lines

drivers/cpufreq/amd-pstate.c +8 -0

+/* defined by CPUID_Fn80000026_EBX BIT [31:28] */
+enum amd_core_type {
+   CPU_CORE_TYPE_NO_HETERO_SUP = -1,
+   CPU_CORE_TYPE_PERFORMANCE = 0,
+   CPU_CORE_TYPE_EFFICIENCY = 1,
+   CPU_CORE_TYPE_UNDEFINED = 2,
+};
+
  /*
   * TODO: We need more time to fine tune processors with shared memory

solution

drivers/cpufreq/amd-pstate.c static u32 amd_pstate_highest_perf_set(struct

Quoted 30 lines

drivers/cpufreq/amd-pstate.c +18 -0

   * with community together.

@@ -358,9 +368,27 @@ static inline int amd_pstate_enable(bool enable)
    return static_call(amd_pstate_enable)(enable);
  }
 
+static void get_this_core_type(void *data)
+{
+   enum amd_core_type *cpu_type = data;
+
+   *cpu_type = amd_get_this_core_type();
+}
+
+static enum amd_core_type  amd_pstate_get_cpu_type(int cpu)
+{
+   enum amd_core_type cpu_type;
+
+   smp_call_function_single(cpu, get_this_core_type, &cpu_type, 1);
+
+   return cpu_type;
+}
+
  static u32 amd_pstate_highest_perf_set(struct amd_cpudata *cpudata)
  {
    struct cpuinfo_x86 *c = &cpu_data(0);
+   u32 highest_perf;
+   enum amd_core_type core_type;
 
    /*
     * For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7f,

@@ -370,7 +398,26 @@ static u32 amd_pstate_highest_perf_set(struct

amd_cpudata *cpudata)

drivers/cpufreq/amd-pstate.c static u32 amd_pstate_highest_perf_set(struct · line 398

Quoted 1 line

drivers/cpufreq/amd-pstate.c +0 -0

    if (c->x86 == 0x19 && (c->x86_model >= 0x70 && c->x86_model <=

0x7f))

Quoted 26 lines

drivers/cpufreq/amd-pstate.c +20 -1

            return CPPC_HIGHEST_PERF_PERFORMANCE;
 
-   return CPPC_HIGHEST_PERF_DEFAULT;
+   core_type = amd_pstate_get_cpu_type(cpudata->cpu);
+   pr_debug("core_type %d found\n", core_type);
+
+   switch (core_type) {
+   case CPU_CORE_TYPE_NO_HETERO_SUP:
+           highest_perf = CPPC_HIGHEST_PERF_DEFAULT;
+           break;
+   case CPU_CORE_TYPE_PERFORMANCE:
+           highest_perf = CPPC_HIGHEST_PERF_PERFORMANCE;
+           break;
+   case CPU_CORE_TYPE_EFFICIENCY:
+           highest_perf = CPPC_HIGHEST_PERF_EFFICIENT;
+           break;
+   default:
+           highest_perf = CPPC_HIGHEST_PERF_DEFAULT;
+           WARN_ONCE(true, "WARNING: Undefined core type found");
+           break;
+   }
+
+    return highest_perf;
  }
 
  static int pstate_init_perf(struct amd_cpudata *cpudata)

Introduces an optimization to the AMD-Pstate driver by implementing a heterogeneous core topology for the initialization of the highest performance value while driver loading.
The two core types supported are "performance" and "efficiency". Each core type has different highest performance and frequency values configured by the platform. The amd_pstate driver needs to identify the type of core to correctly set an appropriate highest perf value.

X86_FEATURE_HETERO_CORE_TOPOLOGY is used to identify whether the processor support heterogeneous core type by reading CPUID leaf Fn_0x80000026_EAX and bit 30. if the bit is set as one, then amd_pstate driver will check EBX 30:28 bits to get the core type.

-----Original Message-----
From: Borislav Petkov <[email protected]>
Sent: Wednesday, June 19, 2024 5:24 AM
To: Yuan, Perry <[email protected]>
Cc: [email protected]; Limonciello, Mario <[email protected]>; [email protected]; Huang, Ray <[email protected]>; Shenoy, Gautham Ranjal <[email protected]>; Deucher, Alexander <[email protected]>; Huang, Shimmer <[email protected]>; Du, Xiaojian <[email protected]>; Meng, Li (Jassmine) <[email protected]>; [email protected]; linux- [email protected]
Subject: Re: [PATCH v4 09/11] cpufreq: amd-pstate: implement heterogeneous core topology for highest performance initialization

Quoted 8 lines · Perry Yuan

Introduces an optimization to the AMD-Pstate driver by implementing a heterogeneous core topology for the initialization of the highest performance value while driver loading.
The two core types supported are "performance" and "efficiency". Each core type has different highest performance and frequency values configured by the platform. The amd_pstate driver needs to identify the type of core to correctly set an appropriate highest perf value.

Quoted 4 lines

X86_FEATURE_HETERO_CORE_TOPOLOGY is used to identify whether the processor support heterogeneous core type by reading CPUID leaf Fn_0x80000026_EAX and bit 30. if the bit is set as one, then amd_pstate driver will check EBX 30:28 bits to get the core type.

There will be a special ->cpu_type member for that eventually:

https://lore.kernel.org/r/20240617-add-cpu-type-v1-1-
[email protected]

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

If the amd-pstate driver is not loaded automatically by default, it is because the kernel command line parameter has not been added. To resolve this issue, it is necessary to call the amd_pstate_set_driver() function to enable the desired mode (passive/active/guided) before registering the driver instance.

This ensures that the driver is loaded correctly without relying on the kernel command line parameter.

When there is no parameter added to command line, Kernel config will provide the default mode to load.

Meanwhle, user can add driver mode in command line which will override

the kernel config default option.

Reported-by Andrei Amuraritei <[email protected]>

Closes https://bugzilla.kernel.org/show_bug.cgi?id=218705

Signed-off-by Perry Yuan <[email protected]>

---
  drivers/cpufreq/amd-pstate.c | 24 +++++++++++++++++-------
  1 file changed, 17 insertions(+), 7 deletions(-)

drivers/cpufreq/amd-pstate.c +17 -7

index cf68343219d1..b48fd60cbc6d 100644
@@ -1857,8 +1857,13 @@ static int __init amd_pstate_init(void)
  	/* check if this machine need CPPC quirks */
  	dmi_check_system(amd_pstate_quirks_table);
 
-	switch (cppc_state) {
-	case AMD_PSTATE_UNDEFINED:
+	/*
+	* determine the driver mode from the command line or kernel config.
+	* If no command line input is provided, cppc_state will be AMD_PSTATE_UNDEFINED.
+	* command line options will override the kernel config settings.
+	*/
+
+	if (cppc_state == AMD_PSTATE_UNDEFINED) {
  		/* Disable on the following configs by default:
  		 * 1. Undefined platforms
  		 * 2. Server platforms

@@ -1870,15 +1875,20 @@ static int __init amd_pstate_init(void)
  			pr_info("driver load is disabled, boot with specific mode to enable this\n");
  			return -ENODEV;
  		}
-		ret = amd_pstate_set_driver(CONFIG_X86_AMD_PSTATE_DEFAULT_MODE);
-		if (ret)
-			return ret;
-		break;
+		/* get driver mode from kernel config option [1:4] */
+		cppc_state = CONFIG_X86_AMD_PSTATE_DEFAULT_MODE;
+	}
+
+	switch (cppc_state) {
  	case AMD_PSTATE_DISABLE:
+		pr_info("driver load is disabled, boot with specific mode to enable this\n");
  		return -ENODEV;
  	case AMD_PSTATE_PASSIVE:
  	case AMD_PSTATE_ACTIVE:
  	case AMD_PSTATE_GUIDED:
+		ret = amd_pstate_set_driver(cppc_state);
+		if (ret)
+			return ret;
  		break;
  	default:
  		return -EINVAL;

@@ -1899,7 +1909,7 @@ static int __init amd_pstate_init(void)
  	/* enable amd pstate feature */
  	ret = amd_pstate_enable(true);
  	if (ret) {
-		pr_err("failed to enable with return %d\n", ret);
+		pr_err("failed to enable driver mode(%d)\n", cppc_state);
  		return ret;
  	}
 

-----Original Message-----
From: Limonciello, Mario <[email protected]>
Sent: Wednesday, June 19, 2024 3:25 AM
To: Yuan, Perry <[email protected]>; [email protected]; [email protected]; Huang, Ray <[email protected]>; Shenoy, Gautham Ranjal <[email protected]>; Petkov, Borislav <[email protected]>
Cc: Deucher, Alexander <[email protected]>; Huang, Shimmer <[email protected]>; Du, Xiaojian <[email protected]>; Meng, Li (Jassmine) <[email protected]>; [email protected]; linux- [email protected]
Subject: Re: [PATCH v4 10/11] cpufreq: amd-pstate: auto-load pstate driver by default

Quoted 12 lines · Perry Yuan

If the amd-pstate driver is not loaded automatically by default, it is because the kernel command line parameter has not been added. To resolve this issue, it is necessary to call the amd_pstate_set_driver() function to enable the desired mode (passive/active/guided) before registering the driver instance.

This ensures that the driver is loaded correctly without relying on the kernel command line parameter.

When there is no parameter added to command line, Kernel config will provide the default mode to load.

Quoted 1 line

Meanwhle, user can add driver mode in command line which will override

Meanwhile

Quoted 1 line

the kernel config default option.

I think you'll probably want to swap the order of patch 10 and patch 11.

Quoted 5 lines

Reported-by Andrei Amuraritei <[email protected]>

Closes https://bugzilla.kernel.org/show_bug.cgi?id=218705

Signed-off-by Perry Yuan <[email protected]>

Reviewed-by Mario Limonciello <[email protected]>

Quoted 6 lines

---
  drivers/cpufreq/amd-pstate.c | 24 +++++++++++++++++-------
  1 file changed, 17 insertions(+), 7 deletions(-)

(quoted diff) +0 -0

b/drivers/cpufreq/amd-pstate.c index cf68343219d1..b48fd60cbc6d

100644

drivers/cpufreq/amd-pstate.c static int __init amd_pstate_init(void) · line 1861

Quoted 10 lines

drivers/cpufreq/amd-pstate.c +2 -2

@@ -1857,8 +1857,13 @@ static int __init amd_pstate_init(void)
    /* check if this machine need CPPC quirks */
    dmi_check_system(amd_pstate_quirks_table);
 
-   switch (cppc_state) {
-   case AMD_PSTATE_UNDEFINED:
+   /*
+   * determine the driver mode from the command line or kernel

config.

drivers/cpufreq/amd-pstate.c static int __init amd_pstate_init(void) · line 1862

Quoted 1 line

drivers/cpufreq/amd-pstate.c +1 -0

+   * If no command line input is provided, cppc_state will be

AMD_PSTATE_UNDEFINED.

drivers/cpufreq/amd-pstate.c static int __init amd_pstate_init(void) · line 1875

Quoted 9 lines

drivers/cpufreq/amd-pstate.c +4 -0

+   * command line options will override the kernel config settings.
+   */
+
+   if (cppc_state == AMD_PSTATE_UNDEFINED) {
            /* Disable on the following configs by default:
             * 1. Undefined platforms
             * 2. Server platforms

@@ -1870,15 +1875,20 @@ static int __init amd_pstate_init(void)
                    pr_info("driver load is disabled, boot with specific

mode to enable this\n");

drivers/cpufreq/amd-pstate.c static int __init amd_pstate_init(void)

Quoted 3 lines

drivers/cpufreq/amd-pstate.c +0 -1

                    return -ENODEV;
            }
-           ret =

amd_pstate_set_driver(CONFIG_X86_AMD_PSTATE_DEFAULT_MODE);

drivers/cpufreq/amd-pstate.c static int __init amd_pstate_init(void)

Quoted 10 lines

drivers/cpufreq/amd-pstate.c +6 -3

-           if (ret)
-                   return ret;
-           break;
+           /* get driver mode from kernel config option [1:4] */
+           cppc_state = CONFIG_X86_AMD_PSTATE_DEFAULT_MODE;
+   }
+
+   switch (cppc_state) {
    case AMD_PSTATE_DISABLE:
+           pr_info("driver load is disabled, boot with specific mode to

enable

Quoted 20 lines

drivers/cpufreq/amd-pstate.c +5 -1

+this\n");
            return -ENODEV;
    case AMD_PSTATE_PASSIVE:
    case AMD_PSTATE_ACTIVE:
    case AMD_PSTATE_GUIDED:
+           ret = amd_pstate_set_driver(cppc_state);
+           if (ret)
+                   return ret;
            break;
    default:
            return -EINVAL;

@@ -1899,7 +1909,7 @@ static int __init amd_pstate_init(void)
    /* enable amd pstate feature */
    ret = amd_pstate_enable(true);
    if (ret) {
-           pr_err("failed to enable with return %d\n", ret);
+           pr_err("failed to enable driver mode(%d)\n", cppc_state);
            return ret;
    }
 

The amd-pstate-epp driver has been implemented and resolves the performance drop issue seen in passive mode. Users who enable the active mode driver will not experience a performance drop compared to the passive mode driver. Therefore, the EPP driver should be loaded by default at system boot.

Signed-off-by Perry Yuan <[email protected]>

---
  drivers/cpufreq/amd-pstate.c | 13 +------------
  1 file changed, 1 insertion(+), 12 deletions(-)

drivers/cpufreq/amd-pstate.c +1 -12

index b48fd60cbc6d..eca2f7dcf7ce 100644
@@ -96,15 +96,6 @@ enum amd_core_type {
  	CPU_CORE_TYPE_UNDEFINED = 2,
  };
 
-/*
- * TODO: We need more time to fine tune processors with shared memory solution
- * with community together.
- *
- * There are some performance drops on the CPU benchmarks which reports from
- * Suse. We are co-working with them to fine tune the shared memory solution. So
- * we disable it by default to go acpi-cpufreq on these processors and add a
- * module parameter to be able to enable it manually for debugging.
- */
  static struct cpufreq_driver *current_pstate_driver;
  static struct cpufreq_driver amd_pstate_driver;
  static struct cpufreq_driver amd_pstate_epp_driver;

@@ -1867,11 +1858,9 @@ static int __init amd_pstate_init(void)
  		/* Disable on the following configs by default:
  		 * 1. Undefined platforms
  		 * 2. Server platforms
-		 * 3. Shared memory designs
  		 */
  		if (amd_pstate_acpi_pm_profile_undefined() ||
-		    amd_pstate_acpi_pm_profile_server() ||
-		    !cpu_feature_enabled(X86_FEATURE_CPPC)) {
+		    amd_pstate_acpi_pm_profile_server()) {
  			pr_info("driver load is disabled, boot with specific mode to enable this\n");
  			return -ENODEV;
  		}

-----Original Message-----
From: Limonciello, Mario <[email protected]>
Sent: Wednesday, June 19, 2024 3:25 AM
To: Yuan, Perry <[email protected]>; [email protected]; [email protected]; Huang, Ray <[email protected]>; Shenoy, Gautham Ranjal <[email protected]>; Petkov, Borislav <[email protected]>
Cc: Deucher, Alexander <[email protected]>; Huang, Shimmer <[email protected]>; Du, Xiaojian <[email protected]>; Meng, Li (Jassmine) <[email protected]>; [email protected]; linux- [email protected]
Subject: Re: [PATCH v4 11/11] cpufreq: amd-pstate: enable shared memory type CPPC by default

Quoted 5 lines · Perry Yuan

The amd-pstate-epp driver has been implemented and resolves the performance drop issue seen in passive mode. Users who enable the active mode driver will not experience a performance drop compared to the passive mode driver. Therefore, the EPP driver should be loaded by default at system boot.

I think this commit message should specifically reference that it's being enabled by default on shared memory designs as that's the net new.

The code change looks good, go ahead and add

Reviewed-by Mario Limonciello <[email protected]>

on the next version as long as you've added a sentence about shared memory designs to commit message.

Quoted 35 lines

Signed-off-by Perry Yuan <[email protected]>

---
  drivers/cpufreq/amd-pstate.c | 13 +------------
  1 file changed, 1 insertion(+), 12 deletions(-)

(quoted diff) +0 -0

b/drivers/cpufreq/amd-pstate.c index b48fd60cbc6d..eca2f7dcf7ce 100644

drivers/cpufreq/amd-pstate.c +0 -2

@@ -96,15 +96,6 @@ enum amd_core_type {
    CPU_CORE_TYPE_UNDEFINED = 2,
  };
 
-/*
- * TODO: We need more time to fine tune processors with shared memory

solution
- * with community together.
- *
- * There are some performance drops on the CPU benchmarks which reports from
- * Suse. We are co-working with them to fine tune the shared memory solution. So
- * we disable it by default to go acpi-cpufreq on these processors and add a
- * module parameter to be able to enable it manually for debugging.
- */
static struct cpufreq_driver *current_pstate_driver; static struct cpufreq_driver amd_pstate_driver;
static struct cpufreq_driver amd_pstate_epp_driver; @@ -1867,11
+1858,9 @@ static int __init amd_pstate_init(void)
/* Disable on the following configs by default:
* 1. Undefined platforms
* 2. Server platforms

drivers/cpufreq/amd-pstate.c static int __init amd_pstate_init(void)

Quoted 7 lines

drivers/cpufreq/amd-pstate.c +1 -3

-            * 3. Shared memory designs
             */
            if (amd_pstate_acpi_pm_profile_undefined() ||
-               amd_pstate_acpi_pm_profile_server() ||
-               !cpu_feature_enabled(X86_FEATURE_CPPC)) {
+               amd_pstate_acpi_pm_profile_server()) {
                    pr_info("driver load is disabled, boot with specific

mode to enable this\n");

Quoted 2 lines

                    return -ENODEV;
            }