| Message ID | 20240807123531.69677-1-amit@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] KVM: SVM: let alternatives handle the cases when RSB filling is required | expand |
Pinging to bring attention to this fix. On Wed, 2024-08-07 at 14:35 +0200, Amit Shah wrote: > From: Amit Shah <amit.shah@amd.com> > > Remove superfluous RSB filling after a VMEXIT when the CPU already > has > flushed the RSB after a VMEXIT when AutoIBRS is enabled. > > The initial implementation for adding RETPOLINES added an > ALTERNATIVES > implementation for filling the RSB after a VMEXIT in > > commit 117cc7a908c836 ("x86/retpoline: Fill return stack buffer on > vmexit") > > Later, X86_FEATURE_RSB_VMEXIT was added in > > commit 2b129932201673 ("x86/speculation: Add RSB VM Exit > protections") > > The AutoIBRS (on AMD CPUs) feature implementation added in > > commit e7862eda309ecf ("x86/cpu: Support AMD Automatic IBRS") > > used the already-implemented logic for EIBRS in > spectre_v2_determine_rsb_fill_type_on_vmexit() -- but did not update > the > code at VMEXIT to act on the mode selected in that function -- > resulting > in VMEXITs continuing to clear the RSB when RETPOLINES are enabled, > despite the presence of AutoIBRS. > > Signed-off-by: Amit Shah <amit.shah@amd.com> > > --- > v4: resend of v3 with subject-prefix fixed > > v3: > - Add a comment mentioning SVM does not need RSB_VMEXIT_LITE unlike > VMX. > v2: > - tweak commit message re: Boris's comments. > > arch/x86/kvm/svm/vmenter.S | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kvm/svm/vmenter.S b/arch/x86/kvm/svm/vmenter.S > index a0c8eb37d3e1..69d9825ebdd9 100644 > --- a/arch/x86/kvm/svm/vmenter.S > +++ b/arch/x86/kvm/svm/vmenter.S > @@ -209,10 +209,14 @@ SYM_FUNC_START(__svm_vcpu_run) > 7: vmload %_ASM_AX > 8: > > -#ifdef CONFIG_MITIGATION_RETPOLINE > - /* IMPORTANT: Stuff the RSB immediately after VM-Exit, > before RET! */ > - FILL_RETURN_BUFFER %_ASM_AX, RSB_CLEAR_LOOPS, > X86_FEATURE_RETPOLINE > -#endif > + /* > + * IMPORTANT: Stuff the RSB immediately after VM-Exit, > before RET! > + * > + * Unlike VMX, AMD does not have the hardware bug that > necessitates > + * RSB_VMEXIT_LITE > + */ > + > + FILL_RETURN_BUFFER %_ASM_AX, RSB_CLEAR_LOOPS, > X86_FEATURE_RSB_VMEXIT > > /* Clobbers RAX, RCX, RDX. */ > RESTORE_HOST_SPEC_CTRL > @@ -348,10 +352,14 @@ SYM_FUNC_START(__svm_sev_es_vcpu_run) > > 2: cli > > -#ifdef CONFIG_MITIGATION_RETPOLINE > - /* IMPORTANT: Stuff the RSB immediately after VM-Exit, > before RET! */ > - FILL_RETURN_BUFFER %rax, RSB_CLEAR_LOOPS, > X86_FEATURE_RETPOLINE > -#endif > + /* > + * IMPORTANT: Stuff the RSB immediately after VM-Exit, > before RET! > + * > + * Unlike VMX, AMD does not have the hardware bug that > necessitates > + * RSB_VMEXIT_LITE > + */ > + > + FILL_RETURN_BUFFER %rax, RSB_CLEAR_LOOPS, > X86_FEATURE_RSB_VMEXIT > > /* Clobbers RAX, RCX, RDX, consumes RDI (@svm) and RSI > (@spec_ctrl_intercepted). */ > RESTORE_HOST_SPEC_CTRL
On Wed, Aug 07, 2024, Amit Shah wrote: > From: Amit Shah <amit.shah@amd.com> > > Remove superfluous RSB filling after a VMEXIT when the CPU already has > flushed the RSB after a VMEXIT when AutoIBRS is enabled. > > The initial implementation for adding RETPOLINES added an ALTERNATIVES > implementation for filling the RSB after a VMEXIT in > > commit 117cc7a908c836 ("x86/retpoline: Fill return stack buffer on vmexit") Nit, no need for 14 digits, 12 is still the "official" recommendation. To make this flow better, I would also prefer to not have each commit reference be on its own line. > Later, X86_FEATURE_RSB_VMEXIT was added in > > commit 2b129932201673 ("x86/speculation: Add RSB VM Exit protections") Oh, the irony. That commit didn't add RSB_VMEXIT, it added RSB_VMEXIT_LITE. Commit 9756bba28470 ("x86/speculation: Fill RSB on vmexit for IBRS") added the "heavy" version. This is also a good opportunity to call out with RSB_VMEXIT actually does. > The AutoIBRS (on AMD CPUs) feature implementation added in > > commit e7862eda309ecf ("x86/cpu: Support AMD Automatic IBRS") > > used the already-implemented logic for EIBRS in > spectre_v2_determine_rsb_fill_type_on_vmexit() -- but did not update the > code at VMEXIT to act on the mode selected in that function -- resulting > in VMEXITs continuing to clear the RSB when RETPOLINES are enabled, > despite the presence of AutoIBRS. > > Signed-off-by: Amit Shah <amit.shah@amd.com> > > --- > diff --git a/arch/x86/kvm/svm/vmenter.S b/arch/x86/kvm/svm/vmenter.S > index a0c8eb37d3e1..69d9825ebdd9 100644 > --- a/arch/x86/kvm/svm/vmenter.S > +++ b/arch/x86/kvm/svm/vmenter.S > @@ -209,10 +209,14 @@ SYM_FUNC_START(__svm_vcpu_run) > 7: vmload %_ASM_AX > 8: > > -#ifdef CONFIG_MITIGATION_RETPOLINE > - /* IMPORTANT: Stuff the RSB immediately after VM-Exit, before RET! */ > - FILL_RETURN_BUFFER %_ASM_AX, RSB_CLEAR_LOOPS, X86_FEATURE_RETPOLINE > -#endif > + /* > + * IMPORTANT: Stuff the RSB immediately after VM-Exit, before RET! > + * > + * Unlike VMX, AMD does not have the hardware bug that necessitates > + * RSB_VMEXIT_LITE > + */ I would rather do nothing than carry these comments. Long term, I would still prefer to have RSB_VMEXIT_LITE, but that's not a hill worth dying on, and I am a-ok waiting to deal with that if/when I (or someone else) takes on the task of unifying the VM-Enter/VM-Exit flows. So, with the comment changes dropped and the changelog massaged to this: Remove superfluous RSB filling after a VMEXIT when the CPU already has flushed the RSB after a VMEXIT when AutoIBRS is enabled. The initial implementation for adding RETPOLINES added an ALTERNATIVES implementation for filling the RSB after a VMEXIT in commit 117cc7a908c8 ("x86/retpoline: Fill return stack buffer on vmexit"). Later, X86_FEATURE_RSB_VMEXIT was added in commit 9756bba28470 ("x86/speculation: Fill RSB on vmexit for IBRS") to handle stuffing the RSB if RETPOLINE=y *or* KERNEL_IBRS=y, i.e. to also stuff the RSB if the kernel is configured to do IBRS mitigations on entry/exit. The AutoIBRS (on AMD) feature implementation added in commit e7862eda309e ("x86/cpu: Support AMD Automatic IBRS") used the already-implemented logic for EIBRS in spectre_v2_determine_rsb_fill_type_on_vmexit() -- but did not update the code at VMEXIT to act on the mode selected in that function -- resulting in VMEXITs continuing to clear the RSB when RETPOLINES are enabled, despite the presence of AutoIBRS. Applied to kvm-x86 svm. [1/1] KVM: SVM: let alternatives handle the cases when RSB filling is required https://github.com/kvm-x86/linux/commit/4440337af4d4 -- https://github.com/kvm-x86/linux/tree/next
diff --git a/arch/x86/kvm/svm/vmenter.S b/arch/x86/kvm/svm/vmenter.S index a0c8eb37d3e1..69d9825ebdd9 100644 --- a/arch/x86/kvm/svm/vmenter.S +++ b/arch/x86/kvm/svm/vmenter.S @@ -209,10 +209,14 @@ SYM_FUNC_START(__svm_vcpu_run) 7: vmload %_ASM_AX 8: -#ifdef CONFIG_MITIGATION_RETPOLINE - /* IMPORTANT: Stuff the RSB immediately after VM-Exit, before RET! */ - FILL_RETURN_BUFFER %_ASM_AX, RSB_CLEAR_LOOPS, X86_FEATURE_RETPOLINE -#endif + /* + * IMPORTANT: Stuff the RSB immediately after VM-Exit, before RET! + * + * Unlike VMX, AMD does not have the hardware bug that necessitates + * RSB_VMEXIT_LITE + */ + + FILL_RETURN_BUFFER %_ASM_AX, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_VMEXIT /* Clobbers RAX, RCX, RDX. */ RESTORE_HOST_SPEC_CTRL @@ -348,10 +352,14 @@ SYM_FUNC_START(__svm_sev_es_vcpu_run) 2: cli -#ifdef CONFIG_MITIGATION_RETPOLINE - /* IMPORTANT: Stuff the RSB immediately after VM-Exit, before RET! */ - FILL_RETURN_BUFFER %rax, RSB_CLEAR_LOOPS, X86_FEATURE_RETPOLINE -#endif + /* + * IMPORTANT: Stuff the RSB immediately after VM-Exit, before RET! + * + * Unlike VMX, AMD does not have the hardware bug that necessitates + * RSB_VMEXIT_LITE + */ + + FILL_RETURN_BUFFER %rax, RSB_CLEAR_LOOPS, X86_FEATURE_RSB_VMEXIT /* Clobbers RAX, RCX, RDX, consumes RDI (@svm) and RSI (@spec_ctrl_intercepted). */ RESTORE_HOST_SPEC_CTRL