Messages in this thread | | | Date | Wed, 17 Aug 2022 09:41:31 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH v3 3/5] x86/microcode: Avoid any chance of MCE's during microcode update |
| |
* Ashok Raj <ashok.raj@intel.com> wrote:
> When a microcode update is in progress, several instructions and MSR's can > be patched by the update. During the update in progress, touching any of > the resources being patched could result in unpredictable results. If > thread0 is doing the update and thread1 happens to get a MCE, the handler > might read an MSR that's being patched. > > In order to have predictable behavior, to avoid this scenario we set the MCIP in > all threads. Since MCE's can't be nested, HW will automatically promote to > shutdown condition. > > After the update is completed, MCIP flag is cleared. The system is going to > shutdown anyway, since the MCE could be a fatal error, or even recoverable > errors in kernel space are treated as unrecoverable. > > Signed-off-by: Ashok Raj <ashok.raj@intel.com> > --- > arch/x86/include/asm/mce.h | 4 ++++ > arch/x86/kernel/cpu/mce/core.c | 9 +++++++++ > arch/x86/kernel/cpu/microcode/core.c | 11 +++++++++++ > 3 files changed, 24 insertions(+) > > diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h > index cc73061e7255..2aef6120e23f 100644 > --- a/arch/x86/include/asm/mce.h > +++ b/arch/x86/include/asm/mce.h > @@ -207,12 +207,16 @@ void mcheck_cpu_init(struct cpuinfo_x86 *c); > void mcheck_cpu_clear(struct cpuinfo_x86 *c); > int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info, > u64 lapic_id); > +extern void mce_set_mcip(void); > +extern void mce_clear_mcip(void); > #else > static inline int mcheck_init(void) { return 0; } > static inline void mcheck_cpu_init(struct cpuinfo_x86 *c) {} > static inline void mcheck_cpu_clear(struct cpuinfo_x86 *c) {} > static inline int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info, > u64 lapic_id) { return -EINVAL; } > +static inline void mce_set_mcip(void) {} > +static inline void mce_clear_mcip(void) {} > #endif > > void mce_setup(struct mce *m); > diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c > index 2c8ec5c71712..72b49d95bb3b 100644 > --- a/arch/x86/kernel/cpu/mce/core.c > +++ b/arch/x86/kernel/cpu/mce/core.c > @@ -402,6 +402,15 @@ static noinstr void mce_wrmsrl(u32 msr, u64 v) > : : "c" (msr), "a"(low), "d" (high) : "memory"); > } > > +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()
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.
> + /* > + * 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?
Did you mean 'predictibly'/'reliably'?
> + */ > + 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?
Thanks,
Ingo
| |