Re: [PATCH 3/4] v6 more sampling fun 3

Mark,

speck for mark gross <[email protected]> writes:
> +#define VULNBL_INTEL_STEPPING(model, steppings, issues)			   \
> +	X86_MATCH_VENDOR_FAM_MODEL_STEPPING_FEATURE(INTEL, 6,		   \
> +					    INTEL_FAM6_##model, steppings, \
> +					    X86_FEATURE_ANY, issues)
> +
> +#define SRBDS	BIT(0)
> +
> +static const struct x86_cpu_id cpu_vuln_blacklist[] __initconst = {
> +	VULNBL_INTEL_STEPPING(IVYBRIDGE,	X86_STEPPING_ANY,	SRBDS),
> +	VULNBL_INTEL_STEPPING(HASWELL,		X86_STEPPING_ANY,	SRBDS),
> +	VULNBL_INTEL_STEPPING(HASWELL_L,	X86_STEPPING_ANY,	SRBDS),
> +	VULNBL_INTEL_STEPPING(HASWELL_G,	X86_STEPPING_ANY,	SRBDS),
> +	VULNBL_INTEL_STEPPING(BROADWELL_G,	X86_STEPPING_ANY,	SRBDS),
> +	VULNBL_INTEL_STEPPING(BROADWELL,	X86_STEPPING_ANY,	SRBDS),
> +	VULNBL_INTEL_STEPPING(SKYLAKE_L,	X86_STEPPING_ANY,	SRBDS),
> +	VULNBL_INTEL_STEPPING(SKYLAKE,		X86_STEPPING_ANY,	SRBDS),
> +	VULNBL_INTEL_STEPPING(KABYLAKE_L,	GENMASK(0xC, 0),	SRBDS), /*06_8E steppings <=C*/
> +	VULNBL_INTEL_STEPPING(KABYLAKE,		GENMASK(0xD, 0),	SRBDS), /*06_9E steppings <=D*/

Please don't use these horrible tail comments. Also the 06_NN part is of
that comment is not helpful either. Please make stuff self explaining,
i.e.

#define X86_STEPPINGS(mins, maxs)	GENMASK(maxs, mins)

and use that in the blacklist:

	VULNBL_INTEL_STEPPING(KABYLAKE_L,	X86_STEPPINGS(0, 0xC),	SRBDS),
	VULNBL_INTEL_STEPPING(KABYLAKE,		X86_STEPPINGS(0, 0xD),	SRBDS),

which is pretty obvious, right?

>  static bool __init cpu_matches(unsigned long which, const struct x86_cpu_id *table)
>  {
>  	const struct x86_cpu_id *m = x86_match_cpu(table);
> @@ -1142,6 +1163,27 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
>  	     (ia32_cap & ARCH_CAP_TSX_CTRL_MSR)))
>  		setup_force_cpu_bug(X86_BUG_TAA);
>  
> +	if (cpu_matches(SRBDS, cpu_vuln_blacklist)) {
> +		/*
> +		 * Some parts on the list don't have RDRAND or RDSEED. Make sure
> +		 * they show as "Not affected".
> +		 */
> +		if (!cpu_has(c, X86_FEATURE_RDRAND) &&
> +		    !cpu_has(c, X86_FEATURE_RDSEED))
> +			goto srbds_not_affected;
> +		/*
> +		 * Parts that have TSX fused off and are not affected by MDS
> +		 * are not affected by SRBDS. TSX_CTRL is only enumerated on
> +		 * parts where TSX fused on.
> +		 */
> +		if ((ia32_cap & ARCH_CAP_MDS_NO) &&
> +		    !(ia32_cap & ARCH_CAP_TSX_CTRL_MSR))
> +			goto srbds_not_affected;

This is confusing vs. the documentation patch:

>+  Kabylake_L     06_8EH        0xB only if TSX is enabled
>+  Kabylake_L     06_8EH        0xC only if TSX is enabled
>+
>+  Kabylake       06_9EH        <=B
>+  Kabylake       06_9EH        0xC only if TSX is enabled
>+  Kabylake       06_9EH        0xD only if TSX is enabled

IOW, this is only true for particular steppings of Kabylake[_L]

It might well be the case that exactly these steppings have MDS_NO and
might have TSX fused off, but can we pretty please make this very
explicit?

	VULNBL_INTEL_STEPPING(KABYLAKE_L,	X86_STEPPINGS(0x0, 0xA),	SRBDS),
	VULNBL_INTEL_STEPPING(KABYLAKE_L,	X86_STEPPINGS(0xB, 0xC),	SRBDS_IF_TSX),
	VULNBL_INTEL_STEPPING(KABYLAKE,		X86_STEPPINGS(0x0, 0xB),	SRBDS),
	VULNBL_INTEL_STEPPING(KABYLAKE,		X86_STEPPINGS(0xC, 0xD),	SRBDS_IF_TSX),

and make the check for SRBDS_IF_TSX & TSX enabled in the code? You
surely figure out how to define SRBDS_IF_TSX so the SRBDS match works
for both :)

Thanks,

        tglx