Re: [patch V3 18/30] x86/microcode: Handle "nosmt" correctly

[Borislav Petkov]

Just textual nitpicks. Otherwise looks nice.

On Tue, Sep 12, 2023 at 09:58:12AM +0200, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> 
> On CPUs where microcode loading is not NMI safe the SMT sibling which is
> parked in one of the play_dead() variants, these parked CPUs still react
> on NMIs.

s/, these parked CPUs still react/still reacts/.

Simpler.

> So if a NMI hits while the primary thread updates the microcode
> the resulting behaviour is undefined. The default play_dead()
> implementation on modern CPUs is using MWAIT, which is not guaranteed to
> be safe against an microcode update which affects MWAIT.

s/an //

> +/*
> + *  Ensure that all required CPUs which are present and have been booted
> + *  once are online.
> + *
> + *    To pass this check, all primary threads must be online.
> + *
> + *    If the microcode load is not safe against NMI then all SMT threads
> + *    must be online as well because they still react on NMI when they are

s/react on NMI/react to NMIs/

> + *    soft-offlined and parked in one of the play_dead() variants. So if a
> + *    NMI hits while the primary thread updates the microcode the resulting
> + *    behaviour is undefined. The default play_dead() implementation on
> + *    modern CPUs is using MWAIT, which is also not guaranteed to be safe

s/is using/uses/

> --- a/arch/x86/kernel/cpu/microcode/internal.h
> +++ b/arch/x86/kernel/cpu/microcode/internal.h
> @@ -20,18 +20,17 @@ enum ucode_state {
>  
>  struct microcode_ops {
>  	enum ucode_state (*request_microcode_fw)(int cpu, struct device *dev);
> -
>  	void (*microcode_fini_cpu)(int cpu);
>  
>  	/*
> -	 * The generic 'microcode_core' part guarantees that
> -	 * the callbacks below run on a target cpu when they
> -	 * are being called.
> +	 * The generic 'microcode_core' part guarantees that the callbacks
> +	 * below run on a target cpu when they are being called.

s/cpu/CPU/

while at it.

>  	 * See also the "Synchronization" section in microcode_core.c.
>  	 */
> -	enum ucode_state (*apply_microcode)(int cpu);
> -	int (*collect_cpu_info)(int cpu, struct cpu_signature *csig);
> -	void (*finalize_late_load)(int result);
> +	enum ucode_state	(*apply_microcode)(int cpu);
> +	int			(*collect_cpu_info)(int cpu, struct cpu_signature *csig);
> +	void			(*finalize_late_load)(int result);
> +	unsigned int		nmi_safe	: 1;
>  };

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette