[PATCH 1/4] x86/cpufeatures: Add enumeration for SERIALIZE instruction

[PATCH 1/4] x86/cpufeatures: Add enumeration for SERIALIZE instruction

[Ricardo Neri]

The Intel architecture defines a set of Serializing Instructions (a
detailed definition can be found in Vol.3 Section 8.3 of the Intel "main"
manual, SDM). However, these instructions do more than what is required,
have side effects and/or may be rather invasive. Furthermore, some of
these instructions are only available in kernel mode or may cause VMExits.
Thus, software using these instructions only to serialize execution (as
defined in the manual) must handle the undesired side effects.

As indicated in the name, SERIALIZE is a new Intel architecture
Serializing Instruction. Crucially, it does not have any of the mentioned
side effects. Also, it does not cause VMExit and can be used in user mode.

This new instruction is currently documented in the latest "extensions"
manual (ISE). It will appear in the "main" manual in the future.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Cathy Zhang <cathy.zhang@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Kyung Min Park <kyung.min.park@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Ravi V. Shankar" <ravi.v.shankar@intel.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: linux-edac@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 03390a1ef8e7..2901d5df4366 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -367,6 +367,7 @@
 #define X86_FEATURE_SRBDS_CTRL		(18*32+ 9) /* "" SRBDS mitigation MSR available */
 #define X86_FEATURE_MD_CLEAR		(18*32+10) /* VERW clears CPU buffers */
 #define X86_FEATURE_TSX_FORCE_ABORT	(18*32+13) /* "" TSX_FORCE_ABORT */
+#define X86_FEATURE_SERIALIZE		(18*32+14) /* SERIALIZE instruction */
 #define X86_FEATURE_PCONFIG		(18*32+18) /* Intel PCONFIG */
 #define X86_FEATURE_ARCH_LBR		(18*32+19) /* Intel ARCH LBR */
 #define X86_FEATURE_SPEC_CTRL		(18*32+26) /* "" Speculation Control (IBRS + IBPB) */
-- 
2.17.1

[PATCH 2/4] x86/cpu: Relocate sync_core() to sync_core.h

[Ricardo Neri]

Having sync_core() in processor.h is problematic since it is not possible
to check for hardware capabilities via the *cpu_has() family of macros.
The latter needs the definitions in processor.h.

It also looks more intuitive to relocate the function to sync_core.h.

This changeset does not make changes in functionality.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Cathy Zhang <cathy.zhang@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Dimitri Sivanich <sivanich@sgi.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Kyung Min Park <kyung.min.park@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Ravi V. Shankar" <ravi.v.shankar@intel.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: linux-edac@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
---
 arch/x86/include/asm/processor.h    | 64 -----------------------------
 arch/x86/include/asm/sync_core.h    | 64 +++++++++++++++++++++++++++++
 arch/x86/kernel/alternative.c       |  1 +
 arch/x86/kernel/cpu/mce/core.c      |  1 +
 drivers/misc/sgi-gru/grufault.c     |  1 +
 drivers/misc/sgi-gru/gruhandles.c   |  1 +
 drivers/misc/sgi-gru/grukservices.c |  1 +
 7 files changed, 69 insertions(+), 64 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 2a1f7e1d7151..97143d87994c 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -676,70 +676,6 @@ static inline unsigned int cpuid_edx(unsigned int op)
 	return edx;
 }
 
