[PATCH 0/2] x86: MCE optimization and cleanups

[PATCH 0/2] x86: MCE optimization and cleanups

[Joerg Roedel]

This patchset includes two patches.

1. Checks for the MCA fetures as early as possible and signedness fixup.
2. Minor coding style cleanup.

[PATCH 1/2] x86: mce init optimization and signedness fixup

[Joerg Roedel]

MCG_CAP never reports a negative count of available error-reporting banks.
Therefore, make nr_mce_banks unsigned.
Check for MCA/MCE feature bits as early as possible.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce.c |   12 ++++++++++--
 arch/x86/kernel/cpu/mcheck/mce.h |    2 +-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 34c781e..e1ac4f7 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -17,7 +17,7 @@
 #include "mce.h"
 
 int mce_disabled = 0;
-int nr_mce_banks;
+unsigned int nr_mce_banks;
 
 EXPORT_SYMBOL_GPL(nr_mce_banks);	/* non-fatal.o */
 
@@ -33,8 +33,16 @@ void fastcall (*machine_check_vector)(struct pt_regs *, long error_code) = unexp
 /* This has to be run for each processor */
 void mcheck_init(struct cpuinfo_x86 *c)
 {
-	if (mce_disabled==1)
+	if (mce_disabled == 1) {
+		printk(KERN_INFO "MCE support disabled by bootparam\n");
 		return;
+	}
+
+	if (!cpu_has(c, X86_FEATURE_MCA) || !cpu_has(c, X86_FEATURE_MCE)) {
+		printk(KERN_INFO "CPU%i: No machine check support available\n",
+			smp_processor_id());
+		return;
+	}
 
 	switch (c->x86_vendor) {
 		case X86_VENDOR_AMD:
diff --git a/arch/x86/kernel/cpu/mcheck/mce.h b/arch/x86/kernel/cpu/mcheck/mce.h
index 81fb6e2..9cbe812 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.h
+++ b/arch/x86/kernel/cpu/mcheck/mce.h
@@ -10,5 +10,5 @@ void winchip_mcheck_init(struct cpuinfo_x86 *c);
 /* Call the installed machine check handler for this CPU setup. */
 extern fastcall void (*machine_check_vector)(struct pt_regs *, long error_code);
 
-extern int nr_mce_banks;
+extern unsigned int nr_mce_banks;
 
-- 
1.5.2.5

Re: [PATCH 1/2] x86: mce init optimization and signedness fixup

[Thomas Gleixner]

On Thu, 11 Oct 2007, Joerg Roedel wrote:
> MCG_CAP never reports a negative count of available error-reporting banks.
> Therefore, make nr_mce_banks unsigned.
> Check for MCA/MCE feature bits as early as possible.

> +
> +	if (!cpu_has(c, X86_FEATURE_MCA) || !cpu_has(c, X86_FEATURE_MCE)) {
> +		printk(KERN_INFO "CPU%i: No machine check support available\n",
> +			smp_processor_id());
> +		return;

This breaks winchip MCE support.

     tglx

Re: [PATCH 1/2] x86: mce init optimization and signedness fixup

[Christoph Egger]

On Thursday 11 October 2007 15:51:49 Thomas Gleixner wrote:
> On Thu, 11 Oct 2007, Joerg Roedel wrote:
> > MCG_CAP never reports a negative count of available error-reporting
> > banks. Therefore, make nr_mce_banks unsigned.
> > Check for MCA/MCE feature bits as early as possible.
> >
> > +
> > +	if (!cpu_has(c, X86_FEATURE_MCA) || !cpu_has(c, X86_FEATURE_MCE)) {
> > +		printk(KERN_INFO "CPU%i: No machine check support available\n",
> > +			smp_processor_id());
> > +		return;
>
> This breaks winchip MCE support.

First, what is a winchip? It sounds to be something windows specific. ;)
Second, can you explain in which way MCE support gets broken, please?

If you actually see the printk message, then machine check
support is not available on that cpu on that machine, right?


-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy

Re: [PATCH 1/2] x86: mce init optimization and signedness fixup

[Thomas Gleixner]

On Thu, 11 Oct 2007, Christoph Egger wrote:

> On Thursday 11 October 2007 15:51:49 Thomas Gleixner wrote:
> > On Thu, 11 Oct 2007, Joerg Roedel wrote:
> > > MCG_CAP never reports a negative count of available error-reporting
> > > banks. Therefore, make nr_mce_banks unsigned.
> > > Check for MCA/MCE feature bits as early as possible.
> > >
> > > +
> > > +	if (!cpu_has(c, X86_FEATURE_MCA) || !cpu_has(c, X86_FEATURE_MCE)) {
> > > +		printk(KERN_INFO "CPU%i: No machine check support available\n",
> > > +			smp_processor_id());
> > > +		return;
> >
> > This breaks winchip MCE support.
> 
> First, what is a winchip? It sounds to be something windows specific. ;)
> Second, can you explain in which way MCE support gets broken, please?

First, winchip is the code name of Centaurs early x86 cpus.

Second, those beasts do not have FEATURE_MCA, but they have FEATURE_MCE, 
so they support the fatal exception, but not the non fatal check.
 
	tglx

Re: [PATCH 1/2] x86: mce init optimization and signedness fixup

[Christoph Egger]

On Thursday 11 October 2007 16:55:36 Thomas Gleixner wrote:
> On Thu, 11 Oct 2007, Christoph Egger wrote:
> > On Thursday 11 October 2007 15:51:49 Thomas Gleixner wrote:
> > > On Thu, 11 Oct 2007, Joerg Roedel wrote:
> > > > MCG_CAP never reports a negative count of available error-reporting
> > > > banks. Therefore, make nr_mce_banks unsigned.
> > > > Check for MCA/MCE feature bits as early as possible.
> > > >
> > > > +
> > > > +	if (!cpu_has(c, X86_FEATURE_MCA) || !cpu_has(c, X86_FEATURE_MCE)) {
> > > > +		printk(KERN_INFO "CPU%i: No machine check support available\n",
> > > > +			smp_processor_id());
> > > > +		return;
> > >
> > > This breaks winchip MCE support.
> >
> > First, what is a winchip? It sounds to be something windows specific. ;)
> > Second, can you explain in which way MCE support gets broken, please?
>
> First, winchip is the code name of Centaurs early x86 cpus.
>
> Second, those beasts do not have FEATURE_MCA, but they have FEATURE_MCE,
> so they support the fatal exception, but not the non fatal check.

So when I change the above code snippet to:

+	if (!cpu_has(c, X86_FEATURE_MCE)) {
+		printk(KERN_INFO "CPU%i: No machine check support available\n",
+			smp_processor_id());
+		return;

Would this make the whole patch acceptable then?



-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy

Re: [PATCH 1/2] x86: mce init optimization and signedness fixup

[Thomas Gleixner]

On Thu, 11 Oct 2007, Christoph Egger wrote:
> On Thursday 11 October 2007 16:55:36 Thomas Gleixner wrote:
> > > > > +
> > > > > +	if (!cpu_has(c, X86_FEATURE_MCA) || !cpu_has(c, X86_FEATURE_MCE)) {
> > > > > +		printk(KERN_INFO "CPU%i: No machine check support available\n",
> > > > > +			smp_processor_id());
> > > > > +		return;
> > > >
> > > > This breaks winchip MCE support.
> > >
> > > First, what is a winchip? It sounds to be something windows specific. ;)
> > > Second, can you explain in which way MCE support gets broken, please?
> >
> > First, winchip is the code name of Centaurs early x86 cpus.
> >
> > Second, those beasts do not have FEATURE_MCA, but they have FEATURE_MCE,
> > so they support the fatal exception, but not the non fatal check.
> 
> So when I change the above code snippet to:
> 
> +	if (!cpu_has(c, X86_FEATURE_MCE)) {
> +		printk(KERN_INFO "CPU%i: No machine check support available\n",
> +			smp_processor_id());
> +		return;
> 
> Would this make the whole patch acceptable then?

Yeah, but then we can clean up the extra checks for _MCE in the various 
cpu type init functions as well.

	tglx

Re: [PATCH 1/2] x86: mce init optimization and signedness fixup

[Dave Jones]

On Thu, Oct 11, 2007 at 06:50:12PM +0200, Thomas Gleixner wrote:
 > On Thu, 11 Oct 2007, Christoph Egger wrote:
 > > On Thursday 11 October 2007 16:55:36 Thomas Gleixner wrote:
 > > > > > > +
 > > > > > > +	if (!cpu_has(c, X86_FEATURE_MCA) || !cpu_has(c, X86_FEATURE_MCE)) {
 > > > > > > +		printk(KERN_INFO "CPU%i: No machine check support available\n",
 > > > > > > +			smp_processor_id());
 > > > > > > +		return;
 > > > > >
 > > > > > This breaks winchip MCE support.
 > > > >
 > > > > First, what is a winchip? It sounds to be something windows specific. ;)
 > > > > Second, can you explain in which way MCE support gets broken, please?
 > > >
 > > > First, winchip is the code name of Centaurs early x86 cpus.
 > > >
 > > > Second, those beasts do not have FEATURE_MCA, but they have FEATURE_MCE,
 > > > so they support the fatal exception, but not the non fatal check.
 > > 
 > > So when I change the above code snippet to:
 > > 
 > > +	if (!cpu_has(c, X86_FEATURE_MCE)) {
 > > +		printk(KERN_INFO "CPU%i: No machine check support available\n",
 > > +			smp_processor_id());
 > > +		return;
 > > 
 > > Would this make the whole patch acceptable then?
 > 
 > Yeah, but then we can clean up the extra checks for _MCE in the various 
 > cpu type init functions as well.
 
I question the value of adding the printk.
It's not a failure, there's nothing the user can do about it,
and it adds no real value, just more noise to the dmesg.

	Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH 1/2] x86: mce init optimization and signedness fixup

[Christoph Egger]

On Thursday 11 October 2007 18:50:12 Thomas Gleixner wrote:
> On Thu, 11 Oct 2007, Christoph Egger wrote:
> > On Thursday 11 October 2007 16:55:36 Thomas Gleixner wrote:
> > > > > > +
> > > > > > +	if (!cpu_has(c, X86_FEATURE_MCA) || !cpu_has(c,
> > > > > > X86_FEATURE_MCE)) { +		printk(KERN_INFO "CPU%i: No machine check
> > > > > > support available\n", +			smp_processor_id());
> > > > > > +		return;
> > > > >
> > > > > This breaks winchip MCE support.
> > > >
> > > > First, what is a winchip? It sounds to be something windows specific.
> > > > ;) Second, can you explain in which way MCE support gets broken,
> > > > please?
> > >
> > > First, winchip is the code name of Centaurs early x86 cpus.
> > >
> > > Second, those beasts do not have FEATURE_MCA, but they have
> > > FEATURE_MCE, so they support the fatal exception, but not the non fatal
> > > check.
> >
> > So when I change the above code snippet to:
> >
> > +	if (!cpu_has(c, X86_FEATURE_MCE)) {
> > +		printk(KERN_INFO "CPU%i: No machine check support available\n",
> > +			smp_processor_id());
> > +		return;
> >
> > Would this make the whole patch acceptable then?
>
> Yeah, but then we can clean up the extra checks for _MCE in the various
> cpu type init functions as well.

Will you do the little modification in your tree after applying my patch
or do you want me to resend the patch with that little modification.

Christoph


-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy

Re: [PATCH 1/2] x86: mce init optimization and signedness fixup

[Alan Cox]

> So when I change the above code snippet to:
> 
> +	if (!cpu_has(c, X86_FEATURE_MCE)) {
> +		printk(KERN_INFO "CPU%i: No machine check support available\n",
> +			smp_processor_id());
> +		return;
> 
> Would this make the whole patch acceptable then?

I think the fundamental direction is wrong. "Do you have a machine check
like facility" is a CPU specific question that belongs in the CPU
specific code.

If some of that CPU specific code is wrong, fix it there.

Alan

Re: [PATCH 1/2] x86: mce init optimization and signedness fixup

[Alan Cox]

On Thu, 11 Oct 2007 16:55:36 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Thu, 11 Oct 2007, Christoph Egger wrote:
> 
> > On Thursday 11 October 2007 15:51:49 Thomas Gleixner wrote:
> > > On Thu, 11 Oct 2007, Joerg Roedel wrote:
> > > > MCG_CAP never reports a negative count of available error-reporting
> > > > banks. Therefore, make nr_mce_banks unsigned.
> > > > Check for MCA/MCE feature bits as early as possible.
> > > >
> > > > +
> > > > +	if (!cpu_has(c, X86_FEATURE_MCA) || !cpu_has(c, X86_FEATURE_MCE)) {
> > > > +		printk(KERN_INFO "CPU%i: No machine check support available\n",
> > > > +			smp_processor_id());
> > > > +		return;
> > >
> > > This breaks winchip MCE support.
> > 
> > First, what is a winchip? It sounds to be something windows specific. ;)
> > Second, can you explain in which way MCE support gets broken, please?
> 
> First, winchip is the code name of Centaurs early x86 cpus.
> 
> Second, those beasts do not have FEATURE_MCA, but they have FEATURE_MCE, 
> so they support the fatal exception, but not the non fatal check.

Ditto some of the original Intel Preventium processors which have limited
MC support hidden in the magic 'Appendix H'

Alan

[PATCH 2/2] x86: mce minor indent cleanup

[Joerg Roedel]

remove one indent level

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce.c |   40 +++++++++++++++++++-------------------
 1 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index e1ac4f7..bc61f1e 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -45,26 +45,26 @@ void mcheck_init(struct cpuinfo_x86 *c)
 	}
 
 	switch (c->x86_vendor) {
-		case X86_VENDOR_AMD:
-			amd_mcheck_init(c);
-			break;
-
-		case X86_VENDOR_INTEL:
-			if (c->x86==5)
-				intel_p5_mcheck_init(c);
-			if (c->x86==6)
-				intel_p6_mcheck_init(c);
-			if (c->x86==15)
-				intel_p4_mcheck_init(c);
-			break;
-
-		case X86_VENDOR_CENTAUR:
-			if (c->x86==5)
-				winchip_mcheck_init(c);
-			break;
-
-		default:
-			break;
+	case X86_VENDOR_AMD:
+		amd_mcheck_init(c);
+		break;
+
+	case X86_VENDOR_INTEL:
+		if (c->x86==5)
+			intel_p5_mcheck_init(c);
+		if (c->x86==6)
+			intel_p6_mcheck_init(c);
+		if (c->x86==15)
+			intel_p4_mcheck_init(c);
+		break;
+
+	case X86_VENDOR_CENTAUR:
+		if (c->x86==5)
+			winchip_mcheck_init(c);
+		break;
+
+	default:
+		break;
 	}
 }
 
-- 
1.5.2.5