Re: [PATCH 01/12] x86/mce: Fix hw MCE injection feature detection

[Dave Hansen]

Isaku, when you report a bug, it would be great to include the folks who
authored and worked on the original patch that introduced the bug.  I've
gone ahead and done that for you here.

On 10/10/23 01:35, isaku.yamahata@xxxxxxxxx wrote:
> From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> 
> When initializing x86 MCE injection framework, it checks if hardware mce
> injection is available or not.  When it's not available on AMD, set the
> boolean variable to false to not use it.  The variable is on by default and
> the feature is AMD specific based on the code.
> 
> Because the variable is default on, it is true on Intel platform (probably
> on other non-AMD x86 platform).  It results in unchecked msr access of
> MSR_K7_HWCR=0xc0010015 when injecting MCE on Intel platform.  (Probably on
> other x86 platform.)
> 
> Make the variable of by default, and set the variable on when the hardware
> feature is usable.

Gah, I'm finding that changelog impenetrable.  Here's what's missing:

  * The entirety of check_hw_inj_possible() is for AMD hardware:
    X86_FEATURE_SMCA, the MSRs, hw_injection_possible, everything.
  * Only AMD systems with SMCA support hardware error injection
    (anything other than "echo sw > /sys/kernel/debug/mce-inject/flags")
  * That AMD-only restriction is enforced by 'hw_injection_possible'
  * 'hw_injection_possible' is true by default and only set to false in
    check_hw_inj_possible() ... the AMD-only code

The end result is that everyone except SMCA-enabled AMD systems (Intel
included) leaves hw_injection_possible=true.  They are free to try and
inject hardware errors.  If they do, they'll get errors when writing to
the MSRs.

To fix this, make disable hw_injection_possible by default.  Only enable
it on SMCA hardware that actually succeeds in ... whatever:

                wrmsrl_safe(mca_msr_reg(bank, MCA_STATUS), status);
                rdmsrl_safe(mca_msr_reg(bank, MCA_STATUS), &status);

is doing.

... and don't do it at the top of the function.  Why bother setting it
to true only to disable it a moment later?

Do something like the following instead.diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 4d8d4bcf915d..01ee886d8540 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -33,7 +33,7 @@
 
 #include "internal.h"
 
-static bool hw_injection_possible = true;
+static bool hw_injection_possible;
 
 /*
  * Collect all the MCi_XXX settings
@@ -748,9 +748,10 @@ static void check_hw_inj_possible(void)
 		rdmsrl_safe(mca_msr_reg(bank, MCA_STATUS), &status);
 
 		if (!status) {
-			hw_injection_possible = false;
 			pr_warn("Platform does not allow *hardware* error injection."
 				"Try using APEI EINJ instead.\n");
+		} else {
+			hw_injection_possible = true;
 		}
 
 		toggle_hw_mce_inject(cpu, false);