[PATCH] x86/MCE: Get microcode revision from cpu_data instead of boot_cpu_data

[PATCH] x86/MCE: Get microcode revision from cpu_data instead of boot_cpu_data

[Filippo Sironi]

Commit fa94d0c6e0f3 ("x86/MCE: Save microcode revision in machine check
records") extended MCE entries to report the microcode revision taken
from boot_cpu_data. Unfortunately, boot_cpu_data isn't updated on late
microcode loading, thus making MCE entries slightly incorrect.  Use
cpu_data instead, which is updated on late microcode loading. This also
fixes the corner case in which the microcode revision isn't coherent
across CPUs (which may happen on late microcode loading failure).

Signed-off-by: Filippo Sironi <sironi@amazon.de>
---
 arch/x86/kernel/cpu/mce/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 7b397370b4d6..e1b033298db0 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -127,7 +127,7 @@ void mce_setup(struct mce *m)
 	m->apicid = cpu_data(m->extcpu).topo.initial_apicid;
 	m->mcgcap = __rdmsr(MSR_IA32_MCG_CAP);
 	m->ppin = cpu_data(m->extcpu).ppin;
-	m->microcode = boot_cpu_data.microcode;
+	m->microcode = cpu_data(m->extcpu).microcode;
 }
 
 DEFINE_PER_CPU(struct mce, injectm);
-- 
2.33.0




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH] x86/MCE: Get microcode revision from cpu_data instead of boot_cpu_data

[Tony Luck]

On Fri, Dec 01, 2023 at 11:23:27AM +0000, Filippo Sironi wrote:
> Commit fa94d0c6e0f3 ("x86/MCE: Save microcode revision in machine check
> records") extended MCE entries to report the microcode revision taken
> from boot_cpu_data. Unfortunately, boot_cpu_data isn't updated on late
> microcode loading, thus making MCE entries slightly incorrect.  Use

This code in intel.c:apply_microcode_late() looks like it tries to update
boot_cpu_data:

466         cpu_data(cpu).microcode  = uci->cpu_sig.rev;
467         if (!cpu)
468                 boot_cpu_data.microcode = uci->cpu_sig.rev;

Is that not working for some reason?

> cpu_data instead, which is updated on late microcode loading. This also
> fixes the corner case in which the microcode revision isn't coherent
> across CPUs (which may happen on late microcode loading failure).

But this does seem a worthwhile change to help diagnose things if late
load is somehow only applied to some subset of CPUs.
> 
> Signed-off-by: Filippo Sironi <sironi@amazon.de>
> ---
>  arch/x86/kernel/cpu/mce/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 7b397370b4d6..e1b033298db0 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -127,7 +127,7 @@ void mce_setup(struct mce *m)
>  	m->apicid = cpu_data(m->extcpu).topo.initial_apicid;
>  	m->mcgcap = __rdmsr(MSR_IA32_MCG_CAP);
>  	m->ppin = cpu_data(m->extcpu).ppin;
> -	m->microcode = boot_cpu_data.microcode;
> +	m->microcode = cpu_data(m->extcpu).microcode;
>  }
>  
>  DEFINE_PER_CPU(struct mce, injectm);
> -- 
> 2.33.0

-Tony

Re: [PATCH] x86/MCE: Get microcode revision from cpu_data instead of boot_cpu_data

[Borislav Petkov]

On Fri, Dec 01, 2023 at 10:58:55AM -0800, Tony Luck wrote:
> But this does seem a worthwhile change to help diagnose things if late
> load is somehow only applied to some subset of CPUs.

We already do that, see load_late_stop_cpus().

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/MCE: Get microcode revision from cpu_data instead of boot_cpu_data

[Sironi, Filippo]

