[PATCH V2] x86/speculation: Support Enhanced IBRS on future CPUs

[PATCH V2] x86/speculation: Support Enhanced IBRS on future CPUs

[Sai Praneeth Prakhya]

From: Sai Praneeth <sai.praneeth.prakhya@intel.com>

Some future Intel processors may support "Enhanced IBRS" which is an
"always on" mode i.e. IBRS bit in SPEC_CTRL MSR is enabled once and
never disabled.

[With enhanced IBRS, the predicted targets of indirect branches executed
cannot be controlled by software that was executed in a less privileged
predictor mode or on another logical processor. As a result, software
operating on a processor with enhanced IBRS need not use WRMSR to set
IA32_SPEC_CTRL.IBRS after every transition to a more privileged
predictor mode. Software can isolate predictor modes effectively simply
by setting the bit once. Software need not disable enhanced IBRS prior
to entering a sleep state such as MWAIT or HLT.] - Specification [1]

Even with enhanced IBRS, we still need to make sure that IBRS bit in
SPEC_CTRL MSR is always set i.e. while booting, if we detect support for
Enhanced IBRS, we enable IBRS bit in SPEC_CTRL MSR and we should also
make sure that it remains set always. In other words, if the guest has
cleared IBRS bit, upon VMEXIT the bit should still be set.

Fortunately, kernel already has the infrastructure ready. kvm/vmx.c does
x86_spec_ctrl_set_guest() before entering guest and
x86_spec_ctrl_restore_host() after leaving guest. So, the guest view of
SPEC_CTRL MSR is restored before entering guest and the host view of
SPEC_CTRL MSR is restored before entering host and hence IBRS will be
set after VMEXIT.

Intel's white paper on Retpoline [2] says that "Retpoline is known to be
an effective branch target injection (Spectre variant 2) mitigation on
Intel processors belonging to family 6 (enumerated by the CPUID
instruction) that do not have support for enhanced IBRS. On processors
that support enhanced IBRS, it should be used for mitigation instead of
retpoline."

This means, Intel recommends using Enhanced IBRS over Retpoline where
available and it also means that retpoline provides less mitigation on
processors with enhanced IBRS compared to those without. Hence, on
processors that support Enhanced IBRS, this patch makes Enhanced IBRS as
the default Spectre V2 mitigation technique instead of retpoline. Also,
we still need IBPB even with enhanced IBRS.

[1] https://software.intel.com/sites/default/files/managed/c5/63/336996-Speculative-Execution-Side-Channel-Mitigations.pdf
[2] https://software.intel.com/sites/default/files/managed/1d/46/Retpoline-A-Branch-Target-Injection-Mitigation.pdf

Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Originally-by: David Woodhouse <dwmw@amazon.co.uk>
Cc: Tim C Chen <tim.c.chen@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ravi Shankar <ravi.v.shankar@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/cpufeatures.h   |  1 +
 arch/x86/include/asm/nospec-branch.h |  2 +-
 arch/x86/kernel/cpu/bugs.c           | 29 +++++++++++++++++++++++++++--
 arch/x86/kernel/cpu/common.c         |  3 +++
 4 files changed, 32 insertions(+), 3 deletions(-)
 
 Changes from V1 to V2:
 1. Explicitly spell out in the change log, the reason for using Enhanced
     IBRS as the default Spectre V2 mitigation technique instead of Retpoline.

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 5701f5cecd31..f75815b1dbee 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -219,6 +219,7 @@
 #define X86_FEATURE_IBPB		( 7*32+26) /* Indirect Branch Prediction Barrier */
 #define X86_FEATURE_STIBP		( 7*32+27) /* Single Thread Indirect Branch Predictors */
 #define X86_FEATURE_ZEN			( 7*32+28) /* "" CPU is AMD family 0x17 (Zen) */
