Re: [PATCH v2 4/4] x86/spec-ctrl: Fix NMI race condition with VT-x MSR_SPEC_CTRL handling

[Andrew Cooper <amc96@xxxxxxxx>]

On 20/01/2022 08:14, Jan Beulich wrote:
> On 19.01.2022 18:00, Andrew Cooper wrote:
>> On 19/01/2022 13:42, Jan Beulich wrote:
>>> On 17.01.2022 19:34, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/hvm/vmx/entry.S
>>>> +++ b/xen/arch/x86/hvm/vmx/entry.S
>>>> @@ -35,7 +35,14 @@ ENTRY(vmx_asm_vmexit_handler)
>>>>  
>>>>          /* 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
>>>> +
>>>> +        .macro restore_spec_ctrl
>>>> +            mov    $MSR_SPEC_CTRL, %ecx
>>>> +            movzbl CPUINFO_xen_spec_ctrl(%rsp), %eax
>>>> +            xor    %edx, %edx
>>>> +            wrmsr
>>>> +        .endm
>>>> +        ALTERNATIVE "", restore_spec_ctrl, 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. */
>>>> @@ -82,8 +89,7 @@ UNLIKELY_END(realmode)
>>>>          mov VCPUMSR_spec_ctrl_raw(%rax), %eax
>>>>  
>>>>          /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. 
>>>> */
>>>> -        /* 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
>>>> +        /* SPEC_CTRL_EXIT_TO_VMX   Req: %rsp=regs/cpuinfo              
>>>> Clob:    */
>>>>          ALTERNATIVE "", __stringify(verw CPUINFO_verw_sel(%rsp)), 
>>>> X86_FEATURE_SC_VERW_HVM
>>> I notice you did update this clobber remark, but what about the one further
>>> up in context?
>> What about it?  It still clobbers %eax, %ecx and %edx.
> Oh, sorry - I did look at DO_OVERWRITE_RSB only, not paying attention
> to the now open-coded 2nd part, which - due to the blank line - doesn't
> appear connected to the comment anymore.

Yeah - it's a little harder now that it isn't a single line.  The
req/clob information really is most useful at the start of the block,
because that's where it is important to get the context correct.

Can I take this as an R-by then?

~Andrew