Re: [patch V3 22/30] x86/microcode: Add per CPU control field

[Borislav Petkov]

On Tue, Sep 12, 2023 at 09:58:18AM +0200, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> 
> Add a per CPU control field to ucode_ctrl and define constants for it:
> 
> SCTRL_WAIT    indicates that the CPU needs to spinwait with timeout
> SCTRL_APPLY   indicates that the CPU needs to invoke the microcode_apply()
> 	      callback
> SCTRL_DONE    indicates that the CPU can proceed without invoking the
> 	      microcode_apply() callback.

Can we put those explanations over the enum definition pls?

Also, s/indicates that //g when you do.

> In theory this could be a global control field, but a global control does
> not cover the following case:
> 
>  15 primary CPUs load microcode successfully
>   1 primary CPU fails and returns with an error code
> 
> With global control the sibling of the failed CPU would either try again or
> the whole operation would be aborted with the consequence that the 15
> siblings do not invoke the apply path and end up with inconsistent software
> state. The result in dmesg would be inconsistent too.
> 
> There are two additional fields added and initialized:
> 
> ctrl_cpu and secondaries. ctrl_cpu is the CPU number of the primary thread
> for now, but with the upcoming uniform loading at package or system scope
> this will be one CPU per package or just one CPU. Secondaries hands the
> control CPU a CPU mask which will be required to release the secondary CPUs
> out of the wait loop.

Also as a comment above their definitions pls.

> Preparatory change for implementing a properly split control flow for
> primary and secondary CPUs.
> 
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> 
> 
> ---
>  arch/x86/kernel/cpu/microcode/core.c |   20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> ---
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -324,8 +324,16 @@ static struct platform_device	*microcode
>   *   requirement can be relaxed in the future. Right now, this is conservative
>   *   and good.
>   */
> +enum sibling_ctrl {
> +	SCTRL_WAIT,
> +	SCTRL_APPLY,
> +	SCTRL_DONE,
> +};
> +
>  struct ucode_ctrl {
> +	enum sibling_ctrl	ctrl;
>  	enum ucode_state	result;
> +	unsigned int		ctrl_cpu;
>  };
>  
>  static DEFINE_PER_CPU(struct ucode_ctrl, ucode_ctrl);
> @@ -468,7 +476,7 @@ static int ucode_load_late_stop_cpus(voi
>   */
>  static bool ucode_setup_cpus(void)
>  {
> -	struct ucode_ctrl ctrl = { .result = -1, };
> +	struct ucode_ctrl ctrl = { .ctrl = SCTRL_WAIT, .result = -1, };
>  	unsigned int cpu;
>  
>  	for_each_cpu_and(cpu, cpu_present_mask, &cpus_booted_once_mask) {
> @@ -478,7 +486,15 @@ static bool ucode_setup_cpus(void)
>  				return false;
>  			}
>  		}
> -		/* Initialize the per CPU state */
> +
> +		/*
> +		 * Initialize the per CPU state. This is core scope for now,
> +		 * but prepared to take package or system scope into account.
> +		 */
> +		if (topology_is_primary_thread(cpu))
> +			ctrl.ctrl_cpu = cpu;
> +		else
> +			ctrl.ctrl_cpu = cpumask_first(topology_sibling_cpumask(cpu));

<---- newline here.

-- 
Regards/Gruss,
    Boris.

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