+#define X86_FEATURE_IBRS_ENHANCED		( 7*32+29) /* "ibrs_enhanced" Use Enhanced IBRS in kernel */
 
 /* Virtualization flags: Linux defined, word 8 */
 #define X86_FEATURE_TPR_SHADOW		( 8*32+ 0) /* Intel TPR Shadow */
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index f6f6c63da62f..fd2a8c1b88bc 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -214,7 +214,7 @@ enum spectre_v2_mitigation {
 	SPECTRE_V2_RETPOLINE_MINIMAL_AMD,
 	SPECTRE_V2_RETPOLINE_GENERIC,
 	SPECTRE_V2_RETPOLINE_AMD,
-	SPECTRE_V2_IBRS,
+	SPECTRE_V2_IBRS_ENHANCED,
 };
 
 /* The Speculative Store Bypass disable variants */
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 5c0ea39311fe..a66517de1301 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -130,6 +130,7 @@ static const char *spectre_v2_strings[] = {
 	[SPECTRE_V2_RETPOLINE_MINIMAL_AMD]	= "Vulnerable: Minimal AMD ASM retpoline",
 	[SPECTRE_V2_RETPOLINE_GENERIC]		= "Mitigation: Full generic retpoline",
 	[SPECTRE_V2_RETPOLINE_AMD]		= "Mitigation: Full AMD retpoline",
+	[SPECTRE_V2_IBRS_ENHANCED]		= "Mitigation: Enhanced IBRS",
 };
 
 #undef pr_fmt
@@ -349,6 +350,8 @@ static void __init spectre_v2_select_mitigation(void)
 
 	case SPECTRE_V2_CMD_FORCE:
 	case SPECTRE_V2_CMD_AUTO:
+		if (boot_cpu_has(X86_FEATURE_IBRS_ENHANCED))
+			goto skip_retpoline_enable_ibrs;
 		if (IS_ENABLED(CONFIG_RETPOLINE))
 			goto retpoline_auto;
 		break;
@@ -385,7 +388,22 @@ static void __init spectre_v2_select_mitigation(void)
 					 SPECTRE_V2_RETPOLINE_MINIMAL;
 		setup_force_cpu_cap(X86_FEATURE_RETPOLINE);
 	}
+	goto enable_other_mitigations;
 
+skip_retpoline_enable_ibrs:
+	mode = SPECTRE_V2_IBRS_ENHANCED;
+
+	/*
+	 * As we don't use IBRS in kernel, nobody should have set
+	 * SPEC_CTRL_IBRS until now. Shout loud if somebody did enable
+	 * SPEC_CTRL_IBRS before us.
+	 */
+	WARN_ON_ONCE(x86_spec_ctrl_base & SPEC_CTRL_IBRS);
+
+	/* Ensure SPEC_CTRL_IBRS is set after VMEXIT from a guest */
+	x86_spec_ctrl_base |= SPEC_CTRL_IBRS;
+
+enable_other_mitigations:
 	spectre_v2_enabled = mode;
 	pr_info("%s\n", spectre_v2_strings[mode]);
 
@@ -415,9 +433,16 @@ static void __init spectre_v2_select_mitigation(void)
 
 	/*
 	 * Retpoline means the kernel is safe because it has no indirect
-	 * branches. But firmware isn't, so use IBRS to protect that.
+	 * branches. Enhanced IBRS protects firmware too, so, enable restricted
+	 * speculation around firmware calls only when Enhanced IBRS isn't
+	 * supported.
+	 *
+	 * Use "mode" to check Enhanced IBRS instead of boot_cpu_has(), because
+	 * user might select retpoline on command line and if CPU supports
+	 * Enhanced IBRS, we might un-intentionally not enable IBRS around
+	 * firmware calls.
 	 */
-	if (boot_cpu_has(X86_FEATURE_IBRS)) {
+	if (boot_cpu_has(X86_FEATURE_IBRS) && mode != SPECTRE_V2_IBRS_ENHANCED) {
 		setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
 		pr_info("Enabling Restricted Speculation for firmware calls\n");
 	}
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index eb4cb3efd20e..8ed73a46511f 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1005,6 +1005,9 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
 	   !cpu_has(c, X86_FEATURE_AMD_SSB_NO))
 		setup_force_cpu_bug(X86_BUG_SPEC_STORE_BYPASS);
 
+	if (ia32_cap & ARCH_CAP_IBRS_ALL)
+		setup_force_cpu_cap(X86_FEATURE_IBRS_ENHANCED);
+
 	if (x86_match_cpu(cpu_no_meltdown))
 		return;
 
-- 
2.7.4

Re: [PATCH V2] x86/speculation: Support Enhanced IBRS on future CPUs

[Thomas Gleixner]

On Mon, 30 Jul 2018, Sai Praneeth Prakhya wrote:
> Some future Intel processors may support "Enhanced IBRS" which is an

