Skip to contentSkip to conversation

Option to disable AMD C1E (allows dynticks to work)

PATCH In review Eduard-Gabriel Munteanu 3 participants 5 messages Dec 13, 2007 – Dec 18, 2007

Subsystems (2)
Documentation x86
lore
Share & tools
lore

Eduard-Gabriel Munteanu

author
#0 lore raw

PATCH 1/1 Option to disable AMD C1E (allows dynticks to work)

Some multiprocessor 64-bit AMD systems don't allow the user to disable the C1E C-state. The kernel detects C1E and marks the LAPIC as broken, thereby disabling dynticks. This patch adds an option to disable C1E when detected. It also allows the user to enable this processor feature even if that means disabling dynticks, which is useful in case C1E might provide better power savings (e.g.: C-states beyond C1 don't work). Tested on a Turion X2 TL-56 laptop. Thanks to Mikhail Kshevetskiy and FreeBSD for pointing out the relevant AMD docs.

Signed-off-by Eduard-Gabriel Munteanu <[email protected]>
---
 Documentation/kernel-parameters.txt |    6 ++++++
 arch/x86/Kconfig                    |   16 ++++++++++++++++
 arch/x86/kernel/setup_64.c          |   27 +++++++++++++++++++++++++--
 3 files changed, 47 insertions(+), 2 deletions(-)
Documentation/kernel-parameters.txt +6 -0
index 33121d6..0deee7a 100644
@@ -643,6 +643,12 @@ and is between 256 and 4096 characters. It is defined in the file
 	floppy=		[HW]
 			See Documentation/floppy.txt.
 
+	force_amd_c1e	[KNL,SMP,HW,BUGS=X86-64]
+			Don't disable C1E on AMD systems even if this means
+			disabling nohz. This is _not_ automatically implied by
+			any other parameters, such as "nohz=off".
+			Depends on CONFIG_X86_AMD_C1E_WORKAROUND.
+	
 	gamecon.map[2|3]=
 			[HW,JOY] Multisystem joystick and NES/SNES/PSX pad
 			support via parallel port (up to 5 devices per port)
arch/x86/Kconfig +16 -0
index 368864d..8b9bb49 100644
@@ -198,6 +198,22 @@ config SMP
 
 	  If you don't know what to do here, say N.
 
+config X86_AMD_C1E_WORKAROUND
+	bool "Disable C1E on AMD systems to make dynticks work"
+	default y
+	depends on X86_64 && SMP && NO_HZ
+	---help---
+	  On some systems, the C1E C-state is enabled by default and cannot be
+	  disabled from the CMOS setup. Local APICs don't behave as they should
+	  in this case. If you say Y here, C1E will be disabled to allow
+	  dynamic ticks to work. It's safe to enable this option even if
+	  your system doesn't have an AMD CPU (there are no side-effects if
+	  such a CPU isn't detected).
+
+	  You can pass the "force_amd_c1e" boot parameter to the kernel to
+	  disable this workaround without recompiling.
+	  See Documentation/kernel-parameters.txt for more details.
+
 choice
 	prompt "Subarchitecture Type"
 	default X86_PC
arch/x86/kernel/setup_64.c +25 -2
index 30d94d1..15556a0 100644
@@ -583,6 +583,17 @@ static void __init amd_detect_cmp(struct cpuinfo_x86 *c)
 #endif
 }
 
+#ifdef CONFIG_X86_AMD_C1E_WORKAROUND
+static int __cpuinit disable_amd_c1e = 1;
+
+static int __cpuinit force_amd_c1e(char *str) {
+	disable_amd_c1e = 0;
+	return 1;
+}
+
+__setup("force_amd_c1e", force_amd_c1e);
+#endif /* CONFIG_X86_AMD_C1E_WORKAROUND */
+
 #define ENABLE_C1E_MASK		0x18000000
 #define CPUID_PROCESSOR_SIGNATURE	1
 #define CPUID_XFAM		0x0ff00000

