[PATCH 1/2] x86/microcode/AMD: Load late on both threads too

[PATCH 1/2] x86/microcode/AMD: Load late on both threads too

[Borislav Petkov]

From: "Borislav Petkov (AMD)" <bp@alien8.de>

Do the same as early loading - load on both threads.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Cc: <stable@kernel.org>
---
 arch/x86/kernel/cpu/microcode/amd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index f14f4ea0b537..87208e46f7ed 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -700,7 +700,7 @@ 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) {
 		ret = UCODE_OK;
 		goto out;
 	}
-- 
2.35.1

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

[Borislav Petkov]

From: "Borislav Petkov (AMD)" <bp@alien8.de>

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@alien8.de>
---
 .../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.
+
 	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,
 	NULL
 };
 
@@ -683,3 +712,18 @@ static int __init microcode_init(void)
 }
 fs_initcall(save_microcode_in_initrd);
 late_initcall(microcode_init);
+
+static int __init parse_cmdline_param(char *str)
+{
+	if (!str)
+		return 0;
+
+	if (*str == '=')
+		str++;
+
+	if (!strcmp(str, "no_late_all"))
+		control &= ~LATE_ALL_THREADS;
+
+        return 1;
+}
+__setup("microcode", parse_cmdline_param);
diff --git a/arch/x86/kernel/cpu/microcode/internal.h b/arch/x86/kernel/cpu/microcode/internal.h
new file mode 100644
index 000000000000..5e3c5fc3851f
--- /dev/null
+++ 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)
+
+#endif /* __X86_MICROCODE_INTERNAL_H__ */
-- 
2.35.1

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@alien8.de>
> 
> 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@alien8.de>
> ---
>  .../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? 

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

[Borislav Petkov]

On Wed, Jun 07, 2023 at 06:55:39PM -0700, Ashok Raj wrote:
> 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?

That's a chicken bit and should not be used usually. I'm adding it just
in case.

> 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.

Unconditionally.

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

No, that's a bitfield and is going to be used for other flags, if
needed and which are not necessarily late-loading related.

-- 
Regards/Gruss,
    Boris.

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

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

[Ashok Raj]

On Fri, Jun 09, 2023 at 02:28:28PM +0200, Borislav Petkov wrote:
> On Wed, Jun 07, 2023 at 06:55:39PM -0700, Ashok Raj wrote:
> > 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?
> 
> That's a chicken bit and should not be used usually. I'm adding it just
> in case.

How about a debugfs opt-out instead? Say something like 

/sys/kernel/debug/microcode/debug_control?

Since the code doesn't use this in the early loading, it appears we can
drop the cmdline entirely?

> 
> > 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.
> 
> Unconditionally.

Thanks, this simplifies.

Since this is quite different from what the "typical" HT behavior w.r.t ,
microcode, maybe good to document this behavior in microcode.rst.

If the user switches the expected flow, should the code "taint" if its opted out?

> 
> > Shouldn't the "control" be under LATE_LOADING? Since this only controls
> > late-loading behavior?
> 
> No, that's a bitfield and is going to be used for other flags, if
> needed and which are not necessarily late-loading related.

My .000002c. Maybe add such sysfs control only when we really need one,
but should not include debug use.

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

[Borislav Petkov]

On Fri, Jun 09, 2023 at 08:37:21AM -0700, Ashok Raj wrote:
> Since the code doesn't use this in the early loading, it appears we can
> drop the cmdline entirely?

The driver needs a control mechanism and this is it. It needs to be
always there so no, no debugfs.

> If the user switches the expected flow, should the code "taint" if its
> opted out?

When it turns out that this needs to happen, it'll get added then.

-- 
Regards/Gruss,
    Boris.

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

[tip: x86/microcode] x86/microcode: Add a "microcode=" command line option

[tip-bot2 for Borislav Petkov (AMD)]

The following commit has been merged into the x86/microcode branch of tip:

