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

From: Tim Chen
Date: Tue Jan 09 2018 - 13:06:03 EST


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@xxxxxxxxxxxxxxx>
>> From: Andrea Arcangeli <aarcange@xxxxxxxxxx>
>
> 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