Date: Fri, 5 Jan 2018 14:44:00 +0100
From: Andrea Arcangeli <aarcange@redhat.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Tim Chen <tim.c.chen@linux.intel.com>,
        Andy Lutomirski <luto@kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Greg KH <gregkh@linuxfoundation.org>,
        Dave Hansen <dave.hansen@intel.com>,
        Andi Kleen <ak@linux.intel.com>,
        Arjan Van De Ven <arjan.van.de.ven@intel.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/7] x86/feature: Detect the x86 feature to control
 Speculation
Message-ID: <20180105134400.GC26807@redhat.com>
References: <cover.1515086770.git.tim.c.chen@linux.intel.com>
 <427aa76dea14532dea7e49f0bce4e7cf1dea7c6f.1515086770.git.tim.c.chen@linux.intel.com>
 <alpine.DEB.2.20.1801051405500.1724@nanos>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.DEB.2.20.1801051405500.1724@nanos>
User-Agent: Mutt/1.9.2 (2017-12-15)
Sender: linux-kernel-owner@vger.kernel.org

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.