Messages in this thread | | | Subject | Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature | From | Tim Chen <> | Date | Tue, 9 Jan 2018 10:05:57 -0800 |
| |
On 01/08/2018 04:29 PM, Borislav Petkov wrote: > On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote: >> From: Tim Chen <tim.c.chen@linux.intel.com> >> From: Andrea Arcangeli <aarcange@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. >
Thanks. Will do.
>> .Lskip_\@: >> + lfence >> +.Ldone_\@: >> .endm > > Why not put all macros in spec_ctrl.h ?
There were a previous discussion thread in v1 patch with Peter Z and Dave. Peter and Dave prefer that all these entrance macros be consolidated in calling.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?
Dynamic ibrs because we enable the IBRS in MSR 0x48 entering the kernel and disable it when we exit the kernel.
> >> +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?
This is to ensure that we only do the operation once during boot.
Thanks.
Tim
| |