Re: [PATCH v3 3/5] x86/microcode: Avoid any chance of MCE's during microcode update

Ashok Raj
2022-08-17T11:41:06Z

On Wed, Aug 17, 2022 at 09:41:31AM +0200, Ingo Molnar wrote:
>
> > +void mce_set_mcip(void)
> > +{
> > + mce_wrmsrl(MSR_IA32_MCG_STATUS, 0x1);
> > +}
> > +
> > +void mce_clear_mcip(void)
> > +{
> > + mce_wrmsrl(MSR_IA32_MCG_STATUS, 0x0);
> > +}
>
> Instead of naming new APIs after how they are doing stuff, please name them
> after *what* they are doing at the highest level: they disable/enable MCEs.
>
> Ie. I'd suggest something like:
>
> mce_disable()
> mce_enable()

We actually aren't disabling MCE's we set things up to promote it to a more
severe shutdown if an MCE were to be signaled while in the ucode update
flow.

I'm struggling to find a suitable name. But I agree with what you are
saying.

promote_mce_to_fatal()? I'll take any names that seem fit.

>
> I'd also suggest to at minimum add a WARN_ON_ONCE() if MSR_IA32_MCG_STATUS
> is already 1 when we disable it - because whoever wanted it disabled will
> now be surprised by us enabling them again.

Ok, will add.

>
> > + /*
> > + * Its dangerous to let MCE while microcode update is in progress.
>
> s/let MCE while
> /let MCEs execute while
>
> > + * Its extremely rare and even if happens they are fatal errors.
> > + * But reading patched areas before the update is complete can be
> > + * leading to unpredictable results. Setting MCIP will guarantee
>
> s/can be leading to
> /can lead to
>
> > + * the platform is taken to reset predictively.
>
> What does 'the platform is taken to reset predictively' mean?

Since we are setting MCG_STATUS.MCIP=1, since MCE's aren't nestable,
if there is a hardware event trying to signal a MCE, it will turn into a
platform reset. The MCE registers that logged the event will be sticky
and preserve in a warm reset case. BIOS or OS can pickup values after the
reboot is complete.

>
> Did you mean 'predictibly'/'reliably'?

I don't know the difference, except both are a trustworthy topic :-)

I like predictable, the system is going down.. not reliable :-)
>
> > + */
> > + mce_set_mcip();
> > /*
> > * On an SMT system, it suffices to load the microcode on one sibling of
> > * the core because the microcode engine is shared between the threads.
> > @@ -457,6 +466,7 @@ static int __reload_late(void *info)
> > * loading attempts happen on multiple threads of an SMT core. See
> > * below.
> > */
> > +
> > if (cpumask_first(topology_sibling_cpumask(cpu)) == cpu)
> > apply_microcode_local(&err);
> > else
>
> Spurious newline added?

It's a gonner !

Cheers,
Ashok