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 <tglx@linutronix.de>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Link: https://lore.kernel.org/lkml/874judpqqd.ffs@tglx/
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: x86 <x86@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
---
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.1On 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
> 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 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
* Borislav Petkov <bp@alien8.de> 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
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.
© 2016 - 2025 Red Hat, Inc.