CPUID-less CPU fixes and improvements

This reverts commit ed68d7e9b9cfb64f3045ffbcb108df03c09a0f98.

The patch wasn't quite correct -- there are non-Intel (and hence non-486) CPUs that we support that don't have CPUID. Since we no longer require CPUID for sync_core(), just revert the patch.

Quoted 5 lines · Andy Lutomirski

This reverts commit ed68d7e9b9cfb64f3045ffbcb108df03c09a0f98.

The patch wasn't quite correct -- there are non-Intel (and hence non-486) CPUs that we support that don't have CPUID. Since we no longer require CPUID for sync_core(), just revert the patch.

Might be useful to enumerate which special parts these are.

The Intel microcode driver is using sync_core() to mean "do CPUID with EAX=1". I want to rework sync_core(), but first the Intel microcode driver needs to stop depending on its current behavior.

Reported-by Henrique de Moraes Holschuh <[email protected]>

Cc Borislav Petkov <[email protected]>

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

---
 arch/x86/kernel/cpu/microcode/intel.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Aside from being excessively slow, CPUID is problematic: Linux runs on a handful of CPUs that don't have CPUID. MOV to CR2 is always available, so use it instead.

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

---
 arch/x86/include/asm/processor.h | 31 ++++++++-----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines · Andy Lutomirski

This reverts commit ed68d7e9b9cfb64f3045ffbcb108df03c09a0f98.

The patch wasn't quite correct -- there are non-Intel (and hence non-486) CPUs that we support that don't have CPUID. Since we no longer require CPUID for sync_core(), just revert the patch.

Quoted 1 line

Might be useful to enumerate which special parts these are.

Right, and since we test for CPUID support at early boot, I think we should use the X86_FEATURE_CPUID aspect from what I proposed earlier:

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

Yes, that makes a lot of sense.

Quoted 1 line · Thomas Gleixner

Yes, that makes a lot of sense.

Here it is:

---
From: Borislav Petkov <[email protected]>
Date: Thu, 1 Dec 2016 12:11:37 +0100
Subject: [PATCH] x86/CPU: Add X86_FEATURE_CPUID

Add a synthetic CPUID flag denoting whether the CPU sports the CPUID instruction or not. This will come useful later when accomodating CPUID-less CPUs.

Quoted 5 lines · Andy Lutomirski

This reverts commit ed68d7e9b9cfb64f3045ffbcb108df03c09a0f98.

The patch wasn't quite correct -- there are non-Intel (and hence non-486) CPUs that we support that don't have CPUID. Since we no longer require CPUID for sync_core(), just revert the patch.

Might be useful to enumerate which special parts these are.

Quoted 8 lines · Andy Lutomirski

Aside from being excessively slow, CPUID is problematic: Linux runs on a handful of CPUs that don't have CPUID. MOV to CR2 is always available, so use it instead.

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

---
 arch/x86/include/asm/processor.h | 31 ++++++++-----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

Looks nice.

I'm wondering if we should leave this one in tip for an additional cycle to have it tested on more hw. I know, it is architectural and so on but who knows what every implementation actually does...

Quoted 11 lines · Borislav Petkov

Quoted 8 lines · Andy Lutomirski

Aside from being excessively slow, CPUID is problematic: Linux runs on a handful of CPUs that don't have CPUID. MOV to CR2 is always available, so use it instead.

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

---
 arch/x86/include/asm/processor.h | 31 ++++++++-----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

Looks nice.

Quoted 3 lines

I'm wondering if we should leave this one in tip for an additional cycle to have it tested on more hw. I know, it is architectural and so on but who knows what every implementation actually does...

I want the Xen opinion as well.

Xen folks, can Linux use write_cr2 to serialize the CPU core on Xen PV or do we need something a bit heavier weight like native_write_cr2?

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines · Andy Lutomirski

This reverts commit ed68d7e9b9cfb64f3045ffbcb108df03c09a0f98.

The patch wasn't quite correct -- there are non-Intel (and hence non-486) CPUs that we support that don't have CPUID. Since we no longer require CPUID for sync_core(), just revert the patch.

Quoted 1 line

Might be useful to enumerate which special parts these are.

Right, and since we test for CPUID support at early boot, I think we should use the X86_FEATURE_CPUID aspect from what I proposed earlier:

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

Quoted 8 lines · Andy Lutomirski

Aside from being excessively slow, CPUID is problematic: Linux runs on a handful of CPUs that don't have CPUID. MOV to CR2 is always available, so use it instead.

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

---
 arch/x86/include/asm/processor.h | 31 ++++++++-----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

Looks nice.

I'm wondering if we should leave this one in tip for an additional cycle to have it tested on more hw. I know, it is architectural and so on but who knows what every implementation actually does...

Ugh, please ignore the accidental re-send of v1. I suck at using git. v2 is what I meant to send, but I accidentally sent both versions. :-(

On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't guaranteed to serialize. (Even CPUID isn't *really* guaranteed to serialize on Xen PV, but, in practice, any trap it generates will serialize.)

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is ~110ns. But Xen PV will trap CPUID if possible, so IRET-to-self should end up being a nice speedup.

