On Fri, 2018-01-12 at 10:51 +0100, Peter Zijlstra wrote:
> On Thu, Jan 11, 2018 at 05:58:11PM -0800, Dave Hansen wrote:
> > On 01/11/2018 05:32 PM, Ashok Raj wrote:
> > > +static void save_guest_spec_ctrl(struct vcpu_vmx *vmx)
> > > +{
> > > +   if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
> > > +           vmx->spec_ctrl = spec_ctrl_get();
> > > +           spec_ctrl_restriction_on();
> > > +   } else
> > > +           rmb();
> > > +}
> > 
> > Does this need to be "ifence()"?  Better yet, do we just need to create
> > a helper for boot_cpu_has(X86_FEATURE_SPEC_CTRL) that does the barrier?
> 
> static_cpu_has() + hard asm-goto requirement. Please drop all the above
> nonsense on the floor hard.

Peter, NO!

static_cpu_has() + asm-goto is NOT SUFFICIENT.

It's still *possible* for a missed optimisation in GCC to still leave
us with a conditional branch around the wrmsr, letting the CPU
speculate around it too.

Come on, Peter, we've learned this lesson long and hard since the 1990s
when every GCC update would break some stupid¹ shit we did. Don't
regress. WE DO NOT RELY ON UNGUARANTEED BEHAVIOUR OF THE COMPILER.

Devise a sanity check which will force a build-fail if GCC ever misses
the optimisation, and that's fine. (Or point me to an existing way that
it's guaranteed to fail, if such is true already. Don't just ignore the
objection.)

Do not just ASSUME that GCC will always and forever manage to make that
optimisation in every case.

-- 
dwmw2


¹ In our defence, back then there was often no *other* way to do some
  of the things we did, except the "stupid" way. These days, it's much
  easier to add features to GCC and elicit guarantees. Although I'm
  getting rather pissed off at them bikeshedding the retpoline patches
  *so* hard they can't even agree on a command-line option and the name
  of the thunk symbol, regardless of the internal implementation
  details we don't give a shit about.