x86/mce fixes

arch/x86/kernel/cpu/mcheck/mce.c +3 -2

@@ -1287,12 +1292,17 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 			no_way_out = worst >= MCE_PANIC_SEVERITY;
 	} else {
 		/*
-		 * Local MCE skipped calling mce_reign()
-		 * If we found a fatal error, we need to panic here.
+		 * If there was a fatal machine check we should have
+		 * already called mce_panic earlier in this function.
+		 * Since we re-read the banks, we might have found

arch/x86/kernel/cpu/mcheck/mce.c +5 -2

@@ -1287,12 +1292,17 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 			no_way_out = worst >= MCE_PANIC_SEVERITY;
 	} else {
 		/*
-		 * Local MCE skipped calling mce_reign()
-		 * If we found a fatal error, we need to panic here.
+		 * If there was a fatal machine check we should have
+		 * already called mce_panic earlier in this function.
+		 * Since we re-read the banks, we might have found
+		 * something new. Check again to see if we found a
+		 * fatal error. We call "mce_severity()" again to

It is still assigning.

I'll simply do:

      if (worst >= MCE_PANIC_SEVERITY && mca_cfg.tolerant < 3) {
              mce_severity(&m, cfg->tolerant, &msg, true);
              mce_panic("Local fatal machine check!", &m, msg);
      }

when committing.

Btw, I did some cleaning up ontop. Please have a look and let me know if it is too ugly to live... :)

https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=rc7%2b0-ras

Ah. That would be because I forgot to "git add" before "git commit --amend" :-(

I had put:

		(void) mce_severity(&m, cfg->tolerant, &msg, true);

but either works.

Will look. I hope you didn't do too much to make the stable backport more difficult.

Nah, the cleanups will all go ontop. This is just a dirty branch to show my intention but yours go first and then the cleanup.

On Tue, May 29, 2018 at 07:53:14PM +0200, Borislav Petkov wrote:

P.S. What happened to my "part 3/3" (updating the Skylake quirk) ... does that belong in somebody else's tree?

Couple of thoughts:

In "x86/mce: Carve out bank scanning code" you drop the extra call to mce_severity() that I just added:

In "x86/mce: Exit properly when no banks to poll" you leap right to the end. I'm wondering whether this can ever happen? I mean, if there are no machine check banks, then how did we get a machine check?

Both the original, and your new code, skip the:

	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0);

which seems bad. That leaves MCG_STATUS.MCIP set ... so a second machine check would just reset the machine.

P.S. What happened to my "part 3/3" (updating the Skylake quirk) ... does that belong in somebody else's tree?

Quoted 4 lines

In "x86/mce: Exit properly when no banks to poll" you leap right to the end. I'm wondering whether this can ever happen? I mean, if there are no machine check banks, then how did we get a machine check?

Right, so this looks like some remnant from old times, lemme do some archeology...

/me goes and dusts off the full history linux repo...

I found this:

commit 7dd1e1d805d15ca63d05badf40026629ba75cbc8

Author: Andi Kleen <[email protected]>
Date:   Tue Feb 24 17:58:41 2004 -0800

[PATCH] New machine check handler for x86-64

and there's no mention why the !banks check is there.

I'm wondering if we should simply remove it. I mean, as you say, if there are no MCA banks, we won't be running in here in the first place...

Quoted 2 lines

Both the original, and your new code, skip the:

Quoted 4 lines

	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0);

which seems bad. That leaves MCG_STATUS.MCIP set ... so a second machine check would just reset the machine.

That's a good point. It goes away as an issue if we simply drop the check.

Quoted 11 lines · Tony Luck

arch/x86/kernel/cpu/mcheck/mce.c +5 -2

@@ -1287,12 +1292,17 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 			no_way_out = worst >= MCE_PANIC_SEVERITY;
 	} else {
 		/*
-		 * Local MCE skipped calling mce_reign()
-		 * If we found a fatal error, we need to panic here.
+		 * If there was a fatal machine check we should have
+		 * already called mce_panic earlier in this function.
+		 * Since we re-read the banks, we might have found
+		 * something new. Check again to see if we found a
+		 * fatal error. We call "mce_severity()" again to

arch/x86/kernel/cpu/mcheck/mce.c void do_machine_check(struct pt_regs *regs, long error_code) · line 1304

Quoted 8 lines

arch/x86/kernel/cpu/mcheck/mce.c +4 -3

+		 * make sure we have the right "msg".
 		 */
-		 if (worst >= MCE_PANIC_SEVERITY && mca_cfg.tolerant < 3)
-			mce_panic("Machine check from unknown source",
-				NULL, NULL);
+		if (worst >= MCE_PANIC_SEVERITY && mca_cfg.tolerant < 3) {
+			severity = mce_severity(&m, cfg->tolerant, &msg, true);
+			mce_panic("Local fatal machine check!", &m, msg);

Haha, this would still make you look at the code to remember was it "fatal local" or "local fatal" the second one. Yeah, there's the "!" but still.

How about:

"Fatal local machine check after banks scan"

or so.

Btw, the code in do_machine_check() has become one helluva spaghetti mess. It could use some clean up a bit... :)

Signature · 5 lines

Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

arch/x86/kernel/cpu/mcheck/mce.c +2 -2

@@ -1287,12 +1292,17 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 			no_way_out = worst >= MCE_PANIC_SEVERITY;
 	} else {
 		/*
-		 * Local MCE skipped calling mce_reign()
-		 * If we found a fatal error, we need to panic here.
+		 * If there was a fatal machine check we should have
+		 * already called mce_panic earlier in this function.

arch/x86/kernel/cpu/mcheck/mce.c void do_machine_check(struct pt_regs *regs, long error_code) · line 1303

Quoted 8 lines

arch/x86/kernel/cpu/mcheck/mce.c +4 -3

+		 * fatal error. We call "mce_severity()" again to
+		 * make sure we have the right "msg".
 		 */
-		 if (worst >= MCE_PANIC_SEVERITY && mca_cfg.tolerant < 3)
-			mce_panic("Machine check from unknown source",
-				NULL, NULL);
+		if (worst >= MCE_PANIC_SEVERITY && mca_cfg.tolerant < 3) {
+			severity = mce_severity(&m, cfg->tolerant, &msg, true);

Looking at this more while cleaning the whole thing up, that severity doesn't get read anywhere past this line, AFAICT...

Currently we just check the "CAPID0" register to see whether the CPU can recover from machine checks.

But there are also some special SKUs which do not have all advanced RAS features, but do enable machine check recovery for use with NVDIMMs.

Add a check for any of bits {8:5} in the "CAPID5" register (each reports some NVDIMM mode available, if any of them are set, then the system supports memory machine check recovery).

Cc [email protected] # 4.9

Signed-off-by Tony Luck <[email protected]>

---

 arch/x86/kernel/quirks.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

arch/x86/kernel/quirks.c +9 -2

index 697a4ce04308..736348ead421 100644
@@ -645,12 +645,19 @@ static void quirk_intel_brickland_xeon_ras_cap(struct pci_dev *pdev)
 /* Skylake */
 static void quirk_intel_purley_xeon_ras_cap(struct pci_dev *pdev)
 {
-	u32 capid0;
+	u32 capid0, capid5;
 
 	pci_read_config_dword(pdev, 0x84, &capid0);
+	pci_read_config_dword(pdev, 0x98, &capid5);
 
-	if ((capid0 & 0xc0) == 0xc0)
+	/*
+	 * CAPID0{7:6} indicate whether this is an advanced RAS SKU
+	 * CAPID5{8:5} indicate that various NVDIMM usage modes are
+	 * enabled, so memory machine check recovery is also enabled.
+	 */
+	if ((capid0 & 0xc0) == 0xc0 || (capid5 & 0x1e0))
 		static_branch_inc(&mcsafe_key);
+
 }
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x0ec3, quirk_intel_brickland_xeon_ras_cap);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2fc0, quirk_intel_brickland_xeon_ras_cap);

