diff mbox series

[RFC,v9,08/51] x86/speculation: Do not enable Automatic IBRS if SEV SNP is enabled

Message ID 20230612042559.375660-9-michael.roth@amd.com (mailing list archive)
State New
Headers show
Series Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support | expand

Commit Message

Michael Roth June 12, 2023, 4:25 a.m. UTC
From: Kim Phillips <kim.phillips@amd.com>

A hardware limitation prevents the host from enabling Automatic IBRS
when SNP is enabled.  Instead, fall back to retpolines.

Signed-off-by: Kim Phillips <kim.phillips@amd.com>
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 arch/x86/kernel/cpu/bugs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Dave Hansen June 12, 2023, 3:39 p.m. UTC | #1
On 6/11/23 21:25, Michael Roth wrote:
> A hardware limitation prevents the host from enabling Automatic IBRS
> when SNP is enabled.  Instead, fall back to retpolines.

"Hardware limitation"?  As in, it is a documented, architectural
restriction?  Or, it's a CPU bug?

> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index f9d060e71c3e..3fba3623ff64 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -1507,7 +1507,12 @@ static void __init spectre_v2_select_mitigation(void)
>  
>  	if (spectre_v2_in_ibrs_mode(mode)) {
>  		if (boot_cpu_has(X86_FEATURE_AUTOIBRS)) {
> -			msr_set_bit(MSR_EFER, _EFER_AUTOIBRS);
> +			if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) {
> +				msr_set_bit(MSR_EFER, _EFER_AUTOIBRS);
> +			} else {
> +				pr_err("SNP feature available, not enabling AutoIBRS on the host.\n");
> +				mode = spectre_v2_select_retpoline();
> +			}

I think this would be nicer if you did something like:

	if (cpu_feature_enabled(X86_FEATURE_SEV_SNP))
		setup_clear_cpu_cap(X86_FEATURE_AUTOIBRS);

somewhere _else_ in the code instead of smack-dab in the middle of the
mitigation selection.
Kim Phillips July 18, 2023, 10:34 p.m. UTC | #2
On 6/12/23 10:39 AM, Dave Hansen wrote:
> On 6/11/23 21:25, Michael Roth wrote:
>> A hardware limitation prevents the host from enabling Automatic IBRS
>> when SNP is enabled.  Instead, fall back to retpolines.
> 
> "Hardware limitation"?  As in, it is a documented, architectural
> restriction?  Or, it's a CPU bug?

It's a documented, architectural restriction:  When Secure Nested Paging
(SEV-SNP) is enabled, processes running as host are protected, but
those running with a non-zero ASID, are not.

>> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
>> index f9d060e71c3e..3fba3623ff64 100644
>> --- a/arch/x86/kernel/cpu/bugs.c
>> +++ b/arch/x86/kernel/cpu/bugs.c
>> @@ -1507,7 +1507,12 @@ static void __init spectre_v2_select_mitigation(void)
>>   
>>   	if (spectre_v2_in_ibrs_mode(mode)) {
>>   		if (boot_cpu_has(X86_FEATURE_AUTOIBRS)) {
>> -			msr_set_bit(MSR_EFER, _EFER_AUTOIBRS);
>> +			if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) {
>> +				msr_set_bit(MSR_EFER, _EFER_AUTOIBRS);
>> +			} else {
>> +				pr_err("SNP feature available, not enabling AutoIBRS on the host.\n");
>> +				mode = spectre_v2_select_retpoline();
>> +			}
> 
> I think this would be nicer if you did something like:
> 
> 	if (cpu_feature_enabled(X86_FEATURE_SEV_SNP))
> 		setup_clear_cpu_cap(X86_FEATURE_AUTOIBRS);
> 
> somewhere _else_ in the code instead of smack-dab in the middle of the
> mitigation selection.

Good idea.  How about this?:

 From 6cf32e8d8426190b1bf1b1e04ceb35bf0bac784b Mon Sep 17 00:00:00 2001
From: Kim Phillips <kim.phillips@amd.com>
Date: Mon, 17 Jul 2023 14:08:15 -0500
Subject: [PATCH] x86/speculation: Do not enable Automatic IBRS if SEV SNP is
  enabled

Automatic IBRS provides protection to [1]:

  - Processes running at CPL=0
  - Processes running as host when Secure Nested Paging (SEV-SNP) is enabled

i.e.,

	(CPL < 3) || ((ASID == 0) && SNP)

Because of this limitation, do not enable Automatic IBRS when SNP is enabled.
Instead, fall back to retpolines.

Note that the AutoIBRS feature may continue to be used within the guest.

[1] "AMD64 Architecture Programmer's Manual Volume 2: System Programming",
     Pub. 24593, rev. 3.41, June 2023, Part 1, Section 3.1.7 "Extended Feature
     Enable Register (EFER)", Automatic IBRS Enable (AIBRSE) Bit.

Link: https://bugzilla.kernel.org/attachment.cgi?id=304652
Signed-off-by: Kim Phillips <kim.phillips@amd.com>
---
  arch/x86/kernel/cpu/common.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 8cd4126d8253..311c0a6422b5 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1348,7 +1348,8 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
  	 * AMD's AutoIBRS is equivalent to Intel's eIBRS - use the Intel feature
  	 * flag and protect from vendor-specific bugs via the whitelist.
  	 */
-	if ((ia32_cap & ARCH_CAP_IBRS_ALL) || cpu_has(c, X86_FEATURE_AUTOIBRS)) {
+	if ((ia32_cap & ARCH_CAP_IBRS_ALL) || (cpu_has(c, X86_FEATURE_AUTOIBRS) &&
+	    !cpu_feature_enabled(X86_FEATURE_SEV_SNP))) {
  		setup_force_cpu_cap(X86_FEATURE_IBRS_ENHANCED);
  		if (!cpu_matches(cpu_vuln_whitelist, NO_EIBRS_PBRSB) &&
  		    !(ia32_cap & ARCH_CAP_PBRSB_NO))
Dave Hansen July 18, 2023, 11:17 p.m. UTC | #3
On 7/18/23 15:34, Kim Phillips wrote:
...
> Automatic IBRS provides protection to [1]:
> 
>  - Processes running at CPL=0
>  - Processes running as host when Secure Nested Paging (SEV-SNP) is enabled
> 
> i.e.,
> 
>     (CPL < 3) || ((ASID == 0) && SNP)
> 
> Because of this limitation, do not enable Automatic IBRS when SNP is
> enabled.

Gah, I found that hard to parse.  I think it's because you're talking
about an SEV-SNP host in one part and "SNP" in the other but _meaning_
SNP host and SNP guest.

Could I maybe suggest that you folks follow the TDX convention and
actually add _GUEST and _HOST to the feature name be explicit about
which side is which?

> Instead, fall back to retpolines.

Now I'm totally lost.

This is talking about falling back to retpolines ... in the kernel.  But
"Automatic IBRS provides protection to ... CPL < 3", aka. the kernel.

> Note that the AutoIBRS feature may continue to be used within the
> guest.

What is this trying to say?

"AutoIBRS can still be used in a guest since it protects CPL < 3"

or

"The AutoIBRS bits can still be twiddled within the guest even though it
doesn't do any good"

?

> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 8cd4126d8253..311c0a6422b5 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1348,7 +1348,8 @@ static void __init cpu_set_bug_bits(struct
> cpuinfo_x86 *c)
>       * AMD's AutoIBRS is equivalent to Intel's eIBRS - use the Intel feature
>       * flag and protect from vendor-specific bugs via the whitelist.
>       */
> -    if ((ia32_cap & ARCH_CAP_IBRS_ALL) || cpu_has(c, X86_FEATURE_AUTOIBRS)) {
> +    if ((ia32_cap & ARCH_CAP_IBRS_ALL) || (cpu_has(c, X86_FEATURE_AUTOIBRS) &&
> +        !cpu_feature_enabled(X86_FEATURE_SEV_SNP))) {
>          setup_force_cpu_cap(X86_FEATURE_IBRS_ENHANCED);
>          if (!cpu_matches(cpu_vuln_whitelist, NO_EIBRS_PBRSB) &&
>              !(ia32_cap & ARCH_CAP_PBRSB_NO))
Kim Phillips July 20, 2023, 7:11 p.m. UTC | #4
On 7/18/23 6:17 PM, Dave Hansen wrote:
> On 7/18/23 15:34, Kim Phillips wrote:
> ...
>> Automatic IBRS provides protection to [1]:
>>
>>   - Processes running at CPL=0
>>   - Processes running as host when Secure Nested Paging (SEV-SNP) is enabled
>>
>> i.e.,
>>
>>      (CPL < 3) || ((ASID == 0) && SNP)
>>
>> Because of this limitation, do not enable Automatic IBRS when SNP is
>> enabled.
> 
> Gah, I found that hard to parse.  I think it's because you're talking
> about an SEV-SNP host in one part and "SNP" in the other but _meaning_
> SNP host and SNP guest.
> 
> Could I maybe suggest that you folks follow the TDX convention and
> actually add _GUEST and _HOST to the feature name be explicit about
> which side is which?
> 
>> Instead, fall back to retpolines.
> 
> Now I'm totally lost.
> 
> This is talking about falling back to retpolines ... in the kernel.  But
> "Automatic IBRS provides protection to ... CPL < 3", aka. the kernel.
> 
>> Note that the AutoIBRS feature may continue to be used within the
>> guest.
> 
> What is this trying to say?
> 
> "AutoIBRS can still be used in a guest since it protects CPL < 3"
> 
> or
> 
> "The AutoIBRS bits can still be twiddled within the guest even though it
> doesn't do any good"
> 
> ?

Hopefully the commit text in this version will help answer all your
questions?:

 From 96dbd72d018287bc5b72f6083884e2125c9d09bc Mon Sep 17 00:00:00 2001
From: Kim Phillips <kim.phillips@amd.com>
Date: Mon, 17 Jul 2023 14:08:15 -0500
Subject: [PATCH] x86/speculation: Do not enable Automatic IBRS if SEV SNP is
  enabled

Automatic IBRS provides protection to [1]:

  - Processes running at CPL=0
  - Processes running as host when Secure Nested Paging (SEV-SNP) is enabled

I.e., from the host side (ASID=0, based on host EFER.AutoIBRS)
If SYSCFG[SNPEn]=0 then:
      IBRS is enabled for supervisor mode (CPL < 3) only

If SYSCFG[SNPEn]=1 then:
      IBRS is enabled at all CPLs

 From the guest side (ASID!=0, based on guest EFER.AutoIBRS)
      IBRS is enabled for supervisor mode (CPL < 3)

Therefore, don't enable Automatic IBRS in host mode if SNP is enabled,
because it will penalize user-mode indirect branch performance.  Have
the kernel fall back to retpolines instead.

Note that the AutoIBRS feature may continue to be used within guests,
where ASID != 0.

[1] "AMD64 Architecture Programmer's Manual Volume 2: System Programming",
     Pub. 24593, rev. 3.41, June 2023, Part 1, Section 3.1.7 "Extended
     Feature Enable Register (EFER)" - accessible via Link.

Link: https://bugzilla.kernel.org/attachment.cgi?id=304652
Signed-off-by: Kim Phillips <kim.phillips@amd.com>
---
  arch/x86/kernel/cpu/common.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 8cd4126d8253..311c0a6422b5 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1348,7 +1348,8 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
  	 * AMD's AutoIBRS is equivalent to Intel's eIBRS - use the Intel feature
  	 * flag and protect from vendor-specific bugs via the whitelist.
  	 */
-	if ((ia32_cap & ARCH_CAP_IBRS_ALL) || cpu_has(c, X86_FEATURE_AUTOIBRS)) {
+	if ((ia32_cap & ARCH_CAP_IBRS_ALL) || (cpu_has(c, X86_FEATURE_AUTOIBRS) &&
+	    !cpu_feature_enabled(X86_FEATURE_SEV_SNP))) {
  		setup_force_cpu_cap(X86_FEATURE_IBRS_ENHANCED);
  		if (!cpu_matches(cpu_vuln_whitelist, NO_EIBRS_PBRSB) &&
  		    !(ia32_cap & ARCH_CAP_PBRSB_NO))
Dave Hansen July 20, 2023, 10:24 p.m. UTC | #5
On 7/20/23 12:11, Kim Phillips wrote:
> Hopefully the commit text in this version will help answer all your
> questions?:

To be honest, it didn't really.  I kinda feel like I was having the APM
contents tossed casually in my direction rather than being provided a
fully considered explanation.

Here's what I came up with instead:

Host-side Automatic IBRS has different behavior based on whether SEV-SNP
is enabled.

Without SEV-SNP, Automatic IBRS protects only the kernel.  But when
SEV-SNP is enabled, the Automatic IBRS protection umbrella widens to all
host-side code, including userspace.  This protection comes at a cost:
reduced userspace indirect branch performance.

To avoid this performance loss, nix using Automatic IBRS on SEV-SNP
hosts.  Fall back to retpolines instead.

=====

Is that about right?

I don't think any chit-chat about the guest side is even relevant.

This also absolutely needs a comment.  Perhaps just pull the code up to
the top level of the function and do this:

	/*
	 * Automatic IBRS imposes unacceptable overhead on host
	 * userspace for SEV-SNP systems.  Zap it instead.
	 */
	if (cpu_feature_enabled(X86_FEATURE_SEV_SNP))
		setup_clear_cpu_cap(X86_FEATURE_AUTOIBRS);

BTW, I assume you've grumbled to folks about this.  It's an awful shame
the hardware (or ucode) was built this was.  It's just throwing
Automatic IBRS out the window because it's not architected in a nice way.

Is there any plan to improve this?
Kim Phillips July 21, 2023, 4:56 p.m. UTC | #6
On 7/20/23 5:24 PM, Dave Hansen wrote:
> On 7/20/23 12:11, Kim Phillips wrote:
>> Hopefully the commit text in this version will help answer all your
>> questions?:
> 
> To be honest, it didn't really.  I kinda feel like I was having the APM
> contents tossed casually in my direction rather than being provided a
> fully considered explanation.

Sorry to hear that, it wasn't the intention.

> Here's what I came up with instead:
> 
> Host-side Automatic IBRS has different behavior based on whether SEV-SNP
> is enabled.
> 
> Without SEV-SNP, Automatic IBRS protects only the kernel.  But when
> SEV-SNP is enabled, the Automatic IBRS protection umbrella widens to all
> host-side code, including userspace.  This protection comes at a cost:
> reduced userspace indirect branch performance.
> 
> To avoid this performance loss, nix using Automatic IBRS on SEV-SNP
> hosts.  Fall back to retpolines instead.
> 
> =====
> 
> Is that about right?

Sure, see new version below.

> I don't think any chit-chat about the guest side is even relevant.
> 
> This also absolutely needs a comment.  Perhaps just pull the code up to
> the top level of the function and do this:
> 
> 	/*
> 	 * Automatic IBRS imposes unacceptable overhead on host
> 	 * userspace for SEV-SNP systems.  Zap it instead.
> 	 */
> 	if (cpu_feature_enabled(X86_FEATURE_SEV_SNP))
> 		setup_clear_cpu_cap(X86_FEATURE_AUTOIBRS);

Clearing X86_FEATURE_AUTOIBRS won't work because it'll unnecessarily
prohibit KVM from subsequently advertising the feature to guests.

I've tried to address the comment comment, see below.

> BTW, I assume you've grumbled to folks about this.  It's an awful shame
> the hardware (or ucode) was built this was.  It's just throwing
> Automatic IBRS out the window because it's not architected in a nice way.
> 
> Is there any plan to improve this?

Sure.  Until then:

 From fb55df544ed066a3c8fdb1581932a23c03ce6d2c Mon Sep 17 00:00:00 2001
From: Kim Phillips <kim.phillips@amd.com>
Date: Mon, 17 Jul 2023 14:08:15 -0500
Subject: [PATCH] x86/speculation: Don't enable Automatic IBRS if SEV SNP is
  enabled

Host-side Automatic IBRS has a different behaviour depending on whether
SEV-SNP is enabled.

Without SEV-SNP, Automatic IBRS protects only the kernel.  But when
SEV-SNP is enabled, the Automatic IBRS protection umbrella widens to all
host-side code, including userspace.  This protection comes at a cost:
reduced userspace indirect branch performance.

To avoid this performance loss, don't use Automatic IBRS on SEV-SNP
hosts.  Fall back to retpolines instead.

Signed-off-by: Kim Phillips <kim.phillips@amd.com>
---
  arch/x86/kernel/cpu/common.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 8cd4126d8253..a93e6204d847 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1347,8 +1347,11 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
  	/*
  	 * AMD's AutoIBRS is equivalent to Intel's eIBRS - use the Intel feature
  	 * flag and protect from vendor-specific bugs via the whitelist.
+	 * Don't use AutoIBRS when SNP is enabled because it degrades host
+	 * userspace indirect branch performance.
  	 */
-	if ((ia32_cap & ARCH_CAP_IBRS_ALL) || cpu_has(c, X86_FEATURE_AUTOIBRS)) {
+	if ((ia32_cap & ARCH_CAP_IBRS_ALL) || (cpu_has(c, X86_FEATURE_AUTOIBRS) &&
+	    !cpu_feature_enabled(X86_FEATURE_SEV_SNP))) {
  		setup_force_cpu_cap(X86_FEATURE_IBRS_ENHANCED);
  		if (!cpu_matches(cpu_vuln_whitelist, NO_EIBRS_PBRSB) &&
  		    !(ia32_cap & ARCH_CAP_PBRSB_NO))
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index f9d060e71c3e..3fba3623ff64 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1507,7 +1507,12 @@  static void __init spectre_v2_select_mitigation(void)
 
 	if (spectre_v2_in_ibrs_mode(mode)) {
 		if (boot_cpu_has(X86_FEATURE_AUTOIBRS)) {
-			msr_set_bit(MSR_EFER, _EFER_AUTOIBRS);
+			if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) {
+				msr_set_bit(MSR_EFER, _EFER_AUTOIBRS);
+			} else {
+				pr_err("SNP feature available, not enabling AutoIBRS on the host.\n");
+				mode = spectre_v2_select_retpoline();
+			}
 		} else {
 			x86_spec_ctrl_base |= SPEC_CTRL_IBRS;
 			update_spec_ctrl(x86_spec_ctrl_base);