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

Andrea Arcangeli
2018-01-05T13:44:04Z

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.