mtrr/x86/drivers: bury MTRR

Quoted 14 lines · Ingo Molnar

Yeah, so many of these seem to be 'leaf only' functions: functions that don't ever call functions themselves.

So lets assume we always have CONFIG_FRAME_POINTERS=y.

If they don't set up a frame pointer then they in essence won't show up in the call chain - but normally they wouldn't because they call nothing.

If they trigger an exception/fault or if they get hit by an interrupt then I think we'll still correctly walk the stack - just those functions might be missing from the deterministic call chain, right? (it will still show up as a '?' entry.)

Quoted 5 lines

If they crash then we'll see them because the crashing RIP will be printed.

So I'm wondering what the x86 policy here should be: to create frame pointers in them or not. Cc:-ed a few more gents for thoughts.

After removing the frame pointer checks for leaf functions, and adding a check for all functions which jump outside of their scope, the number of defconfig warnings dropped from 89 -> 47. The Fedora config warning count dropped from 207 -> 83.

Here are the remaining 47 warnings for defconfig:

stackvalidate: arch/x86/ia32/ia32entry.o: ia32_sysenter_target() is missing frame pointer logic
stackvalidate: arch/x86/ia32/ia32entry.o: return instruction outside of a function at .entry.text+0x52e
stackvalidate: arch/x86/kernel/entry_64.o: return instruction outside of a function at .entry.text+0x359
stackvalidate: arch/x86/kernel/entry_64.o: return instruction outside of a function at .entry.text+0x19be
stackvalidate: arch/x86/kernel/entry_64.o: return instruction outside of a function at .entry.text+0x19e5
stackvalidate: arch/x86/kernel/entry_64.o: return instruction outside of a function at .entry.text+0x1c21
stackvalidate: arch/x86/kernel/entry_64.o: return instruction outside of a function at .entry.text+0x1ceb
stackvalidate: arch/x86/kernel/acpi/wakeup_64.o: unsupported jump to outside of the function at wakeup_long64+0x15
stackvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel() is missing frame pointer logic
stackvalidate: arch/x86/kernel/relocate_kernel_64.o: return instruction outside of a function at .text+0x6b
stackvalidate: arch/x86/kernel/relocate_kernel_64.o: return instruction outside of a function at .text+0xc7
stackvalidate: arch/x86/kernel/relocate_kernel_64.o: return instruction outside of a function at .text+0x110
stackvalidate: arch/x86/kernel/relocate_kernel_64.o: return instruction outside of a function at .text+0x145
stackvalidate: arch/x86/kernel/relocate_kernel_64.o: return instruction outside of a function at .text+0x1c4
stackvalidate: arch/x86/kernel/head_64.o: return instruction outside of a function at .head.text+0x1a2
stackvalidate: arch/x86/kernel/head_64.o: early_idt_handler() is missing frame pointer logic
stackvalidate: arch/x86/platform/efi/efi_stub_64.o: efi_call() is missing frame pointer logic
stackvalidate: arch/x86/realmode/rm/trampoline_64.o: return instruction outside of a function at .text+0x170
stackvalidate: arch/x86/realmode/rm/trampoline_64.o: return instruction outside of a function at .text+0x176
stackvalidate: arch/x86/realmode/rm/reboot.o: return instruction outside of a function at .text+0x2a
stackvalidate: arch/x86/realmode/rm/copy.o: copy_from_fs() is missing frame pointer logic
stackvalidate: arch/x86/realmode/rm/copy.o: copy_to_fs() is missing frame pointer logic
stackvalidate: arch/x86/power/hibernate_asm_64.o: return instruction outside of a function at .text+0x69
stackvalidate: arch/x86/power/hibernate_asm_64.o: return instruction outside of a function at .text+0x16d
stackvalidate: arch/x86/lib/copy_user_64.o: unsupported jump to outside of the function at _copy_to_user+0x25
stackvalidate: arch/x86/lib/copy_user_64.o: unsupported jump to outside of the function at _copy_from_user+0x25
stackvalidate: arch/x86/lib/getuser.o: unsupported jump to outside of the function at __get_user_1+0x14
stackvalidate: arch/x86/lib/getuser.o: unsupported jump to outside of the function at __get_user_2+0x4
stackvalidate: arch/x86/lib/getuser.o: unsupported jump to outside of the function at __get_user_4+0x4
stackvalidate: arch/x86/lib/getuser.o: unsupported jump to outside of the function at __get_user_8+0x4
stackvalidate: arch/x86/lib/getuser.o: return instruction outside of a function at .text+0xc5
stackvalidate: arch/x86/lib/memmove_64.o: return instruction outside of a function at .altinstr_replacement+0x5
stackvalidate: arch/x86/lib/putuser.o: unsupported jump to outside of the function at __put_user_1+0x14
stackvalidate: arch/x86/lib/putuser.o: unsupported jump to outside of the function at __put_user_2+0x1b
stackvalidate: arch/x86/lib/putuser.o: unsupported jump to outside of the function at __put_user_4+0x1b
stackvalidate: arch/x86/lib/putuser.o: unsupported jump to outside of the function at __put_user_8+0x1b
stackvalidate: arch/x86/lib/putuser.o: return instruction outside of a function at .text+0xc1
stackvalidate: arch/x86/lib/rwsem.o: call_rwsem_down_read_failed() is missing frame pointer logic
stackvalidate: arch/x86/lib/rwsem.o: call_rwsem_down_write_failed() is missing frame pointer logic
stackvalidate: arch/x86/lib/rwsem.o: call_rwsem_wake() is missing frame pointer logic
stackvalidate: arch/x86/lib/rwsem.o: call_rwsem_downgrade_wake() is missing frame pointer logic
stackvalidate: arch/x86/boot/copy.o: copy_from_fs() is missing frame pointer logic
stackvalidate: arch/x86/boot/copy.o: copy_to_fs() is missing frame pointer logic
stackvalidate: arch/x86/boot/compressed/head_64.o: return instruction outside of a function at .text+0x16e
stackvalidate: arch/x86/boot/compressed/head_64.o: return instruction outside of a function at .text+0x172
stackvalidate: arch/x86/boot/compressed/head_64.o: startup_32() is missing frame pointer logic
stackvalidate: arch/x86/boot/pmjump.o: unsupported jump to outside of the function at in_pm32+0x1c

Note that only 13 of the 47 warnings are actually due to missing frame pointer logic. The rest are ambiguous conditions which prevent stackvalidate from being able to make sense of things: returning from outside of a proper ELF function, or jumping from inside of a function to outside of its scope.

Similarly, in the Fedora config case, only 27 of the 83 warnings are for missing frame pointer logic.

If there are no objections, I'll go with this approach in the next version of the patch set.

Quoted 61 lines · Josh Poimboeuf

After removing the frame pointer checks for leaf functions, and adding a check for all functions which jump outside of their scope, the number of defconfig warnings dropped from 89 -> 47. The Fedora config warning count dropped from 207 -> 83.

Here are the remaining 47 warnings for defconfig:

