Re: [RFC 05/10] x86/speculation: Add basic IBRS support infrastructure

[David Woodhouse]

On Wed, 2018-01-24 at 09:47 +0100, Peter Zijlstra wrote:
> Typically tglx likes to use x86_match_cpu() for these things; see also
> commit: bd9240a18edfb ("x86/apic: Add TSC_DEADLINE quirk due to
> errata").

Thanks, will fix. I think we might also end up in whitelist mode,
adding "known good" microcodes to the list as they get released or
retroactively blessed.

I would really have liked a new bit in IA32_ARCH_CAPABILITIES to say
that it's safe, but that's not possible for *existing* microcode which
actually turns out to be OK in the end.

That means the whitelist ends up basically empty right now. Should I
add a command line parameter to override it? Otherwise we end up having
to rebuild the kernel every time there's a microcode release which
covers a new CPU SKU (which is why I kind of hate the whitelist, but
Arjan is very insistent...)

I'm kind of tempted to turn it into a whitelist just by adding 1 to the
microcode revision in each table entry. Sure, that N+1 might be another
microcode build that also has issues but never saw the light of day...
but that's OK as long it never *does*. And yes we'd have to tweak it if
revisions that are blacklisted in the Intel doc are subsequently
cleared. But at least it'd require *less* tweaking.

> > 
> > +
> > +static int bad_spectre_microcode(struct cpuinfo_x86 *c)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(spectre_bad_microcodes); i++) {
> > +		if (c->x86_model == spectre_bad_microcodes[i].model &&
> > +		ÂÂÂÂc->x86_mask == spectre_bad_microcodes[i].stepping)
> > +			return (c->microcode <= spectre_bad_microcodes[i].microcode);
> > +	}
> > +	return 0;
> > +}
> The above is Intel only, you should check vendor too I think.

It's in intel.c, called from early_init_intel(). Isn't that sufficient?

> > 
> > Âstatic void early_init_intel(struct cpuinfo_x86 *c)
> > Â{
> > Â	u64 misc_enable;
> > @@ -122,6 +173,18 @@ static void early_init_intel(struct cpuinfo_x86 *c)
> > Â	if (c->x86 >= 6 && !cpu_has(c, X86_FEATURE_IA64))
> > Â		c->microcode = intel_get_microcode_revision();
> > Â
> > +	if ((cpu_has(c, X86_FEATURE_SPEC_CTRL) ||
> > +	ÂÂÂÂÂcpu_has(c, X86_FEATURE_AMD_SPEC_CTRL) ||
> > +	ÂÂÂÂÂcpu_has(c, X86_FEATURE_AMD_PRED_CMD) ||
> > +	ÂÂÂÂÂcpu_has(c, X86_FEATURE_AMD_STIBP)) && bad_spectre_microcode(c)) {
> > +		pr_warn("Intel Spectre v2 broken microcode detected; disabling SPEC_CTRL\n");
> > +		clear_cpu_cap(c, X86_FEATURE_SPEC_CTRL);
> > +		clear_cpu_cap(c, X86_FEATURE_STIBP);
> > +		clear_cpu_cap(c, X86_FEATURE_AMD_SPEC_CTRL);
> > +		clear_cpu_cap(c, X86_FEATURE_AMD_PRED_CMD);
> > +		clear_cpu_cap(c, X86_FEATURE_AMD_STIBP);
> > +	}
> And since its Intel only, what are those AMD features doing there?

Hypervisors which only want to expose PRED_CMD may do so using the AMD
feature bit. SPEC_CTRL requires save/restore and live migration
support, and isn't needed with retpoline anyway (since guests won't be
calling directly into firmware).

Attachment: smime.p7s
Description: S/MIME cryptographic signature