Re: [PATCH 5/5] x86/microcode: Handle NMI's during microcode update.

["Andy Lutomirski" <luto@kernel.org>]

On Sat, Aug 13, 2022, at 5:13 PM, Andy Lutomirski wrote:
> On Sat, Aug 13, 2022, at 3:38 PM, Ashok Raj wrote:
>> Microcode updates need a guarantee that the thread sibling that is waiting
>> for the update to finish on the primary core will not execute any
>> instructions until the update is complete. This is required to guarantee
>> any MSR or instruction that's being patched will be executed before the
>> update is complete.
>>
>> After the stop_machine() rendezvous, an NMI handler is registered. If an
>> NMI were to happen while the microcode update is not complete, the
>> secondary thread will spin until the ucode update state is cleared.
>>
>> Couple of choices discussed are:
>>
>> 1. Rendezvous inside the NMI handler, and also perform the update from
>>    within the handler. This seemed too risky and might cause instability
>>    with the races that we would need to solve. This would be a difficult
>>    choice.
>
> I prefer choice 1.  As I understand it, Xen has done this for a while 
> to good effect.
>
> If I were implementing this, I would rendezvous via stop_machine as 
> usual.  Then I would set a flag or install a handler indicating that we 
> are doing a microcode update, send NMI-to-self, and rendezvous in the 
> NMI handler and do the update.
>

So I thought about this a bit more.

What's the actual goal?  Are we trying to execute no instructions at all on the sibling or are we trying to avoid executing nasty instructions like RDMSR and WRMSR?

If it's the former, we don't have a whole lot of choices.  We need the sibling to be in HLT or MWAIT, and we need NMIs masked or we need all likely NMI sources shut down.  If it's the latter, then we would ideally like to avoid a trip through the entry or exit code -- that code has nasty instructions (RDMSR in the paranoid path if !FSGSBASE, WRMSRs for mitigations, etc).  And we need to be extra careful: there are cases where NMIs are not actually masked but we just simulate NMIs being masked through the delightful logic in the entry code.  Off the top of my head, the NMI-entry-pretend-masked path probably doesn't execute nasty instructions other than IRET, but still, this might be fragile.

So here's my half-backed suggestion.  Add a new feature to the NMI entry code: at this point:

        /* Everything up to here is safe from nested NMIs */

At that point, NMIs are actually masked.  So check a flag indicating that we're trying to do the NMI-protected ucode patch dance.  If so, call a special noinstr C function (this part is distinctly nontrivial) that does the ucode work.  Now the stop_machine handler does NMI-to-self in a loop until it actually hits the special code path.

Alternatively, stop_machine, then change the IDT to a special one with a special non-IST NMI handler that does the dirty work.  NMI-to-self, do the ucode work, set the IDT back *inside the handler* so a latched NMI will do the right thing, and return.  This may be much, much simpler.

Or we stop messing around and do this for real.  Soft-offline the sibling, send it INIT, do the ucode patch, then SIPI, SIPI and resume the world.  What could possibly go wrong?

I have to say: Intel, can you please get your act together?  There is an entity in the system that is *actually* capable of doing this right: the microcode.