stackvalidate: arch/x86/ia32/ia32entry.o: ia32_sysenter_target() is missing frame pointer logic
stackvalidate: arch/x86/ia32/ia32entry.o: return instruction outside of a function at .entry.text+0x52e
stackvalidate: arch/x86/kernel/entry_64.o: return instruction outside of a function at .entry.text+0x359
stackvalidate: arch/x86/kernel/entry_64.o: return instruction outside of a function at .entry.text+0x19be
stackvalidate: arch/x86/kernel/entry_64.o: return instruction outside of a function at .entry.text+0x19e5
stackvalidate: arch/x86/kernel/entry_64.o: return instruction outside of a function at .entry.text+0x1c21
stackvalidate: arch/x86/kernel/entry_64.o: return instruction outside of a function at .entry.text+0x1ceb
stackvalidate: arch/x86/kernel/acpi/wakeup_64.o: unsupported jump to outside of the function at wakeup_long64+0x15
stackvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel() is missing frame pointer logic
stackvalidate: arch/x86/kernel/relocate_kernel_64.o: return instruction outside of a function at .text+0x6b
stackvalidate: arch/x86/kernel/relocate_kernel_64.o: return instruction outside of a function at .text+0xc7
stackvalidate: arch/x86/kernel/relocate_kernel_64.o: return instruction outside of a function at .text+0x110
stackvalidate: arch/x86/kernel/relocate_kernel_64.o: return instruction outside of a function at .text+0x145
stackvalidate: arch/x86/kernel/relocate_kernel_64.o: return instruction outside of a function at .text+0x1c4
stackvalidate: arch/x86/kernel/head_64.o: return instruction outside of a function at .head.text+0x1a2
stackvalidate: arch/x86/kernel/head_64.o: early_idt_handler() is missing frame pointer logic
stackvalidate: arch/x86/platform/efi/efi_stub_64.o: efi_call() is missing frame pointer logic
stackvalidate: arch/x86/realmode/rm/trampoline_64.o: return instruction outside of a function at .text+0x170
stackvalidate: arch/x86/realmode/rm/trampoline_64.o: return instruction outside of a function at .text+0x176
stackvalidate: arch/x86/realmode/rm/reboot.o: return instruction outside of a function at .text+0x2a
stackvalidate: arch/x86/realmode/rm/copy.o: copy_from_fs() is missing frame pointer logic
stackvalidate: arch/x86/realmode/rm/copy.o: copy_to_fs() is missing frame pointer logic
stackvalidate: arch/x86/power/hibernate_asm_64.o: return instruction outside of a function at .text+0x69
stackvalidate: arch/x86/power/hibernate_asm_64.o: return instruction outside of a function at .text+0x16d
stackvalidate: arch/x86/lib/copy_user_64.o: unsupported jump to outside of the function at _copy_to_user+0x25
stackvalidate: arch/x86/lib/copy_user_64.o: unsupported jump to outside of the function at _copy_from_user+0x25
stackvalidate: arch/x86/lib/getuser.o: unsupported jump to outside of the function at __get_user_1+0x14
stackvalidate: arch/x86/lib/getuser.o: unsupported jump to outside of the function at __get_user_2+0x4
stackvalidate: arch/x86/lib/getuser.o: unsupported jump to outside of the function at __get_user_4+0x4
stackvalidate: arch/x86/lib/getuser.o: unsupported jump to outside of the function at __get_user_8+0x4
stackvalidate: arch/x86/lib/getuser.o: return instruction outside of a function at .text+0xc5
stackvalidate: arch/x86/lib/memmove_64.o: return instruction outside of a function at .altinstr_replacement+0x5
stackvalidate: arch/x86/lib/putuser.o: unsupported jump to outside of the function at __put_user_1+0x14
stackvalidate: arch/x86/lib/putuser.o: unsupported jump to outside of the function at __put_user_2+0x1b
stackvalidate: arch/x86/lib/putuser.o: unsupported jump to outside of the function at __put_user_4+0x1b
stackvalidate: arch/x86/lib/putuser.o: unsupported jump to outside of the function at __put_user_8+0x1b
stackvalidate: arch/x86/lib/putuser.o: return instruction outside of a function at .text+0xc1
stackvalidate: arch/x86/lib/rwsem.o: call_rwsem_down_read_failed() is missing frame pointer logic
stackvalidate: arch/x86/lib/rwsem.o: call_rwsem_down_write_failed() is missing frame pointer logic
stackvalidate: arch/x86/lib/rwsem.o: call_rwsem_wake() is missing frame pointer logic
stackvalidate: arch/x86/lib/rwsem.o: call_rwsem_downgrade_wake() is missing frame pointer logic
stackvalidate: arch/x86/boot/copy.o: copy_from_fs() is missing frame pointer logic
stackvalidate: arch/x86/boot/copy.o: copy_to_fs() is missing frame pointer logic
stackvalidate: arch/x86/boot/compressed/head_64.o: return instruction outside of a function at .text+0x16e
stackvalidate: arch/x86/boot/compressed/head_64.o: return instruction outside of a function at .text+0x172
stackvalidate: arch/x86/boot/compressed/head_64.o: startup_32() is missing frame pointer logic
stackvalidate: arch/x86/boot/pmjump.o: unsupported jump to outside of the function at in_pm32+0x1c

Note that only 13 of the 47 warnings are actually due to missing frame pointer logic. The rest are ambiguous conditions which prevent stackvalidate from being able to make sense of things: returning from outside of a proper ELF function, or jumping from inside of a function to outside of its scope.

Quoted 5 lines

Similarly, in the Fedora config case, only 27 of the 83 warnings are for missing frame pointer logic.

If there are no objections, I'll go with this approach in the next version of the patch set.

I'm willing to review anything with "entry" in its filename.

stackvalidate: arch/x86/lib/memmove_64.o: return instruction outside of a function at .altinstr_replacement+0x5

Quoted 1 line · Josh Poimboeuf

stackvalidate: arch/x86/lib/memmove_64.o: return instruction outside of a function at .altinstr_replacement+0x5

That must be something like this:

0000000000000000 <.altinstr_replacement>:

   0:   48 89 d1                mov    %rdx,%rcx
   3:   f3 a4                   rep movsb %ds:(%rsi),%es:(%rdi)
   5:   c3                      retq

right?

In any case, anything with alternatives is probably a false positive because even if instructions appear outside of the containing function, they get patched in and are actually inside. Jump offsets get fixed up properly too. Should, at least :-)

Hm, alternatives do complicate things a bit. It *is* a false positive, but not necessarily because its part of an alternative instruction block.

The above code would be patched into memmove(), which is a leaf function because it doesn't call any other functions. Leaf functions don't need frame pointer logic, so we can ignore them.

If instead the above code were patched into a non-leaf function, we'd have to change it to restore the frame pointer before returning.

Quoted 8 lines · Josh Poimboeuf

Hm, alternatives do complicate things a bit. It *is* a false positive, but not necessarily because its part of an alternative instruction block.

The above code would be patched into memmove(), which is a leaf function because it doesn't call any other functions. Leaf functions don't need frame pointer logic, so we can ignore them.

Quoted 2 lines

If instead the above code were patched into a non-leaf function, we'd have to change it to restore the frame pointer before returning.

Is this really only a problem of alternatives? How about dynamically-enabled tracepoints?

If instead the above code were patched into a non-leaf function, we'd have to change it to restore the frame pointer before returning.

On AMD Carrizo processors (Family 15h, Model 60h-6fh), there is a new feature called MWAITT (Mwait with a timer) as an extension of Monitor/Mwait.

MWAITT, another name is MWAITX (MWAIT with extensions), has a configurable timer that causes MWAITX to exit on expiration.

Compared with MONITOR/MWAIT, there are minor differences in opcode and input parameters.

MWAITX ECX[1]: enable timer if set
MWAITX EBX[31:0]: max wait time expressed in SW P0 clocks

The software P0 frequency is the same as the TSC frequency.

Max timeout = EBX/(TSC frequency)

Signed-off-by Huang Rui <[email protected]>

---
 arch/x86/include/asm/cpufeature.h |  1 +
 arch/x86/include/asm/mwait.h      | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

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