Signature · 1 line

2.17.0

Quoted 6 lines · Tony Luck

Currently we just check the "CAPID0" register to see whether the CPU can recover from machine checks.

But there are also some special SKUs which do not have all advanced RAS features, but do enable machine check recovery for use with NVDIMMs.

Quoted 7 lines

Add a check for any of bits {8:5} in the "CAPID5" register (each reports some NVDIMM mode available, if any of them are set, then the system supports memory machine check recovery).

Cc [email protected] # 4.9

Signed-off-by Tony Luck <[email protected]>

---

Has this stalled somewhere? I'd like to see this one go into the 4.18 merge because it unbreaks some real hardware.

Parts 1 & 2 are nice-to-have, but they just make for better error messages so aren't as critical.

I'm making an effort to get all persistent memory error handling holes covered this cycle, so I think it makes sense for this to go through the nvdimm tree. This looks sufficiently non-controversial that I could justify sending it to Linus along with the other pmem updates.

Quoted 4 lines · Dan Williams

I'm making an effort to get all persistent memory error handling holes covered this cycle, so I think it makes sense for this to go through the nvdimm tree. This looks sufficiently non-controversial that I could justify sending it to Linus along with the other pmem updates.

tglx just took 1 and 3, 2/3 had a minor issue but the merge window happened so I'll send it later. It is nice to have anyway.