-/*
- * This function forces the icache and prefetched instruction stream to
- * catch up with reality in two very specific cases:
- *
- *  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.
- */
-static inline void sync_core(void)
-{
-	/*
-	 * 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.  The "push %cs" is needed because, in
-	 * paravirtual environments, __KERNEL_CS may not be a valid CS
-	 * value when we do IRET directly.
-	 *
-	 * In case NMI unmasking or performance ever 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 (i.e. Xen PV).
-	 *
-	 * 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.
-	 *
-	 * Like all of Linux's memory ordering operations, this is a
-	 * compiler barrier as well.
-	 */
-#ifdef CONFIG_X86_32
-	asm volatile (
-		"pushfl\n\t"
-		"pushl %%cs\n\t"
-		"pushl $1f\n\t"
-		"iret\n\t"
-		"1:"
-		: ASM_CALL_CONSTRAINT : : "memory");
-#else
-	unsigned int tmp;
-
-	asm volatile (
-		"mov %%ss, %0\n\t"
-		"pushq %q0\n\t"
-		"pushq %%rsp\n\t"
-		"addq $8, (%%rsp)\n\t"
-		"pushfq\n\t"
-		"mov %%cs, %0\n\t"
-		"pushq %q0\n\t"
-		"pushq $1f\n\t"
-		"iretq\n\t"
-		"1:"
-		: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
-#endif
-}
-
 extern void select_idle_routine(const struct cpuinfo_x86 *c);
 extern void amd_e400_c1e_apic_setup(void);
 
diff --git a/arch/x86/include/asm/sync_core.h b/arch/x86/include/asm/sync_core.h
index c67caafd3381..9c5573f2c333 100644
--- a/arch/x86/include/asm/sync_core.h
+++ b/arch/x86/include/asm/sync_core.h
@@ -6,6 +6,70 @@
 #include <asm/processor.h>
 #include <asm/cpufeature.h>
 
+/*
+ * This function forces the icache and prefetched instruction stream to
+ * catch up with reality in two very specific cases:
+ *
+ *  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.
+ */
+static inline void sync_core(void)
+{
+	/*
+	 * 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.  The "push %cs" is needed because, in
+	 * paravirtual environments, __KERNEL_CS may not be a valid CS
+	 * value when we do IRET directly.
+	 *
+	 * In case NMI unmasking or performance ever 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 (i.e. Xen PV).
+	 *
+	 * 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.
+	 *
+	 * Like all of Linux's memory ordering operations, this is a
+	 * compiler barrier as well.
+	 */
+#ifdef CONFIG_X86_32
+	asm volatile (
+		"pushfl\n\t"
+		"pushl %%cs\n\t"
+		"pushl $1f\n\t"
+		"iret\n\t"
+		"1:"
+		: ASM_CALL_CONSTRAINT : : "memory");
+#else
+	unsigned int tmp;
+
+	asm volatile (
+		"mov %%ss, %0\n\t"
+		"pushq %q0\n\t"
+		"pushq %%rsp\n\t"
+		"addq $8, (%%rsp)\n\t"
+		"pushfq\n\t"
+		"mov %%cs, %0\n\t"
+		"pushq %q0\n\t"
+		"pushq $1f\n\t"
+		"iretq\n\t"
+		"1:"
+		: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
+#endif
+}
+
 /*
  * Ensure that a core serializing instruction is issued before returning
  * to user-mode. x86 implements return to user-space through sysexit,
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 20e07feb4064..3abc1316f91b 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -16,6 +16,7 @@
 #include <linux/kprobes.h>
 #include <linux/mmu_context.h>
 #include <linux/bsearch.h>
+#include <linux/sync_core.h>
 #include <asm/text-patching.h>
 #include <asm/alternative.h>
 #include <asm/sections.h>
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 1a0139d9a34b..e76c1ddd35e7 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -42,6 +42,7 @@
 #include <linux/export.h>
 #include <linux/jump_label.h>
 #include <linux/set_memory.h>
+#include <linux/sync_core.h>
 #include <linux/task_work.h>
 #include <linux/hardirq.h>
 
diff --git a/drivers/misc/sgi-gru/grufault.c b/drivers/misc/sgi-gru/grufault.c
index b1521112dbbd..723825524ea0 100644
--- a/drivers/misc/sgi-gru/grufault.c
+++ b/drivers/misc/sgi-gru/grufault.c
@@ -20,6 +20,7 @@
 #include <linux/io.h>
 #include <linux/uaccess.h>
 #include <linux/security.h>
+#include <linux/sync_core.h>
 #include <linux/prefetch.h>
 #include "gru.h"
 #include "grutables.h"
diff --git a/drivers/misc/sgi-gru/gruhandles.c b/drivers/misc/sgi-gru/gruhandles.c
index f7224f90f413..1d75d5e540bc 100644
--- a/drivers/misc/sgi-gru/gruhandles.c
+++ b/drivers/misc/sgi-gru/gruhandles.c
@@ -16,6 +16,7 @@
 #define GRU_OPERATION_TIMEOUT	(((cycles_t) local_cpu_data->itc_freq)*10)
 #define CLKS2NSEC(c)		((c) *1000000000 / local_cpu_data->itc_freq)
 #else
+#include <linux/sync_core.h>
 #include <asm/tsc.h>
 #define GRU_OPERATION_TIMEOUT	((cycles_t) tsc_khz*10*1000)
 #define CLKS2NSEC(c)		((c) * 1000000 / tsc_khz)
diff --git a/drivers/misc/sgi-gru/grukservices.c b/drivers/misc/sgi-gru/grukservices.c
index 0197441a1eae..f6e600bfac5d 100644
--- a/drivers/misc/sgi-gru/grukservices.c
+++ b/drivers/misc/sgi-gru/grukservices.c
@@ -16,6 +16,7 @@
 #include <linux/miscdevice.h>
 #include <linux/proc_fs.h>
 #include <linux/interrupt.h>
+#include <linux/sync_core.h>
 #include <linux/uaccess.h>
 #include <linux/delay.h>
 #include <linux/export.h>
-- 
2.17.1

[PATCH 3/4] x86/cpu: Refactor sync_core() for readability

[Ricardo Neri]

Instead of having #ifdef/#endif blocks inside sync_core() for X86_64 and
X86_32, implement the new function iret_to_self() with two versions.

In this manner, avoid having to use even more more #ifdef/#endif blocks
when adding support for SERIALIZE in sync_core().

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Cathy Zhang <cathy.zhang@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Kyung Min Park <kyung.min.park@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Ravi V. Shankar" <ravi.v.shankar@intel.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: linux-edac@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Co-developed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
---
 arch/x86/include/asm/special_insns.h |  1 -
 arch/x86/include/asm/sync_core.h     | 56 ++++++++++++++++------------
 2 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index eb8e781c4353..59a3e13204c3 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -234,7 +234,6 @@ static inline void clwb(volatile void *__p)
 
 #define nop() asm volatile ("nop")
 
-
 #endif /* __KERNEL__ */
 
 #endif /* _ASM_X86_SPECIAL_INSNS_H */
diff --git a/arch/x86/include/asm/sync_core.h b/arch/x86/include/asm/sync_core.h
index 9c5573f2c333..fdb5b356e59b 100644
--- a/arch/x86/include/asm/sync_core.h
+++ b/arch/x86/include/asm/sync_core.h
@@ -6,6 +6,37 @@
 #include <asm/processor.h>
 #include <asm/cpufeature.h>
 
+#ifdef CONFIG_X86_32
+static inline void iret_to_self(void)
+{
+	asm volatile (
+		"pushfl\n\t"
+		"pushl %%cs\n\t"
+		"pushl $1f\n\t"
+		"iret\n\t"
+		"1:"
+		: ASM_CALL_CONSTRAINT : : "memory");
+}
+#else
+static inline void iret_to_self(void)
+{
+	unsigned int tmp;
+
+	asm volatile (
+		"mov %%ss, %0\n\t"
+		"pushq %q0\n\t"
+		"pushq %%rsp\n\t"
+		"addq $8, (%%rsp)\n\t"
+		"pushfq\n\t"
+		"mov %%cs, %0\n\t"
+		"pushq %q0\n\t"
+		"pushq $1f\n\t"
+		"iretq\n\t"
+		"1:"
+		: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
+}
+#endif /* CONFIG_X86_32 */
+
 /*
  * This function forces the icache and prefetched instruction stream to
  * catch up with reality in two very specific cases:
@@ -44,30 +75,7 @@ static inline void sync_core(void)
 	 * Like all of Linux's memory ordering operations, this is a
 	 * compiler barrier as well.
 	 */
-#ifdef CONFIG_X86_32
-	asm volatile (
-		"pushfl\n\t"
-		"pushl %%cs\n\t"
-		"pushl $1f\n\t"
-		"iret\n\t"
-		"1:"
-		: ASM_CALL_CONSTRAINT : : "memory");
-#else
-	unsigned int tmp;
-
-	asm volatile (
-		"mov %%ss, %0\n\t"
-		"pushq %q0\n\t"
-		"pushq %%rsp\n\t"
-		"addq $8, (%%rsp)\n\t"
-		"pushfq\n\t"
-		"mov %%cs, %0\n\t"
-		"pushq %q0\n\t"
-		"pushq $1f\n\t"
-		"iretq\n\t"
-		"1:"
-		: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
-#endif
+	iret_to_self();
 }
 
 /*
-- 
2.17.1

[PATCH 4/4] x86/cpu: Use SERIALIZE in sync_core() when available

[Ricardo Neri]

The SERIALIZE instruction gives software a way to force the processor to
complete all modifications to flags, registers and memory from previous
instructions and drain all buffered writes to memory before the next
instruction is fetched and executed. Thus, it serves the purpose of
sync_core(). Use it when available.

Use boot_cpu_has() and not static_cpu_has(); the most critical paths
(returning to user mode and from interrupt and NMI) will not reach
sync_core().

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Cathy Zhang <cathy.zhang@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Kyung Min Park <kyung.min.park@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Ravi V. Shankar" <ravi.v.shankar@intel.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: linux-edac@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Reviwed-by: Tony Luck <tony.luck@intel.com>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
---
---
 arch/x86/include/asm/special_insns.h |  5 +++++
 arch/x86/include/asm/sync_core.h     | 10 +++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index 59a3e13204c3..0a2a60bba282 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -234,6 +234,11 @@ static inline void clwb(volatile void *__p)
 
 #define nop() asm volatile ("nop")
 
+static inline void serialize(void)
+{
+	asm volatile(".byte 0xf, 0x1, 0xe8");
+}
+
 #endif /* __KERNEL__ */
 
 #endif /* _ASM_X86_SPECIAL_INSNS_H */
diff --git a/arch/x86/include/asm/sync_core.h b/arch/x86/include/asm/sync_core.h
index fdb5b356e59b..bf132c09d61b 100644
--- a/arch/x86/include/asm/sync_core.h
+++ b/arch/x86/include/asm/sync_core.h
@@ -5,6 +5,7 @@
 #include <linux/preempt.h>
 #include <asm/processor.h>
 #include <asm/cpufeature.h>
+#include <asm/special_insns.h>
 
 #ifdef CONFIG_X86_32
 static inline void iret_to_self(void)
@@ -54,7 +55,8 @@ static inline void iret_to_self(void)
 static inline void sync_core(void)
 {
 	/*
-	 * There are quite a few ways to do this.  IRET-to-self is nice
+	 * Hardware can do this for us if SERIALIZE is available. Otherwise,
+	 * 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
@@ -75,6 +77,12 @@ static inline void sync_core(void)
 	 * Like all of Linux's memory ordering operations, this is a
 	 * compiler barrier as well.
 	 */
+
+	if (boot_cpu_has(X86_FEATURE_SERIALIZE)) {
+		serialize();
+		return;
+	}
+
 	iret_to_self();
 }
 
-- 
2.17.1

Re: [PATCH 4/4] x86/cpu: Use SERIALIZE in sync_core() when available

[hpa]

On July 26, 2020 9:31:32 PM PDT, Ricardo Neri <ricardo.neri-calderon@linux.intel.com> wrote:
>The SERIALIZE instruction gives software a way to force the processor
>to
>complete all modifications to flags, registers and memory from previous
>instructions and drain all buffered writes to memory before the next
>instruction is fetched and executed. Thus, it serves the purpose of
>sync_core(). Use it when available.
>
>Use boot_cpu_has() and not static_cpu_has(); the most critical paths
>(returning to user mode and from interrupt and NMI) will not reach
>sync_core().
>
>Cc: Andy Lutomirski <luto@kernel.org>
>Cc: Cathy Zhang <cathy.zhang@intel.com>
>Cc: Dave Hansen <dave.hansen@linux.intel.com>
>Cc: Fenghua Yu <fenghua.yu@intel.com>
>Cc: "H. Peter Anvin" <hpa@zytor.com>
>Cc: Kyung Min Park <kyung.min.park@intel.com>
>Cc: Peter Zijlstra <peterz@infradead.org>
>Cc: "Ravi V. Shankar" <ravi.v.shankar@intel.com>
>Cc: Sean Christopherson <sean.j.christopherson@intel.com>
>Cc: linux-edac@vger.kernel.org
>Cc: linux-kernel@vger.kernel.org
>Reviwed-by: Tony Luck <tony.luck@intel.com>
>Suggested-by: Andy Lutomirski <luto@kernel.org>
>Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
>---
>---
> arch/x86/include/asm/special_insns.h |  5 +++++
> arch/x86/include/asm/sync_core.h     | 10 +++++++++-
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
>diff --git a/arch/x86/include/asm/special_insns.h
>b/arch/x86/include/asm/special_insns.h
>index 59a3e13204c3..0a2a60bba282 100644
>--- a/arch/x86/include/asm/special_insns.h
>+++ b/arch/x86/include/asm/special_insns.h
>@@ -234,6 +234,11 @@ static inline void clwb(volatile void *__p)
> 
> #define nop() asm volatile ("nop")
> 
>+static inline void serialize(void)
>+{
>+	asm volatile(".byte 0xf, 0x1, 0xe8");
>+}
>+
> #endif /* __KERNEL__ */
> 
> #endif /* _ASM_X86_SPECIAL_INSNS_H */
>diff --git a/arch/x86/include/asm/sync_core.h
>b/arch/x86/include/asm/sync_core.h
>index fdb5b356e59b..bf132c09d61b 100644
>--- a/arch/x86/include/asm/sync_core.h
>+++ b/arch/x86/include/asm/sync_core.h
>@@ -5,6 +5,7 @@
> #include <linux/preempt.h>
> #include <asm/processor.h>
> #include <asm/cpufeature.h>
>+#include <asm/special_insns.h>
> 
> #ifdef CONFIG_X86_32
> static inline void iret_to_self(void)
>@@ -54,7 +55,8 @@ static inline void iret_to_self(void)
> static inline void sync_core(void)
> {
> 	/*
>-	 * There are quite a few ways to do this.  IRET-to-self is nice
>+	 * Hardware can do this for us if SERIALIZE is available. Otherwise,
>+	 * 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
>@@ -75,6 +77,12 @@ static inline void sync_core(void)
> 	 * Like all of Linux's memory ordering operations, this is a
> 	 * compiler barrier as well.
> 	 */
>+
>+	if (boot_cpu_has(X86_FEATURE_SERIALIZE)) {
>+		serialize();
>+		return;
>+	}
>+
> 	iret_to_self();
> }
> 

Any reason to not make sync_core() an inline with alternatives?

For a really overenginered solution, but which might perform unnecessary poorly on existing hardware:

asm volatile("1: .byte 0xf, 0x1, 0xe8; 2:"
                        _ASM_EXTABLE(1b,2b));

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 4/4] x86/cpu: Use SERIALIZE in sync_core() when available

[hpa]

On July 26, 2020 10:55:15 PM PDT, hpa@zytor.com wrote:
>On July 26, 2020 9:31:32 PM PDT, Ricardo Neri
><ricardo.neri-calderon@linux.intel.com> wrote:
>>The SERIALIZE instruction gives software a way to force the processor
>>to
>>complete all modifications to flags, registers and memory from
>previous
>>instructions and drain all buffered writes to memory before the next
>>instruction is fetched and executed. Thus, it serves the purpose of
>>sync_core(). Use it when available.
>>
>>Use boot_cpu_has() and not static_cpu_has(); the most critical paths
>>(returning to user mode and from interrupt and NMI) will not reach
>>sync_core().
>>
>>Cc: Andy Lutomirski <luto@kernel.org>
>>Cc: Cathy Zhang <cathy.zhang@intel.com>
>>Cc: Dave Hansen <dave.hansen@linux.intel.com>
>>Cc: Fenghua Yu <fenghua.yu@intel.com>
>>Cc: "H. Peter Anvin" <hpa@zytor.com>
>>Cc: Kyung Min Park <kyung.min.park@intel.com>
>>Cc: Peter Zijlstra <peterz@infradead.org>
>>Cc: "Ravi V. Shankar" <ravi.v.shankar@intel.com>
>>Cc: Sean Christopherson <sean.j.christopherson@intel.com>
>>Cc: linux-edac@vger.kernel.org
>>Cc: linux-kernel@vger.kernel.org
>>Reviwed-by: Tony Luck <tony.luck@intel.com>
>>Suggested-by: Andy Lutomirski <luto@kernel.org>
>>Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
>>---
>>---
>> arch/x86/include/asm/special_insns.h |  5 +++++
>> arch/x86/include/asm/sync_core.h     | 10 +++++++++-
>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>
>>diff --git a/arch/x86/include/asm/special_insns.h
>>b/arch/x86/include/asm/special_insns.h
>>index 59a3e13204c3..0a2a60bba282 100644
>>--- a/arch/x86/include/asm/special_insns.h
>>+++ b/arch/x86/include/asm/special_insns.h
>>@@ -234,6 +234,11 @@ static inline void clwb(volatile void *__p)
>> 
>> #define nop() asm volatile ("nop")
>> 
>>+static inline void serialize(void)
>>+{
>>+	asm volatile(".byte 0xf, 0x1, 0xe8");
>>+}
>>+
>> #endif /* __KERNEL__ */
>> 
>> #endif /* _ASM_X86_SPECIAL_INSNS_H */
>>diff --git a/arch/x86/include/asm/sync_core.h
>>b/arch/x86/include/asm/sync_core.h
>>index fdb5b356e59b..bf132c09d61b 100644
>>--- a/arch/x86/include/asm/sync_core.h
>>+++ b/arch/x86/include/asm/sync_core.h
>>@@ -5,6 +5,7 @@
>> #include <linux/preempt.h>
>> #include <asm/processor.h>
>> #include <asm/cpufeature.h>
>>+#include <asm/special_insns.h>
>> 
>> #ifdef CONFIG_X86_32
>> static inline void iret_to_self(void)
>>@@ -54,7 +55,8 @@ static inline void iret_to_self(void)
>> static inline void sync_core(void)
>> {
>> 	/*
>>-	 * There are quite a few ways to do this.  IRET-to-self is nice
>>+	 * Hardware can do this for us if SERIALIZE is available. Otherwise,
>>+	 * 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
>>@@ -75,6 +77,12 @@ static inline void sync_core(void)
>> 	 * Like all of Linux's memory ordering operations, this is a
>> 	 * compiler barrier as well.
>> 	 */
>>+
>>+	if (boot_cpu_has(X86_FEATURE_SERIALIZE)) {
>>+		serialize();
>>+		return;
>>+	}
>>+
>> 	iret_to_self();
>> }
>> 
>
>Any reason to not make sync_core() an inline with alternatives?
>
>For a really overenginered solution, but which might perform
>unnecessary poorly on existing hardware:
>
>asm volatile("1: .byte 0xf, 0x1, 0xe8; 2:"
>                        _ASM_EXTABLE(1b,2b));

(and : : : "memory" of course.)
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 4/4] x86/cpu: Use SERIALIZE in sync_core() when available

[peterz]

On Sun, Jul 26, 2020 at 09:31:32PM -0700, Ricardo Neri wrote:
> +static inline void serialize(void)
> +{
> +	asm volatile(".byte 0xf, 0x1, 0xe8");
> +}

Can we pretty please have a comment with the binutils version that has
the mnomic? Such that when we increase the required binutils version we
can grep for this.

Re: [PATCH 4/4] x86/cpu: Use SERIALIZE in sync_core() when available

[peterz]

On Sun, Jul 26, 2020 at 09:31:32PM -0700, Ricardo Neri wrote:
> @@ -75,6 +77,12 @@ static inline void sync_core(void)
>  	 * Like all of Linux's memory ordering operations, this is a
>  	 * compiler barrier as well.
>  	 */
> +
> +	if (boot_cpu_has(X86_FEATURE_SERIALIZE)) {
> +		serialize();
> +		return;
> +	}
> +
>  	iret_to_self();

I was sorta expecting something like:

	alternative(IRET_TO_SELF, SERIALIZE, X86_FEATURE_SERIALIZE);

But instead you used boot_cpu_has() which is a dynamic test.

Re: [PATCH 4/4] x86/cpu: Use SERIALIZE in sync_core() when available

[peterz]

On Sun, Jul 26, 2020 at 10:55:15PM -0700, hpa@zytor.com wrote:
> For a really overenginered solution, but which might perform
> unnecessary poorly on existing hardware:
> 
> asm volatile("1: .byte 0xf, 0x1, 0xe8; 2:"
>                         _ASM_EXTABLE(1b,2b));

Ha! cute, you take an #UD ?

We could optimize the #UD exception handler for this I suppose, but that
makes it an even worse hack. The simple alternative() seems like a much
simpler approach.

Re: [PATCH 4/4] x86/cpu: Use SERIALIZE in sync_core() when available

[hpa]

On July 27, 2020 1:20:03 AM PDT, peterz@infradead.org wrote:
>On Sun, Jul 26, 2020 at 09:31:32PM -0700, Ricardo Neri wrote:
>> +static inline void serialize(void)
>> +{
>> +	asm volatile(".byte 0xf, 0x1, 0xe8");
>> +}
>
>Can we pretty please have a comment with the binutils version that has
>the mnomic? Such that when we increase the required binutils version we
>can grep for this.

It also needs : : : "memory" to be a barrier.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 4/4] x86/cpu: Use SERIALIZE in sync_core() when available

