Re: [PATCH 6/7] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Jan 04, 2018 at 09:56:47AM -0800, Tim Chen wrote:
> There are 2 ways to control IBRS
> 
> 1. At boot time
>     noibrs kernel boot parameter will disable IBRS usage
> 
> Otherwise if the above parameters are not specified, the system
> will enable ibrs and ibpb usage if the cpu supports it.
> 
> 2. At run time
>     echo 0 > /sys/kernel/debug/ibrs_enabled will turn off IBRS
>     echo 1 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in kernel
>     echo 2 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in both userspace and kernel
> 
> The implementation was updated with input from Andrea Arcangeli.
> 
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |   4 +
>  arch/x86/include/asm/spec_ctrl.h                | 163 +++++++++++++++++++-----
>  arch/x86/kernel/cpu/Makefile                    |   1 +
>  arch/x86/kernel/cpu/scattered.c                 |  10 ++
>  arch/x86/kernel/cpu/spec_ctrl.c                 | 124 ++++++++++++++++++
>  5 files changed, 270 insertions(+), 32 deletions(-)
>  create mode 100644 arch/x86/kernel/cpu/spec_ctrl.c
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 5dfd262..d64f49f 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2573,6 +2573,10 @@
>  			noexec=on: enable non-executable mappings (default)
>  			noexec=off: disable non-executable mappings
>  
> +	noibrs          [X86]
> +			Don't use indirect branch restricted speculation (IBRS)
> +			feature.
> +
>  	nosmap		[X86]
>  			Disable SMAP (Supervisor Mode Access Prevention)
>  			even if it is supported by processor.
> diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h
> index 23b2804..2c35571 100644
> --- a/arch/x86/include/asm/spec_ctrl.h
> +++ b/arch/x86/include/asm/spec_ctrl.h
> @@ -1,13 +1,17 @@
>  #ifndef _ASM_X86_SPEC_CTRL_H
>  #define _ASM_X86_SPEC_CTRL_H
>  
> -#include <linux/stringify.h>
>  #include <asm/msr-index.h>
>  #include <asm/cpufeatures.h>
> -#include <asm/alternative-asm.h>
> +
> +#define SPEC_CTRL_IBRS_INUSE      	(1<<0)	/* OS enables IBRS usage */
> +#define SPEC_CTRL_IBRS_SUPPORTED	(1<<1)  /* System supports IBRS */
> +#define SPEC_CTRL_IBRS_ADMIN_DISABLED	(1<<2)  /* Admin disables IBRS */
>  
>  #ifdef __ASSEMBLY__
>  
> +.extern spec_ctrl_ibrs
> +
>  .macro PUSH_MSR_REGS
>  	pushq %rax
>  	pushq %rcx
> @@ -27,35 +31,63 @@
>  .endm
>  
>  .macro ENABLE_IBRS
> -	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
> +	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
> +	jz .Lskip_\@
> +
>  	PUSH_MSR_REGS
>  	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
>  	POP_MSR_REGS
> -10:
> +
> +	jmp .Ldone_\@
> +.Lskip_\@:
> +	/*
> +	 * prevent speculation beyond here as we could want to
> +	 * stop speculation by enabling IBRS
> +	 */
> +	lfence
> +.Ldone_\@:
>  .endm
>  
>  .macro DISABLE_IBRS
> -	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
> +	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
> +	jz .Lskip_\@
> +
>  	PUSH_MSR_REGS
>  	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
>  	POP_MSR_REGS
> -10:
> +
> +.Lskip_\@:
>  .endm
>  
>  .macro ENABLE_IBRS_CLOBBER
> -	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
> +	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
> +	jz .Lskip_\@
> +
>  	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
> -10:
> +
> +	jmp .Ldone_\@
> +.Lskip_\@:
> +	/*
> +	 * prevent speculation beyond here as we could want to
> +	 * stop speculation by enabling IBRS
> +	 */
> +	lfence
> +.Ldone_\@:
>  .endm
>  
>  .macro DISABLE_IBRS_CLOBBER
> -	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
> +	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
> +	jz .Lskip_\@
> +
>  	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
> -10:
> +
> +.Lskip_\@:
>  .endm
>  
>  .macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req
> -	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
> +	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
> +	jz .Lskip_\@
> +
>  	movl	$MSR_IA32_SPEC_CTRL, %ecx
>  	rdmsr
>  	movl	%eax, \save_reg
> @@ -63,22 +95,103 @@
>  	movl	$0, %edx
>  	movl	$SPEC_CTRL_FEATURE_ENABLE_IBRS, %eax
>  	wrmsr
> -10:
> +
> +	jmp .Ldone_\@
> +.Lskip_\@:
> +	/*
> +	 * prevent speculation beyond here as we could want to
> +	 * stop speculation by enabling IBRS
> +	 */
> +	lfence
> +.Ldone_\@:
>  .endm
>  
>  .macro RESTORE_IBRS_CLOBBER save_reg:req
> -	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
> +	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
> +	jz .Lskip_\@
> +
>  	/* Set IBRS to the value saved in the save_reg */
>  	movl    $MSR_IA32_SPEC_CTRL, %ecx
>          movl    $0, %edx
>  	movl    \save_reg, %eax
>  	wrmsr
> -10:
> +
> +	jmp .Ldone_\@
> +.Lskip_\@:
> +	/*
> +	 * prevent speculation beyond here as we could want to
> +	 * stop speculation by enabling IBRS
> +	 */
> +	lfence
> +.Ldone_\@:
>  .endm
>  
>  #else
>  #include <asm/microcode.h>
>  
> +extern int spec_ctrl_ibrs;
> +extern struct mutex spec_ctrl_mutex;
> +extern unsigned int ibrs_enabled;
> +
> +enum {
> +        IBRS_DISABLED,
> +        /* in host kernel, disabled in guest and userland */
> +        IBRS_ENABLED,
> +        /* in host kernel and host userland, disabled in guest */
> +        IBRS_ENABLED_USER,
> +        IBRS_MAX = IBRS_ENABLED_USER,

Yup, not a bit field, your documentation is wrong :(

> +};
> +
> +
> +static inline void set_ibrs_inuse(void)
> +{
> +	if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
> +		spec_ctrl_ibrs |= SPEC_CTRL_IBRS_INUSE;
> +}
> +
> +static inline void clear_ibrs_inuse(void)
> +{
> +	spec_ctrl_ibrs &= ~SPEC_CTRL_IBRS_INUSE;
> +}
> +
> +static inline int ibrs_inuse(void)
> +{
> +	if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_INUSE)
> +		return 1;
> +	else
> +		/*
> +		 * prevent speculation beyond here as we could want to
> +		 * stop speculation by enabling IBRS with this check
> +		 */
> +		rmb();
> +	return 0;
> +}
> +
> +static inline void set_ibrs_supported(void)
> +{
> +	spec_ctrl_ibrs |= SPEC_CTRL_IBRS_SUPPORTED;
> +	if (!(spec_ctrl_ibrs & SPEC_CTRL_IBRS_ADMIN_DISABLED))
> +		set_ibrs_inuse();
> +	else
> +		/*
> +		 * prevent speculation beyond here as we could want to
> +		 * stop speculation by enabling IBRS
> +		 */
> +		rmb();
> +}
> +
> +static inline void set_ibrs_disabled(void)
> +{
> +	spec_ctrl_ibrs |= SPEC_CTRL_IBRS_ADMIN_DISABLED;
> +	if (ibrs_inuse())
> +		clear_ibrs_inuse();
> +}
> +
> +static inline void clear_ibrs_disabled(void)
> +{
> +	spec_ctrl_ibrs &= ~SPEC_CTRL_IBRS_ADMIN_DISABLED;
> +}
> +
>  static inline void __disable_indirect_speculation(void)
>  {
>          native_wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS);
> @@ -96,21 +209,14 @@ static inline void __enable_indirect_speculation(void)
>  static inline void unprotected_speculation_begin(void)
>  {
>  	WARN_ON_ONCE(!irqs_disabled());
> -	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
> +	if (ibrs_inuse())
>  		__enable_indirect_speculation();
>  }
>  
>  static inline void unprotected_speculation_end(void)
>  {
> -	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
> +	if (ibrs_inuse())
>  		__disable_indirect_speculation();
> -	else
> -		/*
> -		 * If we intended to disable indirect speculation
> -		 * but come here due to mis-speculation, we need
> -		 * to stop the mis-speculation with rmb.
> -		 */
> -		rmb();
>  }
>  
>  
> @@ -121,20 +227,13 @@ static inline void unprotected_speculation_end(void)
>   */
>  static inline void unprotected_firmware_begin(void)
>  {
> -	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
> +	if (ibrs_inuse())
>                  __disable_indirect_speculation();
> -	else
> -		/*
> -		 * If we intended to disable indirect speculation
> -		 * but come here due to mis-speculation, we need
> -		 * to stop the mis-speculation with rmb.
> -		 */
> -		rmb();
>  }
>  
>  static inline void unprotected_firmware_end(void)
>  {
> -	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
> +	if (ibrs_inuse())
>                  __enable_indirect_speculation();
>  }
>  
> diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
> index 90cb82d..a25f1ab 100644
> --- a/arch/x86/kernel/cpu/Makefile
> +++ b/arch/x86/kernel/cpu/Makefile
> @@ -24,6 +24,7 @@ obj-y			+= match.o
>  obj-y			+= bugs.o
>  obj-$(CONFIG_CPU_FREQ)	+= aperfmperf.o
>  obj-y			+= cpuid-deps.o
> +obj-y			+= spec_ctrl.o
>  
>  obj-$(CONFIG_PROC_FS)	+= proc.o
>  obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o
> diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
> index bc50c40..2d23a2fe 100644
> --- a/arch/x86/kernel/cpu/scattered.c
> +++ b/arch/x86/kernel/cpu/scattered.c
> @@ -8,6 +8,7 @@
>  #include <asm/processor.h>
>  
>  #include <asm/apic.h>
> +#include <asm/spec_ctrl.h>
>  
>  struct cpuid_bit {
>  	u16 feature;
> @@ -56,6 +57,15 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)
>  
>  		if (regs[cb->reg] & (1 << cb->bit))
>  			set_cpu_cap(c, cb->feature);
> +
> +	}
> +	if (!c->cpu_index) {
> +		if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
> +			printk(KERN_INFO "FEATURE SPEC_CTRL Present\n");
> +			set_ibrs_supported();
> +			if (ibrs_inuse())
> +				ibrs_enabled = IBRS_ENABLED;
> +		}
>  	}
>  }
>  
> diff --git a/arch/x86/kernel/cpu/spec_ctrl.c b/arch/x86/kernel/cpu/spec_ctrl.c
> new file mode 100644
> index 0000000..6946678
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/spec_ctrl.c
> @@ -0,0 +1,124 @@
> +#include <linux/export.h>

No copyright or SPDX line?

At least it doesn't have the old horrid Intel header boilerplate, so I
should be thankful of that.  But it isn't ok like this either, sorry.

> +#include <linux/init.h>
> +#include <linux/mutex.h>
> +#include <linux/debugfs.h>
> +#include <linux/uaccess.h>
> +
> +#include <asm/spec_ctrl.h>
> +#include <asm/cpufeature.h>
> +
> +/*
> + * spec_ctrl_ibrs
> + * bit 0 = indicate if ibrs is currently in use
> + * bit 1 = indicate if system supports ibrs
> + * bit 2 = indicate if admin disables ibrs

Why bits and not integer values?  Can you mix them?

> + */
> +
> +int spec_ctrl_ibrs;
> +EXPORT_SYMBOL(spec_ctrl_ibrs);

Why is this exported?  What module will ever need this?

And horrid global symbol name, it doesn't say what it is in an obvious
way just by looking at it.

> +
> +/* mutex to serialize IBRS control changes */
> +DEFINE_MUTEX(spec_ctrl_mutex);
> +EXPORT_SYMBOL(spec_ctrl_mutex);

Why export this?  You only ever use it in one place?  Again, what module
needs it?

And I have to ask, EXPORT_SYMBOL_GPL() for this and spec_ctrl_ibrs as
well?

