x86/MCE: Implement clearbank callback for AMD

PATCH In review Christoph Egger 2 participants 12 messages Oct 12, 2012 – Oct 25, 2012

lore

Share & tools

lore

Christoph Egger

author

2012-10-12 14:46 UTC #0 lore raw

PATCH 1/1 x86/MCE: Implement clearbank callback for AMD

Implement clearbank callbank for AMD.

Signed-off-by Christoph Egger <[email protected]>

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

xen/arch/x86/cpu/mcheck/amd_k8.c +26 -1

diff -r 6b73078a4403 xen/arch/x86/cpu/mcheck/amd_k8.c
@@ -72,7 +72,31 @@
 /* Machine Check Handler for AMD K8 family series */
 static void k8_machine_check(struct cpu_user_regs *regs, long error_code)
 {
-	mcheck_cmn_handler(regs, error_code, mca_allbanks, NULL);
+	mcheck_cmn_handler(regs, error_code, mca_allbanks,
+	    __get_cpu_var(mce_clear_banks));
+}
+
+static int k8_need_clearbank_scan(enum mca_source who, uint64_t status)
+{
+	switch (who) {
+	case MCA_MCE_SCAN:
+	case MCA_MCE_HANDLER:
+		break;
+	default:
+		return 1;
+	}
+
+	/* For fatal error, it shouldn't be cleared so that sticky bank
+	 * have chance to be handled after reboot by polling.
+	 */
+	if ( (status & MCi_STATUS_UC) && (status & MCi_STATUS_PCC) )
+		return 0;
+	/* Spurious need clear bank */
+	if ( !(status & MCi_STATUS_OVER)
+	    && (status & MCi_STATUS_UC) && !(status & MCi_STATUS_EN))
+		return 1;
+
+	return 1;
 }
 
 /* AMD K8 machine check */