Commit-ID:     4a7349239418d78193e8d328f5eebd14a3f77bcd
Gitweb:        https://git.kernel.org/tip/4a7349239418d78193e8d328f5eebd14a3f77bcd
Author:        Borislav Petkov (AMD) <bp@alien8.de>
AuthorDate:    Thu, 18 May 2023 03:02:29 +02:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Mon, 12 Jun 2023 11:02:52 +02:00

x86/microcode: Add a "microcode=" command line option

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@alien8.de>
Link: https://lore.kernel.org/r/20230605141332.25948-2-bp@alien8.de
---
 Documentation/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 9e5bab2..b88ff02 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.
+
 	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 87208e4..76b5306 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 3afcf3d..5f3185d 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,
 	NULL
 };
 
@@ -683,3 +712,18 @@ static int __init microcode_init(void)
 }
 fs_initcall(save_microcode_in_initrd);
 late_initcall(microcode_init);
+
+static int __init parse_cmdline_param(char *str)
+{
+	if (!str)
+		return 0;
+
+	if (*str == '=')
+		str++;
+
+	if (!strcmp(str, "no_late_all"))
+		control &= ~LATE_ALL_THREADS;
+
+        return 1;
+}
+__setup("microcode", parse_cmdline_param);
diff --git a/arch/x86/kernel/cpu/microcode/internal.h b/arch/x86/kernel/cpu/microcode/internal.h
new file mode 100644
index 0000000..5e3c5fc
--- /dev/null
+++ 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)
+
+#endif /* __X86_MICROCODE_INTERNAL_H__ */

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@alien8.de>
>
> 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

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

[Borislav Petkov]

On Mon, Jun 12, 2023 at 05:26:28PM +0200, Thomas Gleixner wrote:
> Why is it suddenly required to prevent late loading on SMT threads?

The intent is, like a chicken bit, to revert to the *old* behavior which
would not load on both threads. In *case* some old configuration of CPU
and microcode cannot handle loading on both threads. Which is from
Bulldozer onwards but I don't think anyone uses Bulldozer anymore.

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

No, see patch 1 - it does exactly the same what this commit does but for
late loading.

Bottomline: on AMD, we should load on both threads by default.

> no_late_all is a horrible option name.

Yeah, at the time dis_ucode_ldr was horrible too. I tried harder this
time.

What do you suggest?

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

Huh, it has "late" in the name. :-)

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

See above. This chicken bit is there, just in case, to help in the case
where the user cannot do anything else. It should not be used, judging
by all the combinations I've tested here.

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

None of them should need it. Thus, a chicken bit as a last option in
production.

> global variable name without a proper prefix. Moo.

Lemme fix.

> Where is the documentation which tells me what I'm supposed to write
> into this file? Also this is a generic file, right?

Lemme write some docs about it.

> So what's the meaning for non AMD? I can write this bit into it
> successfully and nothing happens, right?

Yes, this bit is AMD-only.

> Why ULL bits for a unsigned long variable?

There's no BIT_UL() macro.

-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

Diff ontop with the proposed changes so that they're easily reviewable:

---
diff --git a/Documentation/arch/x86/microcode.rst b/Documentation/arch/x86/microcode.rst
index b627c6f36bcf..767e3e36935e 100644
--- a/Documentation/arch/x86/microcode.rst
+++ b/Documentation/arch/x86/microcode.rst
@@ -238,3 +238,13 @@ the final kernel image. The early loader finds them and applies them.
 Needless to say, this method is not the most flexible one because it
 requires rebuilding the kernel each time updated microcode from the CPU
 vendor is available.
