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

[Ashok Raj <ashok.raj@intel.com>]

Hi Andy

On Sat, Aug 13, 2022 at 06:19:04PM -0700, Andy Lutomirski wrote:
> 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?

Basically when the thread running wrmsr 0x79 asks for exclusive access to
the core, the second CPU is pulled into some ucode context, then the
primary thread updates the ucode. Secondary is released. But if the
secondary was in the middle of an instruction that is being patched, when
it resumes execution, it may go to some non-existent context and cause
weird things to happen. 

I'm not sure if the interrupt entry code does any fancy stuff, which case
dropping in the NMI handler early might be a better option.

I tried to do apic->sendIPIall(), then just wait for the 2 threads of a
core to rendezvous. Maybe instead I should have done the
selfIPI might be better. I'll do some more experiments with what I sent in
this patchset. 
> 
> 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.

Remember, mwait() was patched that caused pain.. I remember Thomas
mentioned this a while back.

> 
> 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?


All these are looking more complicated, and might add some significant
latency. We might as well invoke SMI and let BIOS SMM handler to the update
:-)

> 
> 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.

So we have it.. this dirtyness is for all the current gen products.. Much
improved microcode loading is on the way.