Cc Andrew Cooper <[email protected]>

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

---
 arch/x86/xen/enlighten.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

arch/x86/xen/enlighten.c +33 -0

index bdd855685403..1f765b41eee7 100644
@@ -311,6 +311,39 @@ static __read_mostly unsigned int cpuid_leaf1_ecx_set_mask;
 static __read_mostly unsigned int cpuid_leaf5_ecx_val;
 static __read_mostly unsigned int cpuid_leaf5_edx_val;
 
+static void xen_sync_core(void)
+{
+	register void *__sp asm(_ASM_SP);
+
+#ifdef CONFIG_X86_32
+	asm volatile (
+		"pushl %%ss\n\t"
+		"pushl %%esp\n\t"
+		"addl $4, (%%esp)\n\t"
+		"pushfl\n\t"
+		"pushl %%cs\n\t"
+		"pushl $1f\n\t"
+		"iret\n\t"
+		"1:"
+		: "+r" (__sp) : : "cc");
+#else
+	unsigned long tmp;
+
+	asm volatile (
+		"movq %%ss, %0\n\t"
+		"pushq %0\n\t"
+		"pushq %%rsp\n\t"
+		"addq $8, (%%rsp)\n\t"
+		"pushfq\n\t"
+		"movq %%cs, %0\n\t"
+		"pushq %0\n\t"
+		"pushq $1f\n\t"
+		"iretq\n\t"
+		"1:"
+		: "=r" (tmp), "+r" (__sp) : : "cc");
+#endif
+}
+
 static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 		      unsigned int *cx, unsigned int *dx)
 {

On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't guaranteed to serialize. (Even CPUID isn't *really* guaranteed to serialize on Xen PV, but, in practice, any trap it generates will serialize.)

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is ~110ns. But Xen PV will trap CPUID if possible, so IRET-to-self should end up being a nice speedup.

Cc Andrew Cooper <[email protected]>

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

---
 arch/x86/xen/enlighten.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

arch/x86/xen/enlighten.c +35 -0

index bdd855685403..1f765b41eee7 100644
@@ -311,6 +311,39 @@ static __read_mostly unsigned int cpuid_leaf1_ecx_set_mask;
 static __read_mostly unsigned int cpuid_leaf5_ecx_val;
 static __read_mostly unsigned int cpuid_leaf5_edx_val;
 
+static void xen_sync_core(void)
+{
+	register void *__sp asm(_ASM_SP);
+
+#ifdef CONFIG_X86_32
+	asm volatile (
+		"pushl %%ss\n\t"
+		"pushl %%esp\n\t"
+		"addl $4, (%%esp)\n\t"
+		"pushfl\n\t"
+		"pushl %%cs\n\t"
+		"pushl $1f\n\t"
+		"iret\n\t"
+		"1:"
+		: "+r" (__sp) : : "cc");
+#else
+	unsigned long tmp;
+
+	asm volatile (
+		"movq %%ss, %0\n\t"
+		"pushq %0\n\t"
+		"pushq %%rsp\n\t"
+		"addq $8, (%%rsp)\n\t"
+		"pushfq\n\t"
+		"movq %%cs, %0\n\t"
+		"pushq %0\n\t"
+		"pushq $1f\n\t"
+		"iretq\n\t"
+		"1:"
+		: "=r" (tmp), "+r" (__sp) : : "cc");
+#endif
+}
+
 static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 		      unsigned int *cx, unsigned int *dx)
 {

@@ -1289,6 +1322,8 @@ static const struct pv_cpu_ops xen_cpu_ops __initconst = {
 
 	.start_context_switch = paravirt_start_context_switch,
 	.end_context_switch = xen_end_context_switch,
+
+	.sync_core = xen_sync_core,
 };
 
 static void xen_reboot(int reason)

Quoted 4 lines · Andy Lutomirski

On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't guaranteed to serialize. (Even CPUID isn't *really* guaranteed to serialize on Xen PV, but, in practice, any trap it generates will serialize.)

Well, Xen will enabled CPUID Faulting wherever it can, which is realistically all IvyBridge hardware and newer.

All hypercalls are a privilege change to cpl0. I'd hope this condition is serialising, but I can't actually find any documentation proving or disproving this.

Quoted 8 lines

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is ~110ns. But Xen PV will trap CPUID if possible, so IRET-to-self should end up being a nice speedup.

Cc Andrew Cooper <[email protected]>

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

CC'ing xen-devel and the Xen maintainers in Linux.

As this is the only email from this series in my inbox, I will say this here, but it should really be against patch 6.

A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not serialising on the 486, but I don't have a manual to hand to check.

Quoted 4 lines · Andy Lutomirski

On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't guaranteed to serialize. (Even CPUID isn't *really* guaranteed to serialize on Xen PV, but, in practice, any trap it generates will serialize.)

Well, Xen will enabled CPUID Faulting wherever it can, which is realistically all IvyBridge hardware and newer.