+
+Loader control
+==============
+
+/sys/devices/system/cpu/microcode/control controls different aspects of
+the microcode loader's behavior and can be used to modify it by toggling
+bits in that file. Currently defined controls are:
+
+bit 0:  Do not load on all SMT threads on AMD. Loading on all logical
+        threads is enabled by default.
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 76b530697951..3755f4a3c820 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -703,7 +703,7 @@ static enum ucode_state apply_microcode_amd(int cpu)
 
 	/* need to apply patch? */
 	if ((rev > mc_amd->hdr.patch_id) ||
-	    (rev == mc_amd->hdr.patch_id && !(control & LATE_ALL_THREADS))) {
+	    (rev == mc_amd->hdr.patch_id && !(microcode_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 5f3185d2814c..7ec87872756b 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -47,7 +47,7 @@
 static struct microcode_ops	*microcode_ops;
 static bool dis_ucode_ldr = true;
 
-unsigned long control = LATE_ALL_THREADS;
+unsigned long microcode_control = LATE_ALL_THREADS;
 
 bool initrd_gone;
 
@@ -529,7 +529,7 @@ static ssize_t processor_flags_show(struct device *dev,
 static ssize_t control_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
 {
-	return sprintf(buf, "0x%lx\n", control);
+	return sprintf(buf, "0x%lx\n", microcode_control);
 }
 
 static ssize_t control_store(struct device *dev,
@@ -544,7 +544,7 @@ static ssize_t control_store(struct device *dev,
 	if (val & CONTROL_FLAGS_MASK)
 		return -EINVAL;
 
-	control = val;
+	microcode_control = val;
 
 	return count;
 }
@@ -722,7 +722,7 @@ static int __init parse_cmdline_param(char *str)
 		str++;
 
 	if (!strcmp(str, "no_late_all"))
-		control &= ~LATE_ALL_THREADS;
+		microcode_control &= ~LATE_ALL_THREADS;
 
         return 1;
 }
diff --git a/arch/x86/kernel/cpu/microcode/internal.h b/arch/x86/kernel/cpu/microcode/internal.h
index 5e3c5fc3851f..3823c8ec8f97 100644
--- a/arch/x86/kernel/cpu/microcode/internal.h
+++ b/arch/x86/kernel/cpu/microcode/internal.h
@@ -2,7 +2,7 @@
 #ifndef __X86_MICROCODE_INTERNAL_H__
 #define __X86_MICROCODE_INTERNAL_H__
 
-extern unsigned long control;
+extern unsigned long microcode_control;
 
 /* Loader control flags. */
 enum control_flags {

-- 
Regards/Gruss,
    Boris.

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

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

[Thomas Gleixner]

On Mon, Jun 12 2023 at 17:42, Borislav Petkov wrote:
> On Mon, Jun 12, 2023 at 05:26:28PM +0200, Thomas Gleixner wrote:
>> Why is it suddenly required to prevent late loading on SMT threads?
>
> The intent is, like a chicken bit, to revert to the *old* behavior which
> would not load on both threads. In *case* some old configuration of CPU
> and microcode cannot handle loading on both threads. Which is from
> Bulldozer onwards but I don't think anyone uses Bulldozer anymore.

So why not making the late loading thing depend on >= bulldozer?

Also I'm failing to see how this is different from the early loader:

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

That changelogs says:

  "Btw, change only the early paths. On the late loading paths, there's
   no point in doing per-thread modification because if is it some case
   like in the bugzilla below - removing a CPUID flag - the kernel cannot
   go and un-use features it has detected are there early. For that, one
   should use early loading anyway."

which makes sense to some extent, but obviously there is a use case for
late loading on both threads. So what are you worried about breaking
here?

If the late load does a behavioural change, then it does not matter
whether you make sure both threads are hosed in the same way or not. If
the late load is harmless and just addressing some correctness issue
then loading on the secondary thread should be a noop, right?

> No, see patch 1 - it does exactly the same what this commit does but for
> late loading.

Sorry no. Patch 1 brings the late loading decision in line with the
early loading decision, i.e. ensure that microcode is loaded on both
threads. That condition

	/* need to apply patch? */
-	if (rev >= mc_amd->hdr.patch_id) {
+	if (rev > mc_amd->hdr.patch_id) {

really could do with a proper comment which explains why loading the
same revision makes sense.

> Bottomline: on AMD, we should load on both threads by default.

Fine. Then what is this about? If it survives early loading on both
threads then why do we need a chickenbit for late loading?

So if someone turns that on needlessly then in the worst case the
primary thread behaves differently than the secondary thread, right?

>> Aside of that why is this a kernel side chicken bit and not communicated
>> by the microcode header?
>
> See above. This chicken bit is there, just in case, to help in the case
> where the user cannot do anything else. It should not be used, judging
> by all the combinations I've tested here.

If it should not be used, then where is the big fat warning emitted when
it is actually enabled?

The more I read this the less I'm convinced that this makes any sense at
all:

  1) You did not add a chicken bit when you made this change for the
     early loader. Why needs late loading suddenly one?

  2) Late loading is "use at your own peril" up to the point where the
     minimum revision field is in place. So why do we need a bandaid
     chickenbit for something which is considered unsafe anyway?

  3) The microcode people @AMD should be able to figure out whether
     unconditionally (late) loading on the secondary thread is safe or
     not.

I told Intel to make microcode loading something sane. It's not any
different for AMD. This hastily cobbled together chickenbit thing
without a fundamental technical justification definitely does not
quality for sane.

>> Why ULL bits for a unsigned long variable?
>
> There's no BIT_UL() macro.

git grep '#define BIT(' include/

Thanks,

        tglx

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

[Borislav Petkov]

Just to sum up what we talked about on IRC:

Early loading already does per-thread loading unconditionally and it
doesn't have a chicken bit. I will do the chicken bit for both loading
methods but keep it out-of-tree if it turns out we need it.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/2] x86/microcode/AMD: Load late on both threads too

[Dave Hansen]

On 6/5/23 07:13, Borislav Petkov wrote:
> Do the same as early loading - load on both threads.

Hey Boris,

What's the benefit of doing the loading on both threads?  I would have
naively thought it was just wasted work.

I think I even have a back-burnered Intel patch around somewhere that
ensures that we *never* load on both threads.

Re: [PATCH 1/2] x86/microcode/AMD: Load late on both threads too

[Borislav Petkov]

On Wed, Jun 07, 2023 at 12:36:53PM -0700, Dave Hansen wrote:
> What's the benefit of doing the loading on both threads?  I would have
> naively thought it was just wasted work.

I have the perfect example for this, see:

e7ad18d1169c ("x86/microcode/AMD: Apply the patch early on every logical thread")

so it is for reasons like that.

> I think I even have a back-burnered Intel patch around somewhere that
> ensures that we *never* load on both threads.

Interesting - I guess there are considerable differences in microcode
architecture between the two. :-)

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/2] x86/microcode/AMD: Load late on both threads too

[Dave Hansen]

On 6/7/23 13:03, Borislav Petkov wrote:
> On Wed, Jun 07, 2023 at 12:36:53PM -0700, Dave Hansen wrote:
>> What's the benefit of doing the loading on both threads?  I would have
>> naively thought it was just wasted work.
> I have the perfect example for this, see:
> 
> e7ad18d1169c ("x86/microcode/AMD: Apply the patch early on every logical thread")
> 
> so it is for reasons like that.

Yikes, so the second CMT thread reports a bumped version but not all the
_effects_ of that version?  That's, uh ... fun???

>> I think I even have a back-burnered Intel patch around somewhere that
>> ensures that we *never* load on both threads.
> Interesting - I guess there are considerable differences in microcode
> architecture between the two. 😄

Yeah, sure seems that way.

Re: [PATCH 1/2] x86/microcode/AMD: Load late on both threads too

[Jim Mattson]

On Wed, Jun 07, 2023 at 13:15:42 -0700, Dave Hansen wrote:
>On 6/7/23 13:03, Borislav Petkov wrote:
>> On Wed, Jun 07, 2023 at 12:36:53PM -0700, Dave Hansen wrote:
>>> What's the benefit of doing the loading on both threads?  I would have
>>> naively thought it was just wasted work.
>> I have the perfect example for this, see:
>> 
>> e7ad18d1169c ("x86/microcode/AMD: Apply the patch early on every logical thread")
>> 
>> so it is for reasons like that.
>
>Yikes, so the second CMT thread reports a bumped version but not all the
>_effects_ of that version?  That's, uh ... fun???

That does seem like fun. :)

