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

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <amc96@srcf.net>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Jun Nakajima" <jun.nakajima@intel.com>,
	"Kevin Tian" <kevin.tian@intel.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 4/4] x86/spec-ctrl: Fix NMI race condition with VT-x MSR_SPEC_CTRL handling
Date: Thu, 20 Jan 2022 13:11:51 +0100	[thread overview]
Message-ID: <9415e194-3c31-b7cb-3a6a-3ab6aa432908@suse.com> (raw)
In-Reply-To: <914f2264-d424-e42b-31ac-1832224cc1d9@srcf.net>

On 20.01.2022 12:55, Andrew Cooper wrote:
> 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?

Please do; I should have said so earlier on.

Jan