Future Intel processors will support ...

> "always on" mode i.e. IBRS bit in SPEC_CTRL MSR is enabled once and
> never disabled.
> 
> [With enhanced IBRS, the predicted targets of indirect branches executed
> cannot be controlled by software that was executed in a less privileged
> predictor mode or on another logical processor. As a result, software
> operating on a processor with enhanced IBRS need not use WRMSR to set
> IA32_SPEC_CTRL.IBRS after every transition to a more privileged
> predictor mode. Software can isolate predictor modes effectively simply
> by setting the bit once. Software need not disable enhanced IBRS prior
> to entering a sleep state such as MWAIT or HLT.] - Specification [1]

There is no reason not to use indentation and quotation marks in a
changelog. Squeezing it into square brackets does not really improve
readability.

 From the specification [1]:

  "With enhanced IBRS, the predicted targets of indirect branches executed
   cannot be controlled by software that was executed in a less privileged
   ....
   to entering a sleep state such as MWAIT or HLT."

Hmm?

> Even with enhanced IBRS, we still need to make sure that IBRS bit in

Why even?

> SPEC_CTRL MSR is always set i.e. while booting, if we detect support for
> Enhanced IBRS, we enable IBRS bit in SPEC_CTRL MSR and we should also
> make sure that it remains set always. In other words, if the guest has
> cleared IBRS bit, upon VMEXIT the bit should still be set.
> 
> Fortunately, kernel already has the infrastructure ready. kvm/vmx.c does
> x86_spec_ctrl_set_guest() before entering guest and
> x86_spec_ctrl_restore_host() after leaving guest. So, the guest view of
> SPEC_CTRL MSR is restored before entering guest and the host view of
> SPEC_CTRL MSR is restored before entering host and hence IBRS will be
> set after VMEXIT.

What you are trying to say here is:

 If Enhanced IBRS is selected as mitigation mechanism the IBRS bit is set
 once at boot time and never cleared. This also has to be ensured after a
 VMEXIT because the guest might have cleared the bit. This is already
 covered by the existing x86_spec_ctrl_set_guest() and
 x86_spec_ctrl_restore_host() speculation control functions.

Hmm?

> Intel's white paper on Retpoline [2] says that "Retpoline is known to be
> an effective branch target injection (Spectre variant 2) mitigation on
> Intel processors belonging to family 6 (enumerated by the CPUID
> instruction) that do not have support for enhanced IBRS. On processors
> that support enhanced IBRS, it should be used for mitigation instead of
> retpoline."
> 
> This means, Intel recommends using Enhanced IBRS over Retpoline where
> available and it also means that retpoline provides less mitigation on
> processors with enhanced IBRS compared to those without.

The cited part of the white paper does not say that its a broader
mitigation than what Retpoline covers. It merely recommends to use enhanced
IBRS without providing a reason.

But chapter 4.3 contains the real reason for using Emhanced IBRS: The
processors which support it also support CET and CET does not work well
with Retpoline.

Please provide facts and not interpretations. If you have additional
knowledge about a broader mitigation scope, then clearly say so:

 Aside of that Enhanced IBRS provides broader mitigation scope than
 Retpoline according to Intel internal information.

That makes it clear that it's a fact and not what you think the cited
paragraph means.

> Hence, on processors that support Enhanced IBRS, this patch makes
> Enhanced IBRS as

Please search Documentation/process/submitting-patches.rst for 'This patch' ....

> the default Spectre V2 mitigation technique instead of retpoline. Also,
> we still need IBPB even with enhanced IBRS.

Something like this perhaps:

 If Enhanced IBRS is supported by the processor then use it as the
 preferred mitigation mechanism instead of Retpoline. Intel's Retpoline
 white paper [2] states:

   "Retpoline is known to be an effective branch target injection (Spectre
    variant 2) mitigation on Intel processors belonging to family 6
    (enumerated by the CPUID instruction) that do not have support for
    enhanced IBRS. On processors that support enhanced IBRS, it should be
    used for mitigation instead of retpoline."

 The reason why 'Enhanced IBRS is the recommended mitigation on processors
 which support it is that these processors also support CET which provides
 a defense against ROP attacks. Retpoline is very similar to ROP techniques
 and might trigger false positives in the CET defense.

 Enhanced IBRS still requires IBPB for full mitigation.

