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

[Andrew Cooper <amc96@xxxxxxxx>]

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.

>> --- a/xen/arch/x86/include/asm/msr.h
>> +++ b/xen/arch/x86/include/asm/msr.h
>> @@ -287,7 +287,15 @@ extern struct msr_policy     raw_msr_policy,
>>  /* Container object for per-vCPU MSRs */
>>  struct vcpu_msrs
>>  {
>> -    /* 0x00000048 - MSR_SPEC_CTRL */
>> +    /*
>> +     * 0x00000048 - MSR_SPEC_CTRL
>> +     *
>> +     * For PV guests, this holds the guest kernel value.  It is accessed on
>> +     * every entry/exit path.
>> +     *
>> +     * For VT-x guests, the guest value is held in the MSR guest load/save
>> +     * list.
>> +     */
>>      struct {
>>          uint32_t raw;
>>      } spec_ctrl;
> To stand a chance of noticing bad use of this field for VT-x guests, would
> it make sense to store some sentinel value into this field for all involved
> vCPU-s?

The usage is going to get more complicated before we're done.  I'd like
to wait until the churn reduces before applying invariants like that.

~Andrew