[PATCH v2] x86/speculation: Add support for STIBP always-on preferred mode

[PATCH v2] x86/speculation: Add support for STIBP always-on preferred mode

[Lendacky, Thomas]

Different AMD processors may have different implementations of STIBP.
When STIBP is conditionally enabled, some implementations would benefit
from having STIBP always on instead of toggling the STIBP bit through MSR
writes. This preference is advertised through a CPUID feature bit.

When conditional STIBP support is requested at boot and the CPU advertises
STIBP always-on mode as preferred, switch to STIBP "on" support. Print a
message to let the user know this occurred. Also, provide a boolean that
be used in stibp_state() to return a message tailored to the always-on
support.

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---

This patch is against the x86/pti branch of the tip tree:
  git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/pti

Changes from v1:
- Removed explicit SPECTRE_V2_USER_STRICT_PREFERRED mode
- Added a message when switching to always-on mode
- Set and used a static boolean for the string in stibp_state()

 arch/x86/include/asm/cpufeatures.h |    1 +
 arch/x86/kernel/cpu/bugs.c         |   17 ++++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 28c4a50..df8e94e2 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -284,6 +284,7 @@
 #define X86_FEATURE_AMD_IBPB		(13*32+12) /* "" Indirect Branch Prediction Barrier */
 #define X86_FEATURE_AMD_IBRS		(13*32+14) /* "" Indirect Branch Restricted Speculation */
 #define X86_FEATURE_AMD_STIBP		(13*32+15) /* "" Single Thread Indirect Branch Predictors */
+#define X86_FEATURE_AMD_STIBP_ALWAYS_ON	(13*32+17) /* "" Single Thread Indirect Branch Predictors always-on preferred */
 #define X86_FEATURE_AMD_SSBD		(13*32+24) /* "" Speculative Store Bypass Disable */
 #define X86_FEATURE_VIRT_SSBD		(13*32+25) /* Virtualized Speculative Store Bypass Disable */
 #define X86_FEATURE_AMD_SSB_NO		(13*32+26) /* "" Speculative Store Bypass is fixed in hardware. */
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 58689ac..db156e1 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -61,6 +61,8 @@
 /* Control unconditional IBPB in switch_mm() */
 DEFINE_STATIC_KEY_FALSE(switch_mm_always_ibpb);
 
+static bool stibp_always_on;
+
 void __init check_bugs(void)
 {
 	identify_boot_cpu();
@@ -355,6 +357,18 @@ static void __init spec_v2_user_print_cond(const char *reason, bool secure)
 		break;
 	}
 