index 3d6606f..3ef1f6e 100644
@@ -176,6 +176,7 @@
 #define X86_FEATURE_PERFCTR_NB  ( 6*32+24) /* NB performance counter extensions */
 #define X86_FEATURE_BPEXT	(6*32+26) /* data breakpoint extension */
 #define X86_FEATURE_PERFCTR_L2	( 6*32+28) /* L2 performance counter extensions */
+#define X86_FEATURE_MWAITT	( 6*32+29) /* Mwait extension (MonitorX/MwaitX) */
 
 /*
  * Auxiliary flags: Linux defined - For features scattered in various

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

index 653dfa7..b91136f 100644

arch/x86/include/asm/mwait.h +11 -0

+		     :: "a" (eax), "c" (ecx), "d"(edx));
+}
+
 static inline void __mwait(unsigned long eax, unsigned long ecx)
 {
 	/* "mwait %eax, %ecx;" */

@@ -30,6 +38,14 @@ static inline void __mwait(unsigned long eax, unsigned long ecx)
 		     :: "a" (eax), "c" (ecx));
 }
 
+static inline void __mwaitx(unsigned long eax, unsigned long ebx,
+		unsigned long ecx)
+{
+	/* "mwaitx %eax, %ebx, %ecx;" */
+	asm volatile(".byte 0x0f, 0x01, 0xfb;"
+		     :: "a" (eax), "b" (ebx), "c" (ecx));
+}
+
 static inline void __sti_mwait(unsigned long eax, unsigned long ecx)
 {
 	trace_hardirqs_on();

arch/x86/include/asm/mwait.h +7 -0

+{
+	trace_hardirqs_on();
+	/* "mwaitx %eax, %ebx, %ecx;" */
+	asm volatile("sti; .byte 0x0f, 0x01, 0xfb;"
+		     :: "a" (eax), "b" (ebx), "c" (ecx));
+}
+
 /*
  * This uses new MONITOR/MWAIT instructions on P4 processors with PNI,
  * which can obviate IPI to trigger checking of need_resched.

Signature · 1 line

2.1.0

Quoted 7 lines · Huang Rui

On AMD Carrizo processors (Family 15h, Model 60h-6fh), there is a new feature called MWAITT (Mwait with a timer) as an extension of Monitor/Mwait.

MWAITT, another name is MWAITX (MWAIT with extensions), has a configurable timer that causes MWAITX to exit on expiration.

Quoted 5 lines

Compared with MONITOR/MWAIT, there are minor differences in opcode and input parameters.

MWAITX ECX[1]: enable timer if set
MWAITX EBX[31:0]: max wait time expressed in SW P0 clocks

What's the behavior if you set EBX to some value but don't enable the timer with ECX? Normal MWAIT?

Quoted 3 lines

The software P0 frequency is the same as the TSC frequency.

Max timeout = EBX/(TSC frequency)

That's max timeout in seconds then.

Quoted 26 lines

Signed-off-by Huang Rui <[email protected]>

---
 arch/x86/include/asm/cpufeature.h |  1 +
 arch/x86/include/asm/mwait.h      | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

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

index 3d6606f..3ef1f6e 100644
@@ -176,6 +176,7 @@
 #define X86_FEATURE_PERFCTR_NB  ( 6*32+24) /* NB performance counter extensions */
 #define X86_FEATURE_BPEXT	(6*32+26) /* data breakpoint extension */
 #define X86_FEATURE_PERFCTR_L2	( 6*32+28) /* L2 performance counter extensions */
+#define X86_FEATURE_MWAITT	( 6*32+29) /* Mwait extension (MonitorX/MwaitX) */
 
 /*
  * Auxiliary flags: Linux defined - For features scattered in various

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

index 653dfa7..b91136f 100644
@@ -23,6 +23,14 @@ static inline void __monitor(const void *eax, unsigned long ecx,
 		     :: "a" (eax), "c" (ecx), "d"(edx));
 }
 

arch/x86/include/asm/mwait.h static inline void __monitor(const void *eax, unsigned long ecx, · line 30

Quoted 5 lines

arch/x86/include/asm/mwait.h +5 -0

+static inline void __monitorx(const void *eax, unsigned long ecx,
+			     unsigned long edx)
+{
+	/* "monitorx %eax, %ecx, %edx;" */
+	asm volatile(".byte 0x0f, 0x01, 0xfa;"

Ah ok, ModRM extension to secondary opcode 0x1. Simply filling out the empty slots after SWAPGS, RDTSCP, ... :)

Quoted 25 lines

arch/x86/include/asm/mwait.h +11 -0

+		     :: "a" (eax), "c" (ecx), "d"(edx));
+}
+
 static inline void __mwait(unsigned long eax, unsigned long ecx)
 {
 	/* "mwait %eax, %ecx;" */

@@ -30,6 +38,14 @@ static inline void __mwait(unsigned long eax, unsigned long ecx)
 		     :: "a" (eax), "c" (ecx));
 }
 
+static inline void __mwaitx(unsigned long eax, unsigned long ebx,
+		unsigned long ecx)
+{
+	/* "mwaitx %eax, %ebx, %ecx;" */
+	asm volatile(".byte 0x0f, 0x01, 0xfb;"
+		     :: "a" (eax), "b" (ebx), "c" (ecx));
+}
+
 static inline void __sti_mwait(unsigned long eax, unsigned long ecx)
 {
 	trace_hardirqs_on();

@@ -38,6 +54,15 @@ static inline void __sti_mwait(unsigned long eax, unsigned long ecx)
 		     :: "a" (eax), "c" (ecx));
 }
 

arch/x86/include/asm/mwait.h static inline void __sti_mwait(unsigned long eax, unsigned long ecx) · line 58

Quoted 2 lines

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

+static inline void __sti_mwaitx(unsigned long eax, unsigned long ebx,
+		unsigned long ecx)

Please align the argument on the new line to the opening brace:

static inline void __sti_mwaitx(unsigned long eax, unsigned long ebx,
unsigned long ecx)

Quoted 3 lines

Ah ok, ModRM extension to secondary opcode 0x1. Simply filling out the empty slots after SWAPGS, RDTSCP, ... :)

Sorry, I might not get your meaning.

MWAITX/MWAIT does not let the cpu core go into C1 state on AMD processors. The cpu core still consumes less power while waiting, and has faster exit from waiting than "Halt". This patch implements an interface using the kernel parameter "idle=" to configure mwaitx type and timer value.

If "idle=mwaitx", the timeout will be set as the maximum value ((2^64 - 1) * TSC cycle).
If "idle=mwaitx,100", the timeout will be set as 100ns. If the processor doesn't support MWAITX, then halt is used.

Signed-off-by Huang Rui <[email protected]>

---
 arch/x86/include/asm/mwait.h     |  2 +
 arch/x86/include/asm/processor.h |  2 +-
 arch/x86/kernel/process.c        | 79 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 1 deletion(-)

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

index b91136f..c4e51e7 100644

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

+#define MWAITX_EBX_WAIT_TIMEOUT		0xffffffff
 
 static inline void __monitor(const void *eax, unsigned long ecx,
 			     unsigned long edx)

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

index 23ba676..0f60e94 100644
@@ -733,7 +733,7 @@ extern unsigned long		boot_option_idle_override;
 extern bool			amd_e400_c1e_detected;
 
 enum idle_boot_override {IDLE_NO_OVERRIDE=0, IDLE_HALT, IDLE_NOMWAIT,
-			 IDLE_POLL};
+			 IDLE_POLL, IDLE_MWAITX};
 
 extern void enable_sep_cpu(void);
 extern int sysenter_setup(void);

arch/x86/kernel/process.c +13 -0