[hpa]

On July 27, 2020 1:36:19 AM PDT, peterz@infradead.org wrote:
>On Sun, Jul 26, 2020 at 10:55:15PM -0700, hpa@zytor.com wrote:
>> For a really overenginered solution, but which might perform
>> unnecessary poorly on existing hardware:
>> 
>> asm volatile("1: .byte 0xf, 0x1, 0xe8; 2:"
>>                         _ASM_EXTABLE(1b,2b));
>
>Ha! cute, you take an #UD ?
>
>We could optimize the #UD exception handler for this I suppose, but
>that
>makes it an even worse hack. The simple alternative() seems like a much
>simpler approach.

If this is in any way performance critical, then no :) Taking the #UD has the cute property that we end up IRET on the way back, so we don't even need a fix-up path.


-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 4/4] x86/cpu: Use SERIALIZE in sync_core() when available

[peterz]

On Mon, Jul 27, 2020 at 05:49:28AM -0700, hpa@zytor.com wrote:
> On July 27, 2020 1:36:19 AM PDT, peterz@infradead.org wrote:
> >On Sun, Jul 26, 2020 at 10:55:15PM -0700, hpa@zytor.com wrote:
> >> For a really overenginered solution, but which might perform
> >> unnecessary poorly on existing hardware:
> >> 
> >> asm volatile("1: .byte 0xf, 0x1, 0xe8; 2:"
> >>                         _ASM_EXTABLE(1b,2b));
> >
> >Ha! cute, you take an #UD ?
> >
> >We could optimize the #UD exception handler for this I suppose, but
> >that makes it an even worse hack. The simple alternative() seems like
> >a much simpler approach.
> 
> If this is in any way performance critical, then no :) 