+	/*
+	 * At this point, an STIBP mode other than "off" has been set.
+	 * If STIBP support is not being forced, check if STIBP always-on
+	 * is preferred.
+	 */
+	if (mode != SPECTRE_V2_USER_STRICT &&
+	    boot_cpu_has(X86_FEATURE_AMD_STIBP_ALWAYS_ON)) {
+		stibp_always_on = true;
+		mode = SPECTRE_V2_USER_STRICT;
+		pr_info("mitigation: STIBP always-on is preferred\n");
+	}
+
 	/* Initialize Indirect Branch Prediction Barrier */
 	if (boot_cpu_has(X86_FEATURE_IBPB)) {
 		setup_force_cpu_cap(X86_FEATURE_USE_IBPB);
@@ -1088,7 +1102,8 @@ static char *stibp_state(void)
 	case SPECTRE_V2_USER_NONE:
 		return ", STIBP: disabled";
 	case SPECTRE_V2_USER_STRICT:
-		return ", STIBP: forced";
+		return stibp_always_on ? ", STIBP: always-on"
+				       : ", STIBP: forced";
 	case SPECTRE_V2_USER_PRCTL:
 	case SPECTRE_V2_USER_SECCOMP:
 		if (static_key_enabled(&switch_to_cond_stibp))

Re: [PATCH v2] x86/speculation: Add support for STIBP always-on preferred mode

[Borislav Petkov]

On Tue, Dec 11, 2018 at 10:46:16PM +0000, Lendacky, Thomas wrote:
> Different AMD processors may have different implementations of STIBP.
> When STIBP is conditionally enabled, some implementations would benefit
> from having STIBP always on instead of toggling the STIBP bit through MSR
> writes. This preference is advertised through a CPUID feature bit.
> 
> When conditional STIBP support is requested at boot and the CPU advertises
> STIBP always-on mode as preferred, switch to STIBP "on" support. Print a
> message to let the user know this occurred. Also, provide a boolean that
> be used in stibp_state() to return a message tailored to the always-on
> support.
> 
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> 
> This patch is against the x86/pti branch of the tip tree:
>   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/pti
> 
> Changes from v1:
> - Removed explicit SPECTRE_V2_USER_STRICT_PREFERRED mode
> - Added a message when switching to always-on mode
> - Set and used a static boolean for the string in stibp_state()
> 
>  arch/x86/include/asm/cpufeatures.h |    1 +
>  arch/x86/kernel/cpu/bugs.c         |   17 ++++++++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 28c4a50..df8e94e2 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -284,6 +284,7 @@
>  #define X86_FEATURE_AMD_IBPB		(13*32+12) /* "" Indirect Branch Prediction Barrier */
>  #define X86_FEATURE_AMD_IBRS		(13*32+14) /* "" Indirect Branch Restricted Speculation */
>  #define X86_FEATURE_AMD_STIBP		(13*32+15) /* "" Single Thread Indirect Branch Predictors */
> +#define X86_FEATURE_AMD_STIBP_ALWAYS_ON	(13*32+17) /* "" Single Thread Indirect Branch Predictors always-on preferred */
>  #define X86_FEATURE_AMD_SSBD		(13*32+24) /* "" Speculative Store Bypass Disable */
>  #define X86_FEATURE_VIRT_SSBD		(13*32+25) /* Virtualized Speculative Store Bypass Disable */
>  #define X86_FEATURE_AMD_SSB_NO		(13*32+26) /* "" Speculative Store Bypass is fixed in hardware. */
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 58689ac..db156e1 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -61,6 +61,8 @@
>  /* Control unconditional IBPB in switch_mm() */
>  DEFINE_STATIC_KEY_FALSE(switch_mm_always_ibpb);
>  
> +static bool stibp_always_on;
> +
>  void __init check_bugs(void)
>  {
>  	identify_boot_cpu();
> @@ -355,6 +357,18 @@ static void __init spec_v2_user_print_cond(const char *reason, bool secure)
>  		break;
>  	}
>  
> +	/*
> +	 * At this point, an STIBP mode other than "off" has been set.
> +	 * If STIBP support is not being forced, check if STIBP always-on
> +	 * is preferred.
> +	 */
> +	if (mode != SPECTRE_V2_USER_STRICT &&
> +	    boot_cpu_has(X86_FEATURE_AMD_STIBP_ALWAYS_ON)) {
> +		stibp_always_on = true;
> +		mode = SPECTRE_V2_USER_STRICT;
> +		pr_info("mitigation: STIBP always-on is preferred\n");
> +	}
> +
>  	/* Initialize Indirect Branch Prediction Barrier */
>  	if (boot_cpu_has(X86_FEATURE_IBPB)) {
>  		setup_force_cpu_cap(X86_FEATURE_USE_IBPB);
> @@ -1088,7 +1102,8 @@ static char *stibp_state(void)
>  	case SPECTRE_V2_USER_NONE:
>  		return ", STIBP: disabled";
>  	case SPECTRE_V2_USER_STRICT:
> -		return ", STIBP: forced";
> +		return stibp_always_on ? ", STIBP: always-on"
> +				       : ", STIBP: forced";

I still don't like that separate stibp_always_on variable when we can do
all the querying just by using mode and X86_FEATURE_AMD_STIBP_ALWAYS_ON.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v2] x86/speculation: Add support for STIBP always-on preferred mode

[Thomas Gleixner]

On Wed, 12 Dec 2018, Borislav Petkov wrote:
> On Tue, Dec 11, 2018 at 10:46:16PM +0000, Lendacky, Thomas wrote:
> > +	/*
> > +	 * At this point, an STIBP mode other than "off" has been set.
> > +	 * If STIBP support is not being forced, check if STIBP always-on
> > +	 * is preferred.
> > +	 */
> > +	if (mode != SPECTRE_V2_USER_STRICT &&
> > +	    boot_cpu_has(X86_FEATURE_AMD_STIBP_ALWAYS_ON)) {
> > +		stibp_always_on = true;
> > +		mode = SPECTRE_V2_USER_STRICT;
> > +		pr_info("mitigation: STIBP always-on is preferred\n");
> > +	}
> > +
> >  	/* Initialize Indirect Branch Prediction Barrier */
> >  	if (boot_cpu_has(X86_FEATURE_IBPB)) {
> >  		setup_force_cpu_cap(X86_FEATURE_USE_IBPB);
> > @@ -1088,7 +1102,8 @@ static char *stibp_state(void)
> >  	case SPECTRE_V2_USER_NONE:
> >  		return ", STIBP: disabled";
> >  	case SPECTRE_V2_USER_STRICT:
> > -		return ", STIBP: forced";
> > +		return stibp_always_on ? ", STIBP: always-on"
> > +				       : ", STIBP: forced";
> 
> I still don't like that separate stibp_always_on variable when we can do
> all the querying just by using mode and X86_FEATURE_AMD_STIBP_ALWAYS_ON.

Hmmm. I've not seen the V1 of this (it's not in my inbox) but the v1->v2
changes contain:

> > - Removed explicit SPECTRE_V2_USER_STRICT_PREFERRED mode

Now I really have to ask why?

Neither the extra variable nor the cpu feature check are pretty. An
explicit mode is way better in terms of code clarity and you get the proper
printout via spectre_v2_user_strings.

Hmm?

	tglx

Re: [PATCH v2] x86/speculation: Add support for STIBP always-on preferred mode

[Borislav Petkov]

On Tue, Dec 11, 2018 at 07:37:26PM -0800, Thomas Gleixner wrote:
> Hmmm. I've not seen the V1 of this (it's not in my inbox) but the v1->v2
> changes contain:

