diff mbox series

[v3,3/6] x86/microcode: Display revisions only when update is successful

Message ID 20230103180212.333496-4-ashok.raj@intel.com
State New
Headers
Series Some fixes and cleanups for microcode |

Commit Message

Ashok Raj Jan. 3, 2023, 6:02 p.m. UTC 
  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(-)

Comments

Borislav Petkov Jan. 4, 2023, 7 p.m. UTC | #1 
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")
Ashok Raj Jan. 6, 2023, 7:42 p.m. UTC | #2 
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
Borislav Petkov Jan. 6, 2023, 7:54 p.m. UTC | #3 
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.
Ashok Raj Jan. 6, 2023, 8:29 p.m. UTC | #4 
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 ?
Borislav Petkov Jan. 6, 2023, 8:45 p.m. UTC | #5 
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.
Ashok Raj Jan. 6, 2023, 9:20 p.m. UTC | #6 
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.
Luck, Tony Jan. 6, 2023, 9:35 p.m. UTC | #7 
> 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
Borislav Petkov Jan. 6, 2023, 9:52 p.m. UTC | #8 
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.
Ingo Molnar Jan. 7, 2023, 9:36 a.m. UTC | #9 
* 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
diff mbox series

Patch

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;
 }