tglx just took 1 and 3, 2/3 had a minor issue but the merge window happened so I'll send it later. It is nice to have anyway.

Quoted 2 lines · Borislav Petkov

tglx just took 1 and 3, 2/3 had a minor issue but the merge window happened so I'll send it later. It is nice to have anyway.

Did you fix up part 2/3?

I see 1 & 3 were staged by Thomas in TIP ras/urgent and ras-urgent-for-linus but haven't gone into Linus' tree yet.

Quoted 5 lines · Tony

Quoted 2 lines · Borislav Petkov

tglx just took 1 and 3, 2/3 had a minor issue but the merge window happened so I'll send it later. It is nice to have anyway.

Did you fix up part 2/3?

You said "Parts 1 & 2 are nice-to-have" and we have merge window now. So what exactly do you mean?

Quoted 2 lines

I see 1 & 3 were staged by Thomas in TIP ras/urgent and ras-urgent-for-linus but haven't gone into Linus' tree yet.

Merge window ain't over yet :)

I was just worried that Thomas is holding off asking Linus to pull because he's waiting for part 2/3, while you might be planning to hold onto part 2/3 for the next merge (and add the other cleanups you RFC'd).

I'm OK either way on part 2/3 (it just makes for better error messages, it doesn't fix any bugs).

Quoted 18 lines · Tony

Quoted 6 lines · Tony Luck

Currently we just check the "CAPID0" register to see whether the CPU can recover from machine checks.

But there are also some special SKUs which do not have all advanced RAS features, but do enable machine check recovery for use with NVDIMMs.

Quoted 7 lines

Add a check for any of bits {8:5} in the "CAPID5" register (each reports some NVDIMM mode available, if any of them are set, then the system supports memory machine check recovery).

Cc [email protected] # 4.9

Signed-off-by Tony Luck <[email protected]>

---

Has this stalled somewhere? I'd like to see this one go into the 4.18 merge because it unbreaks some real hardware.

Quoted 2 lines

Parts 1 & 2 are nice-to-have, but they just make for better error messages so aren't as critical.

I'm making an effort to get all persistent memory error handling holes covered this cycle, so I think it makes sense for this to go through the nvdimm tree. This looks sufficiently non-controversial that I could justify sending it to Linus along with the other pmem updates.