Re: [PATCH 1/7] x86/feature: Detect the x86 feature to control Speculation

[Andrea Arcangeli]

On Fri, Jan 05, 2018 at 02:09:43PM +0100, Thomas Gleixner wrote:
> On Thu, 4 Jan 2018, Tim Chen wrote:
> > +#define MSR_IA32_SPEC_CTRL		0x00000048
> > +#define SPEC_CTRL_FEATURE_DISABLE_IBRS	(0 << 0)
> > +#define SPEC_CTRL_FEATURE_ENABLE_IBRS	(1 << 0)
> > +
> > +#define MSR_IA32_PRED_CMD		0x00000049
> > +
> >  #define MSR_IA32_PERFCTR0		0x000000c1
> >  #define MSR_IA32_PERFCTR1		0x000000c2
> >  #define MSR_FSB_FREQ			0x000000cd
> > @@ -439,6 +445,7 @@
> >  #define FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX	(1<<1)
> >  #define FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX	(1<<2)
> >  #define FEATURE_CONTROL_LMCE				(1<<20)
> > +#define FEATURE_SET_IBPB				(1<<0)
> 
> So how is that bit related to the control bits above? This file is
> structured in obvious ways ....

It's not related, FEATURE_SET_IBPB value is specific and only
meaningful to MSR_IA32_PRED_CMD.

The only use that you can ever make of that is:

static inline void __spec_ctrl_ibpb(void)
{
	native_wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB);
}

static inline void spec_ctrl_ibpb(void)
{
	if (static_cpu_has(X86_FEATURE_IBPB_SUPPORT)) {
*obsolete line deleted to be replaced with static key*
			__spec_ctrl_ibpb();
	}
}

You need to set X86_FEATURE_IBPB_SUPPORT by hand if
X86_FEATURE_SPEC_CTRL is present. On some CPU X86_FEATURE_IBPB_SUPPORT
will show up in cpuid, in others only X86_FEATURE_SPEC_CTRL shows up
but that always implies X86_FEATURE_IBPB_SUPPORT.

void spec_ctrl_init(struct cpuinfo_x86 *c)
{
	if (c->x86_vendor != X86_VENDOR_INTEL &&
	    c->x86_vendor != X86_VENDOR_AMD)
		return;

	if (c != &boot_cpu_data) {
		spec_ctrl_cpu_init();
		return;
	}
[..]
	if (cpu_has_spec_ctrl()) {
		setup_force_cpu_cap(X86_FEATURE_IBPB_SUPPORT);
[..]
}

static void identify_cpu(struct cpuinfo_x86 *c)
[..]
	/* Set up SMEP/SMAP */
	setup_smep(c);
	setup_smap(c);

+	spec_ctrl_init(c);
[..]

static inline int cpu_has_spec_ctrl(void)
{
	return static_cpu_has(X86_FEATURE_SPEC_CTRL);
}

Note: if you don't drop all late microcode as discussed yesterday, the
above has to do a if (this/boot_cpu_has()) return 1; rmb() ; return
0;. rmb in the return 0 if there's more than one branch the CPU can
speculate through. Not from where it's called in spec_ctrl_init, but
for all other places that checks cpu_has_spec_ctrl().

Now about the late microcode my preference is not for static_cpu_has
and forcing the early microcode, but my long term preference is to
start with this/boot_cpu_has() and then turn static_cpu_has in a true
static key so that setup_force_cpu_cap shall also flip the static key
for all static_cpu_has(X86_FEATURE_IBPB_SUPPORT) also if run any time
after boot and not only if run before the static_cpu_has alternative
is patched in.