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

Ingo Molnar
2022-08-17T07:41:40Z

* 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