See? You might have noticed that aside of restructuring and enhancing the
information I also got rid of all 'we' instances. Using 'we' is a form of
impersonation which IMO blurs the technicality of a changelog.

 
> [1] https://software.intel.com/sites/default/files/managed/c5/63/336996-Speculative-Execution-Side-Channel-Mitigations.pdf
> [2] https://software.intel.com/sites/default/files/managed/1d/46/Retpoline-A-Branch-Target-Injection-Mitigation.pdf

These links are not really useful as sooner than later they are going to be
invalid. We recently started to put copies of such documents into the
kernel.org bugzilla and I'm pretty sure we have at least one of them
already in one of the speculation mess related BZs. Could you please track
that down and make sure that both files are available there in the latest
version. Then provide links to the BZ entry which are more likely to
survive than content on a corporate website.

> @@ -219,6 +219,7 @@
>  #define X86_FEATURE_IBPB		( 7*32+26) /* Indirect Branch Prediction Barrier */
>  #define X86_FEATURE_STIBP		( 7*32+27) /* Single Thread Indirect Branch Predictors */
>  #define X86_FEATURE_ZEN			( 7*32+28) /* "" CPU is AMD family 0x17 (Zen) */
> +#define X86_FEATURE_IBRS_ENHANCED		( 7*32+29) /* "ibrs_enhanced" Use Enhanced IBRS in kernel */

That "ibrs_enhanced" part is not needed. And 'Use' is also wrong. The
feature merily reflects the availability of Enhanced IBRS and not its
usage.

> @@ -349,6 +350,8 @@ static void __init spectre_v2_select_mitigation(void)
>  
>  	case SPECTRE_V2_CMD_FORCE:
>  	case SPECTRE_V2_CMD_AUTO:
> +		if (boot_cpu_has(X86_FEATURE_IBRS_ENHANCED))
> +			goto skip_retpoline_enable_ibrs;
>  		if (IS_ENABLED(CONFIG_RETPOLINE))
>  			goto retpoline_auto;
>  		break;
> @@ -385,7 +388,22 @@ static void __init spectre_v2_select_mitigation(void)
>  					 SPECTRE_V2_RETPOLINE_MINIMAL;
>  		setup_force_cpu_cap(X86_FEATURE_RETPOLINE);
>  	}
> +	goto enable_other_mitigations;
>  
> +skip_retpoline_enable_ibrs:
> +	mode = SPECTRE_V2_IBRS_ENHANCED;
> +
> +	/*
> +	 * As we don't use IBRS in kernel, nobody should have set
> +	 * SPEC_CTRL_IBRS until now. Shout loud if somebody did enable
> +	 * SPEC_CTRL_IBRS before us.
> +	 */

This comment does not make sense. What prevents the BIOS/bootloader or the
hypervisor from setting it?

> +	WARN_ON_ONCE(x86_spec_ctrl_base & SPEC_CTRL_IBRS);
> +
> +	/* Ensure SPEC_CTRL_IBRS is set after VMEXIT from a guest */
> +	x86_spec_ctrl_base |= SPEC_CTRL_IBRS;

And what exactly writes the MSR?

> +enable_other_mitigations:
>  	spectre_v2_enabled = mode;
>  	pr_info("%s\n", spectre_v2_strings[mode]);
>  
> @@ -415,9 +433,16 @@ static void __init spectre_v2_select_mitigation(void)
>  
>  	/*
>  	 * Retpoline means the kernel is safe because it has no indirect
> -	 * branches. But firmware isn't, so use IBRS to protect that.
> +	 * branches. Enhanced IBRS protects firmware too, so, enable restricted
> +	 * speculation around firmware calls only when Enhanced IBRS isn't
> +	 * supported.
> +	 *
> +	 * Use "mode" to check Enhanced IBRS instead of boot_cpu_has(), because
> +	 * user might select retpoline on command line and if CPU supports

the user ... on the kernel command line and if the CPU .....

> +	 * Enhanced IBRS, we might un-intentionally not enable IBRS around
> +	 * firmware calls.
>  	 */

Thanks,

	tglx

RE: [PATCH V2] x86/speculation: Support Enhanced IBRS on future CPUs

[Prakhya, Sai Praneeth]

