[MODERATED] Re: [patch V6 04/14] MDS basics 4

On Fri, Mar 01, 2019 at 10:47:42PM +0100, speck for Thomas Gleixner wrote:
> Subject: [patch V6 04/14] x86/speculation/mds: Add BUG_MSBDS_ONLY
> From: Thomas Gleixner <[email protected]>
> 
> This bug bit is set on CPUs which are only affected by Microarchitectural
> Store Buffer Data Sampling (MSBDS) and not by any other MDS variant.
> 
> This is important because the Store Buffers are partitioned between
> Hyper-Threads so cross thread forwarding is not possible. But if a thread
> enters or exits a sleep state the store buffer is repartitioned which can
> expose data from one thread to the other. This transition can be mitigated.
> 
> That means that for CPUs which are only affected by MSBDS SMT can be
> enabled, if the CPU is not affected by other SMT sensitive vulnerabilities,
> e.g. L1TF. The XEON PHI variants fall into that category.
> 
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
>  arch/x86/include/asm/cpufeatures.h |    1 +
>  arch/x86/kernel/cpu/common.c       |   10 +++++++---
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -383,5 +383,6 @@
>  #define X86_BUG_SPEC_STORE_BYPASS	X86_BUG(17) /* CPU is affected by speculative store bypass attack */
>  #define X86_BUG_L1TF			X86_BUG(18) /* CPU is affected by L1 Terminal Fault */
>  #define X86_BUG_MDS			X86_BUG(19) /* CPU is affected by Microarchitectural data sampling */
> +#define X86_BUG_MSBDS_ONLY		X86_BUG(20) /* CPU is only affected by the  MSDBS variant of BUG_MDS */
>  
>  #endif /* _ASM_X86_CPUFEATURES_H */
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -953,6 +953,7 @@ static void identify_cpu_without_cpuid(s
>  #define NO_SSB		BIT(2)
>  #define NO_L1TF		BIT(3)
>  #define NO_MDS		BIT(4)
> +#define MSBDS_ONLY	BIT(5)
>  
>  #define VULNWL(_vendor, _family, _model, _whitelist)	\
>  	{ X86_VENDOR_##_vendor, _family, _model, X86_FEATURE_ANY, _whitelist }
> @@ -983,8 +984,8 @@ static const __initconst struct x86_cpu_
>  	VULNWL_INTEL(ATOM_SILVERMONT_X,		NO_SSB | NO_L1TF),
>  	VULNWL_INTEL(ATOM_SILVERMONT_MID,	NO_SSB | NO_L1TF),
>  	VULNWL_INTEL(ATOM_AIRMONT,		NO_SSB | NO_L1TF),
> -	VULNWL_INTEL(XEON_PHI_KNL,		NO_SSB | NO_L1TF),
> -	VULNWL_INTEL(XEON_PHI_KNM,		NO_SSB | NO_L1TF),
> +	VULNWL_INTEL(XEON_PHI_KNL,		NO_SSB | NO_L1TF | MSBDS_ONLY),
> +	VULNWL_INTEL(XEON_PHI_KNM,		NO_SSB | NO_L1TF | MSBDS_ONLY),
>  
>  	VULNWL_INTEL(CORE_YONAH,		NO_SSB),
>  
> @@ -1033,8 +1034,11 @@ static void __init cpu_set_bug_bits(stru
>  	if (ia32_cap & ARCH_CAP_IBRS_ALL)
>  		setup_force_cpu_cap(X86_FEATURE_IBRS_ENHANCED);
>  
> -	if (!cpu_matches(NO_MDS) && !(ia32_cap & ARCH_CAP_MDS_NO))
> +	if (!cpu_matches(NO_MDS) && !(ia32_cap & ARCH_CAP_MDS_NO)) {
>  		setup_force_cpu_bug(X86_BUG_MDS);
> +		if (cpu_matches(MSBDS_ONLY))
> +			setup_force_cpu_bug(X86_BUG_MSBDS_ONLY);
> +	}
>  
>  	if (cpu_matches(NO_MELTDOWN))
>  		return;
> 

It looks weird to have it as a separate bug flag and not as a subset of full
MDS such as:

    #define NO_IDLE_SHARED_MDS BIT(4)
    #define NO_SHARED_MDS      BIT(5)
    #define NO_MDS             (NO_IDLE_SHARED_MDS | NO_SHARED_MDS)

Now that would probably make sense only if the mitigation of full MDS required
to also imply a VERW before entering idle (that's the mitigation of MSBDS_ONLY, right?).
Turning off SMT removes the need to do that so the layout seem to make sense as is.

Reviewed-by: Frederic Weisbecker <[email protected]>