All hypercalls are a privilege change to cpl0. I'd hope this condition is serialising, but I can't actually find any documentation proving or disproving this.

Quoted 8 lines

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is ~110ns. But Xen PV will trap CPUID if possible, so IRET-to-self should end up being a nice speedup.

Cc Andrew Cooper <[email protected]>

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

CC'ing xen-devel and the Xen maintainers in Linux.

As this is the only email from this series in my inbox, I will say this here, but it should really be against patch 6.

A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not serialising on the 486, but I don't have a manual to hand to check.

Quoted 5 lines · Andrew Cooper

Quoted 4 lines · Andy Lutomirski

On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't guaranteed to serialize. (Even CPUID isn't *really* guaranteed to serialize on Xen PV, but, in practice, any trap it generates will serialize.)

Quoted 6 lines

Well, Xen will enabled CPUID Faulting wherever it can, which is realistically all IvyBridge hardware and newer.

All hypercalls are a privilege change to cpl0. I'd hope this condition is serialising, but I can't actually find any documentation proving or disproving this.

I don't know for sure. IRET is serializing, and if Xen returns using IRET, we're fine.

Quoted 7 lines

Quoted 7 lines

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is ~110ns. But Xen PV will trap CPUID if possible, so IRET-to-self should end up being a nice speedup.

Cc Andrew Cooper <[email protected]>

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

Quoted 7 lines

CC'ing xen-devel and the Xen maintainers in Linux.

As this is the only email from this series in my inbox, I will say this here, but it should really be against patch 6.

A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not serialising on the 486, but I don't have a manual to hand to check.

I'll quote the (modern) SDM. For self-modifying code "The use of one of these options is not required for programs intended to run on the Pentium or Intel486 processors,
but are recommended to ensure compatibility with the P6 and more recent processor families.". For cross-modifying code "The use of this option is not required for programs intended to run on the Intel486 processor, but is recommended
to ensure compatibility with the Pentium 4, Intel Xeon, P6 family, and Pentium processors." So I'm not sure there's a problem.

Quoted 5 lines · Andrew Cooper

Quoted 4 lines · Andy Lutomirski

On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't guaranteed to serialize. (Even CPUID isn't *really* guaranteed to serialize on Xen PV, but, in practice, any trap it generates will serialize.)

Quoted 6 lines

Well, Xen will enabled CPUID Faulting wherever it can, which is realistically all IvyBridge hardware and newer.

All hypercalls are a privilege change to cpl0. I'd hope this condition is serialising, but I can't actually find any documentation proving or disproving this.

I don't know for sure. IRET is serializing, and if Xen returns using IRET, we're fine.

Quoted 7 lines

Quoted 7 lines

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is ~110ns. But Xen PV will trap CPUID if possible, so IRET-to-self should end up being a nice speedup.

Cc Andrew Cooper <[email protected]>

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

Quoted 7 lines

CC'ing xen-devel and the Xen maintainers in Linux.

As this is the only email from this series in my inbox, I will say this here, but it should really be against patch 6.

A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not serialising on the 486, but I don't have a manual to hand to check.

I'll quote the (modern) SDM. For self-modifying code "The use of one of these options is not required for programs intended to run on the Pentium or Intel486 processors,
but are recommended to ensure compatibility with the P6 and more recent processor families.". For cross-modifying code "The use of this option is not required for programs intended to run on the Intel486 processor, but is recommended
to ensure compatibility with the Pentium 4, Intel Xeon, P6 family, and Pentium processors." So I'm not sure there's a problem.

Quoted 9 lines · Andy Lutomirski

Quoted 8 lines · Andrew Cooper

Quoted 4 lines · Andy Lutomirski

On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't guaranteed to serialize. (Even CPUID isn't *really* guaranteed to serialize on Xen PV, but, in practice, any trap it generates will serialize.)

Well, Xen will enabled CPUID Faulting wherever it can, which is realistically all IvyBridge hardware and newer.

Quoted 5 lines

Quoted 3 lines

All hypercalls are a privilege change to cpl0. I'd hope this condition is serialising, but I can't actually find any documentation proving or disproving this.

I don't know for sure. IRET is serializing, and if Xen returns using IRET, we're fine.

All returns to a 64bit PV guest at defined points (hypercall return, exception entry, etc) are from SYSRET, not IRET.

Talking of, I still have a patch to remove PARAVIRT_ADJUST_EXCEPTION_FRAME which I need to complete and send upstream.

Quoted 14 lines

Quoted 7 lines

Quoted 7 lines

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is ~110ns. But Xen PV will trap CPUID if possible, so IRET-to-self should end up being a nice speedup.

Cc Andrew Cooper <[email protected]>

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

Quoted 7 lines

CC'ing xen-devel and the Xen maintainers in Linux.

As this is the only email from this series in my inbox, I will say this here, but it should really be against patch 6.

A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not serialising on the 486, but I don't have a manual to hand to check.

Quoted 9 lines

