| Message ID | 20230913202758.508225-5-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/spec-ctrl: AMD DIV fix, and VERW prerequisite bugfixes | expand |
On 13.09.2023 22:27, Andrew Cooper wrote: > --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h > +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h > @@ -218,7 +218,10 @@ > wrmsr > .endm > > -/* Use after an entry from PV context (syscall/sysenter/int80/int82/etc). */ > +/* > + * Used after a synchronous entry from PV context. SYSCALL, SYSENTER, INT, > + * etc. Will always interrupt a guest speculation context. > + */ > .macro SPEC_CTRL_ENTRY_FROM_PV > /* > * Requires %rsp=regs/cpuinfo, %rdx=0 For the entry point comments - not being a native speaker -, the use of "{will,may} interrupt" reads odd. You're describing the macros here, not the the events that led to their invocation. Therefore it would seem to me that "{will,may} have interrupted" would be more appropriate. > @@ -233,7 +236,11 @@ > X86_FEATURE_SC_MSR_PV > .endm > > -/* Use in interrupt/exception context. May interrupt Xen or PV context. */ > +/* > + * Used after a synchronous interrupt or exception. May interrupt Xen or PV > + * context, but will not interrupt Xen with a guest speculation context, > + * outside of fatal error cases. > + */ > .macro SPEC_CTRL_ENTRY_FROM_INTR > /* > * Requires %rsp=regs, %r14=stack_end, %rdx=0 I find "synchronous" here odd, when this includes interrupts. Below you at least qualify things further, by saying "fully asynchronous with finding sane state"; I think further qualification is warranted here as well, to avoid any ambiguity. > @@ -319,7 +334,14 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise): > UNLIKELY_END(\@_serialise) > .endm > > -/* Use when exiting to Xen in IST context. */ > +/* > + * Use when exiting from any entry context, back to Xen context. This > + * includes returning to other SPEC_CTRL_{ENTRY,EXIT}_* regions with > + * unsanitised state. > + * > + * Because we might have interrupted Xen beyond SPEC_CTRL_EXIT_TO_$GUEST, we > + * must treat this as if it were an EXIT_TO_$GUEST case too. > + */ > .macro SPEC_CTRL_EXIT_TO_XEN > /* > * Requires %rbx=stack_end Is it really "must"? At least in theory there are ways to recognize that exit is back to Xen context outside of interrupted entry/exit regions (simply by evaluating how far below stack top %rsp is). > @@ -344,6 +366,9 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise): > wrmsr > > .L\@_skip_sc_msr: > + > + /* TODO VERW */ > + > .endm I don't think this comment is strictly necessary to add here, when the omission is addressed in a later patch. But I also don't mind its addition. Jan
On 14/09/2023 8:58 am, Jan Beulich wrote: > On 13.09.2023 22:27, Andrew Cooper wrote: >> --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h >> +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h >> @@ -218,7 +218,10 @@ >> wrmsr >> .endm >> >> -/* Use after an entry from PV context (syscall/sysenter/int80/int82/etc). */ >> +/* >> + * Used after a synchronous entry from PV context. SYSCALL, SYSENTER, INT, >> + * etc. Will always interrupt a guest speculation context. >> + */ >> .macro SPEC_CTRL_ENTRY_FROM_PV >> /* >> * Requires %rsp=regs/cpuinfo, %rdx=0 > For the entry point comments - not being a native speaker -, the use of > "{will,may} interrupt" reads odd. You're describing the macros here, > not the the events that led to their invocation. Therefore it would seem > to me that "{will,may} have interrupted" would be more appropriate. The salient information is what the speculation state looks like when we're running the asm in these macros. Sync and Async perhaps aren't the best terms. For PV context at least, it boils down to: 1) CPL>0 => you were in fully-good guest speculation context 2) CPL=0 => you were in fully-good Xen speculation context 3) IST && CPL=0 => Here be dragons. HVM is more of a challenge. VT-x behaves like an IST path, while SVM does allow us to atomically switch to good Xen state. Really, this needs to be a separate doc, with diagrams... >> @@ -319,7 +334,14 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise): >> UNLIKELY_END(\@_serialise) >> .endm >> >> -/* Use when exiting to Xen in IST context. */ >> +/* >> + * Use when exiting from any entry context, back to Xen context. This >> + * includes returning to other SPEC_CTRL_{ENTRY,EXIT}_* regions with >> + * unsanitised state. >> + * >> + * Because we might have interrupted Xen beyond SPEC_CTRL_EXIT_TO_$GUEST, we >> + * must treat this as if it were an EXIT_TO_$GUEST case too. >> + */ >> .macro SPEC_CTRL_EXIT_TO_XEN >> /* >> * Requires %rbx=stack_end > Is it really "must"? At least in theory there are ways to recognize that > exit is back to Xen context outside of interrupted entry/exit regions > (simply by evaluating how far below stack top %rsp is). Yes, it is must - it's about how Xen behaves right now, not about some theoretical future with different tracking mechanism. Checking the stack is very fragile and we've had bugs doing this in the past. It would break completely if we were to take things such as the recursive-NMI fix (not that we're liable to at this point with FRED on the horizon.) A perhaps less fragile option would be to have .text.entry.spec_suspect section and check %rip being in that. But neither of these are good options. It's adding complexity (latency) to a fastpath to avoid a small hit in a rare case, so is a concrete anti-optimisation. >> @@ -344,6 +366,9 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise): >> wrmsr >> >> .L\@_skip_sc_msr: >> + >> + /* TODO VERW */ >> + >> .endm > I don't think this comment is strictly necessary to add here, when the > omission is addressed in a later patch. But I also don't mind its > addition. It doesn't especially matter if the series gets committed in one go, but it does matter if it ends up being split. This is the patch which observes that VERW is missing. ~Andrew
On 14.09.2023 21:23, Andrew Cooper wrote: > On 14/09/2023 8:58 am, Jan Beulich wrote: >> On 13.09.2023 22:27, Andrew Cooper wrote: >>> --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h >>> +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h >>> @@ -218,7 +218,10 @@ >>> wrmsr >>> .endm >>> >>> -/* Use after an entry from PV context (syscall/sysenter/int80/int82/etc). */ >>> +/* >>> + * Used after a synchronous entry from PV context. SYSCALL, SYSENTER, INT, >>> + * etc. Will always interrupt a guest speculation context. >>> + */ >>> .macro SPEC_CTRL_ENTRY_FROM_PV >>> /* >>> * Requires %rsp=regs/cpuinfo, %rdx=0 >> For the entry point comments - not being a native speaker -, the use of >> "{will,may} interrupt" reads odd. You're describing the macros here, >> not the the events that led to their invocation. Therefore it would seem >> to me that "{will,may} have interrupted" would be more appropriate. > > The salient information is what the speculation state looks like when > we're running the asm in these macros. > > Sync and Async perhaps aren't the best terms. For PV context at least, > it boils down to: > > 1) CPL>0 => you were in fully-good guest speculation context > 2) CPL=0 => you were in fully-good Xen speculation context > 3) IST && CPL=0 => Here be dragons. > > HVM is more of a challenge. VT-x behaves like an IST path, while SVM > does allow us to atomically switch to good Xen state. > > Really, this needs to be a separate doc, with diagrams... > >>> @@ -319,7 +334,14 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise): >>> UNLIKELY_END(\@_serialise) >>> .endm >>> >>> -/* Use when exiting to Xen in IST context. */ >>> +/* >>> + * Use when exiting from any entry context, back to Xen context. This >>> + * includes returning to other SPEC_CTRL_{ENTRY,EXIT}_* regions with >>> + * unsanitised state. >>> + * >>> + * Because we might have interrupted Xen beyond SPEC_CTRL_EXIT_TO_$GUEST, we >>> + * must treat this as if it were an EXIT_TO_$GUEST case too. >>> + */ >>> .macro SPEC_CTRL_EXIT_TO_XEN >>> /* >>> * Requires %rbx=stack_end >> Is it really "must"? At least in theory there are ways to recognize that >> exit is back to Xen context outside of interrupted entry/exit regions >> (simply by evaluating how far below stack top %rsp is). > > Yes, it is must - it's about how Xen behaves right now, not about some > theoretical future with different tracking mechanism. Well, deleting "must" does exactly that: Describe what we currently do, without imposing that it necessarily has to be that way. That's at least to me, as an - as said - non-native speaker. > Checking the stack is very fragile and we've had bugs doing this in the > past. It would break completely if we were to take things such as the > recursive-NMI fix (not that we're liable to at this point with FRED on > the horizon.) > > A perhaps less fragile option would be to have .text.entry.spec_suspect > section and check %rip being in that. > > But neither of these are good options. It's adding complexity (latency) > to a fastpath to avoid a small hit in a rare case, so is a concrete > anti-optimisation. I fully accept all of this, and I wasn't meaning to suggest we do what might be possible. I merely dislike stronger than necessary statements, as such are liable to cause confusion down the road. That said, I certainly won't refuse to eventually ack this patch just because of this one word. Jan
On 15/09/2023 8:07 am, Jan Beulich wrote: > On 14.09.2023 21:23, Andrew Cooper wrote: >> On 14/09/2023 8:58 am, Jan Beulich wrote: >>> On 13.09.2023 22:27, Andrew Cooper wrote: >>>> @@ -319,7 +334,14 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise): >>>> UNLIKELY_END(\@_serialise) >>>> .endm >>>> >>>> -/* Use when exiting to Xen in IST context. */ >>>> +/* >>>> + * Use when exiting from any entry context, back to Xen context. This >>>> + * includes returning to other SPEC_CTRL_{ENTRY,EXIT}_* regions with >>>> + * unsanitised state. >>>> + * >>>> + * Because we might have interrupted Xen beyond SPEC_CTRL_EXIT_TO_$GUEST, we >>>> + * must treat this as if it were an EXIT_TO_$GUEST case too. >>>> + */ >>>> .macro SPEC_CTRL_EXIT_TO_XEN >>>> /* >>>> * Requires %rbx=stack_end >>> Is it really "must"? At least in theory there are ways to recognize that >>> exit is back to Xen context outside of interrupted entry/exit regions >>> (simply by evaluating how far below stack top %rsp is). >> Yes, it is must - it's about how Xen behaves right now, not about some >> theoretical future with different tracking mechanism. > Well, deleting "must" does exactly that Nonsense. *When* someone changes the logic such that there's an alternative route, the comment necessarily needs updating. And until that point, the logic *must* behave in this way to be correct. ~Andrew
diff --git a/xen/arch/x86/include/asm/spec_ctrl_asm.h b/xen/arch/x86/include/asm/spec_ctrl_asm.h index ee75f2bced42..77f6c35bb9c5 100644 --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h @@ -218,7 +218,10 @@ wrmsr .endm -/* Use after an entry from PV context (syscall/sysenter/int80/int82/etc). */ +/* + * Used after a synchronous entry from PV context. SYSCALL, SYSENTER, INT, + * etc. Will always interrupt a guest speculation context. + */ .macro SPEC_CTRL_ENTRY_FROM_PV /* * Requires %rsp=regs/cpuinfo, %rdx=0 @@ -233,7 +236,11 @@ X86_FEATURE_SC_MSR_PV .endm -/* Use in interrupt/exception context. May interrupt Xen or PV context. */ +/* + * Used after a synchronous interrupt or exception. May interrupt Xen or PV + * context, but will not interrupt Xen with a guest speculation context, + * outside of fatal error cases. + */ .macro SPEC_CTRL_ENTRY_FROM_INTR /* * Requires %rsp=regs, %r14=stack_end, %rdx=0 @@ -248,7 +255,10 @@ X86_FEATURE_SC_MSR_PV .endm -/* Use when exiting to PV guest context. */ +/* + * Used when exiting from any entry context, back to PV context. This + * includes from an IST entry which moved onto the primary stack. + */ .macro SPEC_CTRL_EXIT_TO_PV /* * Requires %rax=spec_ctrl, %rsp=regs/info @@ -260,7 +270,12 @@ .endm /* - * Use in IST interrupt/exception context. May interrupt Xen or PV context. + * Used after an IST entry (i.e. needs special care, consider to be fully + * asynchronous with finding sane state). May interrupt PV or Xen context, + * including other SPEC_CTRL_{ENTRY,EXIT}_* regions with unsanitised state. + * + * An IST entry which interrupts PV context moves onto the primary stack and + * leaves via SPEC_CTRL_EXIT_TO_PV, *not* SPEC_CTRL_EXIT_TO_XEN. */ .macro SPEC_CTRL_ENTRY_FROM_INTR_IST /* @@ -319,7 +334,14 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise): UNLIKELY_END(\@_serialise) .endm -/* Use when exiting to Xen in IST context. */ +/* + * Use when exiting from any entry context, back to Xen context. This + * includes returning to other SPEC_CTRL_{ENTRY,EXIT}_* regions with + * unsanitised state. + * + * Because we might have interrupted Xen beyond SPEC_CTRL_EXIT_TO_$GUEST, we + * must treat this as if it were an EXIT_TO_$GUEST case too. + */ .macro SPEC_CTRL_EXIT_TO_XEN /* * Requires %rbx=stack_end @@ -344,6 +366,9 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise): wrmsr .L\@_skip_sc_msr: + + /* TODO VERW */ + .endm #endif /* __ASSEMBLY__ */
... to better explain how they're used. Doing so highlights that SPEC_CTRL_EXIT_TO_XEN is missing a VERW flush for the corner case when e.g. an NMI hits late in an exit-to-guest path. Leave a TODO, which will be addressed in subsequent patches which arrange for DO_COND_VERW to be safe within SPEC_CTRL_EXIT_TO_XEN. 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> This was decided not to be XSA-worthy, as guests can't usefully control when IST events occur. --- xen/arch/x86/include/asm/spec_ctrl_asm.h | 35 ++++++++++++++++++++---- 1 file changed, 30 insertions(+), 5 deletions(-)