Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Date:   Fri, 22 Sep 2023 15:42:00 +0200
From:   Borislav Petkov <bp@alien8.de>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     LKML <linux-kernel@vger.kernel.org>, x86@kernel.org,
        "Chang S. Bae" <chang.seok.bae@intel.com>,
        Arjan van de Ven <arjan@linux.intel.com>,
        Nikolay Borisov <nik.borisov@suse.com>
Subject: Re: [patch V3 18/30] x86/microcode: Handle "nosmt" correctly
Message-ID: <20230922134200.GIZQ2ZqKbKM/bZHbQ4@fat_crate.local>
References: <20230912065249.695681286@linutronix.de>
 <20230912065501.899886649@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230912065501.899886649@linutronix.de>
Precedence: bulk

Just textual nitpicks. Otherwise looks nice.

On Tue, Sep 12, 2023 at 09:58:12AM +0200, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> 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