> +unsigned int ibrs_enabled __read_mostly;
> +EXPORT_SYMBOL(ibrs_enabled);

Again, what module needs this?

And did __read_mostly really matter in performance tests?

> +
> +static int __init noibrs(char *str)
> +{
> +        set_ibrs_disabled();
> +
> +        return 0;
> +}
> +early_param("noibrs", ex);
> +
> +static ssize_t __enabled_read(struct file *file, char __user *user_buf,
> +                              size_t count, loff_t *ppos, unsigned int *field)
> +{
> +	char buf[32];
> +	unsigned int len;
> +
> +	len = sprintf(buf, "%d\n", READ_ONCE(*field));
> +	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> +}
> +
> +static ssize_t ibrs_enabled_read(struct file *file, char __user *user_buf,
> +                                 size_t count, loff_t *ppos)
> +{
> +	return __enabled_read(file, user_buf, count, ppos, &ibrs_enabled);
> +}

It's a single variable, why such complex debugfs for that?  This should
be handled by a helper macro already, right?

> +static void spec_ctrl_flush_all_cpus(u32 msr_nr, u64 val)
> +{
> +	int cpu;
> +	get_online_cpus();
> +	for_each_online_cpu(cpu)
> +		wrmsrl_on_cpu(cpu, msr_nr, val);
> +	put_online_cpus();
> +}
> +
> +static ssize_t ibrs_enabled_write(struct file *file,
> +                                  const char __user *user_buf,
> +                                  size_t count, loff_t *ppos)
> +{
> +        char buf[32];
> +        ssize_t len;
> +        unsigned int enable;
> +
> +        len = min(count, sizeof(buf) - 1);
> +        if (copy_from_user(buf, user_buf, len))
> +                return -EFAULT;
> +
> +        buf[len] = '\0';
> +        if (kstrtouint(buf, 0, &enable))
> +                return -EINVAL;

3 value "flags" are horrid :(

> +
> +        if (enable > IBRS_MAX)
> +                return -EINVAL;
> +
> +	mutex_lock(&spec_ctrl_mutex);

Always run checkpatch.pl so you don't get grumpy kernel maintainers
telling you to run checkpatch.pl :(

> +
> +	if (enable == IBRS_DISABLED) {
> +		/* disable IBRS usage */
> +		set_ibrs_disabled();
> +		if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
> +			spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_DISABLE_IBRS);
> +	} else if (enable == IBRS_ENABLED) {
> +		/* enable IBRS usage in kernel */
> +		clear_ibrs_disabled();
> +		if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
> +			set_ibrs_inuse();
> +		else
> +			/* Platform don't support IBRS */
> +			enable = IBRS_DISABLED;
> +	} else if (enable == IBRS_ENABLED_USER) {
> +		/* enable IBRS usage in both userspace and kernel */
> +		clear_ibrs_disabled();
> +		/* don't change IBRS value once we set it to always on */
> +		clear_ibrs_inuse();
> +		if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
> +			spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS);
> +		else
> +			/* Platform don't support IBRS */
> +			enable = IBRS_DISABLED;
> +	}
> +
> +	WRITE_ONCE(ibrs_enabled, enable);

It's a debugfs write callback, why do you care about WRITE_ONCE()?

thanks,

greg k-h