Re: [PATCH] x86/speculation: Simplify the CPU bug detection logic

[Konrad Rzeszutek Wilk]

On Tue, May 22, 2018 at 11:05:39AM +0200, Dominik Brodowski wrote:
> Only CPUs which speculate can speculate. Therefore, it seems prudent
> to test for cpu_no_speculation first and only then determine whether
> a specific speculating CPU is susceptible to store bypass speculation.
> This is underlined by all CPUs currently listed in cpu_no_speculation
> were present in cpu_no_spec_store_bypass as well.
> 
> Signed-off-by: Dominik Brodowski <linux@xxxxxxxxxxxxxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/common.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 78decc3e3067..2fcc1fbf11b0 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -942,12 +942,8 @@ static const __initconst struct x86_cpu_id cpu_no_meltdown[] = {
>  	{}
>  };
>  
> +/* Only list CPUs which speculate but are non susceptible to SSB */
>  static const __initconst struct x86_cpu_id cpu_no_spec_store_bypass[] = {
> -	{ X86_VENDOR_INTEL,	6,	INTEL_FAM6_ATOM_PINEVIEW	},
> -	{ X86_VENDOR_INTEL,	6,	INTEL_FAM6_ATOM_LINCROFT	},
> -	{ X86_VENDOR_INTEL,	6,	INTEL_FAM6_ATOM_PENWELL		},
> -	{ X86_VENDOR_INTEL,	6,	INTEL_FAM6_ATOM_CLOVERVIEW	},
> -	{ X86_VENDOR_INTEL,	6,	INTEL_FAM6_ATOM_CEDARVIEW	},
>  	{ X86_VENDOR_INTEL,	6,	INTEL_FAM6_ATOM_SILVERMONT1	},
>  	{ X86_VENDOR_INTEL,	6,	INTEL_FAM6_ATOM_AIRMONT		},
>  	{ X86_VENDOR_INTEL,	6,	INTEL_FAM6_ATOM_SILVERMONT2	},
> @@ -955,14 +951,10 @@ static const __initconst struct x86_cpu_id cpu_no_spec_store_bypass[] = {
>  	{ X86_VENDOR_INTEL,	6,	INTEL_FAM6_CORE_YONAH		},
>  	{ X86_VENDOR_INTEL,	6,	INTEL_FAM6_XEON_PHI_KNL		},
>  	{ X86_VENDOR_INTEL,	6,	INTEL_FAM6_XEON_PHI_KNM		},
> -	{ X86_VENDOR_CENTAUR,	5,					},
> -	{ X86_VENDOR_INTEL,	5,					},
> -	{ X86_VENDOR_NSC,	5,					},
>  	{ X86_VENDOR_AMD,	0x12,					},
>  	{ X86_VENDOR_AMD,	0x11,					},
>  	{ X86_VENDOR_AMD,	0x10,					},
>  	{ X86_VENDOR_AMD,	0xf,					},
> -	{ X86_VENDOR_ANY,	4,					},
>  	{}
>  };
>  
> @@ -973,16 +965,16 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
>  	if (cpu_has(c, X86_FEATURE_ARCH_CAPABILITIES))
>  		rdmsrl(MSR_IA32_ARCH_CAPABILITIES, ia32_cap);

Would it make sense to move that above 'rdmsrl' and the conditional as well
to the logic below?
>  
> -	if (!x86_match_cpu(cpu_no_spec_store_bypass) &&
> -	   !(ia32_cap & ARCH_CAP_SSB_NO))
> -		setup_force_cpu_bug(X86_BUG_SPEC_STORE_BYPASS);
> -
>  	if (x86_match_cpu(cpu_no_speculation))
>  		return;
>  
>  	setup_force_cpu_bug(X86_BUG_SPECTRE_V1);
>  	setup_force_cpu_bug(X86_BUG_SPECTRE_V2);
>  
> +	if (!x86_match_cpu(cpu_no_spec_store_bypass) &&
> +	   !(ia32_cap & ARCH_CAP_SSB_NO))
> +		setup_force_cpu_bug(X86_BUG_SPEC_STORE_BYPASS);
> +
>  	if (x86_match_cpu(cpu_no_meltdown))
>  		return;
>  
> -- 
> 2.17.0
>