I'll quote the (modern) SDM. For self-modifying code "The use of one of these options is not required for programs intended to run on the Pentium or Intel486 processors,
but are recommended to ensure compatibility with the P6 and more recent processor families.". For cross-modifying code "The use of this option is not required for programs intended to run on the Intel486 processor, but is recommended
to ensure compatibility with the Pentium 4, Intel Xeon, P6 family, and Pentium processors." So I'm not sure there's a problem.

Fair enough. (Assuming similar properties hold for the older processors of other vendors.)

Quoted 9 lines · Andy Lutomirski

Quoted 8 lines · Andrew Cooper

Quoted 4 lines · Andy Lutomirski

On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't guaranteed to serialize. (Even CPUID isn't *really* guaranteed to serialize on Xen PV, but, in practice, any trap it generates will serialize.)

Well, Xen will enabled CPUID Faulting wherever it can, which is realistically all IvyBridge hardware and newer.

Quoted 5 lines

Quoted 3 lines

All hypercalls are a privilege change to cpl0. I'd hope this condition is serialising, but I can't actually find any documentation proving or disproving this.

I don't know for sure. IRET is serializing, and if Xen returns using IRET, we're fine.

All returns to a 64bit PV guest at defined points (hypercall return, exception entry, etc) are from SYSRET, not IRET.

Talking of, I still have a patch to remove PARAVIRT_ADJUST_EXCEPTION_FRAME which I need to complete and send upstream.

Quoted 14 lines

Quoted 7 lines

Quoted 7 lines

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is ~110ns. But Xen PV will trap CPUID if possible, so IRET-to-self should end up being a nice speedup.

Cc Andrew Cooper <[email protected]>

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

Quoted 7 lines

CC'ing xen-devel and the Xen maintainers in Linux.

As this is the only email from this series in my inbox, I will say this here, but it should really be against patch 6.

A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not serialising on the 486, but I don't have a manual to hand to check.

Quoted 9 lines

I'll quote the (modern) SDM. For self-modifying code "The use of one of these options is not required for programs intended to run on the Pentium or Intel486 processors,
but are recommended to ensure compatibility with the P6 and more recent processor families.". For cross-modifying code "The use of this option is not required for programs intended to run on the Intel486 processor, but is recommended
to ensure compatibility with the Pentium 4, Intel Xeon, P6 family, and Pentium processors." So I'm not sure there's a problem.

Fair enough. (Assuming similar properties hold for the older processors of other vendors.)

Quoted 13 lines · Andrew Cooper

Quoted 12 lines · Andy Lutomirski

Quoted 11 lines · Andrew Cooper

Quoted 4 lines · Andy Lutomirski

On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't guaranteed to serialize. (Even CPUID isn't *really* guaranteed to serialize on Xen PV, but, in practice, any trap it generates will serialize.)

Well, Xen will enabled CPUID Faulting wherever it can, which is realistically all IvyBridge hardware and newer.

All hypercalls are a privilege change to cpl0. I'd hope this condition is serialising, but I can't actually find any documentation proving or disproving this.

Quoted 4 lines

Quoted 2 lines

I don't know for sure. IRET is serializing, and if Xen returns using IRET, we're fine.

All returns to a 64bit PV guest at defined points (hypercall return, exception entry, etc) are from SYSRET, not IRET.

But CPUID faulting isn't like this, right? Unless Xen does opportunistic SYSRET.

Quoted 26 lines

Talking of, I still have a patch to remove PARAVIRT_ADJUST_EXCEPTION_FRAME which I need to complete and send upstream.

Quoted 13 lines

Quoted 6 lines

Quoted 6 lines

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is ~110ns. But Xen PV will trap CPUID if possible, so IRET-to-self should end up being a nice speedup.

Cc Andrew Cooper <[email protected]>

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

Quoted 7 lines

CC'ing xen-devel and the Xen maintainers in Linux.

As this is the only email from this series in my inbox, I will say this here, but it should really be against patch 6.

A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not serialising on the 486, but I don't have a manual to hand to check.

Quoted 9 lines

I'll quote the (modern) SDM. For self-modifying code "The use of one of these options is not required for programs intended to run on the Pentium or Intel486 processors,
but are recommended to ensure compatibility with the P6 and more recent processor families.". For cross-modifying code "The use of this option is not required for programs intended to run on the Intel486 processor, but is recommended
to ensure compatibility with the Pentium 4, Intel Xeon, P6 family, and Pentium processors." So I'm not sure there's a problem.

Quoted 2 lines

Fair enough. (Assuming similar properties hold for the older processors of other vendors.)

No, you were right -- a different section of the SDM contradicts it:

For Intel486 processors, a write to an instruction in the cache will modify it in both the cache and memory, but if
the instruction was prefetched before the write, the old version of the instruction could be the one executed. To
prevent the old instruction from being executed, flush the instruction prefetch unit by coding a jump instruction
immediately after any write that modifies an instruction.

Quoted 13 lines · Andrew Cooper

Quoted 12 lines · Andy Lutomirski

Quoted 11 lines · Andrew Cooper

Quoted 4 lines · Andy Lutomirski