Yeah, I'm not sure.. the 'funny' thing is that typically call
sync_core() from an IPI anyway. And the synchronous broadcast IPI is by
far the most expensive part of that.

Something like this...

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 20e07feb4064..528e049ee1d9 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -989,12 +989,13 @@ void *text_poke_kgdb(void *addr, const void *opcode, size_t len)
 
 static void do_sync_core(void *info)
 {
-	sync_core();
+	/* IRET implies sync_core() */
 }
 
 void text_poke_sync(void)
 {
 	on_each_cpu(do_sync_core, NULL, 1);
+	sync_core();
 }
 
 struct text_poke_loc {


> Taking the #UD
> has the cute property that we end up IRET on the way back, so we don't
> even need a fix-up path.

I got that, what I had in mind was making sure #UD avoids the overhead
of doing exception entry/exit by adding an early exit.

Something like so:

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 8493f55e1167..a3f41d645944 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -96,6 +96,16 @@ __always_inline int is_valid_bugaddr(unsigned long addr)
 	return *(unsigned short *)addr == INSN_UD2;
 }
 
+__always_inline int handle_serialize(struct pt_regs *regs)
+{
+	const char serialize[3] = { 0x0f, 0xe8, 0x02 };
+
+	if (regs->ip < TASK_SIZE_MAX)
+		return 0;
+
+	return !memcmp((const void *)regs->ip, serialize, 3);
+}
+
 static nokprobe_inline int
 do_trap_no_signal(struct task_struct *tsk, int trapnr, const char *str,
 		  struct pt_regs *regs,	long error_code)