index 6e338e3..9d68193 100644
@@ -30,6 +30,7 @@
 #include <asm/debugreg.h>
 #include <asm/nmi.h>
 #include <asm/tlbflush.h>
+#include <asm/x86_init.h>
 
 /*
  * per-CPU TSS segments. Thre ads are completely 'soft' on Linux,

@@ -276,6 +277,7 @@ unsigned long boot_option_idle_override = IDLE_NO_OVERRIDE;
 EXPORT_SYMBOL(boot_option_idle_override);
 
 static void (*x86_idle)(void);
+static unsigned long idle_param;
 
 #ifndef CONFIG_SMP
 static inline void play_dead(void)

@@ -444,6 +446,17 @@ static int prefer_mwait_c1_over_halt(const struct cpuinfo_x86 *c)
 	return 1;
 }
 
+static int not_support_mwaitx(const struct cpuinfo_x86 *c)
+{
+	if (c->x86_vendor != X86_VENDOR_AMD)
+		return 1;
+
+	if (!cpu_has(c, X86_FEATURE_MWAITT))
+		return 1;
+
+	return 0;
+}
+
 /*
  * MONITOR/MWAIT with no hints, used for default default C1 state.
  * This invokes MWAIT with interrutps enabled and no flags,

arch/x86/kernel/process.c +5 -0

+ * The function is similar to mwait but with a timer. On AMD platforms
+ * mwaitx does not let the core go into C1 state. This provides for a
+ * faster waiting exit speed. The user can configure the idle method and
+ * timer value via the idle kernel parameter.
+ */

Quoted 9 lines · Huang Rui

MWAITX/MWAIT does not let the cpu core go into C1 state on AMD processors. The cpu core still consumes less power while waiting, and has faster exit from waiting than "Halt". This patch implements an interface using the kernel parameter "idle=" to configure mwaitx type and timer value.

If "idle=mwaitx", the timeout will be set as the maximum value ((2^64 - 1) * TSC cycle).
If "idle=mwaitx,100", the timeout will be set as 100ns. If the processor doesn't support MWAITX, then halt is used.

Does it use it to decide how 'deep' a sleep it will go into, i.e. larger timeouts cause longer entry and exit latencies?

I suppose it's also the case that if an interrupt arrives _before_ the expected timeout then MWAITX will try to exit immediately, it won't wait until the timeout, right?

Quoted 2 lines · Ingo Molnar

Does it use it to decide how 'deep' a sleep it will go into, i.e. larger timeouts cause longer entry and exit latencies?

That's what the HLT thing does. Cores go into C1 and then at some point (hysteresis, etc) the whole core complex enters C1E.

The MWAIT* should be used for only shorter sleeps as it remains in C1. IMHO, of course.

But the problem there is another: what happens if the timeout fires, you wake up and see that you can remain idle? Do HLT? Do another MWAITX round?

This means you have an additional unnecessary wakeup which costs.

Quoted 3 lines

I suppose it's also the case that if an interrupt arrives _before_ the expected timeout then MWAITX will try to exit immediately, it won't wait until the timeout, right?

I'd assume so - I mean, it must, right.

BUT!, in talking to Andy about it last night on IRC, he pointed out that when using acpi_idle, we never come to calling x86_idle() and from looking quickly at cpuidle_idle_call(), that still might be the case as we go to use_default only when there's an error with the cpuidle driver or so.

So Rui, before you go and do more work on it, you should probably analyze what cpuidle exactly does (if you haven't done so yet). And on AMD we do use acpi_idle - at least on my F15h box that is the case:

$ grep . /sys/devices/system/cpu/cpuidle/current_*
/sys/devices/system/cpu/cpuidle/current_driver:acpi_idle
/sys/devices/system/cpu/cpuidle/current_governor_ro:menu

Well, HLT does not get any hint from the OS how long the idling is expected to last.

Another MWAITX round - we've got no crystal ball, so the hint might be wrong if an external event occurs that we did not anticipate.

I don't think MWAITX will wake up in itself. (If yes then it's essentially a timer in disguise and needs a whole different approach!)

The question would be: on systems that provide ACPI idle but also have MWAITX support, which one behaves better on the hardware side?

Quoted 2 lines · Ingo Molnar

Well, HLT does not get any hint from the OS how long the idling is expected to last.

MWAIT on AMD doesn't either:

Quoted 3 lines

Another MWAITX round - we've got no crystal ball, so the hint might be wrong if an external event occurs that we did not anticipate.

So if we end up doing a bunch of MWAITX rounds instead of HLT and MWAITX saves less power than HLT, then we practically are worse.

Quoted 3 lines

I don't think MWAITX will wake up in itself. (If yes then it's essentially a timer in disguise and needs a whole different approach!)

I mean when the MWAITX timeout expires. When it does, we wake up.

Also, normal MWAIT allows for interrupts to wake it up:

"ECX specifies optional extensions for the MWAIT instruction. The only extension currently defined is ECX bit 0, which allows interrupts to wake MWAIT, even when eFLAGS.IF = 0. Support for this extension is indicated by a feature flage returned by the CPUID instruction."

Quoted 3 lines

The question would be: on systems that provide ACPI idle but also have MWAITX support, which one behaves better on the hardware side?

I'd venture a guess here that the ACPI side should be using all C-states available (think of other OSes and having optimal power savings there) and MWAITX would be worse or the same. Right now it is entering some funny state between C0 and C1 reportedly:

"But on AMD platform, mwaitx/mwait cannot go to C1 or C1E like intel. The power consumption of waiting phase is somewhere in between (C0 and C1). Actually, it's still in C0 but less power consumption than normal C0."

So my thinking currently is - provided we want to use it at all:

* Use MWAITX on entry to idle, considering that on a busy system, the statistical probability of this sleep timeout to be small, is high.

* When the timeout expires and we wake up and realize there's still nothing to do, we do HLT.

But all that is pointless if we end up in acpi_idle anyway...

- MWAITX takes a 'timeout' parameter, but otherwise behaves exactly like MWAIT: i.e. once idle it won't exit idle on its own

- based on the 'timeout' hint, MWAITX can internally optimize how deep sleep it enters. If the timeout is large it goes deep, if it's small, it goes shallow.

If it's a true timeout, as you suggest, then I don't see any obvious way to support it, especially if it does not give access to deeper sleep states.

Quoted 2 lines · Ingo Molnar

- MWAITX takes a 'timeout' parameter, but otherwise behaves exactly like MWAIT: i.e. once idle it won't exit idle on its own

Let me quote the commit message:

"MWAITT, another name is MWAITX (MWAIT with extensions), has a configurable timer that causes MWAITX to exit on expiration."

You need to set the second bit in ECX to enable the timer.

I guess if you don't, then you get normal MWAIT but then you don't need the timeout either...

Quoted 8 lines · Borislav Petkov

Quoted 2 lines · Ingo Molnar

- MWAITX takes a 'timeout' parameter, but otherwise behaves exactly like MWAIT: i.e. once idle it won't exit idle on its own

Let me quote the commit message:

"MWAITT, another name is MWAITX (MWAIT with extensions), has a configurable timer that causes MWAITX to exit on expiration."

Ah. A useful skill that is, being able to read.

Quoted 2 lines

You need to set the second bit in ECX to enable the timer.

Quoted 2 lines

I guess if you don't, then you get normal MWAIT but then you don't need the timeout either...

Yeah.

So if it's a true timeout then we could use it to implement irq-less
timers: that's actually pretty useful, because it could be faster than getting a local APIC timer irq, etc.

Quoted 18 lines · Ingo Molnar

Quoted 8 lines · Borislav Petkov

Quoted 2 lines · Ingo Molnar