@@ -85,6 +109,7 @@ enum mcheck_type amd_k8_mcheck_init(stru
 
 	mce_handler_init();
 	x86_mce_vector_register(k8_machine_check);
+	mce_need_clearbank_register(k8_need_clearbank_scan);
 
 	for (i = 0; i < nr_mce_banks; i++) {
 		if (quirkflag == MCEQUIRK_K8_GART && i == 4) {

xen/arch/x86/cpu/mcheck/mce.c +4 -0

diff -r 6b73078a4403 xen/arch/x86/cpu/mcheck/mce.c
@@ -35,6 +35,10 @@ bool_t is_mc_panic;
 unsigned int __read_mostly nr_mce_banks;
 unsigned int __read_mostly firstbank;
 
+DEFINE_PER_CPU(struct mca_banks *, poll_bankmask);
+DEFINE_PER_CPU(struct mca_banks *, no_cmci_banks);
+DEFINE_PER_CPU(struct mca_banks *, mce_clear_banks);
+
 static void intpose_init(void);
 static void mcinfo_clear(struct mc_info *);
 struct mca_banks *mca_allbanks;

xen/arch/x86/cpu/mcheck/mce.h +1 -0

diff -r 6b73078a4403 xen/arch/x86/cpu/mcheck/mce.h
@@ -122,6 +122,7 @@ struct mca_summary {
 
 DECLARE_PER_CPU(struct mca_banks *, poll_bankmask);
 DECLARE_PER_CPU(struct mca_banks *, no_cmci_banks);
+DECLARE_PER_CPU(struct mca_banks *, mce_clear_banks);
 
 extern bool_t cmci_support;
 extern bool_t is_mc_panic;

xen/arch/x86/cpu/mcheck/mce_intel.c +1 -3

diff -r 6b73078a4403 xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -21,9 +21,7 @@
 #include "vmce.h"
 #include "mcaction.h"
 
-DEFINE_PER_CPU(struct mca_banks *, mce_banks_owned);
-DEFINE_PER_CPU(struct mca_banks *, no_cmci_banks);
-DEFINE_PER_CPU(struct mca_banks *, mce_clear_banks);
+static DEFINE_PER_CPU(struct mca_banks *, mce_banks_owned);
 bool_t __read_mostly cmci_support = 0;
 static bool_t __read_mostly ser_support = 0;
 static bool_t __read_mostly mce_force_broadcast;

xen/arch/x86/cpu/mcheck/mctelem.h +2 -2

diff -r 6b73078a4403 xen/arch/x86/cpu/mcheck/mctelem.h
@@ -23,7 +23,7 @@
  * urgent uses, intended for use from machine check exception handlers,
  * and non-urgent uses intended for use from error pollers.
  * Associated with each logout entry of whatever class is a data area
- * sized per the single argument to mctelem_init.  mcelem_init should be
+ * sized per the single argument to mctelem_init.  mctelem_init should be
  * called from MCA init code before anybody has the chance to change the
  * machine check vector with mcheck_mca_logout or to use mcheck_mca_logout.
  *

@@ -45,7 +45,7 @@
  * which will return a cookie referencing the oldest (first committed)
  * entry of the requested class.  Access the associated data using
  * mctelem_dataptr and when finished use mctelem_consume_oldest_end - in the
- * begin .. end bracket you are guaranteed that the entry canot be freed
+ * begin .. end bracket you are guaranteed that the entry cannot be freed
  * even if it is ack'd elsewhere).  Once the ultimate consumer of the
  * telemetry has processed it to stable storage it should acknowledge
  * the telemetry quoting the cookie id, at which point we will free

xen/arch/x86/cpu/mcheck/non-fatal.c +0 -1

diff -r 6b73078a4403 xen/arch/x86/cpu/mcheck/non-fatal.c
@@ -23,7 +23,6 @@
 #include "mce.h"
 #include "vmce.h"
 
-DEFINE_PER_CPU(struct mca_banks *, poll_bankmask);
 static struct timer mce_timer;
 
 #define MCE_PERIOD MILLISECS(8000)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Christoph Egger

author

2012-10-19 13:11 UTC #1 lore raw

Ping?

Quoted 5 lines · Christoph Egger

Implement clearbank callbank for AMD.
Signed-off-by Christoph Egger <[email protected]>

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Jan Beulich

2012-10-19 14:59 UTC #2 lore raw

Quoted 2 lines

Quoted 1 line
Quoted 1 line · Christoph Egger
Ping?

I have this on my radar to review (and commit if no significant reason not to turns up), but I can't currently tell when I'll be able to get to it (some time not too early next week maybe).

Sorry, Jan

Quoted 21 lines · Christoph Egger

Quoted 5 lines

Implement clearbank callbank for AMD.
Signed-off-by Christoph Egger <[email protected]>

Signature · 12 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
[email protected] 
http://lists.xen.org/xen-devel

So what's the purpose of first conditionally returning 1, and then also doing so unconditionally? Do anticipate to insert code between the two parts within the very near future? Otherwise I'd drop the whole if() construct.

(No need to re-submit, just let me know.)

Jan

Christoph Egger

author

2012-10-25 08:18 UTC #4 lore raw

Quoted 20 lines · Jan Beulich

Quoted 20 lines

Quoted 1 line

Quoted 1 line · Christoph Egger

xen/arch/x86/cpu/mcheck/amd_k8.c +19 -0

+static int k8_need_clearbank_scan(enum mca_source who, uint64_t status)
+{
+	switch (who) {
+	case MCA_MCE_SCAN:
+	case MCA_MCE_HANDLER:
+		break;
+	default:
+		return 1;
+	}
+
+	/* For fatal error, it shouldn't be cleared so that sticky bank
+	 * have chance to be handled after reboot by polling.
+	 */
+	if ( (status & MCi_STATUS_UC) && (status & MCi_STATUS_PCC) )
+		return 0;
+	/* Spurious need clear bank */
+	if ( !(status & MCi_STATUS_OVER)
+	    && (status & MCi_STATUS_UC) && !(status & MCi_STATUS_EN))
+		return 1;

Quoted 8 lines

Quoted 3 lines
(quoted diff) +2 -0
+
+	return 1;
}
So what's the purpose of first conditionally returning 1, and then also doing so unconditionally? Do anticipate to insert code between the two parts within the very near future? Otherwise I'd drop the whole if() construct.

This function is derived from intel_need_clearbank_scan(). I just took over the relevant parts for AMD.

Christoph

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Jan Beulich

2012-10-25 08:55 UTC #5 lore raw

Quoted 31 lines

Quoted 1 line
Quoted 1 line · Christoph Egger
Quoted 20 lines · Jan Beulich
Quoted 20 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
xen/arch/x86/cpu/mcheck/amd_k8.c +19 -0
+static int k8_need_clearbank_scan(enum mca_source who, uint64_t status)
+{
+	switch (who) {
+	case MCA_MCE_SCAN:
+	case MCA_MCE_HANDLER:
+		break;
+	default:
+		return 1;
+	}
+
+	/* For fatal error, it shouldn't be cleared so that sticky bank
+	 * have chance to be handled after reboot by polling.
+	 */
+	if ( (status & MCi_STATUS_UC) && (status & MCi_STATUS_PCC) )
+		return 0;
+	/* Spurious need clear bank */
+	if ( !(status & MCi_STATUS_OVER)
+	    && (status & MCi_STATUS_UC) && !(status & MCi_STATUS_EN))
+		return 1;
Quoted 8 lines
Quoted 3 lines
(quoted diff) +2 -0
+
+	return 1;
}
So what's the purpose of first conditionally returning 1, and then also doing so unconditionally? Do anticipate to insert code between the two parts within the very near future? Otherwise I'd drop the whole if() construct.

Quoted 2 lines

This function is derived from intel_need_clearbank_scan(). I just took over the relevant parts for AMD.

But that would suggest that the final return be "return 0" rather than "return 1".

Further, the Intel code does no extra checking for the MCA_MCE_HANDLER case, so I'd like you to confirm that this is indeed to be different for your CPUs.

Jan

Christoph Egger

author

2012-10-25 09:29 UTC #6 lore raw

Quoted 34 lines · Jan Beulich

Quoted 31 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
Quoted 20 lines · Jan Beulich
Quoted 20 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
xen/arch/x86/cpu/mcheck/amd_k8.c +19 -0
+static int k8_need_clearbank_scan(enum mca_source who, uint64_t status)
+{
+	switch (who) {
+	case MCA_MCE_SCAN:
+	case MCA_MCE_HANDLER:
+		break;
+	default:
+		return 1;
+	}
+
+	/* For fatal error, it shouldn't be cleared so that sticky bank
+	 * have chance to be handled after reboot by polling.
+	 */
+	if ( (status & MCi_STATUS_UC) && (status & MCi_STATUS_PCC) )
+		return 0;
+	/* Spurious need clear bank */
+	if ( !(status & MCi_STATUS_OVER)
+	    && (status & MCi_STATUS_UC) && !(status & MCi_STATUS_EN))
+		return 1;
Quoted 8 lines
Quoted 3 lines
(quoted diff) +2 -0
+
+	return 1;
}
So what's the purpose of first conditionally returning 1, and then also doing so unconditionally? Do anticipate to insert code between the two parts within the very near future? Otherwise I'd drop the whole if() construct.
Quoted 2 lines
This function is derived from intel_need_clearbank_scan(). I just took over the relevant parts for AMD.

Quoted 6 lines

But that would suggest that the final return be "return 0" rather than "return 1".
Further, the Intel code does no extra checking for the MCA_MCE_HANDLER case, so I'd like you to confirm that this is indeed to be different for your CPUs.

I just noticed that MCA_MCE_HANDLER is not used at all and can be removed entirely.

Christoph

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Jan Beulich

2012-10-25 10:03 UTC #7 lore raw

Quoted 39 lines

Quoted 1 line
Quoted 1 line · Christoph Egger
Quoted 37 lines · Jan Beulich
Quoted 31 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
Quoted 20 lines · Jan Beulich
Quoted 20 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
xen/arch/x86/cpu/mcheck/amd_k8.c +19 -0
+static int k8_need_clearbank_scan(enum mca_source who, uint64_t status)
+{
+	switch (who) {
+	case MCA_MCE_SCAN:
+	case MCA_MCE_HANDLER:
+		break;
+	default:
+		return 1;
+	}
+
+	/* For fatal error, it shouldn't be cleared so that sticky bank
+	 * have chance to be handled after reboot by polling.
+	 */
+	if ( (status & MCi_STATUS_UC) && (status & MCi_STATUS_PCC) )
+		return 0;
+	/* Spurious need clear bank */
+	if ( !(status & MCi_STATUS_OVER)
+	    && (status & MCi_STATUS_UC) && !(status & MCi_STATUS_EN))
+		return 1;
Quoted 8 lines
Quoted 3 lines
(quoted diff) +2 -0
+
+	return 1;
}
So what's the purpose of first conditionally returning 1, and then also doing so unconditionally? Do anticipate to insert code between the two parts within the very near future? Otherwise I'd drop the whole if() construct.
Quoted 2 lines
This function is derived from intel_need_clearbank_scan(). I just took over the relevant parts for AMD.
But that would suggest that the final return be "return 0" rather than "return 1".

Quoted 6 lines

Quoted 3 lines
Further, the Intel code does no extra checking for the MCA_MCE_HANDLER case, so I'd like you to confirm that this is indeed to be different for your CPUs.
I just noticed that MCA_MCE_HANDLER is not used at all and can be removed entirely.

Will do. But how about the "return" related question above?

Jan

Christoph Egger

author

2012-10-25 10:13 UTC #8 lore raw

Quoted 39 lines · Jan Beulich

Quoted 39 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
Quoted 37 lines · Jan Beulich
Quoted 31 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
Quoted 20 lines · Jan Beulich
Quoted 20 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
xen/arch/x86/cpu/mcheck/amd_k8.c +19 -0
+static int k8_need_clearbank_scan(enum mca_source who, uint64_t status)
+{
+	switch (who) {
+	case MCA_MCE_SCAN:
+	case MCA_MCE_HANDLER:
+		break;
+	default:
+		return 1;
+	}
+
+	/* For fatal error, it shouldn't be cleared so that sticky bank
+	 * have chance to be handled after reboot by polling.
+	 */
+	if ( (status & MCi_STATUS_UC) && (status & MCi_STATUS_PCC) )
+		return 0;
+	/* Spurious need clear bank */
+	if ( !(status & MCi_STATUS_OVER)
+	    && (status & MCi_STATUS_UC) && !(status & MCi_STATUS_EN))
+		return 1;
Quoted 8 lines
Quoted 3 lines
(quoted diff) +2 -0
+
+	return 1;
}
So what's the purpose of first conditionally returning 1, and then also doing so unconditionally? Do anticipate to insert code between the two parts within the very near future? Otherwise I'd drop the whole if() construct.
Quoted 2 lines
This function is derived from intel_need_clearbank_scan(). I just took over the relevant parts for AMD.
But that would suggest that the final return be "return 0" rather than "return 1".

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Further, the Intel code does no extra checking for the MCA_MCE_HANDLER case, so I'd like you to confirm that this is indeed to be different for your CPUs.
I just noticed that MCA_MCE_HANDLER is not used at all and can be removed entirely.
Will do.

Ok, thanks.

Quoted 1 line

But how about the "return" related question above?

I just run some tests.
Remove this part:

xen/arch/x86/cpu/mcheck/amd_k8.c line 97

Quoted 4 lines

xen/arch/x86/cpu/mcheck/amd_k8.c +4 -0

+	/* Spurious need clear bank */
+	if ( !(status & MCi_STATUS_OVER)
+	    && (status & MCi_STATUS_UC) && !(status & MCi_STATUS_EN))
+		return 1;

Christoph

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Christoph Egger

author

2012-10-25 10:23 UTC #9 lore raw

Quoted 53 lines · Christoph Egger

Quoted 39 lines · Jan Beulich
Quoted 39 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
Quoted 37 lines · Jan Beulich
Quoted 31 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
Quoted 20 lines · Jan Beulich
Quoted 20 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
(quoted diff) +19 -0
+static int k8_need_clearbank_scan(enum mca_source who, uint64_t status)
+{
+	switch (who) {
+	case MCA_MCE_SCAN:
+	case MCA_MCE_HANDLER:
+		break;
+	default:
+		return 1;
+	}
+
+	/* For fatal error, it shouldn't be cleared so that sticky bank
+	 * have chance to be handled after reboot by polling.
+	 */
+	if ( (status & MCi_STATUS_UC) && (status & MCi_STATUS_PCC) )
+		return 0;
+	/* Spurious need clear bank */
+	if ( !(status & MCi_STATUS_OVER)
+	    && (status & MCi_STATUS_UC) && !(status & MCi_STATUS_EN))
+		return 1;
Quoted 8 lines
Quoted 3 lines
(quoted diff) +2 -0
+
+	return 1;
}
So what's the purpose of first conditionally returning 1, and then also doing so unconditionally? Do anticipate to insert code between the two parts within the very near future? Otherwise I'd drop the whole if() construct.
Quoted 2 lines
This function is derived from intel_need_clearbank_scan(). I just took over the relevant parts for AMD.
But that would suggest that the final return be "return 0" rather than "return 1".
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Further, the Intel code does no extra checking for the MCA_MCE_HANDLER case, so I'd like you to confirm that this is indeed to be different for your CPUs.
I just noticed that MCA_MCE_HANDLER is not used at all and can be removed entirely.
Will do.
Ok, thanks.
Quoted 1 line
But how about the "return" related question above?

Quoted 8 lines

I just run some tests.
Remove this part:
Review
Quoted 4 lines
(quoted diff) +4 -0
+	/* Spurious need clear bank */
+	if ( !(status & MCi_STATUS_OVER)
+	    && (status & MCi_STATUS_UC) && !(status & MCi_STATUS_EN))
+		return 1;

And I changed the switch (who) to:

if (who != MCA_MCE_SCAN)

       return 1;

Christoph

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Jan Beulich

2012-10-25 10:28 UTC #10 lore raw

Quoted 5 lines

Quoted 1 line
Quoted 1 line · Christoph Egger
And I changed the switch (who) to:
   if (who != MCA_MCE_SCAN)
       return 1;

That's what I did too.

Jan

Christoph Egger

author

2012-10-25 10:30 UTC #11 lore raw

Quoted 7 lines · Jan Beulich

Quoted 5 lines
Quoted 1 line
Quoted 1 line · Christoph Egger
And I changed the switch (who) to:
   if (who != MCA_MCE_SCAN)
       return 1;
That's what I did too.

Thanks.

Christoph

Signature · 6 lines

---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632