CPUID-less CPU/sync_core fixes and improvements

Aside from being excessively slow, CPUID is problematic: Linux runs on a handful of CPUs that don't have CPUID. Use IRET-to-self instead. IRET-to-self works everywhere, so it makes testing easy.

For reference, On my laptop, IRET-to-self is ~110ns, CPUID(eax=1, ecx=0) is ~83ns on native and very very slow under KVM, and MOV-to-CR2 is ~42ns.

While we're at it: sync_core() serves a very specific purpose. Document it.

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

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/processor.h | 77 ++++++++++++++++++++++++++++------------
 1 file changed, 55 insertions(+), 22 deletions(-)

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

index 64fbc937d586..201a956e345f 100644
@@ -590,33 +590,66 @@ static __always_inline void cpu_relax(void)
 
 #define cpu_relax_lowlatency() cpu_relax()
 
-/* Stop speculative execution and prefetching of modified code. */
+/*
+ * This function forces the icache and prefetched instruction stream to
+ * catch up with reality in two very specific cases:

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

+ */
 static inline void sync_core(void)
 {
-	int tmp;
-

Aside from being excessively slow, CPUID is problematic: Linux runs on a handful of CPUs that don't have CPUID. Use IRET-to-self instead. IRET-to-self works everywhere, so it makes testing easy.

For reference, On my laptop, IRET-to-self is ~110ns, CPUID(eax=1, ecx=0) is ~83ns on native and very very slow under KVM, and MOV-to-CR2 is ~42ns.

While we're at it: sync_core() serves a very specific purpose. Document it.

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

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/processor.h | 77 ++++++++++++++++++++++++++++------------
 1 file changed, 55 insertions(+), 22 deletions(-)

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

index 64fbc937d586..201a956e345f 100644
@@ -590,33 +590,66 @@ static __always_inline void cpu_relax(void)
 
 #define cpu_relax_lowlatency() cpu_relax()
 
-/* Stop speculative execution and prefetching of modified code. */
+/*
+ * This function forces the icache and prefetched instruction stream to
+ * catch up with reality in two very specific cases:

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

+ */
 static inline void sync_core(void)
 {
-	int tmp;
-

Quoted 29 lines · Andy Lutomirski

Aside from being excessively slow, CPUID is problematic: Linux runs on a handful of CPUs that don't have CPUID. Use IRET-to-self instead. IRET-to-self works everywhere, so it makes testing easy.

For reference, On my laptop, IRET-to-self is ~110ns, CPUID(eax=1, ecx=0) is ~83ns on native and very very slow under KVM, and MOV-to-CR2 is ~42ns.

While we're at it: sync_core() serves a very specific purpose. Document it.

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

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/processor.h | 77 ++++++++++++++++++++++++++++------------
 1 file changed, 55 insertions(+), 22 deletions(-)

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

index 64fbc937d586..201a956e345f 100644
@@ -590,33 +590,66 @@ static __always_inline void cpu_relax(void)
 
 #define cpu_relax_lowlatency() cpu_relax()
 
-/* Stop speculative execution and prefetching of modified code. */
+/*
+ * This function forces the icache and prefetched instruction stream to
+ * catch up with reality in two very specific cases:

arch/x86/include/asm/processor.h static __always_inline void cpu_relax(void) · line 605

Quoted 10 lines

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

+ *
+ *  a) Text was modified using one virtual address and is about to be executed
+ *     from the same physical page at a different virtual address.
+ *
+ *  b) Text was modified on a different CPU, may subsequently be
+ *     executed on this CPU, and you want to make sure the new version
+ *     gets executed.  This generally means you're calling this in a IPI.
+ *
+ * If you're calling this for a different reason, you're probably doing
+ * it wrong.

"... and think hard before you call this - it is slow."

I'd add that now that it is even slower than CPUID.

Quoted 6 lines

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

+ */
 static inline void sync_core(void)
 {
-     int tmp;
-

arch/x86/include/asm/processor.h static __always_inline void cpu_relax(void)

Quoted 10 lines

arch/x86/include/asm/processor.h +6 -3

-#ifdef CONFIG_X86_32
      /*
-      * Do a CPUID if available, otherwise do a jump.  The jump
-      * can conveniently enough be the jump around CPUID.
+      * There are quite a few ways to do this.  IRET-to-self is nice
+      * because it works on every CPU, at any CPL (so it's compatible
+      * with paravirtualization), and it never exits to a hypervisor.
+      * The only down sides are that it's a bit slow (it seems to be
+      * a bit more than 2x slower than the fastest options) and that
+      * it unmasks NMIs.

Ewww, I hadn't thought of that angle. Aren't we going to get in all kinds of hard to debug issues due to that couple of cycles window of unmasked NMIs?

We sync_core in some NMI handler and then right in the middle of it we get another NMI. Yeah, we have the nested NMI stuff still but I'd like to avoid complications if possible.

arch/x86/include/asm/processor.h static __always_inline void cpu_relax(void) · line 622

Quoted 9 lines

The "push %cs" is needed because, in

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

+      * paravirtual environments, __KERNEL_CS may not be a valid CS
+      * value when we do IRET directly.
+      *
+      * In case NMI unmasking or performance every becomes a problem,
+      * the next best option appears to be MOV-to-CR2 and an
+      * unconditional jump.  That sequence also works on all CPUs,
+      * but it will fault at CPL3.

Does it really have to be non-priviledged?

If not, there are a couple more serializing insns:

"• Privileged serializing instructions — INVD, INVEPT, INVLPG, INVVPID, LGDT, LIDT, LLDT, LTR, MOV (to control register, with the exception of MOV CR83), MOV (to debug register), WBINVD, and WRMSR"

What about INVD? It is expensive too :-)

Can't we use, MOV %dr or so? With previously saving/restoring the reg?

WBINVD could be another candidate, albeit a big hammer.

WRMSR maybe too. If it faults, it's fine too because then you have the IRET automagically. Hell, we could even make it fault on purpose by writing into an invalid MSR but then we're back to the unmasking NMIs... :-\

Quoted 29 lines · Andy Lutomirski

Aside from being excessively slow, CPUID is problematic: Linux runs on a handful of CPUs that don't have CPUID. Use IRET-to-self instead. IRET-to-self works everywhere, so it makes testing easy.

For reference, On my laptop, IRET-to-self is ~110ns, CPUID(eax=1, ecx=0) is ~83ns on native and very very slow under KVM, and MOV-to-CR2 is ~42ns.

While we're at it: sync_core() serves a very specific purpose. Document it.

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

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/processor.h | 77 ++++++++++++++++++++++++++++------------
 1 file changed, 55 insertions(+), 22 deletions(-)

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

index 64fbc937d586..201a956e345f 100644
@@ -590,33 +590,66 @@ static __always_inline void cpu_relax(void)
 
 #define cpu_relax_lowlatency() cpu_relax()
 
-/* Stop speculative execution and prefetching of modified code. */
+/*
+ * This function forces the icache and prefetched instruction stream to
+ * catch up with reality in two very specific cases:

arch/x86/include/asm/processor.h static __always_inline void cpu_relax(void) · line 605

Quoted 10 lines

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

+ *
+ *  a) Text was modified using one virtual address and is about to be executed
+ *     from the same physical page at a different virtual address.
+ *
+ *  b) Text was modified on a different CPU, may subsequently be
+ *     executed on this CPU, and you want to make sure the new version
+ *     gets executed.  This generally means you're calling this in a IPI.
+ *
+ * If you're calling this for a different reason, you're probably doing
+ * it wrong.

"... and think hard before you call this - it is slow."

I'd add that now that it is even slower than CPUID.

Quoted 6 lines

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

+ */
 static inline void sync_core(void)
 {
-     int tmp;
-

arch/x86/include/asm/processor.h static __always_inline void cpu_relax(void)

Quoted 10 lines

arch/x86/include/asm/processor.h +6 -3

-#ifdef CONFIG_X86_32
      /*
-      * Do a CPUID if available, otherwise do a jump.  The jump
-      * can conveniently enough be the jump around CPUID.
+      * There are quite a few ways to do this.  IRET-to-self is nice
+      * because it works on every CPU, at any CPL (so it's compatible
+      * with paravirtualization), and it never exits to a hypervisor.
+      * The only down sides are that it's a bit slow (it seems to be
+      * a bit more than 2x slower than the fastest options) and that
+      * it unmasks NMIs.

Ewww, I hadn't thought of that angle. Aren't we going to get in all kinds of hard to debug issues due to that couple of cycles window of unmasked NMIs?

We sync_core in some NMI handler and then right in the middle of it we get another NMI. Yeah, we have the nested NMI stuff still but I'd like to avoid complications if possible.

arch/x86/include/asm/processor.h static __always_inline void cpu_relax(void) · line 622

Quoted 9 lines

The "push %cs" is needed because, in

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

+      * paravirtual environments, __KERNEL_CS may not be a valid CS
+      * value when we do IRET directly.
+      *
+      * In case NMI unmasking or performance every becomes a problem,
+      * the next best option appears to be MOV-to-CR2 and an
+      * unconditional jump.  That sequence also works on all CPUs,
+      * but it will fault at CPL3.

Does it really have to be non-priviledged?

If not, there are a couple more serializing insns:

"• Privileged serializing instructions — INVD, INVEPT, INVLPG, INVVPID, LGDT, LIDT, LLDT, LTR, MOV (to control register, with the exception of MOV CR83), MOV (to debug register), WBINVD, and WRMSR"

What about INVD? It is expensive too :-)

Can't we use, MOV %dr or so? With previously saving/restoring the reg?

WBINVD could be another candidate, albeit a big hammer.

WRMSR maybe too. If it faults, it's fine too because then you have the IRET automagically. Hell, we could even make it fault on purpose by writing into an invalid MSR but then we're back to the unmasking NMIs... :-\

Quoted 1 line

Quoted 1 line · On 05.12.16 at 22:32, <[email protected]> wrote:

static inline void sync_core(void)
{

arch/x86/include/asm/processor.h +3 -5

-	int tmp;
-
-#ifdef CONFIG_X86_32
 	/*
-	 * Do a CPUID if available, otherwise do a jump.  The jump
-	 * can conveniently enough be the jump around CPUID.
+	 * There are quite a few ways to do this.  IRET-to-self is nice
+	 * because it works on every CPU, at any CPL (so it's compatible
+	 * with paravirtualization), and it never exits to a hypervisor.

arch/x86/include/asm/processor.h +3 -7

+	 * CPUID is the conventional way, but it's nasty: it doesn't
+	 * exist on some 486-like CPUs, and it usually exits to a
+	 * hypervisor.
 	 */
-	asm volatile("cmpl %2,%1\n\t"
-		     "jl 1f\n\t"
-		     "cpuid\n"
-		     "1:"
-		     : "=a" (tmp)
-		     : "rm" (boot_cpu_data.cpuid_level), "ri" (0), "0" (1)
-		     : "ebx", "ecx", "edx", "memory");

#else

arch/x86/include/asm/processor.h +4 -9

-	/*
-	 * CPUID is a barrier to speculative execution.
-	 * Prefetched instructions are automatically
-	 * invalidated when modified.
-	 */
-	asm volatile("cpuid"
-		     : "=a" (tmp)
-		     : "0" (1)
-		     : "ebx", "ecx", "edx", "memory");
+	unsigned long tmp;
+
+	asm volatile (
+		"movq %%ss, %0\n\t"

Quoted 1 line

Quoted 1 line · On 05.12.16 at 22:32, <[email protected]> wrote:

static inline void sync_core(void)
{

arch/x86/include/asm/processor.h +3 -5

-	int tmp;
-
-#ifdef CONFIG_X86_32
 	/*
-	 * Do a CPUID if available, otherwise do a jump.  The jump
-	 * can conveniently enough be the jump around CPUID.
+	 * There are quite a few ways to do this.  IRET-to-self is nice
+	 * because it works on every CPU, at any CPL (so it's compatible
+	 * with paravirtualization), and it never exits to a hypervisor.

arch/x86/include/asm/processor.h +3 -7

+	 * CPUID is the conventional way, but it's nasty: it doesn't
+	 * exist on some 486-like CPUs, and it usually exits to a
+	 * hypervisor.
 	 */
-	asm volatile("cmpl %2,%1\n\t"
-		     "jl 1f\n\t"
-		     "cpuid\n"
-		     "1:"
-		     : "=a" (tmp)
-		     : "rm" (boot_cpu_data.cpuid_level), "ri" (0), "0" (1)
-		     : "ebx", "ecx", "edx", "memory");

#else

arch/x86/include/asm/processor.h +4 -9

-	/*
-	 * CPUID is a barrier to speculative execution.
-	 * Prefetched instructions are automatically
-	 * invalidated when modified.
-	 */
-	asm volatile("cpuid"
-		     : "=a" (tmp)
-		     : "0" (1)
-		     : "ebx", "ecx", "edx", "memory");
+	unsigned long tmp;
+
+	asm volatile (
+		"movq %%ss, %0\n\t"

Quoted 4 lines

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

+	asm volatile (
+		"pushfl\n\t"
+		"pushl %%cs\n\t"
+		"pushl $1f\n\t"

arch/x86/include/asm/processor.h static __always_inline void cpu_relax(void) · line 637

Quoted 3 lines

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

+		"iret\n\t"
+		"1:"
+		: "+r" (__sp) : : "cc", "memory");

I don't thing EFLAGS (i.e. "cc") gets modified anywhere here. And the memory clobber would perhaps better be pulled out into an explicit barrier() invocation (making it more obvious what it's needed for)?

Quoted 1 line

Quoted 1 line · On 06.12.16 at 10:25, <[email protected]> wrote:

Quoted 4 lines · Jan Beulich

Quoted 4 lines

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

+	asm volatile (
+		"pushfl\n\t"
+		"pushl %%cs\n\t"
+		"pushl $1f\n\t"

Quoted 8 lines

arch/x86/include/asm/processor.h static __always_inline void cpu_relax(void) · line 637

Quoted 3 lines

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

+		"iret\n\t"
+		"1:"
+		: "+r" (__sp) : : "cc", "memory");

I don't thing EFLAGS (i.e. "cc") gets modified anywhere here. And the memory clobber would perhaps better be pulled out into an explicit barrier() invocation (making it more obvious what it's needed for)?

EVerything that implies a memory barrier (and I think serializing instructions do that) also imply a compiler barrier.

Not doing the memory clobber gets you inconsistency wrt everything else.

Quoted 15 lines

Quoted 1 line

Quoted 1 line · On 06.12.16 at 10:25, <[email protected]> wrote:

Quoted 12 lines · Jan Beulich

arch/x86/include/asm/processor.h static __always_inline void cpu_relax(void) · line 637

Quoted 7 lines

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

+  asm volatile (
+          "pushfl\n\t"
+          "pushl %%cs\n\t"
+          "pushl $1f\n\t"
+          "iret\n\t"
+          "1:"
+          : "+r" (__sp) : : "cc", "memory");

I don't thing EFLAGS (i.e. "cc") gets modified anywhere here. And the memory clobber would perhaps better be pulled out into an explicit barrier() invocation (making it more obvious what it's needed for)?

Quoted 4 lines

EVerything that implies a memory barrier (and I think serializing instructions do that) also imply a compiler barrier.

Not doing the memory clobber gets you inconsistency wrt everything else.

Well, I didn't say dropping the memory clobber altogether, but split it into a separate barrier() invocation (placed perhaps after the #endif).

Quoted 15 lines

Quoted 1 line

Quoted 1 line · On 06.12.16 at 10:25, <[email protected]> wrote:

Quoted 12 lines · Jan Beulich

arch/x86/include/asm/processor.h static __always_inline void cpu_relax(void) · line 637

Quoted 7 lines

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

+  asm volatile (
+          "pushfl\n\t"
+          "pushl %%cs\n\t"
+          "pushl $1f\n\t"
+          "iret\n\t"
+          "1:"
+          : "+r" (__sp) : : "cc", "memory");

I don't thing EFLAGS (i.e. "cc") gets modified anywhere here. And the memory clobber would perhaps better be pulled out into an explicit barrier() invocation (making it more obvious what it's needed for)?

Quoted 4 lines

EVerything that implies a memory barrier (and I think serializing instructions do that) also imply a compiler barrier.

Not doing the memory clobber gets you inconsistency wrt everything else.

Well, I didn't say dropping the memory clobber altogether, but split it into a separate barrier() invocation (placed perhaps after the #endif).

Quoted 6 lines

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

+
+#ifdef CONFIG_X86_32
+	asm volatile (
+		"pushfl\n\t"
+		"pushl %%cs\n\t"
+		"pushl $1f\n\t"

arch/x86/include/asm/processor.h static __always_inline void cpu_relax(void) · line 637

Quoted 3 lines

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

+		"iret\n\t"
+		"1:"
+		: "+r" (__sp) : : "cc", "memory");

I don't thing EFLAGS (i.e. "cc") gets modified anywhere here. And the memory clobber would perhaps better be pulled out into an explicit barrier() invocation (making it more obvious what it's needed for)?