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

[Ashok Raj]

Hi

On Mon, Jun 05, 2023 at 04:13:32PM +0200, 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.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@xxxxxxxxx>
> ---
>  .../admin-guide/kernel-parameters.txt         |  7 +++
>  arch/x86/kernel/cpu/microcode/amd.c           |  5 ++-
>  arch/x86/kernel/cpu/microcode/core.c          | 44 +++++++++++++++++++
>  arch/x86/kernel/cpu/microcode/internal.h      | 16 +++++++
>  4 files changed, 71 insertions(+), 1 deletion(-)
>  create mode 100644 arch/x86/kernel/cpu/microcode/internal.h
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 9e5bab29685f..b88ff022402c 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3228,6 +3228,13 @@
>  
>  	mga=		[HW,DRM]
>  
> +	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.
> +

The default behavior is that the reload happens on all threads for both
early and late. 

The chicken bit in cmdline and the sysfs/control is to  opt-out just in case
they want to change the default behavior? 

When end user changes the behavior, isn't it against the design specification? And if
so, should that result in kernel being tainted after a reload?

Is this reload on all threads required by all models, or only certain
models? I was wondering if the forced reload could be limited to only
affected CPUs instead of doing it on all unconditionally.

>  	min_addr=nn[KMG]	[KNL,BOOT,IA-64] All physical memory below this
>  			physical address is ignored.
>  
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 87208e46f7ed..76b530697951 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -36,6 +36,8 @@
>  #include <asm/cpu.h>
>  #include <asm/msr.h>
>  
> +#include "internal.h"
> +
>  static struct equiv_cpu_table {
>  	unsigned int num_entries;
>  	struct equiv_cpu_entry *entry;
> @@ -700,7 +702,8 @@ static enum ucode_state apply_microcode_amd(int cpu)
>  	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
>  
>  	/* need to apply patch? */
> -	if (rev > mc_amd->hdr.patch_id) {
> +	if ((rev > mc_amd->hdr.patch_id) ||
> +	    (rev == mc_amd->hdr.patch_id && !(control & LATE_ALL_THREADS))) {
>  		ret = UCODE_OK;
>  		goto out;
>  	}
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 3afcf3de0dd4..5f3185d2814c 100644
> --- 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;
> +
>  bool initrd_gone;
>  
>  LIST_HEAD(microcode_cache);
> @@ -522,8 +526,32 @@ static ssize_t processor_flags_show(struct device *dev,
>  	return sprintf(buf, "0x%x\n", uci->cpu_sig.pf);
>  }
>  
> +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;
> +}
> +
>  static DEVICE_ATTR_RO(version);
>  static DEVICE_ATTR_RO(processor_flags);
> +static DEVICE_ATTR_ADMIN_RW(control);
>  
>  static struct attribute *mc_default_attrs[] = {
>  	&dev_attr_version.attr,
> @@ -622,6 +650,7 @@ static struct attribute *cpu_root_microcode_attrs[] = {
>  #ifdef CONFIG_MICROCODE_LATE_LOADING
>  	&dev_attr_reload.attr,
>  #endif
> +	&dev_attr_control.attr,

Shouldn't the "control" be under LATE_LOADING? Since this only controls
late-loading behavior?