- MWAITX takes a 'timeout' parameter, but otherwise behaves exactly like MWAIT: i.e. once idle it won't exit idle on its own

Let me quote the commit message:

"MWAITT, another name is MWAITX (MWAIT with extensions), has a configurable timer that causes MWAITX to exit on expiration."

Ah. A useful skill that is, being able to read.

Quoted 4 lines

You need to set the second bit in ECX to enable the timer.

I guess if you don't, then you get normal MWAIT but then you don't need the timeout either...

Quoted 5 lines

Yeah.

So if it's a true timeout then we could use it to implement irq-less timers: that's actually pretty useful, because it could be faster than getting a local APIC timer irq, etc.

Uurgh, NO NO NO!

We have enough trouble with non functional timers already, we do not need another variant of those.

We can supply the estimated sleep time though if that helps the PM controller underneath to select a state. That's more or less what we do in the governors as well.

That's not what appears to be happening here though: the MWAITX will return after the timeout.

Which isn't really useful unless we use it to drive timers.

Quoted 4 lines

That's not what appears to be happening here though: the MWAITX will return after the timeout.

Which isn't really useful unless we use it to drive timers.

What about things like mdelay() ?

Quoted 6 lines · One Thousand Gnomes

Quoted 4 lines

That's not what appears to be happening here though: the MWAITX will return after the timeout.

Which isn't really useful unless we use it to drive timers.

What about things like mdelay() ?

It has an upper limit on the max timeout though: u32 TSC cycles.

Which would be good enough for mdelay/udelay I think, but we'd need to measure the time spend in MWAITT so we wont return early.

Something like this:

	  delay = usec_to_tsc(delay_usec);

	  end = rdtsc() + delay;
	  while (1) {

		MWAITT(delay);
		now = rdtsc();
		if (end <= now)
		   	  break;
		delay = end - now;
	}

Now we'd need to add alternatives or some other mechanism to it to make this conditionally for those machines.

Not sure if it's worth the trouble.

Quoted 4 lines · Thomas Gleixner

Which would be good enough for mdelay/udelay I think, but we'd need to measure the time spend in MWAITT so we wont return early.

Something like this:

Yeah, with a check maybe:

Quoted 1 line

	  delay = usec_to_tsc(delay_usec);

	if (delay > ((1 << 32) - 1)) {
		mdelay(delay_usec);
		return;
	}

Quoted 2 lines

	  end = rdtsc() + delay;
	  while (1) {

I guess

		monitorx( ...);

first.

Quoted 9 lines

		MWAITT(delay);
		now = rdtsc();
		if (end <= now)
		   	  break;
		delay = end - now;
	}

Now we'd need to add alternatives or some other mechanism to it to make this conditionally for those machines.

alternative_call(mdelay, mdelayx, X86_FEATURE_MWAITT, /* no output */, timeout);

Something like that maybe.

Quoted 1 line

Not sure if it's worth the trouble.

Could be a use case for MWAITX in the kernel!

Looks like good use case. Boris, could we try to implement it?

Quoted 1 line · Huang Rui

Looks like good use case. Boris, could we try to implement it?

Andy had some suggestions on how to do it here:

https://lkml.kernel.org/r/[email protected]

which should be doable. Also, you'd probably need to set ECX[0]=0b too, so that MWAITX doesn't get woken up by interrupts while MWAIT-ing with interrupts disabled. I.e., this sequence:

cli
rdtsc
shove the computed timeout into ebx
mov $2,%ecx # this enables the timer and disables IRQs while MWAITing
mwaitx
sti

The NMI argument is a problem though - if and NMI gets you out of MWAITX, a simple perf tool workload would kill all MWAITX executions. Which is bad. :-\

I must be missing something. In this sequence, you're sleeping with IF=0 and ECX[0] = 0, so an IRQ won't get handled. Don't we want ECX[0] = 1?

Quoted 3 lines

The NMI argument is a problem though - if and NMI gets you out of MWAITX, a simple perf tool workload would kill all MWAITX executions. Which is bad. :-\

I'm not sure it's a show-stopper. NMI handlers are meant to be fast. If an NMI comes in between rdtsc and mwaitx, then we oversleep, but by at most the time it takes to handle an NMI, and nothing would have stopped us from oversleeping that long if an NMI came in right after mwaitx returned.

Quoted 3 lines · Andy Lutomirski

I must be missing something. In this sequence, you're sleeping with IF=0 and ECX[0] = 0, so an IRQ won't get handled. Don't we want ECX[0] = 1?

Hmm, so actually we don't want to sleep with interrupts disabled. If ECX[0]=1b, then an interrupt will wake MWAIT. So then you have to do the loop thing as tglx suggested.

Quoted 9 lines

Quoted 3 lines

The NMI argument is a problem though - if and NMI gets you out of MWAITX, a simple perf tool workload would kill all MWAITX executions. Which is bad. :-\

I'm not sure it's a show-stopper. NMI handlers are meant to be fast. If an NMI comes in between rdtsc and mwaitx, then we oversleep, but by at most the time it takes to handle an NMI, and nothing would have stopped us from oversleeping that long if an NMI came in right after mwaitx returned.

Actually, I'm thinking about an NMI happening after we've issued MWAIT. NMIs wake it up. So you have the same problem as above:

NMIs will wake MWAIT so you'd need to check how long you've slept and sleep for the remaining time. I.e., something like that thing from a couple of mails ago:

        delay = usec_to_tsc(delay_usec);

        if (delay > ((1 << 32) - 1)) {
                mdelay(delay_usec);
                return;
        }

        end = rdtsc() + delay;
        while (1) {

                monitorx( ...); /* Do we need that here? */
                mwaitx(delay);

                /* possible wakeups */

                now = rdtsc();
                if (end <= now)
                        break;
                delay = end - now;
        }

Yes, no?

Quoted 1 line · Huang Rui

Looks like good use case. Boris, could we try to implement it?

Andy had some suggestions on how to do it here:

https://lkml.kernel.org/r/[email protected]

which should be doable. Also, you'd probably need to set ECX[0]=0b too, so that MWAITX doesn't get woken up by interrupts while MWAIT-ing with interrupts disabled. I.e., this sequence:

cli
rdtsc
shove the computed timeout into ebx
mov $2,%ecx # this enables the timer and disables IRQs while MWAITing
mwaitx
sti

Quoted 2 lines · Ingo Molnar

- MWAITX takes a 'timeout' parameter, but otherwise behaves exactly like MWAIT: i.e. once idle it won't exit idle on its own

Let me quote the commit message:

"MWAITT, another name is MWAITX (MWAIT with extensions), has a configurable timer that causes MWAITX to exit on expiration."

You need to set the second bit in ECX to enable the timer.

I guess if you don't, then you get normal MWAIT but then you don't need the timeout either...

Quoted 4 lines

- based on the 'timeout' hint, MWAITX can internally optimize how deep sleep it enters. If the timeout is large it goes deep, if it's small, it goes shallow.

I haven't heard anything about handling the timeout this way and if it is not done this way, maybe Rui could forward this idea to hw people...

Quoted 6 lines · Borislav Petkov

Quoted 2 lines · Ingo Molnar

Well, HLT does not get any hint from the OS how long the idling is expected to last.

MWAIT on AMD doesn't either:

Yeah, MWAIT clearly doesn't, but I was talking about MWAITX, which takes a timeout parameter as per these patches.

Quoted 6 lines

Quoted 3 lines

Another MWAITX round - we've got no crystal ball, so the hint might be wrong if an external event occurs that we did not anticipate.

So if we end up doing a bunch of MWAITX rounds instead of HLT and MWAITX saves less power than HLT, then we practically are worse.

