From: "Borislav Petkov (AMD)" <[email protected]>
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) <[email protected]>
---
.../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
Hi
On Mon, Jun 05, 2023 at 04:13:32PM +0200, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <[email protected]>
>
> 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) <[email protected]>
> ---
> .../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?
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
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.
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) <[email protected]>
AuthorDate: Thu, 18 May 2023 03:02:29 +02:00
Committer: Borislav Petkov (AMD) <[email protected]>
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) <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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__ */
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
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
On Mon, Jun 05 2023 at 16:13, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <[email protected]>
>
> 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
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
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
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