| 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 |
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.
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))
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))
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))
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?
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 --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);