So the way I think it would work ideally is (and note that this is different from how you think it works):

- MWAITX takes a 'timeout' parameter, but otherwise behaves exactly like MWAIT: i.e. once idle it won't exit idle on its own

- based on the 'timeout' hint, MWAITX can internally optimize how deep sleep it enters. If the timeout is large it goes deep, if it's small, it goes shallow. This does not change the fact that no matter which state it enters, it will come back the moment an interrupt is posted.

No, the timeout value doesn't decide how 'deep' the power state enters. Basically, the same power consumption with any timeout.

I summarized the comparison of mwait and mwaitx

MWAIT MWAITX
opcode 0f 01 c9 | 0f 01 fb
ECX[0] value of RFLAGS.IF seen by instruction
ECX[1] unused/#GP if set | enable timer if set
ECX[31:2] unused/#GP if set
EAX unused
EBX[31:0] unused | max wait time (loops)

MONITOR MONITORX
opcode 0f 01 c8 | 0f 01 fa
EAX (logical) address to monitor
ECX #GP if not zero

Quoted 2 lines · Ingo Molnar

Does it use it to decide how 'deep' a sleep it will go into, i.e. larger timeouts cause longer entry and exit latencies?

That's what the HLT thing does. Cores go into C1 and then at some point (hysteresis, etc) the whole core complex enters C1E.

The MWAIT* should be used for only shorter sleeps as it remains in C1. IMHO, of course.

But the problem there is another: what happens if the timeout fires, you wake up and see that you can remain idle? Do HLT? Do another MWAITX round?

This means you have an additional unnecessary wakeup which costs.

Quoted 3 lines

I suppose it's also the case that if an interrupt arrives _before_ the expected timeout then MWAITX will try to exit immediately, it won't wait until the timeout, right?

I'd assume so - I mean, it must, right.

BUT!, in talking to Andy about it last night on IRC, he pointed out that when using acpi_idle, we never come to calling x86_idle() and from looking quickly at cpuidle_idle_call(), that still might be the case as we go to use_default only when there's an error with the cpuidle driver or so.

So Rui, before you go and do more work on it, you should probably analyze what cpuidle exactly does (if you haven't done so yet). And on AMD we do use acpi_idle - at least on my F15h box that is the case:

$ grep . /sys/devices/system/cpu/cpuidle/current_*
/sys/devices/system/cpu/cpuidle/current_driver:acpi_idle
/sys/devices/system/cpu/cpuidle/current_governor_ro:menu

Quoted 9 lines · Huang Rui

MWAITX/MWAIT does not let the cpu core go into C1 state on AMD processors. The cpu core still consumes less power while waiting, and has faster exit from waiting than "Halt". This patch implements an interface using the kernel parameter "idle=" to configure mwaitx type and timer value.

If "idle=mwaitx", the timeout will be set as the maximum value ((2^64 - 1) * TSC cycle).
If "idle=mwaitx,100", the timeout will be set as 100ns. If the processor doesn't support MWAITX, then halt is used.

Ok, I see what you're trying here and I think this is not the optimal approach.

So let me explain how I see it, you correct me if I'm wrong:

So we want to do MWAITX so that we can save us idle entry/exit overhead with HLT. Because MWAITX is faster, reportedly.

Now, if we want to do that, we want to do it dynamically and adjust the MWAITX sleep interval depending on the system, usage pattern, system load and so on.

And for that we would need an adaptive scheme which approximates each idle interval. Simply taking TSC before we enter idle and after we come out would give us each idle residency duration and we can do some simple math to approximate it.

Now, what would that bring us: faster wakeup times.

And here comes the 10^6 $ question: why are we doing all the fun?

I'm thinking we want to find a cutoff duration where for smaller durations it is worth to do MWAITX and have faster entry/exit times and for bigger durations we want to do HLT because it'll get into C1E and give us higher power savings.

We don't want to do MWAITX too long because that'll burn more power relatively to HLT but we don't want to do HLT for shorter periods because then entry/exit costs.

Am I on the right track at least?

Quoted 15 lines

Signed-off-by Huang Rui <[email protected]>

---
 arch/x86/include/asm/mwait.h     |  2 +
 arch/x86/include/asm/processor.h |  2 +-
 arch/x86/kernel/process.c        | 79 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 1 deletion(-)

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

index b91136f..c4e51e7 100644
@@ -14,6 +14,8 @@
 #define CPUID5_ECX_INTERRUPT_BREAK	0x2
 

arch/x86/include/asm/mwait.h line 17

Quoted 2 lines

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

 #define MWAIT_ECX_INTERRUPT_BREAK	0x1
+#define MWAITX_ECX_TIMER_ENABLE		0x2

Use BIT(1) here.

Quoted 60 lines

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

+#define MWAITX_EBX_WAIT_TIMEOUT		0xffffffff
 
 static inline void __monitor(const void *eax, unsigned long ecx,
 			     unsigned long edx)

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

index 23ba676..0f60e94 100644
@@ -733,7 +733,7 @@ extern unsigned long		boot_option_idle_override;
 extern bool			amd_e400_c1e_detected;
 
 enum idle_boot_override {IDLE_NO_OVERRIDE=0, IDLE_HALT, IDLE_NOMWAIT,
-			 IDLE_POLL};
+			 IDLE_POLL, IDLE_MWAITX};
 
 extern void enable_sep_cpu(void);
 extern int sysenter_setup(void);

arch/x86/kernel/process.c +13 -0

index 6e338e3..9d68193 100644
@@ -30,6 +30,7 @@
 #include <asm/debugreg.h>
 #include <asm/nmi.h>
 #include <asm/tlbflush.h>
