Re: [PATCH 2/2] x86/microcode: Add a "microcode=" command line option

[Thomas Gleixner]

On Mon, Jun 05 2023 at 16:13, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@xxxxxxxxx>
>
> It will be used to control microcode loader behavior. Add the first
> chicken bit: to control whether the AMD side should load microcode late
> on all logical SMT threads.

This has a distinct void of information here.

Why is it suddenly required to prevent late loading on SMT threads?

That's the exact opposite of what e7ad18d1169c ("x86/microcode/AMD:
Apply the patch early on every logical thread") is doing.

I'm seriously confused.
>  
> +	microcode=	[X86] Control the behavior of the microcode
> +			loader. Available options:
> +
> +			no_late_all - do not load on all SMT threads on
> +			AMD. Loading on all logical threads is enabled by
> +			default.

no_late_all is a horrible option name.

Also the explanation is not mentioning that this is only relevant for
late loading.

Aside of that why is this a kernel side chicken bit and not communicated
by the microcode header?

How should an administrator know which microcode versions need this
treatment and which do not? How is that supposed to work on a fleet?

> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -40,11 +40,15 @@
>  #include <asm/cmdline.h>
>  #include <asm/setup.h>
>  
> +#include "internal.h"
> +
>  #define DRIVER_VERSION	"2.2"
>  
>  static struct microcode_ops	*microcode_ops;
>  static bool dis_ucode_ldr = true;
>  
> +unsigned long control = LATE_ALL_THREADS;

global variable name without a proper prefix. Moo.

> +static ssize_t control_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "0x%lx\n", control);
> +}
> +
> +static ssize_t control_store(struct device *dev,
> +			     struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	unsigned long val;
> +
> +	if (kstrtoul(buf, 0, &val) < 0)
> +		return -ERANGE;
> +
> +	if (val & CONTROL_FLAGS_MASK)
> +		return -EINVAL;
> +
> +	control = val;
> +
> +	return count;
> +}

Where is the documentation which tells me what I'm supposed to write
into this file? Also this is a generic file, right? So what's the
meaning for non AMD? I can write this bit into it successfully and
nothing happens, right?
> +++ b/arch/x86/kernel/cpu/microcode/internal.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __X86_MICROCODE_INTERNAL_H__
> +#define __X86_MICROCODE_INTERNAL_H__
> +
> +extern unsigned long control;
> +
> +/* Loader control flags. */
> +enum control_flags {
> +	__LATE_ALL_THREADS = 0,
> +	__CONTROL_FLAGS_NUM,
> +};
> +
> +#define LATE_ALL_THREADS	BIT_ULL(__LATE_ALL_THREADS)
> +#define CONTROL_FLAGS_MASK	~(BIT_ULL(__CONTROL_FLAGS_NUM) - 1)

Why ULL bits for a unsigned long variable?

Thanks,

        tglx