@@ -252,8 +262,13 @@ DEFINE_IDTENTRY_RAW(exc_invalid_op)
 	 * handle it before exception entry to avoid recursive WARN
 	 * in case exception entry is the one triggering WARNs.
 	 */
-	if (!user_mode(regs) && handle_bug(regs))
-		return;
+	if (!user_mode(regs)) {
+		if (handle_bug(regs))
+			return;
+
+		if (handle_serialize(regs))
+			return;
+	}
 
 	state = idtentry_enter(regs);
 	instrumentation_begin();

Re: [PATCH 4/4] x86/cpu: Use SERIALIZE in sync_core() when available

[peterz]

On Mon, Jul 27, 2020 at 03:05:36PM +0200, peterz@infradead.org wrote:
> Yeah, I'm not sure.. the 'funny' thing is that typically call
> sync_core() from an IPI anyway. And the synchronous broadcast IPI is by
> far the most expensive part of that.
> 
> Something like this...
> 
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 20e07feb4064..528e049ee1d9 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -989,12 +989,13 @@ void *text_poke_kgdb(void *addr, const void *opcode, size_t len)
>  
>  static void do_sync_core(void *info)
>  {
> -	sync_core();
> +	/* IRET implies sync_core() */
>  }
>  
>  void text_poke_sync(void)
>  {
>  	on_each_cpu(do_sync_core, NULL, 1);
> +	sync_core();
>  }
>  
>  struct text_poke_loc {

So 'people' have wanted to optimize this for NOHZ_FULL and I suppose
virt as well.

IFF VMENTER is serializing, I suppose we can simply do something like:

bool text_poke_cond(int cpu, void *info)
{
	/*
	 * If we observe the vCPU is preempted, it will do VMENTER
	 * no point in sending an IPI to SERIALIZE.
	 */
	return !vcpu_is_preempted(cpu);
}

void text_poke_sync(void)
{
	smp_call_function_many_cond(cpu_possible_mask,
				    do_sync_core, NULL, 1, text_poke_cond);
	sync_core();
}

The 'same' for NOHZ_FULL, except we need to cmpxchg a value such that
if the cmpxchg() succeeds we know the CPU is in userspace and will
SERIALIZE on the next entry. Much like kvm_flush_tlb_others().


Anyway, that's all hand-wavey.. I'll let someone that cares about those
things write actual patches :-)