> There is no reason not to use indentation and quotation marks in a changelog.
> Squeezing it into square brackets does not really improve readability.
> 
>  From the specification [1]:
> 
>   "With enhanced IBRS, the predicted targets of indirect branches executed
>    cannot be controlled by software that was executed in a less privileged
>    ....
>    to entering a sleep state such as MWAIT or HLT."
> 
> Hmm?

Yes, this looks good. I have changed commit message in V3 accordingly.

> > x86_spec_ctrl_set_guest() before entering guest and
> > x86_spec_ctrl_restore_host() after leaving guest. So, the guest view
> > of SPEC_CTRL MSR is restored before entering guest and the host view
> > of SPEC_CTRL MSR is restored before entering host and hence IBRS will
> > be set after VMEXIT.
> 
> What you are trying to say here is:
> 
>  If Enhanced IBRS is selected as mitigation mechanism the IBRS bit is set  once at
> boot time and never cleared. This also has to be ensured after a  VMEXIT
> because the guest might have cleared the bit. This is already  covered by the
> existing x86_spec_ctrl_set_guest() and
>  x86_spec_ctrl_restore_host() speculation control functions.
> 
> Hmm?

Yes, that's correct. It's simple and concise. I have updated commit message as suggested.

> 
> > Intel's white paper on Retpoline [2] says that "Retpoline is known to
> > be an effective branch target injection (Spectre variant 2) mitigation
> > on Intel processors belonging to family 6 (enumerated by the CPUID
> > instruction) that do not have support for enhanced IBRS. On processors
> > that support enhanced IBRS, it should be used for mitigation instead
> > of retpoline."
> >
> > This means, Intel recommends using Enhanced IBRS over Retpoline where
> > available and it also means that retpoline provides less mitigation on
> > processors with enhanced IBRS compared to those without.
> 
> The cited part of the white paper does not say that its a broader mitigation than
> what Retpoline covers. It merely recommends to use enhanced IBRS without
> providing a reason.
> 
> But chapter 4.3 contains the real reason for using Emhanced IBRS: The
> processors which support it also support CET and CET does not work well with
> Retpoline.
> 
> Please provide facts and not interpretations.

Sorry! got it.

> If you have additional knowledge
> about a broader mitigation scope, then clearly say so:
>

Hmm.. no.. not really. I just learned it from Dave.

> > Hence, on processors that support Enhanced IBRS, this patch makes
> > Enhanced IBRS as
> 
> Please search Documentation/process/submitting-patches.rst for 'This patch' ....
> 

Yes, got it. Will refrain myself from using 'This patch' further.

>  The reason why 'Enhanced IBRS is the recommended mitigation on processors
> which support it is that these processors also support CET which provides  a
> defense against ROP attacks. Retpoline is very similar to ROP techniques  and
> might trigger false positives in the CET defense.
> 
>  Enhanced IBRS still requires IBPB for full mitigation.
> 
> See? You might have noticed that aside of restructuring and enhancing the
> information I also got rid of all 'we' instances. Using 'we' is a form of
> impersonation which IMO blurs the technicality of a changelog.

Makes sense. Will stop using 'we' further.

> 
> 
> > [1]
> > https://software.intel.com/sites/default/files/managed/c5/63/336996-Sp
> > eculative-Execution-Side-Channel-Mitigations.pdf
> > [2]
> > https://software.intel.com/sites/default/files/managed/1d/46/Retpoline
> > -A-Branch-Target-Injection-Mitigation.pdf
> 
> These links are not really useful as sooner than later they are going to be invalid.
> We recently started to put copies of such documents into the kernel.org bugzilla
> and I'm pretty sure we have at least one of them already in one of the
> speculation mess related BZs. Could you please track that down and make sure
> that both files are available there in the latest version. Then provide links to the
> BZ entry which are more likely to survive than content on a corporate website.
>

Sure! Makes sense.
I have updated Bugzilla link that has these documents and also updated commit message in V3.

> > @@ -219,6 +219,7 @@
> >  #define X86_FEATURE_IBPB		( 7*32+26) /* Indirect Branch
> Prediction Barrier */
> >  #define X86_FEATURE_STIBP		( 7*32+27) /* Single Thread Indirect
> Branch Predictors */
> >  #define X86_FEATURE_ZEN			( 7*32+28) /* "" CPU is AMD
> family 0x17 (Zen) */
> > +#define X86_FEATURE_IBRS_ENHANCED		( 7*32+29) /*
> "ibrs_enhanced" Use Enhanced IBRS in kernel */
> 
> That "ibrs_enhanced" part is not needed.