Borislav,

Is there any way to tell that a microcode patch has been loaded on only one
thread? And what does this mean for SEV-SNP attestation?

Re: [PATCH 1/2] x86/microcode/AMD: Load late on both threads too

[Borislav Petkov]

On Wed, Aug 16, 2023 at 01:17:00PM -0700, Jim Mattson wrote:
> Is there any way to tell that a microcode patch has been loaded on
> only one thread?

Only the example you see in the bugzilla link above.

> And what does this mean for SEV-SNP attestation?

More specifically? The CPUID page or microcode revision?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/2] x86/microcode/AMD: Load late on both threads too

[Jim Mattson]

On Wed, Aug 16, 2023 at 2:18 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, Aug 16, 2023 at 01:17:00PM -0700, Jim Mattson wrote:
> > Is there any way to tell that a microcode patch has been loaded on
> > only one thread?
>
> Only the example you see in the bugzilla link above.
>
> > And what does this mean for SEV-SNP attestation?
>
> More specifically? The CPUID page or microcode revision?

Specifically, "Security Version Numbers (SVNs) of the currently
executing platform firmware and microcode."

> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/2] x86/microcode/AMD: Load late on both threads too

[Borislav Petkov]

On Wed, Aug 16, 2023 at 02:23:52PM -0700, Jim Mattson wrote:
> Specifically, "Security Version Numbers (SVNs) of the currently
> executing platform firmware and microcode."

