On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote: > From: Tim Chen <tim.c.c...@linux.intel.com> > From: Andrea Arcangeli <aarca...@redhat.com>
There's Co-Developed-by for this: - Co-Developed-by: states that the patch was also created by another developer along with the original author. This is useful at times when multiple people work on a single patch. Note, this person also needs to have a Signed-off-by: line in the patch as well. > 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. s/cpu/CPU/ > > 2. At run time > echo 0 > /sys/kernel/debug/x86/ibrs_enabled will turn off IBRS > echo 1 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in kernel > echo 2 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in both > userspace and kernel > > The implementation was updated with input and suggestions from Andrea > Arcangeli. > > Signed-off-by: Tim Chen <tim.c.c...@linux.intel.com> > --- > arch/x86/entry/calling.h | 42 ++++++++-- > arch/x86/include/asm/spec_ctrl.h | 15 ++++ > arch/x86/kernel/cpu/Makefile | 1 + > arch/x86/kernel/cpu/scattered.c | 2 + > arch/x86/kernel/cpu/spec_ctrl.c | 160 > +++++++++++++++++++++++++++++++++++++++ > 5 files changed, 214 insertions(+), 6 deletions(-) > create mode 100644 arch/x86/include/asm/spec_ctrl.h > create mode 100644 arch/x86/kernel/cpu/spec_ctrl.c > > diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h > index 09c870d..6b65d47 100644 > --- a/arch/x86/entry/calling.h > +++ b/arch/x86/entry/calling.h > @@ -373,35 +373,55 @@ For 32-bit we have the following conventions - kernel > is built with > .endm > > .macro ENABLE_IBRS > - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL > + testl $1, dynamic_ibrs > + jz .Lskip_\@ > + > 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 > - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL > + testl $1, dynamic_ibrs > + jz .Lskip_\@ > + > PUSH_MSR_REGS > WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS > POP_MSR_REGS > + > .Lskip_\@: > .endm > > .macro ENABLE_IBRS_CLOBBER > - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL > + testl $1, dynamic_ibrs > + jz .Lskip_\@ > + > WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS > + jmp .Ldone_\@ > + > .Lskip_\@: > + lfence > +.Ldone_\@: > .endm > > .macro DISABLE_IBRS_CLOBBER > - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL > + testl $1, dynamic_ibrs > + jz .Lskip_\@ > + > WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS > + > .Lskip_\@: > .endm > > .macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req > - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL > + testl $1, dynamic_ibrs > + jz .Lskip_\@ > + > movl $MSR_IA32_SPEC_CTRL, %ecx > rdmsr > movl %eax, \save_reg > @@ -409,15 +429,25 @@ For 32-bit we have the following conventions - kernel > is built with > movl $0, %edx > movl $SPEC_CTRL_FEATURE_ENABLE_IBRS, %eax > wrmsr > + jmp .Ldone_\@ > + > .Lskip_\@: > + lfence > +.Ldone_\@: > .endm > > .macro RESTORE_IBRS_CLOBBER save_reg:req > - ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL > + testl $1, dynamic_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 > + jmp .Ldone_\@ > + > .Lskip_\@: > + lfence > +.Ldone_\@: > .endm Why not put all macros in spec_ctrl.h ? > diff --git a/arch/x86/include/asm/spec_ctrl.h > b/arch/x86/include/asm/spec_ctrl.h > new file mode 100644 > index 0000000..4fda38b > --- /dev/null > +++ b/arch/x86/include/asm/spec_ctrl.h > @@ -0,0 +1,15 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#ifndef _ASM_X86_SPEC_CTRL_H > +#define _ASM_X86_SPEC_CTRL_H > + > +#include <asm/msr-index.h> > +#include <asm/cpufeatures.h> > +#include <asm/microcode.h> Left over include from a previous version. Instead, you need #include <linux/cpu.h> in spec_ctrl.c for get/put_online_cpus(). > +void scan_spec_ctrl_feature(struct cpuinfo_x86 *c); > +bool ibrs_inuse(void); > + > +extern unsigned int dynamic_ibrs; > + > +#endif /* _ASM_X86_SPEC_CTRL_H */ > diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile > index 570e8bb..3ffbd24 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-y += 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..5756d14 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; > @@ -57,6 +58,7 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c) > if (regs[cb->reg] & (1 << cb->bit)) > set_cpu_cap(c, cb->feature); > } > + scan_spec_ctrl_feature(c); Hell no! This function is only for setting the feature bits. > u32 get_scattered_cpuid_leaf(unsigned int level, unsigned int sub_leaf, > diff --git a/arch/x86/kernel/cpu/spec_ctrl.c b/arch/x86/kernel/cpu/spec_ctrl.c > new file mode 100644 > index 0000000..1641bec > --- /dev/null > +++ b/arch/x86/kernel/cpu/spec_ctrl.c > @@ -0,0 +1,160 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#include <linux/mutex.h> > +#include <linux/debugfs.h> > +#include <linux/uaccess.h> > + > +#include <asm/spec_ctrl.h> > +#include <asm/cpufeature.h> > + > +unsigned int dynamic_ibrs __read_mostly; WTH is dynamic_ibrs? > +EXPORT_SYMBOL_GPL(dynamic_ibrs); > + > +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, > +}; > +static unsigned int ibrs_enabled; > +static bool ibrs_admin_disabled; Srsly?! That's *three* variables controlling IBRS. This needs simplification. > + > +/* mutex to serialize IBRS control changes */ > +DEFINE_MUTEX(spec_ctrl_mutex); static > +void scan_spec_ctrl_feature(struct cpuinfo_x86 *c) > +{ > + if ((!c->cpu_index) && (boot_cpu_has(X86_FEATURE_SPEC_CTRL))) { What is !c->cpu_index? Checking whether this is the BSP? What for? > + if (!ibrs_admin_disabled) { > + dynamic_ibrs = 1; > + ibrs_enabled = IBRS_ENABLED; > + } > + } > +} > +EXPORT_SYMBOL_GPL(scan_spec_ctrl_feature); > + > +/* > + * Used after boot phase to rescan spec_ctrl feature, > + * serialize scan with spec_ctrl_mutex. > + */ > +void rescan_spec_ctrl_feature(struct cpuinfo_x86 *c) > +{ > + mutex_lock(&spec_ctrl_mutex); > + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) { > + if (!ibrs_admin_disabled) { > + dynamic_ibrs = 1; > + ibrs_enabled = IBRS_ENABLED; > + } > + } > + mutex_unlock(&spec_ctrl_mutex); > +} > +EXPORT_SYMBOL_GPL(rescan_spec_ctrl_feature); > + > +bool ibrs_inuse(void) > +{ > + return ibrs_enabled == IBRS_ENABLED; > +} So from looking at this, you can have a single ibrs_state variable and set bits in it and have *all* possible settings you wanna have. > +EXPORT_SYMBOL_GPL(ibrs_inuse); That naming is not really correct: otherwise IBRS_ENABLED_USER means ibrs is *not* in use. > + > +static int __init noibrs(char *str) > +{ > + ibrs_admin_disabled = true; > + ibrs_enabled = IBRS_DISABLED; > + > + return 0; > +} > +early_param("noibrs", noibrs); > + > +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); > +} > + > +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; > + > + if (enable > IBRS_MAX) > + return -EINVAL; > + > + if (!boot_cpu_has(X86_FEATURE_SPEC_CTRL)) { That capability check needs to happen first: no need to do anything if the CPU doesn't have IBRS. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.