Messages in this thread | | | Date | Wed, 17 Aug 2022 11:40:35 +0000 | From | Ashok Raj <> | Subject | Re: [PATCH v3 3/5] x86/microcode: Avoid any chance of MCE's during microcode update |
| |
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
| |