Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Peter Zijlstra]

On Mon, Jan 08, 2018 at 01:47:07PM +0100, Peter Zijlstra wrote:
> > a good suggestion, but we encountered some issues with it either
> > crashing the kernel at boot or not properly turning on/off.

The below boots, but I lack stuff to test the enabling.

--- a/arch/x86/include/asm/spec_ctrl.h
+++ b/arch/x86/include/asm/spec_ctrl.h
@@ -9,9 +9,8 @@
 
 void scan_spec_ctrl_feature(struct cpuinfo_x86 *c);
 void rescan_spec_ctrl_feature(struct cpuinfo_x86 *c);
-bool ibrs_inuse(void);
 
-extern unsigned int dynamic_ibrs;
+DECLARE_STATIC_KEY_FALSE(ibrs_key);
 
 static inline void __disable_indirect_speculation(void)
 {
@@ -31,21 +30,14 @@ static inline void __enable_indirect_spe
 static inline void unprotected_speculation_begin(void)
 {
 	lockdep_assert_irqs_disabled();
-	if (dynamic_ibrs)
+	if (static_branch_unlikely(&ibrs_key))
 		__enable_indirect_speculation();
 }
 
 static inline void unprotected_speculation_end(void)
 {
-	if (dynamic_ibrs) {
+	if (static_branch_unlikely(&ibrs_key))
 		__disable_indirect_speculation();
-	} else {
-		/*
-		 * rmb prevent unwanted speculation when we
-		 * are setting IBRS
-		 */
-		rmb();
-	}
 }
 
 #endif /* _ASM_X86_SPEC_CTRL_H */
--- a/arch/x86/kernel/cpu/spec_ctrl.c
+++ b/arch/x86/kernel/cpu/spec_ctrl.c
@@ -7,8 +7,7 @@
 #include <asm/spec_ctrl.h>
 #include <asm/cpufeature.h>
 
-unsigned int dynamic_ibrs __read_mostly;
-EXPORT_SYMBOL_GPL(dynamic_ibrs);
+DEFINE_STATIC_KEY_FALSE(ibrs_key);
 
 enum {
 	IBRS_DISABLED,
@@ -27,7 +26,7 @@ DEFINE_MUTEX(spec_ctrl_mutex);
 static inline void set_ibrs_feature(void)
 {
 	if (!ibrs_admin_disabled) {
-		dynamic_ibrs = 1;
+		static_branch_enable(&ibrs_key);
 		ibrs_enabled = IBRS_ENABLED;
 	}
 }
@@ -54,12 +53,6 @@ void rescan_spec_ctrl_feature(struct cpu
 }
 EXPORT_SYMBOL_GPL(rescan_spec_ctrl_feature);
 
-bool ibrs_inuse(void)
-{
-	return ibrs_enabled == IBRS_ENABLED;
-}
-EXPORT_SYMBOL_GPL(ibrs_inuse);
-
 static int __init noibrs(char *str)
 {
 	ibrs_admin_disabled = true;
@@ -124,19 +117,23 @@ static ssize_t ibrs_enabled_write(struct
 	if (enable == IBRS_DISABLED) {
 		/* disable IBRS usage */
 		ibrs_admin_disabled = true;
-		dynamic_ibrs = 0;
+		static_branch_disable(&ibrs_key);
 		spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL,
 					 SPEC_CTRL_FEATURE_DISABLE_IBRS);
 
 	} else if (enable == IBRS_ENABLED) {
 		/* enable IBRS usage in kernel */
 		ibrs_admin_disabled = false;
-		dynamic_ibrs = 1;
+		static_branch_enable(&ibrs_key);
+		/*
+		 * This too needs an IPI to force all active CPUs into a known state.
+		 */
 
 	} else if (enable == IBRS_ENABLED_USER) {
 		/* enable IBRS all the time in both userspace and kernel */
 		ibrs_admin_disabled = false;
-		dynamic_ibrs = 0;
+		static_branch_disable(&ibrs_key);
+
 		spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL,
 					 SPEC_CTRL_FEATURE_ENABLE_IBRS);
 	}
--- a/arch/x86/entry/calling.h
+++ b/arch/x86/entry/calling.h
@@ -373,22 +373,17 @@ For 32-bit we have the following convent
 .endm
 
 .macro ENABLE_IBRS
-	testl	$1, dynamic_ibrs
-	jz	.Lskip_\@
+	STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0
 
 	PUSH_MSR_REGS
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
 	POP_MSR_REGS
-	jmp	.Ldone_\@
 
 .Lskip_\@:
-	lfence
-.Ldone_\@:
 .endm
 
 .macro DISABLE_IBRS
-	testl	$1, dynamic_ibrs
-	jz	.Lskip_\@
+	STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0
 
 	PUSH_MSR_REGS
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
@@ -398,20 +393,15 @@ For 32-bit we have the following convent
 .endm
 
 .macro ENABLE_IBRS_CLOBBER
-	testl	$1, dynamic_ibrs
-	jz	.Lskip_\@
+	STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0
 
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
-	jmp	.Ldone_\@
 
 .Lskip_\@:
-	lfence
-.Ldone_\@:
 .endm
 
 .macro DISABLE_IBRS_CLOBBER
-	testl	$1, dynamic_ibrs
-	jz	.Lskip_\@
+	STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0
 
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
 
@@ -419,8 +409,7 @@ For 32-bit we have the following convent
 .endm
 
 .macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req
-	testl	$1, dynamic_ibrs
-	jz	.Lskip_\@
+	STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0
 
 	movl	$MSR_IA32_SPEC_CTRL, %ecx
 	rdmsr
@@ -429,25 +418,18 @@ For 32-bit we have the following convent
 	movl	$0, %edx
 	movl	$SPEC_CTRL_FEATURE_ENABLE_IBRS, %eax
 	wrmsr
-	jmp	.Ldone_\@
 
 .Lskip_\@:
-	lfence
-.Ldone_\@:
 .endm
 
 .macro RESTORE_IBRS_CLOBBER save_reg:req
-	testl	$1, dynamic_ibrs
-	jz	.Lskip_\@
+	STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0
 
 	/* Set IBRS to the value saved in the save_reg */
 	movl    $MSR_IA32_SPEC_CTRL, %ecx
 	movl    $0, %edx
 	movl    \save_reg, %eax
 	wrmsr
-	jmp	.Ldone_\@
 
 .Lskip_\@:
-	lfence
-.Ldone_\@:
 .endm