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

[Ingo Molnar <mingo@kernel.org>]

* 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