You're on CC:

https://lkml.kernel.org/r/20181211190959.28321.56433.stgit@tlendack-t1.amdoffice.net

> 
> > > - Removed explicit SPECTRE_V2_USER_STRICT_PREFERRED mode
> 
> Now I really have to ask why?
> 
> Neither the extra variable nor the cpu feature check are pretty. An
> explicit mode is way better in terms of code clarity and you get the proper
> printout via spectre_v2_user_strings.

Actually, now that I've slept on it, I think we should do this just
like X86_FEATURE_IBRS_ENHANCED - as a modifier for SPECTRE_V2_CMD_AUTO
or _FORCE options.

And that has its own mode SPECTRE_V2_IBRS_ENHANCED.

Makes sense?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v2] x86/speculation: Add support for STIBP always-on preferred mode

[Lendacky, Thomas]

On 12/11/2018 09:37 PM, Thomas Gleixner wrote:
> On Wed, 12 Dec 2018, Borislav Petkov wrote:
>> On Tue, Dec 11, 2018 at 10:46:16PM +0000, Lendacky, Thomas wrote:
>>> +	/*
>>> +	 * At this point, an STIBP mode other than "off" has been set.
>>> +	 * If STIBP support is not being forced, check if STIBP always-on
>>> +	 * is preferred.
>>> +	 */
>>> +	if (mode != SPECTRE_V2_USER_STRICT &&
>>> +	    boot_cpu_has(X86_FEATURE_AMD_STIBP_ALWAYS_ON)) {
>>> +		stibp_always_on = true;
>>> +		mode = SPECTRE_V2_USER_STRICT;
>>> +		pr_info("mitigation: STIBP always-on is preferred\n");
>>> +	}
>>> +
>>>  	/* Initialize Indirect Branch Prediction Barrier */
>>>  	if (boot_cpu_has(X86_FEATURE_IBPB)) {
>>>  		setup_force_cpu_cap(X86_FEATURE_USE_IBPB);
>>> @@ -1088,7 +1102,8 @@ static char *stibp_state(void)
>>>  	case SPECTRE_V2_USER_NONE:
>>>  		return ", STIBP: disabled";
>>>  	case SPECTRE_V2_USER_STRICT:
>>> -		return ", STIBP: forced";
>>> +		return stibp_always_on ? ", STIBP: always-on"
>>> +				       : ", STIBP: forced";
>>
>> I still don't like that separate stibp_always_on variable when we can do
>> all the querying just by using mode and X86_FEATURE_AMD_STIBP_ALWAYS_ON.
> 
> Hmmm. I've not seen the V1 of this (it's not in my inbox) but the v1->v2
> changes contain:

That's strange, you were on the cc: list. Anyway, here's a link to the
first version: https://lkml.org/lkml/2018/12/11/1248

> 
>>> - Removed explicit SPECTRE_V2_USER_STRICT_PREFERRED mode
> 
> Now I really have to ask why?
> 
> Neither the extra variable nor the cpu feature check are pretty. An
> explicit mode is way better in terms of code clarity and you get the proper
> printout via spectre_v2_user_strings.
> 
> Hmm?

That is what the first version did. See if that's in-line with what
you're thinking.

Thanks,
Tom

> 
> 	tglx
> 

Re: [PATCH v2] x86/speculation: Add support for STIBP always-on preferred mode

[Lendacky, Thomas]

On 12/12/2018 03:59 AM, Borislav Petkov wrote:
> On Tue, Dec 11, 2018 at 07:37:26PM -0800, Thomas Gleixner wrote:
>> Hmmm. I've not seen the V1 of this (it's not in my inbox) but the v1->v2
>> changes contain:
> 
> You're on CC:
> 
> https://lkml.kernel.org/r/20181211190959.28321.56433.stgit@tlendack-t1.amdoffice.net
> 
>>
>>>> - Removed explicit SPECTRE_V2_USER_STRICT_PREFERRED mode
>>
>> Now I really have to ask why?
>>
>> Neither the extra variable nor the cpu feature check are pretty. An
>> explicit mode is way better in terms of code clarity and you get the proper
>> printout via spectre_v2_user_strings.
> 
> Actually, now that I've slept on it, I think we should do this just
> like X86_FEATURE_IBRS_ENHANCED - as a modifier for SPECTRE_V2_CMD_AUTO
> or _FORCE options.
> 
> And that has its own mode SPECTRE_V2_IBRS_ENHANCED.
> 
> Makes sense?

Not sure I completely follow. Are you saying to do what I did in my
first patch or something different from that yet?

Thanks,
Tom

> 

Re: [PATCH v2] x86/speculation: Add support for STIBP always-on preferred mode

[Thomas Gleixner]

On Wed, 12 Dec 2018, Lendacky, Thomas wrote:
> On 12/11/2018 09:37 PM, Thomas Gleixner wrote:
> > On Wed, 12 Dec 2018, Borislav Petkov wrote:
> >> I still don't like that separate stibp_always_on variable when we can do
> >> all the querying just by using mode and X86_FEATURE_AMD_STIBP_ALWAYS_ON.
> > 
> > Hmmm. I've not seen the V1 of this (it's not in my inbox) but the v1->v2
> > changes contain:
> 
> That's strange, you were on the cc: list. Anyway, here's a link to the
> first version: https://lkml.org/lkml/2018/12/11/1248