Just wanted to confirm with you before removing it, 
Presently, on platforms that support features like arch_capabilities, stibp, ibrs and ibpb 
/proc/cpuinfo does show them. Do you think we should really skip showing 
Enhanced IBRS capability?

> And 'Use' is also wrong. The feature
> merily reflects the availability of Enhanced IBRS and not its usage.
> 

Makes sense. Updated comment in V3.

> > +	/*
> > +	 * As we don't use IBRS in kernel, nobody should have set
> > +	 * SPEC_CTRL_IBRS until now. Shout loud if somebody did enable
> > +	 * SPEC_CTRL_IBRS before us.
> > +	 */
> 
> This comment does not make sense. What prevents the BIOS/bootloader or the
> hypervisor from setting it?
>

Makes sense. Removed it from V3.

> > +	WARN_ON_ONCE(x86_spec_ctrl_base & SPEC_CTRL_IBRS);
> > +
> > +	/* Ensure SPEC_CTRL_IBRS is set after VMEXIT from a guest */
> > +	x86_spec_ctrl_base |= SPEC_CTRL_IBRS;
> 
> And what exactly writes the MSR?
>

While booting, x86_spec_ctrl_setup_ap() does that and after VMEXIT 
x86_spec_ctrl_restore_host().

As x86_spec_ctrl_setup_ap() does wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base), 
I thought writing here would be redundant.

Did I answer your question or did I get it wrong?

Regards,
Sai

RE: [PATCH V2] x86/speculation: Support Enhanced IBRS on future CPUs

[Thomas Gleixner]

On Tue, 31 Jul 2018, Prakhya, Sai Praneeth wrote:
> > > +#define X86_FEATURE_IBRS_ENHANCED		( 7*32+29) /*
> > "ibrs_enhanced" Use Enhanced IBRS in kernel */
> > 
> > That "ibrs_enhanced" part is not needed.
> 
> Just wanted to confirm with you before removing it, 
> Presently, on platforms that support features like arch_capabilities, stibp, ibrs and ibpb 
> /proc/cpuinfo does show them. Do you think we should really skip showing 
> Enhanced IBRS capability?

The feature strings are automatically generated from the define. The
comment can be used to supress them by an empty "" string or to modify them
by a "override" string at the beginning of the comment.

> > > +	/*
> > > +	 * As we don't use IBRS in kernel, nobody should have set
> > > +	 * SPEC_CTRL_IBRS until now. Shout loud if somebody did enable
> > > +	 * SPEC_CTRL_IBRS before us.
> > > +	 */
> > 
> > This comment does not make sense. What prevents the BIOS/bootloader or the
> > hypervisor from setting it?
> >
> 
> Makes sense. Removed it from V3.
> 
> > > +	WARN_ON_ONCE(x86_spec_ctrl_base & SPEC_CTRL_IBRS);

Please remove the warnon as well.

> > > +	/* Ensure SPEC_CTRL_IBRS is set after VMEXIT from a guest */
> > > +	x86_spec_ctrl_base |= SPEC_CTRL_IBRS;
> > 
> > And what exactly writes the MSR?
> >
> 
> While booting, x86_spec_ctrl_setup_ap() does that and after VMEXIT 
> x86_spec_ctrl_restore_host().
> 
> As x86_spec_ctrl_setup_ap() does wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base), 
> I thought writing here would be redundant.

x86_spec_ctrl_setup_ap() is only called on the AP but not on the BP. So the
boot processor will not have it set, unless something else writes the
MSR. So you really want to have an explicit write there.

Thanks,

	tglx

RE: [PATCH V2] x86/speculation: Support Enhanced IBRS on future CPUs

[Prakhya, Sai Praneeth]

> The feature strings are automatically generated from the define. The comment
> can be used to supress them by an empty "" string or to modify them by a
> "override" string at the beginning of the comment.

I overlooked "override" part. Sorry! about that.
It's clear now. Thanks for the explanation.

> >
> > > > +	WARN_ON_ONCE(x86_spec_ctrl_base & SPEC_CTRL_IBRS);
> 
> Please remove the warnon as well.
>