RE: [PATCH 4/4] x86/cpu: Use SERIALIZE in sync_core() when available

[Luck, Tony]

> For a really overenginered solution, but which might perform unnecessary poorly on existing hardware:
>
> asm volatile("1: .byte 0xf, 0x1, 0xe8; 2:"
>                        _ASM_EXTABLE(1b,2b));

You win the prize for the smallest code.  Might need (the already large) comment to double
in size to explain the subtleties!

-Tony

Re: [PATCH 4/4] x86/cpu: Use SERIALIZE in sync_core() when available

[Ricardo Neri]

On Mon, Jul 27, 2020 at 10:20:03AM +0200, peterz@infradead.org wrote:
> On Sun, Jul 26, 2020 at 09:31:32PM -0700, Ricardo Neri wrote:
> > +static inline void serialize(void)
> > +{
> > +	asm volatile(".byte 0xf, 0x1, 0xe8");
> > +}
> 
> Can we pretty please have a comment with the binutils version that has
> the mnomic? Such that when we increase the required binutils version we
> can grep for this.

This is supported since binutils 2.35[1]. I'll add a comment with the
clarification.

Thanks and BR,
Ricardo

[1]. https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob_plain;f=gas/NEWS;;hb=refs/tags/binutils-2_35

Re: [PATCH 4/4] x86/cpu: Use SERIALIZE in sync_core() when available

