[PATCH] x86/mce: add support SRAO reported via CMC check

[PATCH] x86/mce: add support SRAO reported via CMC check

[Xie XiuQi]

In Intel SDM Volume 3B (253669-063US, July 2017), SRAO could be
reported via CMC:

  In cases when SRAO is signaled via CMCI the error signature is
  indicated via UC=1, PCC=0, S=0.

So we add those known AO MCACODs check in mce_severity().

Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
Tested-by: Chen Wei <chenwei68@huawei.com>
---
 arch/x86/kernel/cpu/mcheck/mce-severity.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/kernel/cpu/mcheck/mce-severity.c b/arch/x86/kernel/cpu/mcheck/mce-severity.c
index 4ca632a..48f239a 100644
--- a/arch/x86/kernel/cpu/mcheck/mce-severity.c
+++ b/arch/x86/kernel/cpu/mcheck/mce-severity.c
@@ -101,6 +101,16 @@
 		NOSER, BITCLR(MCI_STATUS_UC)
 		),
 
+	/* known AO MCACODs reported via CMC: */
+	MCESEV(
+		AO, "Action optional: memory scrubbing error",
+		SER, MASK(MCI_UC_SAR|MCACOD_SCRUBMSK, MCI_STATUS_UC|MCACOD_SCRUB)
+		),
+	MCESEV(
+		AO, "Action optional: last level cache writeback error",
+		SER, MASK(MCI_UC_SAR|MCACOD, MCI_STATUS_UC|MCACOD_L3WB)
+		),
+
 	/* ignore OVER for UCNA */
 	MCESEV(
 		UCNA, "Uncorrected no action required",
-- 
1.8.3.1

Re: [PATCH] x86/mce: add support SRAO reported via CMC check

[Luck, Tony]

On Tue, Nov 14, 2017 at 01:55:11PM +0800, Xie XiuQi wrote:
> +	/* known AO MCACODs reported via CMC: */
> +	MCESEV(
> +		AO, "Action optional: memory scrubbing error",
> +		SER, MASK(MCI_UC_SAR|MCACOD_SCRUBMSK, MCI_STATUS_UC|MCACOD_SCRUB)

I think you should check for OVER=0 (as the existing AO cases do).
If there was a patrol scrub reported by CMCI, and then another UC
error, we can't safely treat this as an AO ... because we have no
idea what the second UC error was.

-Tony

Re: [PATCH] x86/mce: add support SRAO reported via CMC check

[Xie XiuQi]

Hi Tony,

On 2017/11/15 2:51, Luck, Tony wrote:
> On Tue, Nov 14, 2017 at 01:55:11PM +0800, Xie XiuQi wrote:
>> +	/* known AO MCACODs reported via CMC: */
>> +	MCESEV(
>> +		AO, "Action optional: memory scrubbing error",
>> +		SER, MASK(MCI_UC_SAR|MCACOD_SCRUBMSK, MCI_STATUS_UC|MCACOD_SCRUB)
> 
> I think you should check for OVER=0 (as the existing AO cases do).
> If there was a patrol scrub reported by CMCI, and then another UC
> error, we can't safely treat this as an AO ... because we have no
> idea what the second UC error was.

OK, I'll add check for OVER=0 in v2.

-- 
Thanks,
Xie XiuQi

> 
> -Tony
> 
> .
> 

RE: [PATCH] x86/mce: add support SRAO reported via CMC check

[Luck, Tony]

> OK, I'll add check for OVER=0 in v2.

Thinking a bit more on this ... do you just need to remove the check
for S=1 from the existing "AO" entries?  Or do we need the CMCI
match entry early in the table?

-Tony

Re: [PATCH] x86/mce: add support SRAO reported via CMC check

[Xie XiuQi]

Hi Tony,

On 2017/11/15 10:13, Luck, Tony wrote:
>> OK, I'll add check for OVER=0 in v2.
> 
> Thinking a bit more on this ... do you just need to remove the check
> for S=1 from the existing "AO" entries?  Or do we need the CMCI
> match entry early in the table?

Yes, we could just remove the check for S=1, and place "AO" entries before
"UCNA" entries.

Is that right?

> 
> -Tony
> --
> To unsubscribe from this list: send the line "unsubscribe linux-edac" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

-- 
Thanks,
Xie XiuQi

RE: [PATCH] x86/mce: add support SRAO reported via CMC check

[Luck, Tony]

> Yes, we could just remove the check for S=1, and place "AO" entries before
> "UCNA" entries.
>
> Is that right?

Probably. But you should give more thought than I just did to the order to
make sure that we don't give the wrong classification to something because
it now matches something early in the table when it ought to wait and match
a later entry.

This code is subtle :-(

-Tony

Re: [PATCH] x86/mce: add support SRAO reported via CMC check

[Borislav Petkov]

On Wed, Nov 15, 2017 at 02:44:07AM +0000, Luck, Tony wrote:
> This code is subtle :-(

I'm glad that we agree on this! :-)

Anyone wanting to rewrite it yet?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86/mce: add support SRAO reported via CMC check

[Xie XiuQi]

Hi Borislav, Tony,

On 2017/11/15 18:33, Borislav Petkov wrote:
> On Wed, Nov 15, 2017 at 02:44:07AM +0000, Luck, Tony wrote:
>> This code is subtle :-(
> 
> I'm glad that we agree on this! :-)
> 
> Anyone wanting to rewrite it yet?
> 

In Intel SDM Volume 3B (253669-063US, July 2017), SRAO could be
reported either via MCE or CMC:

  In cases when SRAO is signaled via CMCI the error signature is
  indicated via UC=1, PCC=0, S=0.

  Type(*1)	UC	EN	PCC	S	AR	Signaling
  ---------------------------------------------------------------
  UC		1	1	1	x	x	MCE
  SRAR		1	1	0	1	1	MCE
  SRAO		1	x(*2)	0	x(*2)	0	MCE/CMC
  UCNA		1	x	0	0	0	CMC
  CE		0	x	x	x	x	CMC

  NOTES:
  1. SRAR, SRAO and UCNA errors are supported by the processor only
     when IA32_MCG_CAP[24] (MCG_SER_P) is set.
  2. EN=1, S=1 when signaled via MCE. EN=x, S=0 when signaled via CMC.

And there is a description in 15.6.2 UCR Error Reporting and Logging, for bit S:

  S (Signaling) flag, bit 56 - Indicates (when set) that a machine check
  exception was generated for the UCR error reported in this MC bank...
  When the S flag in the IA32_MCi_STATUS register is clear, this UCR error
  was not signaled via a machine check exception and instead was reported
  as a corrected machine check (CMC).

As the description in SDM, I think this flag could be used to determine whether
MCE or CMC was triggered. So we could merge this two case in one and just
remove the S=0 check for SRAO.

How about this patch?

>From a06b2a781a86e3b1fe241591b53f7a6d33d63331 Mon Sep 17 00:00:00 2001
From: Xie XiuQi <xiexiuqi@huawei.com>
Date: Tue, 14 Nov 2017 10:13:22 +0800
Subject: [PATCH] x86/mce: add support SRAO reported via CMC check

In Intel SDM Volume 3B (253669-063US, July 2017), SRAO could be
reported either via MCE or CMC:

  In cases when SRAO is signaled via CMCI the error signature is
  indicated via UC=1, PCC=0, S=0.

  Type(*1)	UC	EN	PCC	S	AR	Signaling
  ---------------------------------------------------------------
  UC		1	1	1	x	x	MCE
  SRAR		1	1	0	1	1	MCE
  SRAO		1	x(*2)	0	x(*2)	0	MCE/CMC
  UCNA		1	x	0	0	0	CMC
  CE		0	x	x	x	x	CMC

  NOTES:
  1. SRAR, SRAO and UCNA errors are supported by the processor only
     when IA32_MCG_CAP[24] (MCG_SER_P) is set.
  2. EN=1, S=1 when signaled via MCE. EN=x, S=0 when signaled via CMC.

And there is a description in 15.6.2 UCR Error Reporting and Logging, for bit S:

  S (Signaling) flag, bit 56 - Indicates (when set) that a machine check
  exception was generated for the UCR error reported in this MC bank...
  When the S flag in the IA32_MCi_STATUS register is clear, this UCR error
  was not signaled via a machine check exception and instead was reported
  as a corrected machine check (CMC).

So we could merge this two case, and just remove the S=0 check for SRAO
in mce_severity().

Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
Tested-by: Chen Wei <chenwei68@huawei.com>
---
 arch/x86/kernel/cpu/mcheck/mce-severity.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce-severity.c b/arch/x86/kernel/cpu/mcheck/mce-severity.c
index 4ca632a..5bbd06f 100644
--- a/arch/x86/kernel/cpu/mcheck/mce-severity.c
+++ b/arch/x86/kernel/cpu/mcheck/mce-severity.c
@@ -59,6 +59,7 @@
 #define  MCGMASK(x, y)	.mcgmask = x, .mcgres = y
 #define  MASK(x, y)	.mask = x, .result = y
 #define MCI_UC_S (MCI_STATUS_UC|MCI_STATUS_S)
+#define MCI_UC_AR (MCI_STATUS_UC|MCI_STATUS_AR)
 #define MCI_UC_SAR (MCI_STATUS_UC|MCI_STATUS_S|MCI_STATUS_AR)
 #define	MCI_ADDR (MCI_STATUS_ADDRV|MCI_STATUS_MISCV)

@@ -101,6 +102,22 @@
 		NOSER, BITCLR(MCI_STATUS_UC)
 		),

+	/*
+	 * known AO MCACODs reported via MCE or CMC:
+	 *
+	 * SRAO could be signaled either via a machine check exception or
+	 * CMCI with the corresponding bit S 1 or 0. So we don't need to
+	 * check bit S for SRAO.
+	 */
+	MCESEV(
+		AO, "Action optional: memory scrubbing error",
+		SER, MASK(MCI_STATUS_OVER|MCI_UC_AR|MCACOD_SCRUBMSK, MCI_STATUS_UC|MCACOD_SCRUB)
+		),
+	MCESEV(
+		AO, "Action optional: last level cache writeback error",
+		SER, MASK(MCI_STATUS_OVER|MCI_UC_AR|MCACOD, MCI_STATUS_UC|MCACOD_L3WB)
+		),
+
 	/* ignore OVER for UCNA */
 	MCESEV(
 		UCNA, "Uncorrected no action required",
@@ -149,15 +166,6 @@
 		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_UC_SAR)
 		),

-	/* known AO MCACODs: */
-	MCESEV(
-		AO, "Action optional: memory scrubbing error",
-		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCACOD_SCRUBMSK, MCI_UC_S|MCACOD_SCRUB)
-		),
-	MCESEV(
-		AO, "Action optional: last level cache writeback error",
-		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCACOD, MCI_UC_S|MCACOD_L3WB)
-		),
 	MCESEV(
 		SOME, "Action optional: unknown MCACOD",
 		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_UC_S)
-- 
1.8.3.1

Re: [PATCH] x86/mce: add support SRAO reported via CMC check

[Luck, Tony]

On Thu, Nov 16, 2017 at 11:00:40AM +0800, Xie XiuQi wrote:
> How about this patch?

> +	/*
> +	 * known AO MCACODs reported via MCE or CMC:
> +	 *
> +	 * SRAO could be signaled either via a machine check exception or
> +	 * CMCI with the corresponding bit S 1 or 0. So we don't need to
> +	 * check bit S for SRAO.
> +	 */
> +	MCESEV(
> +		AO, "Action optional: memory scrubbing error",
> +		SER, MASK(MCI_STATUS_OVER|MCI_UC_AR|MCACOD_SCRUBMSK, MCI_STATUS_UC|MCACOD_SCRUB)
> +		),
> +	MCESEV(
> +		AO, "Action optional: last level cache writeback error",
> +		SER, MASK(MCI_STATUS_OVER|MCI_UC_AR|MCACOD, MCI_STATUS_UC|MCACOD_L3WB)
> +		),
> +

Yes. This looks good.

Reviewed-by: Tony Luck <tony.luck@intel.com>

-Tony