+#include <asm/x86_init.h>
 
 /*
  * per-CPU TSS segments. Thre ads are completely 'soft' on Linux,

@@ -276,6 +277,7 @@ unsigned long boot_option_idle_override = IDLE_NO_OVERRIDE;
 EXPORT_SYMBOL(boot_option_idle_override);
 
 static void (*x86_idle)(void);
+static unsigned long idle_param;
 
 #ifndef CONFIG_SMP
 static inline void play_dead(void)

@@ -444,6 +446,17 @@ static int prefer_mwait_c1_over_halt(const struct cpuinfo_x86 *c)
 	return 1;
 }
 
+static int not_support_mwaitx(const struct cpuinfo_x86 *c)
+{
+	if (c->x86_vendor != X86_VENDOR_AMD)
+		return 1;
+
+	if (!cpu_has(c, X86_FEATURE_MWAITT))
+		return 1;
+
+	return 0;
+}
+
 /*
  * MONITOR/MWAIT with no hints, used for default default C1 state.
  * This invokes MWAIT with interrutps enabled and no flags,

@@ -470,12 +483,45 @@ static void mwait_idle(void)
 	__current_clr_polling();
 }
 

arch/x86/kernel/process.c static void mwait_idle(void) · line 487

Quoted 2 lines

arch/x86/kernel/process.c +2 -0

+/*
+ * AMD Excavator processors support the new MONITORX/MWAITX instructions.

No need for that especially when newer than XV processors start supporting those too.

MWAITX/MWAIT does not let the cpu core go into C1 state on AMD processors. The cpu core still consumes less power while waiting, and has faster exit from waiting than "Halt". This patch implements an interface using the kernel parameter "idle=" to configure mwaitx type and timer value.

If "idle=mwaitx", the timeout will be set as the maximum value ((2^64 - 1) * TSC cycle).
If "idle=mwaitx,100", the timeout will be set as 100ns. If the processor doesn't support MWAITX, then halt is used.

If we did that *and* we had a non-crappy mwaitx, then we could apply an optimization: when going idle, we could turn off the TSC deadline timer and use mwaitx instead. This would about an interrupt if the event that wakes us is our timer.

Quoted 2 lines · Andy Lutomirski

If we did that *and* we had a non-crappy mwaitx, then we could apply an optimization: when going idle, we could turn off the TSC deadline timer and use mwaitx instead. This would about an interrupt if the event that wakes us is our timer.

Hey, Intel, want to document your secret "Timed MWAIT" feature? It causes a transition to C0 when the deadline expires (see 4.2.4 of the Desktop 4th Generation Intel Core Processor Family Datasheet Volume 1, order number 328897-001) and it even has an erratum (HSD63 / BDM32), but the instruction itself doesn't appear to be documented.

https://chromium-review.googlesource.com/#/c/205161/

Oddly, Coreboot seems to have mis-spelled that MSR. It's MSR_PKG_CST_CONFIG_CONTROL, and bit 31 isn't defined in the SDM (unsurprisingly).

John, Peter, Thomas: would it actually make sense to teach the core timer/clockevent code about perfect time sources like invariant TSC + TSC

deadline? AFAICT right now we're not doing anything particularly interesting with the TSC deadline timer.

This patch removes the redundant comment.

Signed-off-by Huang Rui <[email protected]>

---
 arch/x86/kernel/process.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/kernel/process.c +0 -0

index 9d68193..e3e12b6 100644

Hi,

This patch set introduces a new instruction support on AMD Carrizo (Family 15h, Model 60h-6fh). It adds mwaitx idle function with a configurable timer. The user can configure the idle method and timer value via the idle kernel parameter.

Some discussions of the background, please see:
http://marc.info/?l=linux-kernel&m=143202042530498&w=2
http://marc.info/?l=linux-kernel&m=143161327003541&w=2

They are rebased on tip/sched/core.

Quoted 7 lines · Huang Rui

Hi,

This patch set introduces a new instruction support on AMD Carrizo (Family 15h, Model 60h-6fh). It adds mwaitx idle function with a configurable timer. The user can configure the idle method and timer value via the idle kernel parameter.

Quoted 5 lines

Some discussions of the background, please see:
http://marc.info/?l=linux-kernel&m=143202042530498&w=2
http://marc.info/?l=linux-kernel&m=143161327003541&w=2

They are rebased on tip/sched/core.

Just a note for the future - please use tip/master to base your patches on.

When comparing 'model name' fields in /proc/cpuinfo it was noticed that a simple test comparing the model name fields was failing. After some quick investigation it was noticed that the model name fields were actually different -- processor 0's model name field had trailing white space removed, while the other processors did not.

Another way of seeing this behaviour is to convert spaces into underscores in the output of /proc/cpuinfo,

[thetango@prarit ~]# grep "^model name" /proc/cpuinfo | uniq -c | sed 's/\ /_/g'
______1_model_name :_AMD_Opteron(TM)_Processor_6272
_____63_model_name :_AMD_Opteron(TM)_Processor_6272_________________

which shows two different model name fields even though they should be the same.

This occurs because the kernel calls strim() on cpu 0's x86_model_id field to output a pretty message to the console in print_cpu_info(), and as a result truncates the whitespace at the end of the x86_model_id field.

The x86_model_id field should be the same for the same processors. This patch uses string functions to remove both leading and trailing whitespace in the x86_model_id field. As a result the print_cpu_info() output looks like

smpboot: CPU0: AMD Opteron(TM) Processor 6272 (fam: 15, model: 01, stepping: 02)

and the x86_model_id field is correct on all processors on AMD platforms

[thetango@prarit ~]# grep "^model name" /proc/cpuinfo | uniq -c | sed 's/\ /_/g'
_____64_model_name :_AMD_Opteron(TM)_Processor_6272

and the functionality is correct on an Intel box:

[thetango@prarit2]# grep "^model name" /proc/cpuinfo | uniq -c | sed 's/\ /_/g'
____144_model_name      :_Intel(R)_Xeon(R)_CPU_E7-8890_v3_@_2.50GHz

Signed-off-by Prarit Bhargava <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc [email protected]

Cc Andy Lutomirski <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Igor Mammedov <[email protected]>

Cc Fenghua Yu <[email protected]>

Cc Brian Gerst <[email protected]>

---
 arch/x86/kernel/cpu/common.c |   17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

When comparing 'model name' fields in /proc/cpuinfo it was noticed that a simple test comparing the model name fields was failing. After some quick investigation it was noticed that the model name fields were actually different -- processor 0's model name field had trailing white space removed, while the other processors did not.

Another way of seeing this behaviour is to convert spaces into underscores in the output of /proc/cpuinfo,

[thetango@prarit ~]# grep "^model name" /proc/cpuinfo | uniq -c | sed 's/\ /_/g'
______1_model_name :_AMD_Opteron(TM)_Processor_6272
_____63_model_name :_AMD_Opteron(TM)_Processor_6272_________________

which shows two different model name fields even though they should be the same.

This occurs because the kernel calls strim() on cpu 0's x86_model_id field to output a pretty message to the console in print_cpu_info(), and as a result truncates the whitespace at the end of the x86_model_id field.

The x86_model_id field should be the same for the same processors. This patch uses string functions to remove both leading and trailing whitespace in the x86_model_id field. As a result the print_cpu_info() output looks like

smpboot: CPU0: AMD Opteron(TM) Processor 6272 (fam: 15, model: 01, stepping: 02)

and the x86_model_id field is correct on all processors on AMD platforms

[thetango@prarit ~]# grep "^model name" /proc/cpuinfo | uniq -c | sed 's/\ /_/g'
_____64_model_name :_AMD_Opteron(TM)_Processor_6272

and the functionality is correct on an Intel box:

[thetango@prarit2]# grep "^model name" /proc/cpuinfo | uniq -c | sed 's/\ /_/g'
____144_model_name      :_Intel(R)_Xeon(R)_CPU_E7-8890_v3_@_2.50GHz

Signed-off-by Prarit Bhargava <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc [email protected]

Cc Andy Lutomirski <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Igor Mammedov <[email protected]>

Cc Fenghua Yu <[email protected]>

Cc Brian Gerst <[email protected]>

---
 arch/x86/kernel/cpu/common.c |   17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

arch/x86/kernel/cpu/common.c +2 -4

index a62cf04..9405c1e 100644
@@ -419,7 +419,6 @@ static const struct cpu_dev *cpu_devs[X86_VENDOR_NUM] = {};
 static void get_model_name(struct cpuinfo_x86 *c)
 {
        unsigned int *v;
-       char *p, *q;
 
        if (c->extended_cpuid_level < 0x80000004)
                return;

@@ -431,18 +430,10 @@ static void get_model_name(struct cpuinfo_x86 *c)
        c->x86_model_id[48] = 0;
 
        /*
-        * Intel chips right-justify this string for some dumb reason;
-        * undo that brain damage:
+        * Remove leading whitespace on Intel processors and trailing
+        * whitespace on AMD processors.
         */
-       p = q = &c->x86_model_id[0];

Using strlcpy in this manner could fail if it does larger than byte copies and they overlap.

Quoted 2 lines · Brian Gerst

Using strlcpy in this manner could fail if it does larger than byte copies and they overlap.

Why?

AFAICT, strlcpy() calls memcpy() and memcpy should handle overlapping buffers just fine.

Quoted 5 lines · Borislav Petkov

Quoted 2 lines · Brian Gerst

Using strlcpy in this manner could fail if it does larger than byte copies and they overlap.

Quoted 4 lines

Why?

