| Message ID | 20220113163833.3831-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/spec-ctrl: Fix NMI race condition | expand |
On Thu, Jan 13, 2022 at 04:38:32PM +0000, Andrew Cooper wrote: > These were written before Spectre/Meltdown went public, and there was large > uncertainty in how the protections would evolve. As it turns out, they're > very specific to Intel hardware, and not very suitable for AMD. > > Expand and drop the macros. No change at all for VT-x. > > For AMD, the only relevant piece of functionality is DO_OVERWRITE_RSB, > although we will soon be adding (different) logic to handle MSR_SPEC_CTRL. > > This has a marginal improvement of removing an unconditional pile of long-nops > from the vmentry/exit path. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> I wonder however if it would be clearer to define SPEC_CTRL_ENTRY_FROM_{SVM,VMX} and EXIT macros in spec_ctrl_asm.h (even if just used in a single place) so that all the related SPEC macros are in a single file. Thanks, Roger.
On 14/01/2022 11:42, Roger Pau Monné wrote: > On Thu, Jan 13, 2022 at 04:38:32PM +0000, Andrew Cooper wrote: >> These were written before Spectre/Meltdown went public, and there was large >> uncertainty in how the protections would evolve. As it turns out, they're >> very specific to Intel hardware, and not very suitable for AMD. >> >> Expand and drop the macros. No change at all for VT-x. >> >> For AMD, the only relevant piece of functionality is DO_OVERWRITE_RSB, >> although we will soon be adding (different) logic to handle MSR_SPEC_CTRL. >> >> This has a marginal improvement of removing an unconditional pile of long-nops >> from the vmentry/exit path. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > I wonder however if it would be clearer to define > SPEC_CTRL_ENTRY_FROM_{SVM,VMX} and EXIT macros in spec_ctrl_asm.h > (even if just used in a single place) so that all the related SPEC > macros are in a single file. For AMD MSR_SPEC_CTRL support, I'm going to need to shift the STGI/CLGI, then call up into C, and I do not thing this is appropriate to have separated out into spec_ctrl_asm.h I left the comments intact deliberately so `grep SPEC_CTRL_ENTRY` still lets you find everything. The main difference between VT-x/SVM and PV is that for HVM, we have this dance exactly once. For PV, it is repeated multiple times in subtly different ways, which is why having all the spec ctrl shadowing logic together makes sense. Its pretty ugly whichever way you look at it. ~Andrew
On 13.01.2022 17:38, Andrew Cooper wrote: > --- a/xen/arch/x86/hvm/svm/entry.S > +++ b/xen/arch/x86/hvm/svm/entry.S > @@ -59,7 +59,7 @@ __UNLIKELY_END(nsvm_hap) > mov VCPUMSR_spec_ctrl_raw(%rax), %eax > > /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */ > - SPEC_CTRL_EXIT_TO_HVM /* Req: a=spec_ctrl %rsp=regs/cpuinfo, Clob: cd */ > + /* SPEC_CTRL_EXIT_TO_SVM (nothing currently) */ > > pop %r15 > pop %r14 > @@ -86,7 +86,8 @@ __UNLIKELY_END(nsvm_hap) > > GET_CURRENT(bx) > > - SPEC_CTRL_ENTRY_FROM_HVM /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */ > + /* SPEC_CTRL_ENTRY_FROM_SVM Req: b=curr %rsp=regs/cpuinfo, Clob: ac */ > + ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM Just for my own understanding: The comments you add aren't commented out macro invocations (as I did read it first), but comments naming would-be-macros which are then expanded "manually"? That is partly because initially I read the description saying "Expand and drop the macros" as meaning that the macros grow in what they contain, which looked contradictory to them getting dropped at the same time. Perhaps me not sufficiently understanding the difference between all possible meanings of "expand" vs "extend" ... Jan
On 17/01/2022 11:22, Jan Beulich wrote: > On 13.01.2022 17:38, Andrew Cooper wrote: >> --- a/xen/arch/x86/hvm/svm/entry.S >> +++ b/xen/arch/x86/hvm/svm/entry.S >> @@ -59,7 +59,7 @@ __UNLIKELY_END(nsvm_hap) >> mov VCPUMSR_spec_ctrl_raw(%rax), %eax >> >> /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */ >> - SPEC_CTRL_EXIT_TO_HVM /* Req: a=spec_ctrl %rsp=regs/cpuinfo, Clob: cd */ >> + /* SPEC_CTRL_EXIT_TO_SVM (nothing currently) */ >> >> pop %r15 >> pop %r14 >> @@ -86,7 +86,8 @@ __UNLIKELY_END(nsvm_hap) >> >> GET_CURRENT(bx) >> >> - SPEC_CTRL_ENTRY_FROM_HVM /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */ >> + /* SPEC_CTRL_ENTRY_FROM_SVM Req: b=curr %rsp=regs/cpuinfo, Clob: ac */ >> + ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM > Just for my own understanding: The comments you add aren't commented > out macro invocations (as I did read it first), but comments naming > would-be-macros which are then expanded "manually"? That is partly > because initially I read the description saying "Expand and drop the > macros" as meaning that the macros grow in what they contain, which > looked contradictory to them getting dropped at the same time. > Perhaps me not sufficiently understanding the difference between all > possible meanings of "expand" vs "extend" ... They're grep fodder to be able to easily locate where we're doing entry/exit speculation logic. These paths will continue to diverge over time, and cannot have a common implementation. ~Andrew
On 17.01.2022 12:41, Andrew Cooper wrote: > On 17/01/2022 11:22, Jan Beulich wrote: >> On 13.01.2022 17:38, Andrew Cooper wrote: >>> --- a/xen/arch/x86/hvm/svm/entry.S >>> +++ b/xen/arch/x86/hvm/svm/entry.S >>> @@ -59,7 +59,7 @@ __UNLIKELY_END(nsvm_hap) >>> mov VCPUMSR_spec_ctrl_raw(%rax), %eax >>> >>> /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */ >>> - SPEC_CTRL_EXIT_TO_HVM /* Req: a=spec_ctrl %rsp=regs/cpuinfo, Clob: cd */ >>> + /* SPEC_CTRL_EXIT_TO_SVM (nothing currently) */ >>> >>> pop %r15 >>> pop %r14 >>> @@ -86,7 +86,8 @@ __UNLIKELY_END(nsvm_hap) >>> >>> GET_CURRENT(bx) >>> >>> - SPEC_CTRL_ENTRY_FROM_HVM /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */ >>> + /* SPEC_CTRL_ENTRY_FROM_SVM Req: b=curr %rsp=regs/cpuinfo, Clob: ac */ >>> + ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM >> Just for my own understanding: The comments you add aren't commented >> out macro invocations (as I did read it first), but comments naming >> would-be-macros which are then expanded "manually"? That is partly >> because initially I read the description saying "Expand and drop the >> macros" as meaning that the macros grow in what they contain, which >> looked contradictory to them getting dropped at the same time. >> Perhaps me not sufficiently understanding the difference between all >> possible meanings of "expand" vs "extend" ... > > They're grep fodder to be able to easily locate where we're doing > entry/exit speculation logic. And I don't suppose I could talk you into replacing the use of "expand" in the description, even if it was just me who was mislead? Perhaps by "Open-code and drop the macros"? > These paths will continue to diverge over > time, and cannot have a common implementation. That's understood. Jan
diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S index e208a4b32ae7..276215d36aff 100644 --- a/xen/arch/x86/hvm/svm/entry.S +++ b/xen/arch/x86/hvm/svm/entry.S @@ -59,7 +59,7 @@ __UNLIKELY_END(nsvm_hap) mov VCPUMSR_spec_ctrl_raw(%rax), %eax /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */ - SPEC_CTRL_EXIT_TO_HVM /* Req: a=spec_ctrl %rsp=regs/cpuinfo, Clob: cd */ + /* SPEC_CTRL_EXIT_TO_SVM (nothing currently) */ pop %r15 pop %r14 @@ -86,7 +86,8 @@ __UNLIKELY_END(nsvm_hap) GET_CURRENT(bx) - SPEC_CTRL_ENTRY_FROM_HVM /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */ + /* SPEC_CTRL_ENTRY_FROM_SVM Req: b=curr %rsp=regs/cpuinfo, Clob: ac */ + ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */ stgi diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S index 27c8c5ca4943..30139ae58e9d 100644 --- a/xen/arch/x86/hvm/vmx/entry.S +++ b/xen/arch/x86/hvm/vmx/entry.S @@ -33,7 +33,9 @@ ENTRY(vmx_asm_vmexit_handler) movb $1,VCPU_vmx_launched(%rbx) mov %rax,VCPU_hvm_guest_cr2(%rbx) - SPEC_CTRL_ENTRY_FROM_HVM /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */ + /* SPEC_CTRL_ENTRY_FROM_VMX Req: b=curr %rsp=regs/cpuinfo, Clob: acd */ + ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM + ALTERNATIVE "", DO_SPEC_CTRL_ENTRY_FROM_HVM, X86_FEATURE_SC_MSR_HVM /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */ /* Hardware clears MSR_DEBUGCTL on VMExit. Reinstate it if debugging Xen. */ @@ -80,7 +82,9 @@ UNLIKELY_END(realmode) mov VCPUMSR_spec_ctrl_raw(%rax), %eax /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */ - SPEC_CTRL_EXIT_TO_HVM /* Req: a=spec_ctrl %rsp=regs/cpuinfo, Clob: cd */ + /* SPEC_CTRL_EXIT_TO_VMX Req: a=spec_ctrl %rsp=regs/cpuinfo, Clob: cd */ + ALTERNATIVE "", DO_SPEC_CTRL_EXIT_TO_GUEST, X86_FEATURE_SC_MSR_HVM + ALTERNATIVE "", __stringify(verw CPUINFO_verw_sel(%rsp)), X86_FEATURE_SC_VERW_HVM mov VCPU_hvm_guest_cr2(%rbx),%rax diff --git a/xen/arch/x86/include/asm/spec_ctrl_asm.h b/xen/arch/x86/include/asm/spec_ctrl_asm.h index cb34299a865b..18ecfcd70375 100644 --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h @@ -68,14 +68,14 @@ * * The following ASM fragments implement this algorithm. See their local * comments for further details. - * - SPEC_CTRL_ENTRY_FROM_HVM + * - SPEC_CTRL_ENTRY_FROM_{SVM,VMX} (See appropriate entry.S files) * - SPEC_CTRL_ENTRY_FROM_PV * - SPEC_CTRL_ENTRY_FROM_INTR * - SPEC_CTRL_ENTRY_FROM_INTR_IST * - SPEC_CTRL_EXIT_TO_XEN_IST * - SPEC_CTRL_EXIT_TO_XEN * - SPEC_CTRL_EXIT_TO_PV - * - SPEC_CTRL_EXIT_TO_HVM + * - SPEC_CTRL_EXIT_TO_{SVM,VMX} */ .macro DO_OVERWRITE_RSB tmp=rax @@ -225,12 +225,6 @@ wrmsr .endm -/* Use after a VMEXIT from an HVM guest. */ -#define SPEC_CTRL_ENTRY_FROM_HVM \ - ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM; \ - ALTERNATIVE "", DO_SPEC_CTRL_ENTRY_FROM_HVM, \ - X86_FEATURE_SC_MSR_HVM - /* Use after an entry from PV context (syscall/sysenter/int80/int82/etc). */ #define SPEC_CTRL_ENTRY_FROM_PV \ ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_PV; \ @@ -255,13 +249,6 @@ ALTERNATIVE "", __stringify(verw CPUINFO_verw_sel(%rsp)), \ X86_FEATURE_SC_VERW_PV -/* Use when exiting to HVM guest context. */ -#define SPEC_CTRL_EXIT_TO_HVM \ - ALTERNATIVE "", \ - DO_SPEC_CTRL_EXIT_TO_GUEST, X86_FEATURE_SC_MSR_HVM; \ - ALTERNATIVE "", __stringify(verw CPUINFO_verw_sel(%rsp)), \ - X86_FEATURE_SC_VERW_HVM - /* * Use in IST interrupt/exception context. May interrupt Xen or PV context. * Fine grain control of SCF_ist_wrmsr is needed for safety in the S3 resume
These were written before Spectre/Meltdown went public, and there was large uncertainty in how the protections would evolve. As it turns out, they're very specific to Intel hardware, and not very suitable for AMD. Expand and drop the macros. No change at all for VT-x. For AMD, the only relevant piece of functionality is DO_OVERWRITE_RSB, although we will soon be adding (different) logic to handle MSR_SPEC_CTRL. This has a marginal improvement of removing an unconditional pile of long-nops from the vmentry/exit path. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Jun Nakajima <jun.nakajima@intel.com> CC: Kevin Tian <kevin.tian@intel.com> --- xen/arch/x86/hvm/svm/entry.S | 5 +++-- xen/arch/x86/hvm/vmx/entry.S | 8 ++++++-- xen/arch/x86/include/asm/spec_ctrl_asm.h | 17 ++--------------- 3 files changed, 11 insertions(+), 19 deletions(-)