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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.01.2022 18:23, Andrew Cooper wrote:
> On 17/01/2022 11:51, Jan Beulich wrote:
>> On 13.01.2022 17:38, Andrew Cooper wrote:
>>> @@ -119,12 +125,11 @@ UNLIKELY_END(realmode)
>>>          SAVE_ALL
>>>  
>>>          /*
>>> -         * PV variant needed here as no guest code has executed (so
>>> -         * MSR_SPEC_CTRL can't have changed value), and NMIs/MCEs are 
>>> liable
>>> -         * to hit (in which case the HVM variant might corrupt things).
>>> +         * SPEC_CTRL_ENTRY notes
>>> +         *
>>> +         * If we end up here, no guest code has executed.  We still have 
>>> Xen's
>>> +         * choice of MSR_SPEC_CTRL in context, and the RSB is safe.
>>>           */
>>> -        SPEC_CTRL_ENTRY_FROM_PV /* Req: %rsp=regs/cpuinfo Clob: acd */
>>> -        /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>> Is "no guest code has executed" actually enough here? If VM entry failed
>> due to a later MSR-load-list entry, SPEC_CTRL could have changed value
>> already, couldn't it?
> 
> No, it can't.
> 
> See the very start of SDM Chapter 25 "VMEntries" for the distinction
> between early and late entry failures.  (i.e. those which cause
> execution to continue beyond VMLAUNCH/VMRESUME, and those which cause
> execution to continue from the vmexit handler.)
> 
> Early failures are conditions such as `pop %ss; vmlaunch`, and bad host
> state in the VMCS.
> 
> Everything pertaining to guest state is late failure, so by the time we
> get to processing the MSR load/save list, we're definitely not taking
> this path.

I see. This still means that the answer to my 1st question is "yes". In
which case I'd like to ask to extend the comment to include "no MSRs
have been loaded from the load list" or something equivalent, despite
realizing that such an amendment would have helped already before your
change.

Jan