Hi Boris,
Here is a followup after v3[1] with all comments addressed.
Please review and apply.
Changes since v3:
Tony, Ingo
- Display clear message when microcode load fails.
Boris
- Changed function names microcode_store_cpu_caps() ->
store_cpu_caps().
- Fix commit logs
- Document new parameter to microcode_check()
Dave Hansen
- Fix commit log
- Change parameter names from generic to something that's
meaningful.
[1] https://lore.kernel.org/lkml/[email protected]/
Ashok Raj (6):
x86/microcode: Add a parameter to microcode_check() to store CPU
capabilities
x86/microcode/core: Take a snapshot before and after applying
microcode
x86/microcode: Display revisions only when update is successful
x86/microcode/intel: Use a plain revision argument for
print_ucode_rev()
x86/microcode/intel: Print old and new rev during early boot
x86/microcode/intel: Print when early microcode loading fails
arch/x86/include/asm/processor.h | 3 +-
arch/x86/kernel/cpu/common.c | 48 ++++++++++++++++++-------
arch/x86/kernel/cpu/microcode/core.c | 20 ++++++++---
arch/x86/kernel/cpu/microcode/intel.c | 52 +++++++++++++--------------
4 files changed, 77 insertions(+), 46 deletions(-)
base-commit: b7bfaa761d760e72a969d116517eaa12e404c262
prerequisite-patch-id: 450e9658e9f802a2f3f784ba18267bc47ee878b8
--
2.34.1
Make early loading message to match late loading messages. Print
both old and new revisions.
This is helpful to know what the BIOS loaded revision is before an early
update.
New dmesg log is shown below.
microcode: early update: 0x2b000041 -> 0x2b000070 date = 2000-01-01
Cache the early BIOS revision before the microcode update and change the
print_ucode_info() so it prints both the old and new revision in the same
format as microcode_reload_late().
Signed-off-by: Ashok Raj <[email protected]>
Reviewed-by: Thomas Gleixner <[email protected]>
Cc: LKML <[email protected]>
Cc: x86 <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Alison Schofield <[email protected]>
Cc: Reinette Chatre <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
Updates since V1:
Thomas: Commit log updates as suggested.
---
arch/x86/kernel/cpu/microcode/intel.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 1d709b72cfd0..f24300830ed7 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -310,10 +310,10 @@ static bool load_builtin_intel_microcode(struct cpio_data *cp)
/*
* Print ucode update info.
*/
-static void print_ucode_info(unsigned int new_rev, unsigned int date)
+static void print_ucode_info(int old_rev, int new_rev, unsigned int date)
{
- pr_info_once("microcode updated early to revision 0x%x, date = %04x-%02x-%02x\n",
- new_rev, date & 0xffff, date >> 24,
+ pr_info_once("early update: 0x%x -> 0x%x, date = %04x-%02x-%02x\n",
+ old_rev, new_rev, date & 0xffff, date >> 24,
(date >> 16) & 0xff);
}
@@ -321,6 +321,7 @@ static void print_ucode_info(unsigned int new_rev, unsigned int date)
static int delay_ucode_info;
static int current_mc_date;
+static int early_old_rev;
/*
* Print early updated ucode info after printk works. This is delayed info dump.
@@ -331,7 +332,7 @@ void show_ucode_info_early(void)
if (delay_ucode_info) {
intel_cpu_collect_info(&uci);
- print_ucode_info(uci.cpu_sig.rev. current_mc_date);
+ print_ucode_info(early_old_rev, uci.cpu_sig.rev, current_mc_date);
delay_ucode_info = 0;
}
}
@@ -340,30 +341,33 @@ void show_ucode_info_early(void)
* At this point, we can not call printk() yet. Delay printing microcode info in
* show_ucode_info_early() until printk() works.
*/
-static void print_ucode(int new_rev, int date)
+static void print_ucode(int old_rev, int new_rev, int date)
{
struct microcode_intel *mc;
int *delay_ucode_info_p;
int *current_mc_date_p;
+ int *early_old_rev_p;
delay_ucode_info_p = (int *)__pa_nodebug(&delay_ucode_info);
current_mc_date_p = (int *)__pa_nodebug(¤t_mc_date);
+ early_old_rev_p = (int *)__pa_nodebug(&early_old_rev);
*delay_ucode_info_p = 1;
*current_mc_date_p = date;
+ *early_old_rev_p = old_rev;
}
#else
-static inline void print_ucode(int new_rev, int date)
+static inline void print_ucode(int old_rev, int new_rev, int date)
{
- print_ucode_info(new_rev, date);
+ print_ucode_info(old_rev, new_rev, date);
}
#endif
static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
{
struct microcode_intel *mc;
- u32 rev;
+ u32 rev, old_rev;
mc = uci->mc;
if (!mc)
@@ -389,6 +393,7 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
/* write microcode via MSR 0x79 */
native_wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
+ old_rev = rev;
rev = intel_get_microcode_revision();
if (rev != mc->hdr.rev)
return -1;
@@ -396,9 +401,9 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
uci->cpu_sig.rev = rev;
if (early)
- print_ucode(uci->cpu_sig.rev, mc->hdr.date);
+ print_ucode(old_rev, uci->cpu_sig.rev, mc->hdr.date);
else
- print_ucode_info(uci->cpu_sig.rev, mc->hdr.date);
+ print_ucode_info(old_rev, uci->cpu_sig.rev, mc->hdr.date);
return 0;
}
--
2.34.1
The kernel caches features about each CPU's features at boot in an
x86_capability[] structure. The microcode update takes one snapshot and
compares it with the saved copy at boot.
However, the capabilities in the boot copy can be turned off as a result of
certain command line parameters or configuration restrictions. This can
cause a mismatch when comparing the values before and after the microcode
update.
microcode_check() is called after an update to report any previously
cached CPUID bits might have changed due to the update.
store_cpu_caps() basically stores the original CPU reported
values and not the OS modified values. This will avoid giving a false
warning even when no capabilities have changed.
Ignore the capabilities recorded at boot. Take a new snapshot before the
update and compare with a snapshot after the update to eliminate the false
warning.
Signed-off-by: Ashok Raj <[email protected]>
Fixes: 1008c52c09dc ("x86/CPU: Add a microcode loader callback")
Cc: LKML <[email protected]>
Cc: x86 <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Alison Schofield <[email protected]>
Cc: Reinette Chatre <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
Changes since V3:
- Boris
- Change function from microcode_store_cpu_caps -> store_cpu_caps
- Split comments in store_cpu_caps().
- Dave Hansen
- Change parameters names to something meaninful.
- Cleaned up some commit log.
Changes since V2:
- Boris
- Keep microcode_check() inside cpu/common.c and not bleed
get_cpu_caps() outside of core code.
- Thomas
- Commit log changes.
---
arch/x86/include/asm/processor.h | 1 +
arch/x86/kernel/cpu/common.c | 39 ++++++++++++++++++++--------
arch/x86/kernel/cpu/microcode/core.c | 7 +++++
3 files changed, 36 insertions(+), 11 deletions(-)
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index f256a4ddd25d..a77dee6a2bf2 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -698,6 +698,7 @@ bool xen_set_default_idle(void);
void __noreturn stop_this_cpu(void *dummy);
void microcode_check(struct cpuinfo_x86 *prev_info);
+void store_cpu_caps(struct cpuinfo_x86 *info);
enum l1tf_mitigations {
L1TF_MITIGATION_OFF,
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 0f5a173d0871..f5c6feed6c26 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2297,6 +2297,29 @@ void cpu_init_secondary(void)
#endif
#ifdef CONFIG_MICROCODE_LATE_LOADING
+/**
+ * store_cpu_caps() - Store a snapshot of CPU capabilities
+ *
+ * Returns: None
+ */
+void store_cpu_caps(struct cpuinfo_x86 *curr_info)
+{
+ /* Reload CPUID max function as it might've changed. */
+ curr_info->cpuid_level = cpuid_eax(0);
+
+ /*
+ * Copy all capability leafs to pick up the synthetic ones
+ */
+ memcpy(&curr_info->x86_capability, &boot_cpu_data.x86_capability,
+ sizeof(curr_info->x86_capability));
+
+ /*
+ * Capabilities copied from BSP will get overwritten
+ * with the snapshot below
+ */
+ get_cpu_cap(curr_info);
+}
+
/**
* microcode_check() - Check if any CPU capabilities changed after an update.
* @prev_info: CPU capabilities stored before an update.
@@ -2309,22 +2332,16 @@ void cpu_init_secondary(void)
*/
void microcode_check(struct cpuinfo_x86 *prev_info)
{
- perf_check_microcode();
+ struct cpuinfo_x86 curr_info;
- /* Reload CPUID max function as it might've changed. */
- prev_info->cpuid_level = cpuid_eax(0);
+ perf_check_microcode();
/*
- * Copy all capability leafs to pick up the synthetic ones so that
- * memcmp() below doesn't fail on that. The ones coming from CPUID will
- * get overwritten in get_cpu_cap().
+ * Get a snapshot of CPU capabilities
*/
- memcpy(&prev_info->x86_capability, &boot_cpu_data.x86_capability,
- sizeof(prev_info->x86_capability));
-
- get_cpu_cap(prev_info);
+ store_cpu_caps(&curr_info);
- if (!memcmp(&prev_info->x86_capability, &boot_cpu_data.x86_capability,
+ if (!memcmp(&prev_info->x86_capability, &curr_info.x86_capability,
sizeof(prev_info->x86_capability)))
return;
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index e39d83be794b..bb943a91a364 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -447,6 +447,13 @@ static int microcode_reload_late(void)
atomic_set(&late_cpus_in, 0);
atomic_set(&late_cpus_out, 0);
+ /*
+ * Take a snapshot before the microcode update, so we can compare
+ * them after the update is successful to check for any bits
+ * changed.
+ */
+ store_cpu_caps(&prev_info);
+
ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
if (ret == 0)
microcode_check(&prev_info);
--
2.34.1
The following commit has been merged into the x86/microcode branch of tip:
Commit-ID: c0dd9245aa9e25a697181f6085692272c9ec61bc
Gitweb: https://git.kernel.org/tip/c0dd9245aa9e25a697181f6085692272c9ec61bc
Author: Ashok Raj <[email protected]>
AuthorDate: Mon, 09 Jan 2023 07:35:51 -08:00
Committer: Borislav Petkov (AMD) <[email protected]>
CommitterDate: Sat, 21 Jan 2023 14:53:20 +01:00
x86/microcode: Check CPU capabilities after late microcode update correctly
The kernel caches each CPU's feature bits at boot in an x86_capability[]
structure. However, the capabilities in the BSP's copy can be turned off
as a result of certain command line parameters or configuration
restrictions, for example the SGX bit. This can cause a mismatch when
comparing the values before and after the microcode update.
Another example is X86_FEATURE_SRBDS_CTRL which gets added only after
microcode update:
--- cpuid.before 2023-01-21 14:54:15.652000747 +0100
+++ cpuid.after 2023-01-21 14:54:26.632001024 +0100
@@ -10,7 +10,7 @@ CPU:
0x00000004 0x04: eax=0x00000000 ebx=0x00000000 ecx=0x00000000 edx=0x00000000
0x00000005 0x00: eax=0x00000040 ebx=0x00000040 ecx=0x00000003 edx=0x11142120
0x00000006 0x00: eax=0x000027f7 ebx=0x00000002 ecx=0x00000001 edx=0x00000000
- 0x00000007 0x00: eax=0x00000000 ebx=0x029c6fbf ecx=0x40000000 edx=0xbc002400
+ 0x00000007 0x00: eax=0x00000000 ebx=0x029c6fbf ecx=0x40000000 edx=0xbc002e00
^^^
and which proves for a gazillionth time that late loading is a bad bad
idea.
microcode_check() is called after an update to report any previously
cached CPUID bits which might have changed due to the update.
Therefore, store the cached CPU caps before the update and compare them
with the CPU caps after the microcode update has succeeded.
Thus, the comparison is done between the CPUID *hardware* bits before
and after the upgrade instead of using the cached, possibly runtime
modified values in BSP's boot_cpu_data copy.
As a result, false warnings about CPUID bits changes are avoided.
[ bp:
- Massage.
- Add SRBDS_CTRL example.
- Add kernel-doc.
- Incorporate forgotten review feedback from dhansen.
]
Fixes: 1008c52c09dc ("x86/CPU: Add a microcode loader callback")
Signed-off-by: Ashok Raj <[email protected]>
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/include/asm/processor.h | 1 +-
arch/x86/kernel/cpu/common.c | 36 +++++++++++++++++----------
arch/x86/kernel/cpu/microcode/core.c | 6 +++++-
3 files changed, 30 insertions(+), 13 deletions(-)
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index f256a4d..a77dee6 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -698,6 +698,7 @@ bool xen_set_default_idle(void);
void __noreturn stop_this_cpu(void *dummy);
void microcode_check(struct cpuinfo_x86 *prev_info);
+void store_cpu_caps(struct cpuinfo_x86 *info);
enum l1tf_mitigations {
L1TF_MITIGATION_OFF,
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 0f5a173..5ff73ba 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2298,6 +2298,25 @@ void cpu_init_secondary(void)
#ifdef CONFIG_MICROCODE_LATE_LOADING
/**
+ * store_cpu_caps() - Store a snapshot of CPU capabilities
+ * @curr_info: Pointer where to store it
+ *
+ * Returns: None
+ */
+void store_cpu_caps(struct cpuinfo_x86 *curr_info)
+{
+ /* Reload CPUID max function as it might've changed. */
+ curr_info->cpuid_level = cpuid_eax(0);
+
+ /* Copy all capability leafs and pick up the synthetic ones. */
+ memcpy(&curr_info->x86_capability, &boot_cpu_data.x86_capability,
+ sizeof(curr_info->x86_capability));
+
+ /* Get the hardware CPUID leafs */
+ get_cpu_cap(curr_info);
+}
+
+/**
* microcode_check() - Check if any CPU capabilities changed after an update.
* @prev_info: CPU capabilities stored before an update.
*
@@ -2309,22 +2328,13 @@ void cpu_init_secondary(void)
*/
void microcode_check(struct cpuinfo_x86 *prev_info)
{
- perf_check_microcode();
-
- /* Reload CPUID max function as it might've changed. */
- prev_info->cpuid_level = cpuid_eax(0);
+ struct cpuinfo_x86 curr_info;
- /*
- * Copy all capability leafs to pick up the synthetic ones so that
- * memcmp() below doesn't fail on that. The ones coming from CPUID will
- * get overwritten in get_cpu_cap().
- */
- memcpy(&prev_info->x86_capability, &boot_cpu_data.x86_capability,
- sizeof(prev_info->x86_capability));
+ perf_check_microcode();
- get_cpu_cap(prev_info);
+ store_cpu_caps(&curr_info);
- if (!memcmp(&prev_info->x86_capability, &boot_cpu_data.x86_capability,
+ if (!memcmp(&prev_info->x86_capability, &curr_info.x86_capability,
sizeof(prev_info->x86_capability)))
return;
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index dc5dfba..8ec38c1 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -446,6 +446,12 @@ static int microcode_reload_late(void)
atomic_set(&late_cpus_in, 0);
atomic_set(&late_cpus_out, 0);
+ /*
+ * Take a snapshot before the microcode update in order to compare and
+ * check whether any bits changed after an update.
+ */
+ store_cpu_caps(&prev_info);
+
ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
if (ret == 0)
microcode_check(&prev_info);