On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't guaranteed to serialize. (Even CPUID isn't *really* guaranteed to serialize on Xen PV, but, in practice, any trap it generates will serialize.)

Well, Xen will enabled CPUID Faulting wherever it can, which is realistically all IvyBridge hardware and newer.

All hypercalls are a privilege change to cpl0. I'd hope this condition is serialising, but I can't actually find any documentation proving or disproving this.

Quoted 4 lines

Quoted 2 lines

I don't know for sure. IRET is serializing, and if Xen returns using IRET, we're fine.

All returns to a 64bit PV guest at defined points (hypercall return, exception entry, etc) are from SYSRET, not IRET.

But CPUID faulting isn't like this, right? Unless Xen does opportunistic SYSRET.

Quoted 26 lines

Talking of, I still have a patch to remove PARAVIRT_ADJUST_EXCEPTION_FRAME which I need to complete and send upstream.

Quoted 13 lines

Quoted 6 lines

Quoted 6 lines

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is ~110ns. But Xen PV will trap CPUID if possible, so IRET-to-self should end up being a nice speedup.

Cc Andrew Cooper <[email protected]>

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

Quoted 7 lines

CC'ing xen-devel and the Xen maintainers in Linux.

As this is the only email from this series in my inbox, I will say this here, but it should really be against patch 6.

A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not serialising on the 486, but I don't have a manual to hand to check.

Quoted 9 lines

I'll quote the (modern) SDM. For self-modifying code "The use of one of these options is not required for programs intended to run on the Pentium or Intel486 processors,
but are recommended to ensure compatibility with the P6 and more recent processor families.". For cross-modifying code "The use of this option is not required for programs intended to run on the Intel486 processor, but is recommended
to ensure compatibility with the Pentium 4, Intel Xeon, P6 family, and Pentium processors." So I'm not sure there's a problem.

Quoted 2 lines

Fair enough. (Assuming similar properties hold for the older processors of other vendors.)

No, you were right -- a different section of the SDM contradicts it:

For Intel486 processors, a write to an instruction in the cache will modify it in both the cache and memory, but if
the instruction was prefetched before the write, the old version of the instruction could be the one executed. To
prevent the old instruction from being executed, flush the instruction prefetch unit by coding a jump instruction
immediately after any write that modifies an instruction.

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is ~110ns. But Xen PV will trap CPUID if possible, so IRET-to-self should end up being a nice speedup.

Quoted 4 lines · Andy Lutomirski

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is ~110ns. But Xen PV will trap CPUID if possible, so IRET-to-self should end up being a nice speedup.

So if we care deeply about the performance of this, we should really ask ourselves how much we need this...

There are *very* few places where we really need to do a full serializing instruction, and I'd worry that we really don't need it in many of the places we do this.

The only real case I'm aware of is modifying code that is modified through a different linear address than it's executed.

Is there anything else where we _really_ need this sync-core thing? Sure, the microcode loader looks fine, but that doesn't look particularly performance-critical either.

So I'd like to know which sync_core is actually so performance-critical that w e care about it, and then I'd like to understand why it's needed at all, because I suspect a number of them has been added with the model of "sprinkle random things around and hope".

Adding Peter Anvin to the participants list, because iirc he was the one who really talked to hardwre engineers about the synchronization issues with serializing kernel code.

Linus

apply_alternatives, unfortunately. It's performance-critical because it's intensely stupid and does sync_core() for every single patch. Fixing that would be nice, too.

apply_alternatives, unfortunately. It's performance-critical because it's intensely stupid and does sync_core() for every single patch. Fixing that would be nice, too.

SNB:
* before:
* after:

Quoted 4 lines · Andy Lutomirski

On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't guaranteed to serialize. (Even CPUID isn't *really* guaranteed to serialize on Xen PV, but, in practice, any trap it generates will serialize.)

Well, Xen will enabled CPUID Faulting wherever it can, which is realistically all IvyBridge hardware and newer.

All hypercalls are a privilege change to cpl0. I'd hope this condition is serialising, but I can't actually find any documentation proving or disproving this.

Quoted 6 lines

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is ~110ns. But Xen PV will trap CPUID if possible, so IRET-to-self should end up being a nice speedup.

Cc Andrew Cooper <[email protected]>

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

CC'ing xen-devel and the Xen maintainers in Linux.

As this is the only email from this series in my inbox, I will say this here, but it should really be against patch 6.

A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not serialising on the 486, but I don't have a manual to hand to check.

~Andrew

Quoted 12 lines

---
 arch/x86/xen/enlighten.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

arch/x86/xen/enlighten.c +0 -0

index bdd855685403..1f765b41eee7 100644
@@ -311,6 +311,39 @@ static __read_mostly unsigned int cpuid_leaf1_ecx_set_mask;
 static __read_mostly unsigned int cpuid_leaf5_ecx_val;
 static __read_mostly unsigned int cpuid_leaf5_edx_val;
 

Quoted 5 lines

arch/x86/xen/enlighten.c +5 -0

