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

[mark gross <mgross@linux.intel.com>]

On Fri, Apr 10, 2020 at 12:36:09AM +0200, speck for Thomas Gleixner wrote:
> Mark,
> 
> speck for mark gross <speck@linutronix.de> 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.
tail comments gone.

> #define X86_STEPPINGS(mins, maxs)	GENMASK(maxs, mins)
Adding this to the patch where I introduced steppings to x86_cpu_id.


> 
> 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?
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:
I'll try to make it better.
attempt made.  I will post it Monday.


> >+  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?
I thought that might be the case to but, I was told that the choice to fuse
stuff off is made by marketing per bin splits.  i.e. Kabylake i3's or Celerons
will get the features fused off not all kabylakes or specific steppings.

> 	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 :)
In fact that was somewhat how I hand the code initialy implemented treating the
"onyl if tsx" case as a separate match bit.  

Added SRBDS_IF_TSX to make the code more self documenting and more closely
match documentation.

done.

> 
> Thanks,
> 
>         tglx

Thank you!
--mark