Re: [PATCH 7/8] x86/spec-ctrl: Issue VERW during IST exit to Xen

[Jan Beulich <jbeulich@xxxxxxxx>]

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.cooper3@xxxxxxxxxx>

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; in particular you
don't re-use any of the other macros which require use of %r14. You
could as well use %r14b (or about any other register that isn't already
used for something, yet whichever was picked apparently wouldn't make a
difference) for the flags here, getting away with fewer new REX prefixes
overall.

>   */
> -    testb $SCF_ist_sc_msr, STACK_CPUINFO_FIELD(spec_ctrl_flags)(%r14)
> +    movzbl STACK_CPUINFO_FIELD(spec_ctrl_flags)(%r14), %ebx
> +
> +    testb $SCF_ist_sc_msr, %bl
>      jz .L\@_skip_sc_msr
>  
>      /*
> @@ -358,7 +360,7 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>       */
>      xor %edx, %edx
>  
> -    testb $SCF_use_shadow, STACK_CPUINFO_FIELD(spec_ctrl_flags)(%r14)
> +    testb $SCF_use_shadow, %bl
>      jz .L\@_skip_sc_msr
>  
>      mov STACK_CPUINFO_FIELD(shadow_spec_ctrl)(%r14), %eax
> @@ -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.

> +.L\@_skip_ist_exit:

I was going to ask why the separate label (and whether the two JZ above
couldn't sensibly be folded), but the to both answer lies in the next
patch.

Jan