Yes, I know the microcode revision is part of the attestation. 

What exactly are you asking?

If you don't state your question, how do you expect me to answer?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/2] x86/microcode/AMD: Load late on both threads too

[Jim Mattson]

On Wed, Aug 16, 2023 at 2:30 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, Aug 16, 2023 at 02:23:52PM -0700, Jim Mattson wrote:
> > Specifically, "Security Version Numbers (SVNs) of the currently
> > executing platform firmware and microcode."
>
> Yes, I know the microcode revision is part of the attestation.
>
> What exactly are you asking?
>
> If you don't state your question, how do you expect me to answer?

Doesn't this render that attestation misleading, since the microcode
patch may not have been loaded on all logical processors?

--
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/2] x86/microcode/AMD: Load late on both threads too

[Borislav Petkov]

On Wed, Aug 16, 2023 at 02:36:57PM -0700, Jim Mattson wrote:
> Doesn't this render that attestation misleading, since the microcode
> patch may not have been loaded on all logical processors?

For that it doesn't matter because the microcode engine is shared
between the two threads. The updated microcode revision is shown on any
of the two threads so you can load on one only. And we did this for
years.

Only recently we started loading on both and we will be doing that from
now on.

What could be problematic is if it simply fails loading on some cores
- regardless of SMT - but that would be problematic not only to SEV-SNP
attestation but to the general system health. tglx has some patches
which verify what has been successfully loaded where so hopefully we'll
be verifying more in that area.

HTH.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/2] x86/microcode/AMD: Load late on both threads too

[Jim Mattson]

On Wed, Aug 16, 2023 at 2:59 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Wed, Aug 16, 2023 at 02:36:57PM -0700, Jim Mattson wrote:
> > Doesn't this render that attestation misleading, since the microcode
> > patch may not have been loaded on all logical processors?
>
> For that it doesn't matter because the microcode engine is shared
> between the two threads. The updated microcode revision is shown on any
> of the two threads so you can load on one only. And we did this for
> years.
>
> Only recently we started loading on both and we will be doing that from
> now on.