Sure! I removed it but forgot to mention it.

> > > > +	/* Ensure SPEC_CTRL_IBRS is set after VMEXIT from a guest */
> > > > +	x86_spec_ctrl_base |= SPEC_CTRL_IBRS;
> > >
> > > And what exactly writes the MSR?
> > >
> >
> > While booting, x86_spec_ctrl_setup_ap() does that and after VMEXIT
> > x86_spec_ctrl_restore_host().
> >
> > As x86_spec_ctrl_setup_ap() does wrmsrl(MSR_IA32_SPEC_CTRL,
> > x86_spec_ctrl_base), I thought writing here would be redundant.
> 
> x86_spec_ctrl_setup_ap() is only called on the AP but not on the BP. So the boot
> processor will not have it set, unless something else writes the MSR. So you
> really want to have an explicit write there.

Yes, that makes sense.
But on the machine, I see IBRS bit set on all cores. As you said, someone else might 
be writing the MSR. I will try to find that out and will update the patch accordingly.

I initially suspected it to be __ssb_select_mitigation() as I have 
"spec_store_bypass_disable=on" in the kernel command line, but turns out it's not so.
I will update you more on this.

Regards,
Sai

RE: [PATCH V2] x86/speculation: Support Enhanced IBRS on future CPUs

[Thomas Gleixner]

On Tue, 31 Jul 2018, Prakhya, Sai Praneeth wrote:

> > The feature strings are automatically generated from the define. The comment
> > can be used to supress them by an empty "" string or to modify them by a
> > "override" string at the beginning of the comment.
> 
> I overlooked "override" part. Sorry! about that.

Nothing to be sorry about. I just knew it because I stumbled over it my
self quite some time ago.

> > > > > +	/* Ensure SPEC_CTRL_IBRS is set after VMEXIT from a guest */
> > > > > +	x86_spec_ctrl_base |= SPEC_CTRL_IBRS;
> > > >
> > > > And what exactly writes the MSR?
> > > >
> > >
> > > While booting, x86_spec_ctrl_setup_ap() does that and after VMEXIT
> > > x86_spec_ctrl_restore_host().
> > >
> > > As x86_spec_ctrl_setup_ap() does wrmsrl(MSR_IA32_SPEC_CTRL,
> > > x86_spec_ctrl_base), I thought writing here would be redundant.
> > 
> > x86_spec_ctrl_setup_ap() is only called on the AP but not on the BP. So the boot
> > processor will not have it set, unless something else writes the MSR. So you
> > really want to have an explicit write there.
> 
> Yes, that makes sense.
> But on the machine, I see IBRS bit set on all cores. As you said, someone else might 
> be writing the MSR. I will try to find that out and will update the patch accordingly.
> 
> I initially suspected it to be __ssb_select_mitigation() as I have 
> "spec_store_bypass_disable=on" in the kernel command line, but turns out it's not so.
> I will update you more on this.

There are lots of places like the firmware mitigation stuff and other
things which write that MSR. And because the bit is set in
x86_spec_ctrl_base it will be on at some point and stay so.

Writing it explicitely at the point where it is set makes it independent of
other mechanisms which touch that MSR and Just Works.

Thanks,

	tglx

RE: [PATCH V2] x86/speculation: Support Enhanced IBRS on future CPUs

[Prakhya, Sai Praneeth]

> > Yes, that makes sense.
> > But on the machine, I see IBRS bit set on all cores. As you said,
> > someone else might be writing the MSR. I will try to find that out and will
> update the patch accordingly.
> >
> > I initially suspected it to be __ssb_select_mitigation() as I have
> > "spec_store_bypass_disable=on" in the kernel command line, but turns out it's
> not so.
> > I will update you more on this.
> 
> There are lots of places like the firmware mitigation stuff and other things which
> write that MSR. And because the bit is set in x86_spec_ctrl_base it will be on at
> some point and stay so.

True! After a bit of experimenting with printk(), I see that it's being set by 
intel_set_ssb_state() during systemd initialization.

> 
> Writing it explicitely at the point where it is set makes it independent of other
> mechanisms which touch that MSR and Just Works.

Yes, that makes sense. I will add an explicit wrmsrl().
Just wanted to have a better understanding of how things work.

Regards,
Sai