[MODERATED] Re: [PATCH v5 04/27] MDSv5 15

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Jan 18, 2019 at 04:50:19PM -0800, speck for Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> Subject:  x86/speculation/mds: Support mds=full
> 
> Support a new command line option to support unconditional flushing
> on each kernel exit. This is not enabled by default.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> 
> ---
> 
> v2: Don't enable mds=full for MDS_NO because it will be a nop.

> ---
>  Documentation/admin-guide/kernel-parameters.txt | 5 +++++
>  arch/x86/entry/common.c                         | 7 ++++++-
>  arch/x86/include/asm/clearcpu.h                 | 2 ++
>  arch/x86/kernel/cpu/bugs.c                      | 5 +++++
>  4 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 9c967d0caeca..5f5a8808c475 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2360,6 +2360,11 @@
>  	mds=off		[X86, Intel]
>  			Disable workarounds for Micro-architectural Data Sampling.
>  
> +	mds=full	[X86, Intel]
> +			Always flush cpu buffers when exiting kernel for MDS.

.. which implies that the microcode must be loaded. But right now you could do
'mds=full' on a machine _without_ the microcode and it would just do 'verw'.

And that unpatched 'verw' would most certainly _not_ flush CPU buffers. See below
in  mds_select_mitigation

> +			Normally the kernel decides dynamically when flushing is
> +			needed or not.

Can you follow the same standard as 'ssbd' and 'l1tf' - which is that this turns
in 'mds=[off,full] and then each one has an explanation please?

> +
>  	mem=nn[KMG]	[KNL,BOOT] Force usage of a specific amount of memory
>  			Amount of memory to be used when the kernel is not able
>  			to see the whole system memory or for test.
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index 924f8dab2068..66c08e1d493a 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -173,7 +173,9 @@ static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags)
>  
>  		if (cached_flags & _TIF_CLEAR_CPU) {
>  			clear_thread_flag(TIF_CLEAR_CPU);
> -			clear_cpu();
> +			/* Don't do it twice if forced */
> +			if (!static_key_enabled(&force_cpu_clear))
> +				clear_cpu();
>  		}
>  
>  		/* Disable IRQs and retry */
> @@ -217,6 +219,9 @@ __visible inline void prepare_exit_to_usermode(struct pt_regs *regs)
>  	ti->status &= ~(TS_COMPAT|TS_I386_REGS_POKED);
>  #endif
>  
> +	if (static_key_enabled(&force_cpu_clear))
> +		clear_cpu();
> +
>  	user_enter_irqoff();
>  }
>  
> diff --git a/arch/x86/include/asm/clearcpu.h b/arch/x86/include/asm/clearcpu.h
> index 530ef619ac1b..3b8ee76b9c07 100644
> --- a/arch/x86/include/asm/clearcpu.h
> +++ b/arch/x86/include/asm/clearcpu.h
> @@ -20,4 +20,6 @@ static inline void clear_cpu(void)
>  		[kernelds] "m" (kernel_ds));
>  }
>  
> +DECLARE_STATIC_KEY_FALSE(force_cpu_clear);

'force_cpu_clear' sounds quite vague. As in in three months I will not remember the name
of this. Perhaps 'force_verw' ? Or 'force_mds_verw'?


> +
>  #endif
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 2fd8faa7e23a..ce0e367753ff 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -1061,11 +1061,16 @@ early_param("l1tf", l1tf_cmdline);
>  
>  #undef pr_fmt
>  
> +DEFINE_STATIC_KEY_FALSE(force_cpu_clear);
> +
>  static void mds_select_mitigation(void)
>  {
>  	if (cmdline_find_option_bool(boot_command_line, "mds=off") ||
>  		!boot_cpu_has_bug(X86_BUG_MDS))
>  		setup_force_cpu_cap(X86_FEATURE_NO_VERW);
> +	if (cmdline_find_option_bool(boot_command_line, "mds=full") &&
> +		boot_cpu_has_bug(X86_BUG_MDS))
> +		static_branch_enable(&force_cpu_clear);

The 'mds=full' can be done on machines without the new microcode and it sets MDS
    (twice) and also does 'VERW' without any benefit. 

Why not make this:

if (!boot_cpu_has_bug(X86_BUG_MDS) {
	setup_force_cpu_cap(X86_FEATURE_NO_VERW);
	return;
} else {
	if (cmdline_find_option_bool(boot_command_line, "mds=off"))
		setup_force_cpu_cap(X86_FEATURE_NO_VERW);
	if (cmdline_find_option_bool(boot_command_line, "mds=full") && boot_cpu_has_bug(X86_BUG_MDS))
		static_branch_enable(&force_cpu_clear);
}

?


>  }
>  
>  #ifdef CONFIG_SYSFS
> -- 
> 2.17.2