+static void xen_sync_core(void)
+{
+	register void *__sp asm(_ASM_SP);
+
+#ifdef CONFIG_X86_32

arch/x86/xen/enlighten.c static __read_mostly unsigned int cpuid_leaf1_ecx_set_mask; · line 328

Quoted 10 lines

arch/x86/xen/enlighten.c +10 -0

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

Quoted 30 lines

arch/x86/xen/enlighten.c +20 -0

+#else
+	unsigned long tmp;
+
+	asm volatile (
+		"movq %%ss, %0\n\t"
+		"pushq %0\n\t"
+		"pushq %%rsp\n\t"
+		"addq $8, (%%rsp)\n\t"
+		"pushfq\n\t"
+		"movq %%cs, %0\n\t"
+		"pushq %0\n\t"
+		"pushq $1f\n\t"
+		"iretq\n\t"
+		"1:"
+		: "=r" (tmp), "+r" (__sp) : : "cc");
+#endif
+}
+
 static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 		      unsigned int *cx, unsigned int *dx)
 {

@@ -1289,6 +1322,8 @@ static const struct pv_cpu_ops xen_cpu_ops __initconst = {
 
 	.start_context_switch = paravirt_start_context_switch,
 	.end_context_switch = xen_end_context_switch,
+
+	.sync_core = xen_sync_core,
 };
 
 static void xen_reboot(int reason)

That is, of course, assuming that there is no really subtle reason why that stupid sync_core() is there.

Quoted 4 lines · Andy Lutomirski

On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't guaranteed to serialize. (Even CPUID isn't *really* guaranteed to serialize on Xen PV, but, in practice, any trap it generates will serialize.)

Well, Xen will enabled CPUID Faulting wherever it can, which is realistically all IvyBridge hardware and newer.

All hypercalls are a privilege change to cpl0. I'd hope this condition is serialising, but I can't actually find any documentation proving or disproving this.

Quoted 6 lines

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is ~110ns. But Xen PV will trap CPUID if possible, so IRET-to-self should end up being a nice speedup.

Cc Andrew Cooper <[email protected]>

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

CC'ing xen-devel and the Xen maintainers in Linux.

As this is the only email from this series in my inbox, I will say this here, but it should really be against patch 6.

A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not serialising on the 486, but I don't have a manual to hand to check.

~Andrew

Quoted 12 lines

---
 arch/x86/xen/enlighten.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

arch/x86/xen/enlighten.c +0 -0

index bdd855685403..1f765b41eee7 100644
@@ -311,6 +311,39 @@ static __read_mostly unsigned int cpuid_leaf1_ecx_set_mask;
 static __read_mostly unsigned int cpuid_leaf5_ecx_val;
 static __read_mostly unsigned int cpuid_leaf5_edx_val;
 

Quoted 5 lines

arch/x86/xen/enlighten.c +5 -0