[Ricardo Neri]

On Mon, Jul 27, 2020 at 05:47:32AM -0700, hpa@zytor.com wrote:
> On July 27, 2020 1:20:03 AM PDT, peterz@infradead.org wrote:
> >On Sun, Jul 26, 2020 at 09:31:32PM -0700, Ricardo Neri wrote:
> >> +static inline void serialize(void)
> >> +{
> >> +	asm volatile(".byte 0xf, 0x1, 0xe8");
> >> +}
> >
> >Can we pretty please have a comment with the binutils version that has
> >the mnomic? Such that when we increase the required binutils version we
> >can grep for this.
> 
> It also needs : : : "memory" to be a barrier.

Sure Peter, I will make this change.

Thanks and BR,
Ricardo

Re: [PATCH 4/4] x86/cpu: Use SERIALIZE in sync_core() when available

[Ricardo Neri]

On Mon, Jul 27, 2020 at 03:30:20PM +0200, peterz@infradead.org wrote:
> On Mon, Jul 27, 2020 at 03:05:36PM +0200, peterz@infradead.org wrote:
> > Yeah, I'm not sure.. the 'funny' thing is that typically call
> > sync_core() from an IPI anyway. And the synchronous broadcast IPI is by
> > far the most expensive part of that.
> > 
> > Something like this...
> > 
> > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> > index 20e07feb4064..528e049ee1d9 100644
> > --- a/arch/x86/kernel/alternative.c
> > +++ b/arch/x86/kernel/alternative.c
> > @@ -989,12 +989,13 @@ void *text_poke_kgdb(void *addr, const void *opcode, size_t len)
> >  
> >  static void do_sync_core(void *info)
> >  {
> > -	sync_core();
> > +	/* IRET implies sync_core() */
> >  }
> >  
> >  void text_poke_sync(void)
> >  {
> >  	on_each_cpu(do_sync_core, NULL, 1);
> > +	sync_core();
> >  }
> >  
> >  struct text_poke_loc {
> 
> So 'people' have wanted to optimize this for NOHZ_FULL and I suppose
> virt as well.
> 
> IFF VMENTER is serializing, I suppose we can simply do something like:
> 
> bool text_poke_cond(int cpu, void *info)
> {
> 	/*
> 	 * If we observe the vCPU is preempted, it will do VMENTER
> 	 * no point in sending an IPI to SERIALIZE.
> 	 */
> 	return !vcpu_is_preempted(cpu);
> }
> 
> void text_poke_sync(void)
> {
> 	smp_call_function_many_cond(cpu_possible_mask,
> 				    do_sync_core, NULL, 1, text_poke_cond);
> 	sync_core();
> }
> 
> The 'same' for NOHZ_FULL, except we need to cmpxchg a value such that
> if the cmpxchg() succeeds we know the CPU is in userspace and will
> SERIALIZE on the next entry. Much like kvm_flush_tlb_others().
> 
> 
> Anyway, that's all hand-wavey.. I'll let someone that cares about those
> things write actual patches :-)

