On 14.09.2023 21:49, Andrew Cooper wrote: > On 14/09/2023 11:01 am, Jan Beulich wrote: >> On 13.09.2023 22:27, Andrew Cooper wrote: >>> There is a corner case where e.g. an NMI hitting an exit-to-guest path after >>> SPEC_CTRL_EXIT_TO_* would have run the entire NMI handler *after* the VERW >>> flush to scrub potentially sensitive data from uarch buffers. >>> >>> In order to compensate, issue VERW when exiting to Xen from an IST entry. >>> >>> SPEC_CTRL_EXIT_TO_XEN already has two reads of spec_ctrl_flags off the >>> stack, >>> and we're about to add a third. Load the field into %ebx, and list the >>> register as clobbered. >>> >>> %r12 has been arranged to be the ist_exit signal, so add this as an input >>> dependency and use it to identify when to issue a VERW. >>> >>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com> >> While looking technically okay, I still have a couple of remarks: >> >>> --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h >>> +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h >>> @@ -344,10 +344,12 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise): >>> */ >>> .macro SPEC_CTRL_EXIT_TO_XEN >>> /* >>> - * Requires %r14=stack_end >>> - * Clobbers %rax, %rcx, %rdx >>> + * Requires %r12=ist_exit, %r14=stack_end >>> + * Clobbers %rax, %rbx, %rcx, %rdx >> While I'd generally be a little concerned of the growing dependency and >> clobber lists, there being a single use site makes this pretty okay. The >> macro invocation being immediately followed by RESTORE_ALL effectively >> means we can clobber almost everything here. >> >> As to register usage and my comment on patch 5: There's no real need >> to switch %rbx to %r14 there if I'm not mistaken > > As said, it's about consistency of the helpers.
While I'm not entirely happy with this, ... >>> @@ -367,8 +369,16 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise): >>> >>> .L\@_skip_sc_msr: >>> >>> - /* TODO VERW */ >>> + test %r12, %r12 >>> + jz .L\@_skip_ist_exit >>> + >>> + /* Logically DO_SPEC_CTRL_COND_VERW but without the %rsp=cpuinfo >>> dependency */ >>> + testb $SCF_verw, %bl >>> + jz .L\@_verw_skip >>> + verw STACK_CPUINFO_FIELD(verw_sel)(%r14) >>> +.L\@_verw_skip: >> Nit: .L\@_skip_verw would be more consistent with the other label names. > > So it would. I'll tweak. ... then Reviewed-by: Jan Beulich <jbeul...@suse.com> Jan