+static void xen_sync_core(void)
+{
+	register void *__sp asm(_ASM_SP);
+
+#ifdef CONFIG_X86_32

arch/x86/xen/enlighten.c static __read_mostly unsigned int cpuid_leaf1_ecx_set_mask; · line 328

Quoted 10 lines

arch/x86/xen/enlighten.c +10 -0

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

Quoted 23 lines

arch/x86/xen/enlighten.c +18 -0

+#else
+	unsigned long tmp;
+
+	asm volatile (
+		"movq %%ss, %0\n\t"
+		"pushq %0\n\t"
+		"pushq %%rsp\n\t"
+		"addq $8, (%%rsp)\n\t"
+		"pushfq\n\t"
+		"movq %%cs, %0\n\t"
+		"pushq %0\n\t"
+		"pushq $1f\n\t"
+		"iretq\n\t"
+		"1:"
+		: "=r" (tmp), "+r" (__sp) : : "cc");
+#endif
+}
+
 static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 		      unsigned int *cx, unsigned int *dx)
 {

@@ -1289,6 +1322,8 @@ static const struct pv_cpu_ops xen_cpu_ops __initconst = {
 

arch/x86/xen/enlighten.c static const struct pv_cpu_ops xen_cpu_ops __initconst = { · line 1329

Quoted 7 lines

arch/x86/xen/enlighten.c +2 -0

 	.start_context_switch = paravirt_start_context_switch,
 	.end_context_switch = xen_end_context_switch,
+
+	.sync_core = xen_sync_core,
 };
 
 static void xen_reboot(int reason)

Right, we can try to do something like invalidate_icache() or so in there with the JMP so that the BSP refetches modified code and see where it gets us.

The good thing is, the early patching paths run before SMP is up but from looking at load_module(), for example, which does post_relocation()->module_finalize()->apply_alternatives(), this can happen late.

Now there I'd like to avoid other cores walking into that address being patched. Or are we "safe" there in the sense that load_module() happens on one CPU only sequentially? (I haven't looked at that code to see what's going on there, actually).

I'd really rather rjust mark it noinline with a comment. That way the return from the function acts as the control flow change.

'sync_core()' doesn't help for other CPU's anyway, you need to do the cross-call IPI. So worrying about other CPU's is *not* a valid reason to keep a "sync_core()" call.

Seriously, the only reason I can see for "sync_core()" really is:

- some deep non-serialized MSR access or similar (ie things like
firmware loading etc really might want it, and a mchine check might want it)

The issues with modifying code while another CPU may be just about to access it is a separate issue. And as noted, "sync_core()" is not sufficient for that, you have to do a whole careful dance with single-byte debug instruction writes and then a final cross-call.

See the whole "text_poke_bp()" and "text_poke()" for *that* whole dance. That's a much more complex thing from the normal apply_alternatives().

Quoted 4 lines · Borislav Petkov

Right, we can try to do something like invalidate_icache() or so in there with the JMP so that the BSP refetches modified code and see where it gets us.

I'd really rather rjust mark it noinline with a comment. That way the return from the function acts as the control flow change.

Quoted 9 lines

The good thing is, the early patching paths run before SMP is up but from looking at load_module(), for example, which does post_relocation()->module_finalize()->apply_alternatives(), this can happen late.

Now there I'd like to avoid other cores walking into that address being patched. Or are we "safe" there in the sense that load_module() happens on one CPU only sequentially? (I haven't looked at that code to see what's going on there, actually).

'sync_core()' doesn't help for other CPU's anyway, you need to do the cross-call IPI. So worrying about other CPU's is *not* a valid reason to keep a "sync_core()" call.

Seriously, the only reason I can see for "sync_core()" really is:

- some deep non-serialized MSR access or similar (ie things like
firmware loading etc really might want it, and a mchine check might want it)

Something like below?

The optimize-nops thing needs it too, I think.

Again, this will never matter in practice (even if somebody has a i486 s till, the prefetch window size is like 16 bytes or something), but from a documentation standpoint it's good.

Not even firmware loading wants it. Firmware loading needs

specifically cpuid(eax=1). It has nothing to do with serializing

Quoted 2 lines · Borislav Petkov

Something like below?

The optimize-nops thing needs it too, I think.

Again, this will never matter in practice (even if somebody has a i486 s till, the prefetch window size is like 16 bytes or something), but from a documentation standpoint it's good.

Quoted 40 lines · Andrew Cooper

Quoted 4 lines · Andy Lutomirski

On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't guaranteed to serialize. (Even CPUID isn't *really* guaranteed to serialize on Xen PV, but, in practice, any trap it generates will serialize.)

Well, Xen will enabled CPUID Faulting wherever it can, which is realistically all IvyBridge hardware and newer.

All hypercalls are a privilege change to cpl0. I'd hope this condition is serialising, but I can't actually find any documentation proving or disproving this.

Quoted 6 lines

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is ~110ns. But Xen PV will trap CPUID if possible, so IRET-to-self should end up being a nice speedup.

Cc Andrew Cooper <[email protected]>

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

CC'ing xen-devel and the Xen maintainers in Linux.

As this is the only email from this series in my inbox, I will say this here, but it should really be against patch 6.

A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not serialising on the 486, but I don't have a manual to hand to check.

~Andrew

Quoted 12 lines

---
 arch/x86/xen/enlighten.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

arch/x86/xen/enlighten.c +0 -0

index bdd855685403..1f765b41eee7 100644
@@ -311,6 +311,39 @@ static __read_mostly unsigned int cpuid_leaf1_ecx_set_mask;
 static __read_mostly unsigned int cpuid_leaf5_ecx_val;
 static __read_mostly unsigned int cpuid_leaf5_edx_val;
 

Quoted 15 lines

Quoted 5 lines

arch/x86/xen/enlighten.c +5 -0

+static void xen_sync_core(void)
+{
+    register void *__sp asm(_ASM_SP);
+
+#ifdef CONFIG_X86_32

arch/x86/xen/enlighten.c static __read_mostly unsigned int cpuid_leaf1_ecx_set_mask; · line 328

Quoted 10 lines

arch/x86/xen/enlighten.c +10 -0

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

This breaks 32-bit PV guests.

Why are we pushing %ss? We are not changing privilege levels so why not just flags, cs and eip (which, incidentally, does work)?

Quoted 40 lines · Andrew Cooper

Quoted 4 lines · Andy Lutomirski

On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't guaranteed to serialize. (Even CPUID isn't *really* guaranteed to serialize on Xen PV, but, in practice, any trap it generates will serialize.)

Well, Xen will enabled CPUID Faulting wherever it can, which is realistically all IvyBridge hardware and newer.

All hypercalls are a privilege change to cpl0. I'd hope this condition is serialising, but I can't actually find any documentation proving or disproving this.

Quoted 6 lines

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is ~110ns. But Xen PV will trap CPUID if possible, so IRET-to-self should end up being a nice speedup.

Cc Andrew Cooper <[email protected]>

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

CC'ing xen-devel and the Xen maintainers in Linux.

As this is the only email from this series in my inbox, I will say this here, but it should really be against patch 6.

A write to %cr2 is apparently (http://sandpile.org/x86/coherent.htm) not serialising on the 486, but I don't have a manual to hand to check.

~Andrew

Quoted 12 lines

---
 arch/x86/xen/enlighten.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

arch/x86/xen/enlighten.c +0 -0

index bdd855685403..1f765b41eee7 100644
@@ -311,6 +311,39 @@ static __read_mostly unsigned int cpuid_leaf1_ecx_set_mask;
 static __read_mostly unsigned int cpuid_leaf5_ecx_val;
 static __read_mostly unsigned int cpuid_leaf5_edx_val;
 

Quoted 15 lines

Quoted 5 lines

arch/x86/xen/enlighten.c +5 -0

+static void xen_sync_core(void)
+{
+    register void *__sp asm(_ASM_SP);
+
+#ifdef CONFIG_X86_32

arch/x86/xen/enlighten.c static __read_mostly unsigned int cpuid_leaf1_ecx_set_mask; · line 328

Quoted 10 lines

arch/x86/xen/enlighten.c +10 -0

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

This breaks 32-bit PV guests.

Why are we pushing %ss? We are not changing privilege levels so why not just flags, cs and eip (which, incidentally, does work)?

How's this?

I suppose it could be an unconditional IRET-to-self, but that's a good deal slower and not a whole lot simpler. Although if we start doing it right, performance won't really matter here.

Quoted 4 lines · Andy Lutomirski

On Xen PV, CPUID is likely to trap, and Xen hypercalls aren't guaranteed to serialize. (Even CPUID isn't *really* guaranteed to serialize on Xen PV, but, in practice, any trap it generates will serialize.)

Well, Xen will enabled CPUID Faulting wherever it can, which is realistically all IvyBridge hardware and newer.

All hypercalls are a privilege change to cpl0. I'd hope this condition is serialising, but I can't actually find any documentation proving or disproving this.

Quoted 6 lines

On my laptop, CPUID(eax=1, ecx=0) is ~83ns and IRET-to-self is ~110ns. But Xen PV will trap CPUID if possible, so IRET-to-self should end up being a nice speedup.

Cc Andrew Cooper <[email protected]>

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

Quoted 2 lines · Andy Lutomirski

How's this?

Looks ok. I do think that

Quoted 3 lines

I suppose it could be an unconditional IRET-to-self, but that's a good deal slower and not a whole lot simpler. Although if we start doing it right, performance won't really matter here.

Considering you already got the iret-to-self wrong in the first version, I really like the "one single unconditional version" so that everybody tests that _one_ thing and there isn't anything subtle going on.

Hmm?

And yes, if it turns out that performance matters, we almost certainly are doing something really wrong, and we shouldn't be using that sync_core() thing in that place anyway.

Because, if so, we should maybe serialize whenever we migrate a process to a different CPU.

(We *definitely* need to flush the store buffer when migrating,

Quoted 3 lines · Andy Lutomirski

Because, if so, we should maybe serialize whenever we migrate a process to a different CPU.

The intel docs are bad on this issue.

Technically what we do could fall under the "cross-modifying code" case, where one CPU does the write, and then we run it on another CPU.

And no, we do *not* do a serializing instruction before returning to user space. Sure, we might do an iret (which is serializing), but we equally well might be doing a systret (which is not).

Honestly, I think Intel should clean up their documentation.

Quoted 2 lines

Honestly, I think Intel should clean up their documentation.

I'm not sure I follow. If a user program gets migrated, it might end up doing cross-modification when it expects self-modification. If that trips the program up, is that a user bug or a kernel bug?

TBH, I didn't start down this path for performance. I did it because I wanted to kill off a CPUID that was breaking on old CPUs that don't have CPUID. So I propose MOV-to-CR2 followed by an unconditional jump. My goal here is to make the #*!& thing work reliably and not be ludicrously slow. Borislav and I mulled over using an alternative to use CPUID if and only if we have CPUID, but that doesn't work because we call sync_core() before we're done applying alternatives.

Ah, it is called only from apply_alternatives() but sure, it is safer this way. Lemme do that and run it through the boxes to see whether anything catches fire.

Quoted 3 lines · Borislav Petkov

Ah, it is called only from apply_alternatives() but sure, it is safer this way. Lemme do that and run it through the boxes to see whether anything catches fire.

Looks good, ran it on a bunch of machines, two of them huge AMD and Intel, 8-socket monsters.

Here is a version with a commit message:

---
From: Borislav Petkov <[email protected]>
Date: Fri, 2 Dec 2016 23:18:51 +0100
Subject: [PATCH] x86/alternatives: Do not use sync_core() to serialize I$

We use sync_core() in the alternatives code to stop speculative execution of prefetched instructions because we are potentially changing them and don't want to execute stale bytes.

What it does on most machines is call CPUID which is a serializing instruction. And that's expensive.

However, the instruction cache is serialized when we're on the local CPU and are changing the data through the same virtual address. So then, we don't need the serializing CPUID but a simple control flow change. Last being accomplished with a CALL/RET which the noinline causes.

So let's try it.