Hi Boris
This is a followup after earlier post [1]
Sending the rest of the patches after first 2 patches that were merged.
Please review and consider applying.
Changes since v1:
- Updated comments and added reviewed by from Thomas.
- Moved microcode_check() to where it originally was based on your
feedback. [2]
[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://lore.kernel.org/lkml/Y6tMgcU2aGbx%[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 | 29 ++++++++++-----
arch/x86/kernel/cpu/microcode/core.c | 17 ++++++---
arch/x86/kernel/cpu/microcode/intel.c | 52 +++++++++++++--------------
4 files changed, 60 insertions(+), 41 deletions(-)
base-commit: 88603b6dc419445847923fcb7fe5080067a30f98
--
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]>
---
Updates since previous post.
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
Right now, microcode loading failures and successes print the same
message "Reloading completed". This is misleading to users.
Display the updated revision number only if an update was successful.
Suggested-by: Thomas Gleixner <[email protected]>
Signed-off-by: Ashok Raj <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Link: https://lore.kernel.org/lkml/874judpqqd.ffs@tglx/
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]>
---
arch/x86/kernel/cpu/microcode/core.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 14d9031ed68a..e67f8923f119 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -455,11 +455,12 @@ static int microcode_reload_late(void)
microcode_store_cpu_caps(&info);
ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
- if (ret == 0)
- microcode_check(&info);
- pr_info("Reload completed, microcode revision: 0x%x -> 0x%x\n",
- old, boot_cpu_data.microcode);
+ if (ret == 0) {
+ pr_info("Reload completed, microcode revision: 0x%x -> 0x%x\n",
+ old, boot_cpu_data.microcode);
+ microcode_check(&info);
+ }
return ret;
}
--
2.34.1
This is a preparation before the next patch uses this to compare CPU
capabilities after performing an update.
Add a parameter to store CPU capabilities before performing a microcode
update.
Signed-off-by: Ashok Raj <[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]>
---
arch/x86/include/asm/processor.h | 2 +-
arch/x86/kernel/cpu/common.c | 12 +++++-------
arch/x86/kernel/cpu/microcode/core.c | 3 ++-
3 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 4e35c66edeb7..387578049de0 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -697,7 +697,7 @@ bool xen_set_default_idle(void);
#endif
void __noreturn stop_this_cpu(void *dummy);
-void microcode_check(void);
+void microcode_check(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 9cfca3d7d0e2..b9c7529c920e 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2302,25 +2302,23 @@ void cpu_init_secondary(void)
* only when microcode has been updated. Caller holds microcode_mutex and CPU
* hotplug lock.
*/
-void microcode_check(void)
+void microcode_check(struct cpuinfo_x86 *info)
{
- struct cpuinfo_x86 info;
-
perf_check_microcode();
/* Reload CPUID max function as it might've changed. */
- info.cpuid_level = cpuid_eax(0);
+ info->cpuid_level = cpuid_eax(0);
/*
* 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(&info.x86_capability, &boot_cpu_data.x86_capability, sizeof(info.x86_capability));
+ memcpy(&info->x86_capability, &boot_cpu_data.x86_capability, sizeof(info->x86_capability));
- get_cpu_cap(&info);
+ get_cpu_cap(info);
- if (!memcmp(&info.x86_capability, &boot_cpu_data.x86_capability, sizeof(info.x86_capability)))
+ if (!memcmp(&info->x86_capability, &boot_cpu_data.x86_capability, sizeof(info->x86_capability)))
return;
pr_warn("x86/CPU: CPU features have changed after loading microcode, but might not take effect.\n");
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index c4cd7328177b..d86a4f910a6b 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -439,6 +439,7 @@ static int __reload_late(void *info)
static int microcode_reload_late(void)
{
int old = boot_cpu_data.microcode, ret;
+ struct cpuinfo_x86 info;
pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
pr_err("You should switch to early loading, if possible.\n");
@@ -448,7 +449,7 @@ static int microcode_reload_late(void)
ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
if (ret == 0)
- microcode_check();
+ microcode_check(&info);
pr_info("Reload completed, microcode revision: 0x%x -> 0x%x\n",
old, boot_cpu_data.microcode);
--
2.34.1
print_ucode_rev() takes a struct ucode_cpu_info argument. The sole purpose
of it is to print the microcode revision.
The only available ucode_cpu_info always describes the currently loaded
microcode revision. After a microcode update is successful, this is the new
revision, or on failure it is the original revision.
Subsequent changes need to print both the original and new revision, but
the original version will be cached in a plain integer, which makes the
code inconsistent.
Replace the struct ucode_cpu_info argument with a plain integer which
contains the revision number and adjust the call sites accordingly.
No functional change.
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]>
---
Changes since earlier post.
Thomas:
- Updated commit log as suggested
- Remove the line break after static void before print_ucode_info
---
arch/x86/kernel/cpu/microcode/intel.c | 31 ++++++++-------------------
1 file changed, 9 insertions(+), 22 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 6bebc46ad8b1..1d709b72cfd0 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -310,13 +310,10 @@ static bool load_builtin_intel_microcode(struct cpio_data *cp)
/*
* Print ucode update info.
*/
-static void
-print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
+static void print_ucode_info(unsigned int new_rev, unsigned int date)
{
pr_info_once("microcode updated early to revision 0x%x, date = %04x-%02x-%02x\n",
- uci->cpu_sig.rev,
- date & 0xffff,
- date >> 24,
+ new_rev, date & 0xffff, date >> 24,
(date >> 16) & 0xff);
}
@@ -334,7 +331,7 @@ void show_ucode_info_early(void)
if (delay_ucode_info) {
intel_cpu_collect_info(&uci);
- print_ucode_info(&uci, current_mc_date);
+ print_ucode_info(uci.cpu_sig.rev. current_mc_date);
delay_ucode_info = 0;
}
}
@@ -343,33 +340,23 @@ 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(struct ucode_cpu_info *uci)
+static void print_ucode(int new_rev, int date)
{
struct microcode_intel *mc;
int *delay_ucode_info_p;
int *current_mc_date_p;
- mc = uci->mc;
- if (!mc)
- return;
-
delay_ucode_info_p = (int *)__pa_nodebug(&delay_ucode_info);
current_mc_date_p = (int *)__pa_nodebug(¤t_mc_date);
*delay_ucode_info_p = 1;
- *current_mc_date_p = mc->hdr.date;
+ *current_mc_date_p = date;
}
#else
-static inline void print_ucode(struct ucode_cpu_info *uci)
+static inline void print_ucode(int new_rev, int date)
{
- struct microcode_intel *mc;
-
- mc = uci->mc;
- if (!mc)
- return;
-
- print_ucode_info(uci, mc->hdr.date);
+ print_ucode_info(new_rev, date);
}
#endif
@@ -409,9 +396,9 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
uci->cpu_sig.rev = rev;
if (early)
- print_ucode(uci);
+ print_ucode(uci->cpu_sig.rev, mc->hdr.date);
else
- print_ucode_info(uci, mc->hdr.date);
+ print_ucode_info(uci->cpu_sig.rev, mc->hdr.date);
return 0;
}
--
2.34.1
On Tue, Jan 03, 2023 at 10:02:07AM -0800, Ashok Raj wrote:
> This is a preparation before the next patch uses this to compare CPU
Once a patch is in git, the concept of "subsequent" or "next" patch becomes
ambiguous depending on how you're sorting them.
So you should strive for your commit messages to make sense on their own,
without referencing other "subsequent" or "next" patches.
> capabilities after performing an update.
>
> Add a parameter to store CPU capabilities before performing a microcode
> update.
" ... so that code later can do X."
And that is enough for an explanation.
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 9cfca3d7d0e2..b9c7529c920e 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -2302,25 +2302,23 @@ void cpu_init_secondary(void)
> * only when microcode has been updated. Caller holds microcode_mutex and CPU
> * hotplug lock.
<--- I guess you can document that new parameter here.
> */
> -void microcode_check(void)
> +void microcode_check(struct cpuinfo_x86 *info)
...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Jan 03, 2023 at 10:02:09AM -0800, Ashok Raj wrote:
> Right now, microcode loading failures and successes print the same
> message "Reloading completed". This is misleading to users.
9bd681251b7c ("x86/microcode: Announce reload operation's completion")
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Jan 04, 2023 at 08:00:05PM +0100, Borislav Petkov wrote:
> On Tue, Jan 03, 2023 at 10:02:09AM -0800, Ashok Raj wrote:
> > Right now, microcode loading failures and successes print the same
> > message "Reloading completed". This is misleading to users.
>
> 9bd681251b7c ("x86/microcode: Announce reload operation's completion")
Thanks.. yes I can add when I resent the series.
Cheers,
Ashok
On Fri, Jan 06, 2023 at 11:42:32AM -0800, Ashok Raj wrote:
> > 9bd681251b7c ("x86/microcode: Announce reload operation's completion")
>
> Thanks.. yes I can add when I resent the series.
No, you should read the commit message:
"issue a single line to dmesg after the reload ... to let the user know that a
reload has at least been attempted."
and drop your patch.
We issue that line unconditionally to give feedback to the user that the attempt
was actually tried. Otherwise, you don't get any feedback and you wonder whether
it is doing anything.
The prev and next revision:
"microcode revision: 0x%x -> 0x%x\n",
will tell you whether something got loaded or not.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Fri, Jan 06, 2023 at 08:54:30PM +0100, Borislav Petkov wrote:
> On Fri, Jan 06, 2023 at 11:42:32AM -0800, Ashok Raj wrote:
> > > 9bd681251b7c ("x86/microcode: Announce reload operation's completion")
> >
> > Thanks.. yes I can add when I resent the series.
>
> No, you should read the commit message:
>
> "issue a single line to dmesg after the reload ... to let the user know that a
> reload has at least been attempted."
>
> and drop your patch.
>
> We issue that line unconditionally to give feedback to the user that the attempt
> was actually tried. Otherwise, you don't get any feedback and you wonder whether
> it is doing anything.
>
> The prev and next revision:
>
> "microcode revision: 0x%x -> 0x%x\n",
>
> will tell you whether something got loaded or not.
Yes, that makes sense, Do you think we can add a note that the loading
failed? since the old -> new, new is coming from new microcode rev.
Reload failed/completed ?
On Fri, Jan 06, 2023 at 12:29:00PM -0800, Ashok Raj wrote:
> Yes, that makes sense, Do you think we can add a note that the loading
> failed? since the old -> new, new is coming from new microcode rev.
It has failed when
old == new.
I.e.,
"microcode revision: 0x1a -> 0x1a"
when the current revision on the CPU is 0x1a.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
> We issue that line unconditionally to give feedback to the user that the attempt
> was actually tried. Otherwise, you don't get any feedback and you wonder whether
> it is doing anything.
>
> The prev and next revision:
>
> "microcode revision: 0x%x -> 0x%x\n",
>
> will tell you whether something got loaded or not.
Seems like a very subtle indication of a possibly important failure.
E.g. There is some security update with new microcode to mitigate.
User downloads the new microcode. Runs:
# echo 1 > /sys/devices/system/cpu/microcode/reload
[This fails for some reason]
Looks at console
# dmesg | tail -1
microcode revision: 0x40001a -> 0x4001a
User doesn't notice that the version didn't change and thinks
that all is well.
Is there an earlier message that says something like this?
microcode: initiating update to version 0x5003f
-Tony
On Fri, Jan 06, 2023 at 09:35:41PM +0000, Luck, Tony wrote:
> # dmesg | tail -1
> microcode revision: 0x40001a -> 0x4001a
>
> User doesn't notice that the version didn't change and thinks
> that all is well.
The version did change. One '0' less.
Users don't read even if you put in there:
"Corrected error, no action required."
User still complains about maybe her hardware going bad and "should I replace my
CPU" yadda yadda...
But whatever, since both of you think this is sooo important, then pls do this:
Success: "Reload completed, microcode revision: 0x%x -> 0x%x\n"
Failure: "Reload failed, current microcode revision: 0x%x\n"
Or something along those lines.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Fri, Jan 06, 2023 at 09:45:24PM +0100, Borislav Petkov wrote:
> On Fri, Jan 06, 2023 at 12:29:00PM -0800, Ashok Raj wrote:
> > Yes, that makes sense, Do you think we can add a note that the loading
> > failed? since the old -> new, new is coming from new microcode rev.
>
> It has failed when
>
> old == new.
>
> I.e.,
>
> "microcode revision: 0x1a -> 0x1a"
>
> when the current revision on the CPU is 0x1a.
That's fine too. I'll drop the patch.
* Borislav Petkov <[email protected]> wrote:
> On Fri, Jan 06, 2023 at 12:29:00PM -0800, Ashok Raj wrote:
> > Yes, that makes sense, Do you think we can add a note that the loading
> > failed? since the old -> new, new is coming from new microcode rev.
>
> It has failed when
>
> old == new.
>
> I.e.,
>
> "microcode revision: 0x1a -> 0x1a"
>
> when the current revision on the CPU is 0x1a.
So wouldn't it make sense to also display the fact that the microcode
loading failed?
Seeing '0x1a -> 0x1a' one might naively assume from the wording alone that
it got "reloaded" or somehow reset, or that there's some sub-revision
update that isn't visible in the revision version - when in fact nothing
happened, right?
The kernel usually tries to tell users unambigiously when some requested
operation didn't succeed - not just hint at it somewhat
passive-aggressively.
Thanks,
Ingo