> On Fri, Dec 01, 2023 at 11:23:27AM +0000, Filippo Sironi wrote:
>> Commit fa94d0c6e0f3 ("x86/MCE: Save microcode revision in machine check
>> records") extended MCE entries to report the microcode revision taken
>> from boot_cpu_data. Unfortunately, boot_cpu_data isn't updated on late
>> microcode loading, thus making MCE entries slightly incorrect. Use
>
> This code in intel.c:apply_microcode_late() looks like it tries to update
> boot_cpu_data:
>
>
> 466 cpu_data(cpu).microcode = uci->cpu_sig.rev;
> 467 if (!cpu)
> 468 boot_cpu_data.microcode = uci->cpu_sig.rev;
>
>
> Is that not working for some reason?

It is... I had this change in our tree for a long long while and just
realized that the issue of boot_cpu_data not being updated was addressed
with commit 370a132bb222 ("x86/microcode: Make sure
boot_cpu_data.microcode is up-to-date").

>> cpu_data instead, which is updated on late microcode loading. This also
>> fixes the corner case in which the microcode revision isn't coherent
>> across CPUs (which may happen on late microcode loading failure).
>
>
> But this does seem a worthwhile change to help diagnose things if late
> load is somehow only applied to some subset of CPUs.

Yes, but, as Boris points out:

>> But this does seem a worthwhile change to help diagnose things if late
>> load is somehow only applied to some subset of CPUs.
> 
> We already do that, see load_late_stop_cpus().

Boris, I just took a quick look and I might be missing something. If cores
fail to load the microcode or timeout, we taint the kernel, print an error
message, and then bubble up an error to userspace via:

load_late_stop_cpus
load_late_locked
reload_store

Right?

We would take servers that fail out of production; however, for others it might
be interesting to have the correct information. The patch - with a reworked
commit message - might still be useful to a few. 

Filippo





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH] x86/MCE: Get microcode revision from cpu_data instead of boot_cpu_data

[Borislav Petkov]

On Fri, Dec 01, 2023 at 07:56:59PM +0000, Sironi, Filippo wrote:
> Boris, I just took a quick look and I might be missing something. If cores
> fail to load the microcode or timeout, we taint the kernel, print an error
> message, and then bubble up an error to userspace via:
> 
> load_late_stop_cpus
> load_late_locked
> reload_store
> 
> Right?

Yap.

> We would take servers that fail out of production;

And I'd like to hear about such issues. We added this failure checking
only recently because something might go wrong and we want to warn. But
it all updates fine here so kinda hard to test.

My expectation is that if microcode fails loading on a subset of
machines, the machine would more or less freeze. Depending, ofc, on what
the microcode is updating...

> however, for others it might be interesting to have the correct
> information. The patch - with a reworked commit message - might still
> be useful to a few.

https://lore.kernel.org/r/20231118193248.1296798-3-yazen.ghannam@amd.com

:)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/MCE: Get microcode revision from cpu_data instead of boot_cpu_data

[Sironi, Filippo]

> > Boris, I just took a quick look and I might be missing something. If cores
> > fail to load the microcode or timeout, we taint the kernel, print an error
> > message, and then bubble up an error to userspace via:
> >
> > load_late_stop_cpus
> > load_late_locked
> > reload_store
> >
> > Right?
> 
> Yap.
> 
> > We would take servers that fail out of production;
> 
> And I'd like to hear about such issues. We added this failure checking
> only recently because something might go wrong and we want to warn. But
> it all updates fine here so kinda hard to test.

In a very large fleet, let's say that we have a handful of DPMs when considering
the entire processor, which means that in terms of cores, the defect rate is
much much lower.

What we've seen in these cases is that early loading - through the BIOS, I
actually never tried via the hypervisor - is successful while late loading
consistently fails. When it fails, we've seen two cases: 1/ the core still
reports the old microcode version or 2/ the core reports a bogus microcode
version (0xfffffffe is quite common, at least on Intel).

> My expectation is that if microcode fails loading on a subset of
> machines, the machine would more or less freeze. Depending, ofc, on what
> the microcode is updating...

It's bi-modal. We've seen servers that move along till we take them out of
production as well as servers that fail with an MCE of some sort likely leading
to a CATERR/IERR.

> > however, for others it might be interesting to have the correct
> > information. The patch - with a reworked commit message - might still
> > be useful to a few.
> 
> 
> https://lore.kernel.org/r/20231118193248.1296798-3-yazen.ghannam@amd.com <mailto:20231118193248.1296798-3-yazen.ghannam@amd.com>
> 
> 
> :)

:looking:




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH] x86/MCE: Get microcode revision from cpu_data instead of boot_cpu_data

[Borislav Petkov]

On Fri, Dec 01, 2023 at 11:23:27AM +0000, Filippo Sironi wrote:
> Unfortunately, boot_cpu_data isn't updated on late microcode loading,

Looks good to me here:

[   50.881231] microcode: Unsafe microcode update: Microcode header does not specify a required min version
[   50.892577] microcode: Late microcode loading without minimal revision check.
[   50.900099] microcode: You should switch to early loading, if possible.
[   50.958919] microcode: load: updated on 6 primary CPUs with 0 siblings
[   50.965856] microcode: revision: 0xbe -> 0xf0

That comes from:

	pr_info("revision: 0x%x -> 0x%x\n", old_rev, boot_cpu_data.microcode);

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette