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

[Tim Chen]

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