Must have been my sleep deprived brain. Found it now :) Sorry for not
paying attention back then.

> >>> - Removed explicit SPECTRE_V2_USER_STRICT_PREFERRED mode
> > 
> > Now I really have to ask why?
> > 
> > Neither the extra variable nor the cpu feature check are pretty. An
> > explicit mode is way better in terms of code clarity and you get the proper
> > printout via spectre_v2_user_strings.
> > 
> > Hmm?
> 
> That is what the first version did. See if that's in-line with what
> you're thinking.

Yes, though I'm not too fond about the preferred wording, but can't come up
with anything better.

Thanks,

	tglx

Re: [PATCH v2] x86/speculation: Add support for STIBP always-on preferred mode

[Boris Petkov]

On December 12, 2018 3:04:35 PM GMT+01:00, "Lendacky, Thomas" <Thomas.Lendacky@amd.com> wrote:
>Not sure I completely follow. Are you saying to do what I did in my
>first patch or something different from that yet?

I'm saying that STIBP_ALWAYS_ON should be implemented the same way like IBRS_ENHANCED is and there's no need for a static bool. AFAICT.

Or am I missing something? 

-- 
Sent from a small device: formatting sux and brevity is inevitable.

Re: [PATCH v2] x86/speculation: Add support for STIBP always-on preferred mode

[Lendacky, Thomas]

On 12/12/2018 08:32 AM, Boris Petkov wrote:
> On December 12, 2018 3:04:35 PM GMT+01:00, "Lendacky, Thomas" <Thomas.Lendacky@amd.com> wrote:
>> Not sure I completely follow. Are you saying to do what I did in my
>> first patch or something different from that yet?
> 
> I'm saying that STIBP_ALWAYS_ON should be implemented the same way like IBRS_ENHANCED is and there's no need for a static bool. AFAICT.
> 
> Or am I missing something? 

Ok, I think you're saying to do what my first patch did or something very
close to that. Which is to just set always-on mode if STIBP protection is
requested and the STIBP_ALWAYS_ON CPUID bit is set (even if the equivalent
"=on" is supplied on the command line?). That would mean having the new
mode and new string(s).

If that's not what you're saying, then maybe I just need more coffee this
morning :)

Thanks,
Tom

> 

Re: [PATCH v2] x86/speculation: Add support for STIBP always-on preferred mode

[Lendacky, Thomas]

On 12/12/2018 08:23 AM, Thomas Gleixner wrote:
> On Wed, 12 Dec 2018, Lendacky, Thomas wrote:
>> On 12/11/2018 09:37 PM, Thomas Gleixner wrote:
>>> On Wed, 12 Dec 2018, Borislav Petkov wrote:
>>>> I still don't like that separate stibp_always_on variable when we can do
>>>> all the querying just by using mode and X86_FEATURE_AMD_STIBP_ALWAYS_ON.
>>>
>>> Hmmm. I've not seen the V1 of this (it's not in my inbox) but the v1->v2
>>> changes contain:
>>
>> That's strange, you were on the cc: list. Anyway, here's a link to the
>> first version: https://lkml.org/lkml/2018/12/11/1248
> 
> Must have been my sleep deprived brain. Found it now :) Sorry for not
> paying attention back then.
> 
>>>>> - Removed explicit SPECTRE_V2_USER_STRICT_PREFERRED mode
>>>
>>> Now I really have to ask why?
>>>
>>> Neither the extra variable nor the cpu feature check are pretty. An
>>> explicit mode is way better in terms of code clarity and you get the proper
>>> printout via spectre_v2_user_strings.
>>>
>>> Hmm?
>>
>> That is what the first version did. See if that's in-line with what
>> you're thinking.
> 
> Yes, though I'm not too fond about the preferred wording, but can't come up
> with anything better.

I'm not crazy about that either. Maybe getting rid of "preferred" and just
having "always-on" for the wording is enough?

Thanks,
Tom

> 
> Thanks,
> 
> 	tglx
>