@@ -597,6 +608,7 @@ static __cpuinit int amd_apic_timer_broken(void)
 {
 	u32 lo, hi;
 	u32 eax = cpuid_eax(CPUID_PROCESSOR_SIGNATURE);
+
 	switch (eax & CPUID_XFAM) {
 	case CPUID_XFAM_K8:
 		if ((eax & CPUID_XMOD) < CPUID_XMOD_REV_F)

@@ -604,8 +616,19 @@ static __cpuinit int amd_apic_timer_broken(void)
 	case CPUID_XFAM_10H:
 	case CPUID_XFAM_11H:
 		rdmsr(MSR_K8_ENABLE_C1E, lo, hi);
-		if (lo & ENABLE_C1E_MASK)
-			return 1;
+#ifdef CONFIG_X86_AMD_C1E_WORKAROUND
+		if ((lo & ENABLE_C1E_MASK) && disable_amd_c1e) {
+			printk(KERN_INFO "Disabling AMD C1E on CPU %d\n",
+			       smp_processor_id());
+			/* 
+			 * See AMD's "BIOS and Kernel Developer's Guide for AMD
+			 * NPT Family 0Fh Processors", publication #32559,
+			 * for details.
+			 */
+			wrmsr(MSR_K8_ENABLE_C1E, lo & ~ENABLE_C1E_MASK, hi); 
+		} else 
+#endif /* CONFIG_X86_AMD_C1E_WORKAROUND */
+		if (lo & ENABLE_C1E_MASK) return 1;
 		break;
 	default:
 		/* err on the side of caution */
#1 lore raw

Hi,

Quoted 39 lines · Eduard-Gabriel Munteanu
arch/x86/kernel/setup_64.c +15 -2
index 30d94d1..15556a0 100644
@@ -583,6 +583,17 @@ static void __init amd_detect_cmp(struct cpuinfo_x86 *c)
 #endif
 }
 
+#ifdef CONFIG_X86_AMD_C1E_WORKAROUND
+static int __cpuinit disable_amd_c1e = 1;
+
+static int __cpuinit force_amd_c1e(char *str) {
+	disable_amd_c1e = 0;
+	return 1;
+}
+
+__setup("force_amd_c1e", force_amd_c1e);
+#endif /* CONFIG_X86_AMD_C1E_WORKAROUND */
+
 #define ENABLE_C1E_MASK		0x18000000
 #define CPUID_PROCESSOR_SIGNATURE	1
 #define CPUID_XFAM		0x0ff00000

@@ -597,6 +608,7 @@ static __cpuinit int amd_apic_timer_broken(void)
 {
 	u32 lo, hi;
 	u32 eax = cpuid_eax(CPUID_PROCESSOR_SIGNATURE);
+
 	switch (eax & CPUID_XFAM) {
 	case CPUID_XFAM_K8:
 		if ((eax & CPUID_XMOD) < CPUID_XMOD_REV_F)

@@ -604,8 +616,19 @@ static __cpuinit int amd_apic_timer_broken(void)
 	case CPUID_XFAM_10H:
 	case CPUID_XFAM_11H:
 		rdmsr(MSR_K8_ENABLE_C1E, lo, hi);
-		if (lo & ENABLE_C1E_MASK)
-			return 1;
+#ifdef CONFIG_X86_AMD_C1E_WORKAROUND
+		if ((lo & ENABLE_C1E_MASK) && disable_amd_c1e) {
+			printk(KERN_INFO "Disabling AMD C1E on CPU %d\n",

arch/x86/kernel/setup_64.c static __cpuinit int amd_apic_timer_broken(void) · line 631

Quoted 10 lines
arch/x86/kernel/setup_64.c +10 -0
+			       smp_processor_id());
+			/* 
+			 * See AMD's "BIOS and Kernel Developer's Guide for AMD
+			 * NPT Family 0Fh Processors", publication #32559,
+			 * for details.
+			 */
+			wrmsr(MSR_K8_ENABLE_C1E, lo & ~ENABLE_C1E_MASK, hi); 
+		} else 
+#endif /* CONFIG_X86_AMD_C1E_WORKAROUND */
+		if (lo & ENABLE_C1E_MASK) return 1;

I would find this way more readable:

	if (lo & ENABLE_C1E_MASK) {
#ifdef CONFIG_X86_AMD_C1E_WORKAROUND
		if (disable_amd_c1e) {
			...
#else
		return 1;
#endif
	}

Why does it require to be enabled by compile-time AND run-time? Is that something you might want to decide on every boot? Could we make it settable at boot-time xor at compile-time?

Hannes

author
#2 lore raw
Quoted 10 lines · On Thu, 13 Dec 2007 23:58:50 +0100 Johannes Weiner…

I would find this way more readable:

	if (lo & ENABLE_C1E_MASK) {
#ifdef CONFIG_X86_AMD_C1E_WORKAROUND
		if (disable_amd_c1e) {
			...
#else
		return 1;
#endif
	}

I wanted to avoid using too many tabs, as that would require the lines inside the if block to be split too many times. I'll resubmit the patch if you think it's necessary.

Quoted 3 lines

Why does it require to be enabled by compile-time AND run-time? Is that something you might want to decide on every boot? Could we make it settable at boot-time xor at compile-time?

Making it settable only at boot-time means compiling in unnecessary code. Also, making it settable only at compile-time would be bad for distros that distribute binary kernels, as some users may experience worse power savings with dynticks than with C1E (especially when C2, C3 and higher C-states are not available, due to brain-dead firmware). This allows them to opt for leaving C1E enabled.

It is not required to enable it at both compile-time and boot-time. You enable it at compile-time and then you _can disable_ it at boot-time.

#3 lore raw
In reply to Eduard-Gabriel Munteanu
Quoted 8 lines · Eduard-Gabriel Munteanu

Some multiprocessor 64-bit AMD systems don't allow the user to disable the C1E C-state. The kernel detects C1E and marks the LAPIC as broken, thereby disabling dynticks. This patch adds an option to disable C1E when detected. It also allows the user to enable this processor feature even if that means disabling dynticks, which is useful in case C1E might provide better power savings (e.g.: C-states beyond C1 don't work). Tested on a Turion X2 TL-56 laptop. Thanks to Mikhail Kshevetskiy and FreeBSD for pointing out the relevant AMD docs.

...

Quoted 1 line

+static int __cpuinit force_amd_c1e(char *str) {

{ on new line, please.

Quoted 3 lines
arch/x86/kernel/setup_64.c +3 -0
+	disable_amd_c1e = 0;
+	return 1;
+}
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
author
#4 lore raw
Quoted 9 lines · On Tue, 18 Dec 2007 15:43:52 +0000 Pavel Machek <[email protected]> wrote:
Quoted 1 line · Eduard-Gabriel Munteanu

+static int __cpuinit force_amd_c1e(char *str) {

{ on new line, please.

arch/x86/kernel/setup_64.c static void __init amd_detect_cmp(struct cpuinfo_x86 *c) · line 592

Quoted 3 lines
arch/x86/kernel/setup_64.c +3 -0
+	disable_amd_c1e = 0;
+	return 1;
+}

Sorry, I know it's a bad habit. I'll be more careful next time. Thanks for pointing it out, though.

Anyway, there's no point in resubmitting a corrected patch, as we already discussed that this approach isn't helpful.