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

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

On Tue, Apr 14, 2020 at 12:58:10PM +0200, speck for Thomas Gleixner wrote:
> Mark,
> 
> speck for mark gross <speck@linutronix.de> writes:
> > +static void __init srbds_select_mitigation(void)
> > +{
> > +	u64 ia32_cap;
> > +
> > +	if (!boot_cpu_has_bug(X86_BUG_SRBDS))
> > +		return;
> > +	/*
> > +	 * Check to see if this is one of the MDS_NO systems supporting
> > +	 * TSX that are only exposed to SRBDS when TSX is enabled.
> > +	 */
> > +	ia32_cap = x86_read_arch_cap_msr();
> > +	if ((ia32_cap & ARCH_CAP_MDS_NO) && !boot_cpu_has(X86_FEATURE_RTM)) {
> 
> This does not work for the CPUs which have the IF_TSX thing because your
> setup magic does not set X86_BUG_SRBDS for those.... See below.

There is some requirements flux coming from the white paper that looks like may
need have to drop the IF_TSX and replace it with tests for MDS_NO and TSX
anyway.

FWIW I was trying to avoid adding a extra bug bit and using the IF_TSX only
with file scope of common.c to better match what is in the white paper. The
white paper might be changing. I can't close this until they get back from
passover vacation.


> 
> > +		srbds_mitigation = SRBDS_MITIGATION_NOT_AFFECTED_TSX_OFF;
> > +		goto out;
> > +	}
> > +
> > +	if (boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
> > +		srbds_mitigation = SRBDS_MITIGATION_HYPERVISOR;
> > +		goto out;
> > +	}
> > +
> > +	if (!boot_cpu_has(X86_FEATURE_SRBDS_CTRL)) {
> > +		srbds_mitigation = SRBDS_MITIGATION_UCODE_NEEDED;
> > +		goto out;
> > +	}
> > +
> > +	if (cpu_mitigations_off() || srbds_off) {
> > +		if (srbds_mitigation != SRBDS_MITIGATION_NOT_AFFECTED_TSX_OFF)
> 
> That's pointless. You already jumped over this part in case of TSX off.
right!  that block needs to be after the out: label.
> 
> > +			srbds_mitigation = SRBDS_MITIGATION_OFF;
> > +	}
> 
> The whole goto stuff can be completely avoided.
> 
> 	if (!boot_cpu_has_bug(X86_BUG_SRBDS))
> 		return;
> 
> 	if (boot_cpu_has(X86_BUG_SRBDS_IF_TSX) && !boot_cpu_has(X86_FEATURE_RTM)))
>         	srbds_mitigation = SRBDS_MITIGATION_NOT_AFFECTED_TSX_OFF;
>         else if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
> 		srbds_mitigation = SRBDS_MITIGATION_HYPERVISOR;
> 	else if (!boot_cpu_has(X86_FEATURE_SRBDS_CTRL))
> 		srbds_mitigation = SRBDS_MITIGATION_UCODE_NEEDED;
> 	else if (cpu_mitigations_off() || srbds_off)
> 		srbds_mitigation = SRBDS_MITIGATION_OFF;
Yes, I buggered the goto logic too may ways. Yours looks right.



> 
> > +out:
> > +	update_srbds_msr();
> > +	pr_info("%s\n", srbds_strings[srbds_mitigation]);
> > +}
> 
> > +#define SRBDS		BIT(0)
> > +#define SRBDS_IF_TSX	BIT(1)
> > +
> > +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,	X86_STEPPINGS(0, 0xA),		SRBDS),
> > +	VULNBL_INTEL_STEPPING(KABYLAKE_L,	X86_STEPPINGS(0xB, 0xC),	SRBDS_IF_TSX),
> > +	VULNBL_INTEL_STEPPING(KABYLAKE,		X86_STEPPINGS(0, 0xB),		SRBDS),
> > +	VULNBL_INTEL_STEPPING(KABYLAKE,		X86_STEPPINGS(0xC, 0xD),	SRBDS_IF_TSX),
> > +	{}
> > +};
> > +
> >  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 +1166,34 @@ 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|SRBDS_IF_TSX, 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 in the blacklist that enumerate MDS_NO are only
> > +		 * vulneralbe if TSX can be used.  To handle cases where TSX
> > +		 * gets fused off check to see if TSX is fused off and thus not
> > +		 * affected.
> > +		 *
> > +		 * When running with up to day microcode TSX_CTRL is only
> > +		 * enumerated on parts where TSX fused on.
> > +		 * When running with microcode not supporting TSX_CTRL we check
> > +		 * for RTM
> > +		 */
> > +		if ((ia32_cap & ARCH_CAP_MDS_NO) &&
> > +		    !((ia32_cap & ARCH_CAP_TSX_CTRL_MSR) ||
> > +		      cpu_has(c, X86_FEATURE_RTM)))
> 
> So you added SRBDS_IF_TSX and then you check for both and still have
> that check for _all_ CPUs. Also the TSX_CTRL_MSR part is weird. That's
> completely irrelevant. The only interesting part is X86_FEATURE_RTM.
> 
> What you really want is:
> 
> 	VULNBL_INTEL_STEPPING(KABYLAKE_L,	X86_STEPPINGS(0xB, 0xC),	SRBDS | SRBDS_IF_TSX),
> 	VULNBL_INTEL_STEPPING(KABYLAKE,		X86_STEPPINGS(0, 0xB),		SRBDS),
> 	VULNBL_INTEL_STEPPING(KABYLAKE,		X86_STEPPINGS(0xC, 0xD),	SRBDS | SRBDS_IF_TSX),

well, KABYLAKE steppings 0xC and 0xD are affected only if TSX is on.  I have
been thinking of the SRBDS bit to be for parts without the "only if TSX"
conditional vulnerability.

Anyway,like I said above, there is some flux happening in the white paper I'm
trying to align my code too that I hope to close in a few days.  There is talk
of only listing yes/no in the affected processor list and apply the above test
to every yes entry to ID the subset of parts affected "only if" TSX is
available.  If that happens I'll drop all the SRBDS_IF_TSX stuff and rely on
the test above.


> 	/*
> 	 * CPUs which have neither RDRAND nor RDSEED are not affected.
> 	 */
> 	if (cpu_has(c, X86_FEATURE_RDRAND) || cpu_has(c, X86_FEATURE_RDSEED)) {
> 		if (cpu_matches(SRBDS, cpu_vuln_blacklist)) {
> 			setup_force_cpu_bug(X86_BUG_SRBDS);
> 			/* These CPUs are only vulnerable when TSX is usable. */
> 	                if (cpu_matches(SRBDS_IF_TSX, cpu_vuln_blacklist))
> 				setup_force_cpu_bug(X86_BUG_SRBDS_IF_TSX);
> 		}
> 	}
> 
> Hmm?
That looks like it will work.

Thanks!

--mark