AFAICT, strlcpy() calls memcpy() and memcpy should handle overlapping buffers just fine.

Are you thinking of memmove?

Quoted 6 lines · Andy Lutomirski

Quoted 5 lines · Borislav Petkov

Quoted 2 lines · Brian Gerst

Using strlcpy in this manner could fail if it does larger than byte copies and they overlap.

Quoted 6 lines

Quoted 4 lines

Why?

AFAICT, strlcpy() calls memcpy() and memcpy should handle overlapping buffers just fine.

Are you thinking of memmove?

I guess I'm trying to find out why don't we have a BIG FAT WARNING over memcpy saying not to use it with overlapping buffers and larger than byte sizes. Or maybe this is something everyone, except me, just knows and that's a "Doh, Boris, of course!".

Btw, can we still avoid using the temporary buffer and use strncpy() instead? AFAICT, that does byte copies, from looking at the asm.

Quoted 8 lines · Borislav Petkov

Quoted 7 lines · Andy Lutomirski

Quoted 6 lines · Borislav Petkov

Quoted 2 lines · Brian Gerst

Using strlcpy in this manner could fail if it does larger than byte copies and they overlap.

Why?

Quoted 9 lines · Andy Lutomirski

Quoted 8 lines · Borislav Petkov

Quoted 7 lines · Andy Lutomirski

Quoted 6 lines · Borislav Petkov

Quoted 2 lines · Brian Gerst

Using strlcpy in this manner could fail if it does larger than byte copies and they overlap.

Why?

I think this is traditionally handled by specifying that the strcpy strings may not overlap, so the suggested

(quoted diff) +1 -0

+	strlcpy(c->x86_model_id, strim(c->x86_model_id), 48);

isn't good code.

A temporary intermediate buffer is required.

It's not just chunk size; it's the direction. If the dest starts after the source but overlaps it and you copy forwards, then you can clobber the end of the source before you read it. memmove is

specifically intended to avoid this.

Would it be possible to just use memmove directly?

Quoted 3 lines · Andy Lutomirski

It's not just chunk size; it's the direction. If the dest starts after the source but overlaps it and you copy forwards, then you can clobber the end of the source before you read it. memmove is

Some things should simply be done solely in userspace :-)

Quoted 3 lines

specifically intended to avoid this.

Would it be possible to just use memmove directly?

Yeah.

Prarit, care to send a v2?

Thanks.

I guess I'm trying to find out why don't we have a BIG FAT WARNING over memcpy saying not to use it with overlapping buffers and larger than byte sizes. Or maybe this is something everyone, except me, just knows and that's a "Doh, Boris, of course!".

 arch/x86/kernel/cpu/common.c |   17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

arch/x86/kernel/cpu/common.c +2 -3

@@ -431,18 +430,10 @@ static void get_model_name(struct cpuinfo_x86 *c)
 	c->x86_model_id[48] = 0;
 
 	/*
-	 * Intel chips right-justify this string for some dumb reason;
-	 * undo that brain damage:
+	 * Remove leading whitespace on Intel processors and trailing
+	 * whitespace on AMD processors.
 	 */
-	p = q = &c->x86_model_id[0];

Quoted 2 lines · Prarit Bhargava

 arch/x86/kernel/cpu/common.c |   17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

So I saw this title:

[PATCH] x86, cpuinfo x86_model_id whitespace cleanup

... and in an early morning deconcentrated state was skipped the changelog and was looking for a whitespace coding style cleanup:

Quoted 10 lines

arch/x86/kernel/cpu/common.c +2 -3

@@ -431,18 +430,10 @@ static void get_model_name(struct cpuinfo_x86 *c)
 	c->x86_model_id[48] = 0;
 
 	/*
-	 * Intel chips right-justify this string for some dumb reason;
-	 * undo that brain damage:
+	 * Remove leading whitespace on Intel processors and trailing
+	 * whitespace on AMD processors.
 	 */
-	p = q = &c->x86_model_id[0];

arch/x86/kernel/cpu/common.c static void get_model_name(struct cpuinfo_x86 *c) · line 437

Quoted 10 lines

arch/x86/kernel/cpu/common.c +1 -8

-	while (*p == ' ')
-		p++;
-	if (p != q) {
-		while (*p)
-			*q++ = *p++;
-		while (q <= &c->x86_model_id[48])
-			*q++ = '\0';	/* Zero-pad the rest */
-	}
+	strlcpy(c->x86_model_id, strim(c->x86_model_id), 48);
 }

Which this clearly isnt!

Fortunately before complaining about that I read the changelog as well, and realized that the 'whitespace cleanup' is done to /proc/cpuinfo ABI output.

Could you please make the title less ambiguous, so that sleepy kernel developers get the right idea what the patch looks like, from the title alone? Git shortlogs will vastly improve as well.

Something like:

[PATCH] x86/cpu: Strip leading and trailing spaces from the /proc/cpuinfo CPU model field

... or so would work very well for me!

Thanks,

Ingo

I use "mce=1,10000000" in cmdline to change the monarch timeout, but it does not work.

The cause is that get_option() has parsed the ',' already, we need not to check the ',' again.

--
get_option(): read an int from an option string;
if available accept a subsequent comma as well.

Return values:
0 - no int in string
1 - int found, no subsequent comma
2 - int found including a subsequent comma
3 - hyphen found to denote a range

Cc <[email protected]> # 2.6.32+

Signed-off-by Xie XiuQi <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +0 -0

index 2a2bb91..46ca8e7 100644

Quoted 10 lines · Xie XiuQi

I use "mce=1,10000000" in cmdline to change the monarch timeout, but it does not work.

The cause is that get_option() has parsed the ',' already, we need not to check the ',' again.

--
get_option(): read an int from an option string;
if available accept a subsequent comma as well.

Quoted 7 lines

Return values:
0 - no int in string
1 - int found, no subsequent comma
2 - int found including a subsequent comma
3 - hyphen found to denote a range

Cc <[email protected]> # 2.6.32+

I don't think that's a serious enough a bug to justify the stable tag.

Quoted 2 lines

Signed-off-by Xie XiuQi <[email protected]>

---

Quoted 1 line

return 0;

Anyway, I fixed it up and applied it.

Thanks.

(quoted diff) +29 -0

+/**
+ * mtrr_type_lookup - look up memory type in MTRR
+ *
+ * Return Values:
+ * MTRR_TYPE_(type)  - The effective MTRR type for the region
+ * MTRR_TYPE_INVALID - MTRR is disabled
  */
 u8 mtrr_type_lookup(u64 start, u64 end)
 {
 
 	int repeat;
 	u64 partial_end;
 
+	if (!mtrr_state_set)
+		return MTRR_TYPE_INVALID;
+
+	if (!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED))
+		return MTRR_TYPE_INVALID;
+
+	/*
+	 * Look up the fixed ranges first, which take priority over
+	 * the variable ranges.
+	 */
+	if ((start < 0x100000) &&
+	    (mtrr_state.have_fixed) &&
+	    (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
+		return mtrr_type_lookup_fixed(start, end);
+
+	/*
+	 * Look up the variable ranges.  Look of multiple ranges matching
+	 * this address and pick type as per MTRR precedence.
+	 */
+	type = mtrr_type_lookup_variable(start, end, &partial_end, &repeat);
 
 	/*
 	 * Common path is with repeat = 0.
 	 * However, we can have cases where [start:end] spans across some
+	 * MTRR ranges and/or the default type.  Do repeated lookups for
+	 * that case here.
 	 */
 	while (repeat) {
 		prev_type = type;
 		start = partial_end;
+		type = mtrr_type_lookup_variable(start, end, &partial_end, &repeat);