I think I got a little lost here. If I understand correctly, there are
two alternatives to implement support for serialize better:

  a) alternative(IRET_TO_SELF, SERIALIZE, X86_FEATURE_SERIALIZE); or
  b) asm volatile("1:.byte 0xf, 0x1, 0xe8;2:" _ASM_EXTABLE(1b:2b)

a) would be the traditional and simpler solution. b) would rely on
causing an #UD and getting an IRET on existing hardware b) would need some
more optimization work when handling the exception and a few reworks on
the poke patching code.

Which option should I focus on? Which option would be more desirable/better?

Thanks and BR,
Ricardo

Re: [PATCH 4/4] x86/cpu: Use SERIALIZE in sync_core() when available

[peterz]

On Mon, Jul 27, 2020 at 09:41:14PM -0700, Ricardo Neri wrote:
> I think I got a little lost here.

Hehe, sorry. I got carried away, it's just that recently people
expressed interest in 'fixing' some of the text_poke_sync() issues
again.

> If I understand correctly, there are
> two alternatives to implement support for serialize better:
> 
>   a) alternative(IRET_TO_SELF, SERIALIZE, X86_FEATURE_SERIALIZE); or
>   b) asm volatile("1:.byte 0xf, 0x1, 0xe8;2:" _ASM_EXTABLE(1b:2b)
> 
> a) would be the traditional and simpler solution. b) would rely on
> causing an #UD and getting an IRET on existing hardware b) would need some
> more optimization work when handling the exception and a few reworks on
> the poke patching code.
> 
> Which option should I focus on? Which option would be more desirable/better?

I'd say go with (a) for now. We can always go overboard later ;-)