SEV-SNP is supposed to protect the guest from a malicious host. A
malicious host may not load the microcode update on both threads. As a
result, it gives me some concern when I see something like this
(https://lore.kernel.org/lkml/20230808190239.131508-1-john.allen@amd.com/):

+NOTE: For Genoa (Family=0x19 Model=0x11) and Bergamo (Family=0x19 Model=0xa0),
+either AGESA version >= 1.0.0.8 OR a kernel with the following commit is
+required:
+a32b0f0db3f3 ("x86/microcode/AMD: Load late on both threads too")

It seems problematic if the guest can't tell from the attestation
whether or not the identified microcode revision has been correctly
applied.

> What could be problematic is if it simply fails loading on some cores
> - regardless of SMT - but that would be problematic not only to SEV-SNP
> attestation but to the general system health. tglx has some patches
> which verify what has been successfully loaded where so hopefully we'll
> be verifying more in that area.

I had assumed that the SEV-SNP microcode revision attestation was for
all logical processors on the host. Are you saying that it is not?

Re: [PATCH 1/2] x86/microcode/AMD: Load late on both threads too

[Borislav Petkov]

On Wed, Aug 16, 2023 at 03:37:20PM -0700, Jim Mattson wrote:
> SEV-SNP is supposed to protect the guest from a malicious host. A
> malicious host may not load the microcode update on both threads. As a
> result, it gives me some concern when I see something like this
> (https://lore.kernel.org/lkml/20230808190239.131508-1-john.allen@amd.com/):

All I can say is that if this is needed for a SEV/SNP-relevant fix, the
attestation flow will be adjusted to handle this properly.

> I had assumed that the SEV-SNP microcode revision attestation was for
> all logical processors on the host. Are you saying that it is not?

The attestation flow is fine as it takes into account the lowest
microcode revision across all cores in the system. So it doesn't matter
for SNP which core has been applied successfully or which one hasn't.

HTH.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/2] x86/microcode/AMD: Load late on both threads too

[Peter Shier]

> tglx has some patches which verify what has been successfully loaded where so hopefully we'll
> be verifying more in that area.

Are those ready for some early testing?

Re: [PATCH 1/2] x86/microcode/AMD: Load late on both threads too

[Borislav Petkov]

On Thu, Aug 17, 2023 at 11:02:21AM -0700, Peter Shier wrote:
> > tglx has some patches which verify what has been successfully loaded where so hopefully we'll
> > be verifying more in that area.
> 
> Are those ready for some early testing?

Yap, I'm sure tglx will be happy if people test and report bugs.

Thx.

-- 
Regards/Gruss,
    Boris.

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

[tip: x86/microcode] x86/microcode/AMD: Load late on both threads too

[tip-bot2 for Borislav Petkov (AMD)]

The following commit has been merged into the x86/microcode branch of tip:

Commit-ID:     a32b0f0db3f396f1c9be2fe621e77c09ec3d8e7d
Gitweb:        https://git.kernel.org/tip/a32b0f0db3f396f1c9be2fe621e77c09ec3d8e7d
Author:        Borislav Petkov (AMD) <bp@alien8.de>
AuthorDate:    Tue, 02 May 2023 19:53:50 +02:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Mon, 12 Jun 2023 11:02:17 +02:00

x86/microcode/AMD: Load late on both threads too

Do the same as early loading - load on both threads.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Cc: <stable@kernel.org>
Link: https://lore.kernel.org/r/20230605141332.25948-1-bp@alien8.de
---
 arch/x86/kernel/cpu/microcode/amd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index f14f4ea..87208e4 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -700,7 +700,7 @@ 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) {
 		ret = UCODE_OK;
 		goto out;
 	}