[PATCH 0/5] x86, NMI: give NMI handler a face-lift

[PATCH 0/5] x86, NMI: give NMI handler a face-lift

[Don Zickus]

Restructuring the nmi handler to be more readable and simpler.

This is just laying the ground work for future improvements in this area.

I know people were complaining about having DIE_NMI and DIE_NMI_IPI, but
for now it is consistent with what was there before.  Hopefully, we can
modify the notifier chain to make it smarter and combine the two events.

I also left out one of Huang's patch until we figure out how we are going
to proceed with a new notifier.

Tested 32-bit and 64-bit on AMD and Intel machines.

Cheers,
Don

Huang Ying (5):
  x86, NMI: Add NMI symbol constants and rename memory parity to PCI
    SERR
  x86, NMI: Add touch_nmi_watchdog to io_check_error delay
  x86, NMI: Rewrite NMI handler
  x86, NMI: Allow NMI reason io port (0x61) to be processed on any CPU
  x86, NMI: Remove do_nmi_callback logic

 arch/x86/include/asm/mach_traps.h |   12 +++-
 arch/x86/include/asm/nmi.h        |   10 +++-
 arch/x86/kernel/apic/hw_nmi.c     |    1 -
 arch/x86/kernel/apic/nmi.c        |   29 +-------
 arch/x86/kernel/cpu/perf_event.c  |    1 -
 arch/x86/kernel/traps.c           |  141 ++++++++++++++++++++----------------
 arch/x86/oprofile/nmi_int.c       |    1 -
 arch/x86/oprofile/nmi_timer_int.c |    2 +-
 drivers/char/ipmi/ipmi_watchdog.c |    2 +-
 drivers/watchdog/hpwdt.c          |    2 +-
 10 files changed, 102 insertions(+), 99 deletions(-)

-- 
1.7.2.3

[PATCH 1/5] x86, NMI: Add NMI symbol constants and rename memory parity to PCI SERR

[Don Zickus]

From: Huang Ying <ying.huang@intel.com>

Replace the NMI related magic numbers with symbol constants.

memory parity error is only valid for IBM PC-AT, newer machine use
bit 7 (0x80) of 0x61 port for PCI SERR. While memory error is usually
reported via MCE. So corresponding function name and kernel log string
is changed.

But on some machines, PCI SERR line is still used to report memory
errors. This is used by EDAC, so corresponding EDAC call is reserved.

v3:

- Merge adding symbol constants and renaming patches

v2:

- EDAC call in pci_serr_error is reserved.

Signed-off-by: Huang Ying <ying.huang@intel.com>
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 arch/x86/include/asm/mach_traps.h |   12 ++++++++-
 arch/x86/kernel/traps.c           |   51 +++++++++++++++++++------------------
 2 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/arch/x86/include/asm/mach_traps.h b/arch/x86/include/asm/mach_traps.h
index f792060..72a8b52 100644
--- a/arch/x86/include/asm/mach_traps.h
+++ b/arch/x86/include/asm/mach_traps.h
@@ -7,9 +7,19 @@
 
 #include <asm/mc146818rtc.h>
 
+#define NMI_REASON_PORT		0x61
+
+#define NMI_REASON_SERR		0x80
+#define NMI_REASON_IOCHK	0x40
+#define NMI_REASON_MASK		(NMI_REASON_SERR | NMI_REASON_IOCHK)
+
+#define NMI_REASON_CLEAR_SERR	0x04
+#define NMI_REASON_CLEAR_IOCHK	0x08
+#define NMI_REASON_CLEAR_MASK	0x0f
+
 static inline unsigned char get_nmi_reason(void)
 {
-	return inb(0x61);
+	return inb(NMI_REASON_PORT);
 }
 
 static inline void reassert_nmi(void)
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 60788de..e04e59b 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -301,15 +301,15 @@ gp_in_kernel:
 }
 
 static notrace __kprobes void
-mem_parity_error(unsigned char reason, struct pt_regs *regs)
+pci_serr_error(unsigned char reason, struct pt_regs *regs)
 {
-	printk(KERN_EMERG
-		"Uhhuh. NMI received for unknown reason %02x on CPU %d.\n",
-			reason, smp_processor_id());
-
-	printk(KERN_EMERG
-		"You have some hardware problem, likely on the PCI bus.\n");
+	pr_emerg("NMI: PCI system error (SERR) for reason %02x on CPU %d.\n",
+		 reason, smp_processor_id());
 
+	/*
+	 * On some machines, PCI SERR line is used to report memory
+	 * errors. EDAC makes use of it.
+	 */
 #if defined(CONFIG_EDAC)
 	if (edac_handler_set()) {
 		edac_atomic_assert_error();
@@ -320,11 +320,11 @@ mem_parity_error(unsigned char reason, struct pt_regs *regs)
 	if (panic_on_unrecovered_nmi)
 		panic("NMI: Not continuing");
 
-	printk(KERN_EMERG "Dazed and confused, but trying to continue\n");
+	pr_emerg("Dazed and confused, but trying to continue\n");
 
-	/* Clear and disable the memory parity error line. */
-	reason = (reason & 0xf) | 4;
-	outb(reason, 0x61);
+	/* Clear and disable the PCI SERR error line. */
+	reason = (reason & NMI_REASON_CLEAR_MASK) | NMI_REASON_CLEAR_SERR;
+	outb(reason, NMI_REASON_PORT);
 }
 
 static notrace __kprobes void
@@ -332,22 +332,24 @@ io_check_error(unsigned char reason, struct pt_regs *regs)
 {
 	unsigned long i;
 
-	printk(KERN_EMERG "NMI: IOCK error (debug interrupt?)\n");
+	pr_emerg(
+	"NMI: IOCK error (debug interrupt?) for reason %02x on CPU %d.\n",
+		 reason, smp_processor_id());
 	show_registers(regs);
 
 	if (panic_on_io_nmi)
 		panic("NMI IOCK error: Not continuing");
 
 	/* Re-enable the IOCK line, wait for a few seconds */
-	reason = (reason & 0xf) | 8;
-	outb(reason, 0x61);
+	reason = (reason & NMI_REASON_CLEAR_MASK) | NMI_REASON_CLEAR_IOCHK;
+	outb(reason, NMI_REASON_PORT);
 
 	i = 2000;
 	while (--i)
 		udelay(1000);
 
-	reason &= ~8;
-	outb(reason, 0x61);
+	reason &= ~NMI_REASON_CLEAR_IOCHK;
+	outb(reason, NMI_REASON_PORT);
 }
 
 static notrace __kprobes void
@@ -366,15 +368,14 @@ unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
 		return;
 	}
 #endif
-	printk(KERN_EMERG
-		"Uhhuh. NMI received for unknown reason %02x on CPU %d.\n",
-			reason, smp_processor_id());
+	pr_emerg("Uhhuh. NMI received for unknown reason %02x on CPU %d.\n",
+		 reason, smp_processor_id());
 
-	printk(KERN_EMERG "Do you have a strange power saving mode enabled?\n");
+	pr_emerg("Do you have a strange power saving mode enabled?\n");
 	if (panic_on_unrecovered_nmi)
 		panic("NMI: Not continuing");
 
-	printk(KERN_EMERG "Dazed and confused, but trying to continue\n");
+	pr_emerg("Dazed and confused, but trying to continue\n");
 }
 
 static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
@@ -388,7 +389,7 @@ static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
 	if (!cpu)
 		reason = get_nmi_reason();
 
-	if (!(reason & 0xc0)) {
+	if (!(reason & NMI_REASON_MASK)) {
 		if (notify_die(DIE_NMI_IPI, "nmi_ipi", regs, reason, 2, SIGINT)
 								== NOTIFY_STOP)
 			return;
@@ -418,9 +419,9 @@ static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
 		return;
 
 	/* AK: following checks seem to be broken on modern chipsets. FIXME */
-	if (reason & 0x80)
-		mem_parity_error(reason, regs);
-	if (reason & 0x40)
+	if (reason & NMI_REASON_SERR)
+		pci_serr_error(reason, regs);
+	if (reason & NMI_REASON_IOCHK)
 		io_check_error(reason, regs);
 #ifdef CONFIG_X86_32
 	/*
-- 
1.7.2.3

[tip:perf/core] x86, NMI: Add NMI symbol constants and rename memory parity to PCI SERR

[tip-bot for Huang Ying]

Commit-ID:  88de48ce3d1c384079bf5b603d3056ae2d60a185
Gitweb:     http://git.kernel.org/tip/88de48ce3d1c384079bf5b603d3056ae2d60a185
Author:     Huang Ying <ying.huang@intel.com>
AuthorDate: Fri, 15 Oct 2010 22:22:14 -0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sat, 16 Oct 2010 15:01:27 +0200

x86, NMI: Add NMI symbol constants and rename memory parity to PCI SERR

Replace the NMI related magic numbers with symbol constants.

memory parity error is only valid for IBM PC-AT, newer machine
use bit 7 (0x80) of 0x61 port for PCI SERR. While memory error
is usually reported via MCE. So corresponding function name and
kernel log string is changed.

But on some machines, PCI SERR line is still used to report
memory errors. This is used by EDAC, so corresponding EDAC call
is reserved.

v3:

- Merge adding symbol constants and renaming patches

v2:

- EDAC call in pci_serr_error is reserved.

Signed-off-by: Huang Ying <ying.huang@intel.com>
Signed-off-by: Don Zickus <dzickus@redhat.com>
Cc: andi@firstfloor.org
Cc: robert.richter@amd.com
Cc: peterz@infradead.org
LKML-Reference: <1287195738-3136-2-git-send-email-dzickus@redhat.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/include/asm/mach_traps.h |   12 ++++++++-
 arch/x86/kernel/traps.c           |   51 +++++++++++++++++++------------------
 2 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/arch/x86/include/asm/mach_traps.h b/arch/x86/include/asm/mach_traps.h
index f792060..72a8b52 100644
--- a/arch/x86/include/asm/mach_traps.h
+++ b/arch/x86/include/asm/mach_traps.h
@@ -7,9 +7,19 @@
 
 #include <asm/mc146818rtc.h>
 
+#define NMI_REASON_PORT		0x61
+
+#define NMI_REASON_SERR		0x80
+#define NMI_REASON_IOCHK	0x40
+#define NMI_REASON_MASK		(NMI_REASON_SERR | NMI_REASON_IOCHK)
+
+#define NMI_REASON_CLEAR_SERR	0x04
+#define NMI_REASON_CLEAR_IOCHK	0x08
+#define NMI_REASON_CLEAR_MASK	0x0f
+
 static inline unsigned char get_nmi_reason(void)
 {
-	return inb(0x61);
+	return inb(NMI_REASON_PORT);
 }
 
 static inline void reassert_nmi(void)
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 60788de..e04e59b 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -301,15 +301,15 @@ gp_in_kernel:
 }
 
 static notrace __kprobes void
-mem_parity_error(unsigned char reason, struct pt_regs *regs)
+pci_serr_error(unsigned char reason, struct pt_regs *regs)
 {
-	printk(KERN_EMERG
-		"Uhhuh. NMI received for unknown reason %02x on CPU %d.\n",
-			reason, smp_processor_id());
-
-	printk(KERN_EMERG
-		"You have some hardware problem, likely on the PCI bus.\n");
+	pr_emerg("NMI: PCI system error (SERR) for reason %02x on CPU %d.\n",
+		 reason, smp_processor_id());
 
+	/*
+	 * On some machines, PCI SERR line is used to report memory
+	 * errors. EDAC makes use of it.
+	 */
 #if defined(CONFIG_EDAC)
 	if (edac_handler_set()) {
 		edac_atomic_assert_error();
@@ -320,11 +320,11 @@ mem_parity_error(unsigned char reason, struct pt_regs *regs)
 	if (panic_on_unrecovered_nmi)
 		panic("NMI: Not continuing");
 
-	printk(KERN_EMERG "Dazed and confused, but trying to continue\n");
+	pr_emerg("Dazed and confused, but trying to continue\n");
 
-	/* Clear and disable the memory parity error line. */
-	reason = (reason & 0xf) | 4;
-	outb(reason, 0x61);
+	/* Clear and disable the PCI SERR error line. */
+	reason = (reason & NMI_REASON_CLEAR_MASK) | NMI_REASON_CLEAR_SERR;
+	outb(reason, NMI_REASON_PORT);
 }
 
 static notrace __kprobes void
@@ -332,22 +332,24 @@ io_check_error(unsigned char reason, struct pt_regs *regs)
 {
 	unsigned long i;
 
-	printk(KERN_EMERG "NMI: IOCK error (debug interrupt?)\n");
+	pr_emerg(
+	"NMI: IOCK error (debug interrupt?) for reason %02x on CPU %d.\n",
+		 reason, smp_processor_id());
 	show_registers(regs);
 
 	if (panic_on_io_nmi)
 		panic("NMI IOCK error: Not continuing");
 
 	/* Re-enable the IOCK line, wait for a few seconds */
-	reason = (reason & 0xf) | 8;
-	outb(reason, 0x61);
+	reason = (reason & NMI_REASON_CLEAR_MASK) | NMI_REASON_CLEAR_IOCHK;
+	outb(reason, NMI_REASON_PORT);
 
 	i = 2000;
 	while (--i)
 		udelay(1000);
 
-	reason &= ~8;
-	outb(reason, 0x61);
+	reason &= ~NMI_REASON_CLEAR_IOCHK;
+	outb(reason, NMI_REASON_PORT);
 }
 
 static notrace __kprobes void
@@ -366,15 +368,14 @@ unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
 		return;
 	}
 #endif
-	printk(KERN_EMERG
-		"Uhhuh. NMI received for unknown reason %02x on CPU %d.\n",
-			reason, smp_processor_id());
+	pr_emerg("Uhhuh. NMI received for unknown reason %02x on CPU %d.\n",
+		 reason, smp_processor_id());
 
-	printk(KERN_EMERG "Do you have a strange power saving mode enabled?\n");
+	pr_emerg("Do you have a strange power saving mode enabled?\n");
 	if (panic_on_unrecovered_nmi)
 		panic("NMI: Not continuing");
 
-	printk(KERN_EMERG "Dazed and confused, but trying to continue\n");
+	pr_emerg("Dazed and confused, but trying to continue\n");
 }
 
 static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
@@ -388,7 +389,7 @@ static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
 	if (!cpu)
 		reason = get_nmi_reason();
 
-	if (!(reason & 0xc0)) {
+	if (!(reason & NMI_REASON_MASK)) {
 		if (notify_die(DIE_NMI_IPI, "nmi_ipi", regs, reason, 2, SIGINT)
 								== NOTIFY_STOP)
 			return;
@@ -418,9 +419,9 @@ static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
 		return;
 
 	/* AK: following checks seem to be broken on modern chipsets. FIXME */
-	if (reason & 0x80)
-		mem_parity_error(reason, regs);
-	if (reason & 0x40)
+	if (reason & NMI_REASON_SERR)
+		pci_serr_error(reason, regs);
+	if (reason & NMI_REASON_IOCHK)
 		io_check_error(reason, regs);
 #ifdef CONFIG_X86_32
 	/*

[PATCH 2/5] x86, NMI: Add touch_nmi_watchdog to io_check_error delay

[Don Zickus]

From: Huang Ying <ying.huang@intel.com>

Prevent the long delay in io_check_error making NMI watchdog timeout.

Signed-off-by: Huang Ying <ying.huang@intel.com>
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 arch/x86/kernel/traps.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index e04e59b..bef859e 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -344,9 +344,11 @@ io_check_error(unsigned char reason, struct pt_regs *regs)
 	reason = (reason & NMI_REASON_CLEAR_MASK) | NMI_REASON_CLEAR_IOCHK;
 	outb(reason, NMI_REASON_PORT);
 
-	i = 2000;
-	while (--i)
-		udelay(1000);
+	i = 20000;
+	while (--i) {
+		touch_nmi_watchdog();
+		udelay(100);
+	}
 
 	reason &= ~NMI_REASON_CLEAR_IOCHK;
 	outb(reason, NMI_REASON_PORT);
-- 
1.7.2.3

[tip:perf/core] x86, NMI: Add touch_nmi_watchdog to io_check_error delay

[tip-bot for Huang Ying]

Commit-ID:  3ef3df20bcfa7d271b40ec79ea6db9534ecd1322
Gitweb:     http://git.kernel.org/tip/3ef3df20bcfa7d271b40ec79ea6db9534ecd1322
Author:     Huang Ying <ying.huang@intel.com>
AuthorDate: Fri, 15 Oct 2010 22:22:15 -0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sat, 16 Oct 2010 15:01:27 +0200

x86, NMI: Add touch_nmi_watchdog to io_check_error delay

Prevent the long delay in io_check_error triggering a NMI watchdog
timeout.

Signed-off-by: Huang Ying <ying.huang@intel.com>
Signed-off-by: Don Zickus <dzickus@redhat.com>
Cc: andi@firstfloor.org
Cc: robert.richter@amd.com
Cc: peterz@infradead.org
LKML-Reference: <1287195738-3136-3-git-send-email-dzickus@redhat.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/traps.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index e04e59b..bef859e 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -344,9 +344,11 @@ io_check_error(unsigned char reason, struct pt_regs *regs)
 	reason = (reason & NMI_REASON_CLEAR_MASK) | NMI_REASON_CLEAR_IOCHK;
 	outb(reason, NMI_REASON_PORT);
 
-	i = 2000;
-	while (--i)
-		udelay(1000);
+	i = 20000;
+	while (--i) {
+		touch_nmi_watchdog();
+		udelay(100);
+	}
 
 	reason &= ~NMI_REASON_CLEAR_IOCHK;
 	outb(reason, NMI_REASON_PORT);

[PATCH 3/5] x86, NMI: Rewrite NMI handler

[Don Zickus]

From: Huang Ying <ying.huang@intel.com>

The original NMI handler is quite outdated in many aspects. This patch
try to fix it.

The order to process the NMI sources are changed as follow:

notify_die(DIE_NMI_IPI);
notify_die(DIE_NMI);
/* process io port 0x61 */
nmi_watchdog_touch();
unknown_nmi();

DIE_NMI_IPI is used to process CPU specific NMI sources, such as perf
event, oprofile, crash IPI, etc. While DIE_NMI is used to process
non-CPU-specific NMI sources, such as APEI (ACPI Platform Error
Interface) GHES (Generic Hardware Error Source), etc. Non-CPU-specific
NMI sources can be processed on any CPU,

DIE_NMI_IPI must be processed before DIE_NMI. For example, perf event
trigger a NMI on CPU 1, at the same time, APEI GHES trigger another
NMI on CPU 0. If DIE_NMI is processed before DIE_NMI_IPI, it is
possible that APEI GHES is processed on CPU 1, while unknown NMI is
gotten on CPU 0.

In this new order of processing, performance sensitive NMI sources
such as oprofile or perf event will have better performance because
the time consuming IO port reading is done after them.

Only one NMI is eaten for each NMI handler call, even for PCI SERR and
IOCHK NMIs. Because one NMI should be raised for each of them, eating
too many NMI will cause unnecessary unknown NMI.

The die value used in NMI sources are fixed accordingly.

The NMI handler in the patch is designed by Andi Kleen.

v3:

- Make DIE_NMI and DIE_NMI_UNKNOWN work in more traditional way.

v2:

- Split process NMI reason (0x61) on non-BSP into another patch

Signed-off-by: Huang Ying <ying.huang@intel.com>
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 arch/x86/kernel/cpu/perf_event.c  |    1 -
 arch/x86/kernel/traps.c           |   78 +++++++++++++++++++------------------
 arch/x86/oprofile/nmi_int.c       |    1 -
 arch/x86/oprofile/nmi_timer_int.c |    2 +-
 drivers/char/ipmi/ipmi_watchdog.c |    2 +-
 drivers/watchdog/hpwdt.c          |    2 +-
 6 files changed, 43 insertions(+), 43 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index e2513f2..8ed55be 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1245,7 +1245,6 @@ perf_event_nmi_handler(struct notifier_block *self,
 		return NOTIFY_DONE;
 
 	switch (cmd) {
-	case DIE_NMI:
 	case DIE_NMI_IPI:
 		break;
 	case DIE_NMIUNKNOWN:
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index bef859e..d8acab3 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -385,53 +385,55 @@ static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
 	unsigned char reason = 0;
 	int cpu;
 
-	cpu = smp_processor_id();
+	/*
+	 * CPU-specific NMI must be processed before non-CPU-specific
+	 * NMI, otherwise we may lose it, because the CPU-specific
+	 * NMI can not be detected/processed on other CPUs.
+	 */
+
+	/*
+	 * CPU-specific NMI: send to specific CPU or NMI sources must
+	 * be processed on specific CPU
+	 */
+	if (notify_die(DIE_NMI_IPI, "nmi_ipi", regs, 0, 2, SIGINT)
+	    == NOTIFY_STOP)
+		return;
 
+	/* Non-CPU-specific NMI: NMI sources can be processed on any CPU */
+	cpu = smp_processor_id();
 	/* Only the BSP gets external NMIs from the system. */
-	if (!cpu)
+	if (!cpu) {
 		reason = get_nmi_reason();
-
-	if (!(reason & NMI_REASON_MASK)) {
-		if (notify_die(DIE_NMI_IPI, "nmi_ipi", regs, reason, 2, SIGINT)
-								== NOTIFY_STOP)
-			return;
-
-#ifdef CONFIG_X86_LOCAL_APIC
-		if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT)
-							== NOTIFY_STOP)
+		if (reason & NMI_REASON_MASK) {
+			if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT)
+			    == NOTIFY_STOP)
+				return;
+			if (reason & NMI_REASON_SERR)
+				pci_serr_error(reason, regs);
+			else if (reason & NMI_REASON_IOCHK)
+				io_check_error(reason, regs);
+#ifdef CONFIG_X86_32
+			/*
+			 * Reassert NMI in case it became active
+			 * meanwhile as it's edge-triggered:
+			 */
+			reassert_nmi();
+#endif
 			return;
+		}
+	}
 
-#ifndef CONFIG_LOCKUP_DETECTOR
-		/*
-		 * Ok, so this is none of the documented NMI sources,
-		 * so it must be the NMI watchdog.
-		 */
-		if (nmi_watchdog_tick(regs, reason))
-			return;
-		if (!do_nmi_callback(regs, cpu))
-#endif /* !CONFIG_LOCKUP_DETECTOR */
-			unknown_nmi_error(reason, regs);
-#else
-		unknown_nmi_error(reason, regs);
-#endif
+	if (notify_die(DIE_NMI, "nmi", regs, 0, 2, SIGINT) == NOTIFY_STOP)
+		return;
 
+#if defined(CONFIG_X86_LOCAL_APIC) && !defined(CONFIG_LOCKUP_DETECTOR)
+	if (nmi_watchdog_tick(regs, reason))
 		return;
-	}
-	if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT) == NOTIFY_STOP)
+	if (do_nmi_callback(regs, cpu))
 		return;
-
-	/* AK: following checks seem to be broken on modern chipsets. FIXME */
-	if (reason & NMI_REASON_SERR)
-		pci_serr_error(reason, regs);
-	if (reason & NMI_REASON_IOCHK)
-		io_check_error(reason, regs);
-#ifdef CONFIG_X86_32
-	/*
-	 * Reassert NMI in case it became active meanwhile
-	 * as it's edge-triggered:
-	 */
-	reassert_nmi();
 #endif
+
+	unknown_nmi_error(reason, regs);
 }
 
 dotraplinkage notrace __kprobes void
diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c
index f1575c9..69f2453 100644
--- a/arch/x86/oprofile/nmi_int.c
+++ b/arch/x86/oprofile/nmi_int.c
@@ -64,7 +64,6 @@ static int profile_exceptions_notify(struct notifier_block *self,
 	int ret = NOTIFY_DONE;
 
 	switch (val) {
-	case DIE_NMI:
 	case DIE_NMI_IPI:
 		if (ctr_running)
 			model->check_ctrs(args->regs, &__get_cpu_var(cpu_msrs));
diff --git a/arch/x86/oprofile/nmi_timer_int.c b/arch/x86/oprofile/nmi_timer_int.c
index e3ecb71..ab72a21 100644
--- a/arch/x86/oprofile/nmi_timer_int.c
+++ b/arch/x86/oprofile/nmi_timer_int.c
@@ -25,7 +25,7 @@ static int profile_timer_exceptions_notify(struct notifier_block *self,
 	int ret = NOTIFY_DONE;
 
 	switch (val) {
-	case DIE_NMI:
+	case DIE_NMI_IPI:
 		oprofile_add_sample(args->regs, 0);
 		ret = NOTIFY_STOP;
 		break;
diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/char/ipmi/ipmi_watchdog.c
index 654d566..6a273fb 100644
--- a/drivers/char/ipmi/ipmi_watchdog.c
+++ b/drivers/char/ipmi/ipmi_watchdog.c
@@ -1080,7 +1080,7 @@ ipmi_nmi(struct notifier_block *self, unsigned long val, void *data)
 {
 	struct die_args *args = data;
 
-	if (val != DIE_NMI)
+	if (val != DIE_NMIUNKNOWN)
 		return NOTIFY_OK;
 
 	/* Hack, if it's a memory or I/O error, ignore it. */
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 3d77116..d8010cc 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -469,7 +469,7 @@ static int hpwdt_pretimeout(struct notifier_block *nb, unsigned long ulReason,
 	unsigned long rom_pl;
 	static int die_nmi_called;
 
-	if (ulReason != DIE_NMI && ulReason != DIE_NMI_IPI)
+	if (ulReason != DIE_NMIUNKNOWN)
 		goto out;
 
 	if (!hpwdt_nmi_decoding)
-- 
1.7.2.3

[tip:perf/core] x86, NMI: Rewrite NMI handler

[tip-bot for Huang Ying]

Commit-ID:  e21f3e4957f4a55f546873605a5caa59556bb144
Gitweb:     http://git.kernel.org/tip/e21f3e4957f4a55f546873605a5caa59556bb144
Author:     Huang Ying <ying.huang@intel.com>
AuthorDate: Fri, 15 Oct 2010 22:22:16 -0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sat, 16 Oct 2010 15:01:27 +0200

x86, NMI: Rewrite NMI handler

The original NMI handler is quite outdated in many aspects. This
patch tries to fix it.

The order to process the NMI sources are changed as follow:

 notify_die(DIE_NMI_IPI);
 notify_die(DIE_NMI);
 /* process io port 0x61 */
 nmi_watchdog_touch();
 unknown_nmi();

DIE_NMI_IPI is used to process CPU specific NMI sources, such as
perf event, oprofile, crash IPI, etc. While DIE_NMI is used to
process non-CPU-specific NMI sources, such as APEI (ACPI
Platform Error Interface) GHES (Generic Hardware Error Source),
etc. Non-CPU-specific NMI sources can be processed on any CPU,

DIE_NMI_IPI must be processed before DIE_NMI. For example, perf
event trigger a NMI on CPU 1, at the same time, APEI GHES
trigger another NMI on CPU 0. If DIE_NMI is processed before
DIE_NMI_IPI, it is possible that APEI GHES is processed on CPU
1, while unknown NMI is gotten on CPU 0.

In this new order of processing, performance sensitive NMI
sources such as oprofile or perf event will have better
performance because the time consuming IO port reading is done
after them.

Only one NMI is eaten for each NMI handler call, even for PCI
SERR and IOCHK NMIs. Because one NMI should be raised for each
of them, eating too many NMI will cause unnecessary unknown NMI.

The die value used in NMI sources are fixed accordingly.

The NMI handler in the patch is designed by Andi Kleen.

v3:

- Make DIE_NMI and DIE_NMI_UNKNOWN work in more traditional way.

v2:

- Split process NMI reason (0x61) on non-BSP into another patch

Signed-off-by: Huang Ying <ying.huang@intel.com>
Signed-off-by: Don Zickus <dzickus@redhat.com>
Cc: andi@firstfloor.org
Cc: robert.richter@amd.com
Cc: peterz@infradead.org
LKML-Reference: <1287195738-3136-4-git-send-email-dzickus@redhat.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/cpu/perf_event.c  |    1 -
 arch/x86/kernel/traps.c           |   78 +++++++++++++++++++------------------
 arch/x86/oprofile/nmi_int.c       |    1 -
 arch/x86/oprofile/nmi_timer_int.c |    2 +-
 drivers/char/ipmi/ipmi_watchdog.c |    2 +-
 drivers/watchdog/hpwdt.c          |    2 +-
 6 files changed, 43 insertions(+), 43 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index e2513f2..8ed55be 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1245,7 +1245,6 @@ perf_event_nmi_handler(struct notifier_block *self,
 		return NOTIFY_DONE;
 
 	switch (cmd) {
-	case DIE_NMI:
 	case DIE_NMI_IPI:
 		break;
 	case DIE_NMIUNKNOWN:
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index bef859e..d8acab3 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -385,53 +385,55 @@ static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
 	unsigned char reason = 0;
 	int cpu;
 
-	cpu = smp_processor_id();
+	/*
+	 * CPU-specific NMI must be processed before non-CPU-specific
+	 * NMI, otherwise we may lose it, because the CPU-specific
+	 * NMI can not be detected/processed on other CPUs.
+	 */
+
+	/*
+	 * CPU-specific NMI: send to specific CPU or NMI sources must
+	 * be processed on specific CPU
+	 */
+	if (notify_die(DIE_NMI_IPI, "nmi_ipi", regs, 0, 2, SIGINT)
+	    == NOTIFY_STOP)
+		return;
 
+	/* Non-CPU-specific NMI: NMI sources can be processed on any CPU */
+	cpu = smp_processor_id();
 	/* Only the BSP gets external NMIs from the system. */
-	if (!cpu)
+	if (!cpu) {
 		reason = get_nmi_reason();
-
-	if (!(reason & NMI_REASON_MASK)) {
-		if (notify_die(DIE_NMI_IPI, "nmi_ipi", regs, reason, 2, SIGINT)
-								== NOTIFY_STOP)
-			return;
-
-#ifdef CONFIG_X86_LOCAL_APIC
-		if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT)
-							== NOTIFY_STOP)
+		if (reason & NMI_REASON_MASK) {
+			if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT)
+			    == NOTIFY_STOP)
+				return;
+			if (reason & NMI_REASON_SERR)
+				pci_serr_error(reason, regs);
+			else if (reason & NMI_REASON_IOCHK)
+				io_check_error(reason, regs);
+#ifdef CONFIG_X86_32
+			/*
+			 * Reassert NMI in case it became active
+			 * meanwhile as it's edge-triggered:
+			 */
+			reassert_nmi();
+#endif
 			return;
+		}
+	}
 
-#ifndef CONFIG_LOCKUP_DETECTOR
-		/*
-		 * Ok, so this is none of the documented NMI sources,
-		 * so it must be the NMI watchdog.
-		 */
-		if (nmi_watchdog_tick(regs, reason))
-			return;
-		if (!do_nmi_callback(regs, cpu))
-#endif /* !CONFIG_LOCKUP_DETECTOR */
-			unknown_nmi_error(reason, regs);
-#else
-		unknown_nmi_error(reason, regs);
-#endif
+	if (notify_die(DIE_NMI, "nmi", regs, 0, 2, SIGINT) == NOTIFY_STOP)
+		return;
 
+#if defined(CONFIG_X86_LOCAL_APIC) && !defined(CONFIG_LOCKUP_DETECTOR)
+	if (nmi_watchdog_tick(regs, reason))
 		return;
-	}
-	if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT) == NOTIFY_STOP)
+	if (do_nmi_callback(regs, cpu))
 		return;
-
-	/* AK: following checks seem to be broken on modern chipsets. FIXME */
-	if (reason & NMI_REASON_SERR)
-		pci_serr_error(reason, regs);
-	if (reason & NMI_REASON_IOCHK)
-		io_check_error(reason, regs);
-#ifdef CONFIG_X86_32
-	/*
-	 * Reassert NMI in case it became active meanwhile
-	 * as it's edge-triggered:
-	 */
-	reassert_nmi();
 #endif
+
+	unknown_nmi_error(reason, regs);
 }
 
 dotraplinkage notrace __kprobes void
diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c
index f1575c9..69f2453 100644
--- a/arch/x86/oprofile/nmi_int.c
+++ b/arch/x86/oprofile/nmi_int.c
@@ -64,7 +64,6 @@ static int profile_exceptions_notify(struct notifier_block *self,
 	int ret = NOTIFY_DONE;
 
 	switch (val) {
-	case DIE_NMI:
 	case DIE_NMI_IPI:
 		if (ctr_running)
 			model->check_ctrs(args->regs, &__get_cpu_var(cpu_msrs));
diff --git a/arch/x86/oprofile/nmi_timer_int.c b/arch/x86/oprofile/nmi_timer_int.c
index e3ecb71..ab72a21 100644
--- a/arch/x86/oprofile/nmi_timer_int.c
+++ b/arch/x86/oprofile/nmi_timer_int.c
@@ -25,7 +25,7 @@ static int profile_timer_exceptions_notify(struct notifier_block *self,
 	int ret = NOTIFY_DONE;
 
 	switch (val) {
-	case DIE_NMI:
+	case DIE_NMI_IPI:
 		oprofile_add_sample(args->regs, 0);
 		ret = NOTIFY_STOP;
 		break;
diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/char/ipmi/ipmi_watchdog.c
index 654d566..6a273fb 100644
--- a/drivers/char/ipmi/ipmi_watchdog.c
+++ b/drivers/char/ipmi/ipmi_watchdog.c
@@ -1080,7 +1080,7 @@ ipmi_nmi(struct notifier_block *self, unsigned long val, void *data)
 {
 	struct die_args *args = data;
 
-	if (val != DIE_NMI)
+	if (val != DIE_NMIUNKNOWN)
 		return NOTIFY_OK;
 
 	/* Hack, if it's a memory or I/O error, ignore it. */
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 3d77116..d8010cc 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -469,7 +469,7 @@ static int hpwdt_pretimeout(struct notifier_block *nb, unsigned long ulReason,
 	unsigned long rom_pl;
 	static int die_nmi_called;
 
-	if (ulReason != DIE_NMI && ulReason != DIE_NMI_IPI)
+	if (ulReason != DIE_NMIUNKNOWN)
 		goto out;
 
 	if (!hpwdt_nmi_decoding)

Re: [tip:perf/core] x86, NMI: Rewrite NMI handler

[Peter Zijlstra]

On Sat, 2010-10-16 at 16:36 +0000, tip-bot for Huang Ying wrote:
> Commit-ID:  e21f3e4957f4a55f546873605a5caa59556bb144
> Gitweb:     http://git.kernel.org/tip/e21f3e4957f4a55f546873605a5caa59556bb144
> Author:     Huang Ying <ying.huang@intel.com>
> AuthorDate: Fri, 15 Oct 2010 22:22:16 -0400
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Sat, 16 Oct 2010 15:01:27 +0200
> 
> x86, NMI: Rewrite NMI handler
> 
> The original NMI handler is quite outdated in many aspects. This
> patch tries to fix it.
> 
> The order to process the NMI sources are changed as follow:
> 
>  notify_die(DIE_NMI_IPI);
>  notify_die(DIE_NMI);
>  /* process io port 0x61 */
>  nmi_watchdog_touch();
>  unknown_nmi();

NACK!

I object to the DIE_NMI_IPI existance..

Re: [tip:perf/core] x86, NMI: Rewrite NMI handler

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Sat, 2010-10-16 at 16:36 +0000, tip-bot for Huang Ying wrote:
> > Commit-ID:  e21f3e4957f4a55f546873605a5caa59556bb144
> > Gitweb:     http://git.kernel.org/tip/e21f3e4957f4a55f546873605a5caa59556bb144
> > Author:     Huang Ying <ying.huang@intel.com>
> > AuthorDate: Fri, 15 Oct 2010 22:22:16 -0400
> > Committer:  Ingo Molnar <mingo@elte.hu>
> > CommitDate: Sat, 16 Oct 2010 15:01:27 +0200
> > 
> > x86, NMI: Rewrite NMI handler
> > 
> > The original NMI handler is quite outdated in many aspects. This
> > patch tries to fix it.
> > 
> > The order to process the NMI sources are changed as follow:
> > 
> >  notify_die(DIE_NMI_IPI);
> >  notify_die(DIE_NMI);
> >  /* process io port 0x61 */
> >  nmi_watchdog_touch();
> >  unknown_nmi();
> 
> NACK!
> 
> I object to the DIE_NMI_IPI existance..

Ok - i've zapped the commits for the time being.

Thanks,

	Ingo

Re: [tip:perf/core] x86, NMI: Rewrite NMI handler

[Anca Emanuel]

>> NACK!
>>
>> I object to the DIE_NMI_IPI existance..
>
> Ok - i've zapped the commits for the time being.

Hi, Peter.

If you have any ideas how things can be better, I want to see them.
Some code please.

Re: [tip:perf/core] x86, NMI: Rewrite NMI handler

[Don Zickus]

On Sat, Oct 16, 2010 at 07:29:17PM +0200, Peter Zijlstra wrote:
> On Sat, 2010-10-16 at 16:36 +0000, tip-bot for Huang Ying wrote:
> > Commit-ID:  e21f3e4957f4a55f546873605a5caa59556bb144
> > Gitweb:     http://git.kernel.org/tip/e21f3e4957f4a55f546873605a5caa59556bb144
> > Author:     Huang Ying <ying.huang@intel.com>
> > AuthorDate: Fri, 15 Oct 2010 22:22:16 -0400
> > Committer:  Ingo Molnar <mingo@elte.hu>
> > CommitDate: Sat, 16 Oct 2010 15:01:27 +0200
> > 
> > x86, NMI: Rewrite NMI handler
> > 
> > The original NMI handler is quite outdated in many aspects. This
> > patch tries to fix it.
> > 
> > The order to process the NMI sources are changed as follow:
> > 
> >  notify_die(DIE_NMI_IPI);
> >  notify_die(DIE_NMI);
> >  /* process io port 0x61 */
> >  nmi_watchdog_touch();
> >  unknown_nmi();
> 
> NACK!
> 
> I object to the DIE_NMI_IPI existance..

Hmm, well to his defense, he isn't adding DIE_NMI_IPI.  That code is
already there.  He is just re-arranging it, making it look painfully
obvious the die_chain probably isn't as efficient as it should be.

I think a bunch of us agree that we need to revamp the NMI notifier to
make it less wasteful.  I was hoping we could do that in a separate patch
that would be layered on top of Huangs.

Would you object to at least consider having this patch series in a
work-in-progress git branch that we can build on top of, with the final
outcome containing an nmi notifier that meets your expectations?

Cheers,
Don

Re: [tip:perf/core] x86, NMI: Rewrite NMI handler

[Peter Zijlstra]

On Sat, 2010-10-16 at 20:46 -0400, Don Zickus wrote:

> Hmm, well to his defense, he isn't adding DIE_NMI_IPI.  That code is
> already there.  He is just re-arranging it, making it look painfully
> obvious the die_chain probably isn't as efficient as it should be.
> 
> I think a bunch of us agree that we need to revamp the NMI notifier to
> make it less wasteful.  I was hoping we could do that in a separate patch
> that would be layered on top of Huangs.
> 
> Would you object to at least consider having this patch series in a
> work-in-progress git branch that we can build on top of, with the final
> outcome containing an nmi notifier that meets your expectations?

What's the point of keeping it in a git tree? Its just a few patches.

We've spend more time talking about this than it would take to actually
do the patch that kills DIE_NMI_IPI.

Re: [tip:perf/core] x86, NMI: Rewrite NMI handler

[Huang Ying]

On Sun, 2010-10-17 at 18:42 +0800, Peter Zijlstra wrote:
> On Sat, 2010-10-16 at 20:46 -0400, Don Zickus wrote:
> 
> > Hmm, well to his defense, he isn't adding DIE_NMI_IPI.  That code is
> > already there.  He is just re-arranging it, making it look painfully
> > obvious the die_chain probably isn't as efficient as it should be.
> > 
> > I think a bunch of us agree that we need to revamp the NMI notifier to
> > make it less wasteful.  I was hoping we could do that in a separate patch
> > that would be layered on top of Huangs.
> > 
> > Would you object to at least consider having this patch series in a
> > work-in-progress git branch that we can build on top of, with the final
> > outcome containing an nmi notifier that meets your expectations?
> 
> What's the point of keeping it in a git tree? Its just a few patches.
> 
> We've spend more time talking about this than it would take to actually
> do the patch that kills DIE_NMI_IPI.

You can kill DIE_NMI_IPI. But you will add at least NMI_PRIORITY_LOCAL
(like DIE_NMI_IPI), NMI_PRIORITY_GLOBAL (like original DIE_NMI). So
thing is similar, just moved from one place to another place.

Best Regards,
Huang Ying

Re: [tip:perf/core] x86, NMI: Rewrite NMI handler

[Peter Zijlstra]

On Mon, 2010-10-18 at 11:06 +0800, Huang Ying wrote:
> You can kill DIE_NMI_IPI. But you will add at least NMI_PRIORITY_LOCAL
> (like DIE_NMI_IPI), NMI_PRIORITY_GLOBAL (like original DIE_NMI). So
> thing is similar, just moved from one place to another place.

The nice thing is that notifier chains aren't limited to 2 priorities,
in fact there's a good argument to use a wider range.

Also, like previously mentioned, it gets rid of the double iteration.

[PATCH 4/5] x86, NMI: Allow NMI reason io port (0x61) to be processed on any CPU

[Don Zickus]

From: Huang Ying <ying.huang@intel.com>

In original NMI handler, NMI reason io port (0x61) is only processed
on BSP. This makes it impossible to hot-remove BSP. To solve the
issue, a raw spinlock is used to make the port can be processed on any
CPU.

Signed-off-by: Huang Ying <ying.huang@intel.com>
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 arch/x86/kernel/traps.c |   45 +++++++++++++++++++++++++--------------------
 1 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index d8acab3..accb2f4 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -83,6 +83,12 @@ EXPORT_SYMBOL_GPL(used_vectors);
 
 static int ignore_nmis;
 
+/*
+ * Prevent NMI reason port (0x61) being accessed simultaneously, can
+ * only be used in NMI handler.
+ */
+static DEFINE_RAW_SPINLOCK(nmi_reason_lock);
+
 static inline void conditional_sti(struct pt_regs *regs)
 {
 	if (regs->flags & X86_EFLAGS_IF)
@@ -383,7 +389,6 @@ unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
 static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
 {
 	unsigned char reason = 0;
-	int cpu;
 
 	/*
 	 * CPU-specific NMI must be processed before non-CPU-specific
@@ -400,28 +405,28 @@ static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
 		return;
 
 	/* Non-CPU-specific NMI: NMI sources can be processed on any CPU */
-	cpu = smp_processor_id();
-	/* Only the BSP gets external NMIs from the system. */
-	if (!cpu) {
-		reason = get_nmi_reason();
-		if (reason & NMI_REASON_MASK) {
-			if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT)
-			    == NOTIFY_STOP)
-				return;
-			if (reason & NMI_REASON_SERR)
-				pci_serr_error(reason, regs);
-			else if (reason & NMI_REASON_IOCHK)
-				io_check_error(reason, regs);
+	raw_spin_lock(&nmi_reason_lock);
+	reason = get_nmi_reason();
+	if (reason & NMI_REASON_MASK) {
+		if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT)
+		    == NOTIFY_STOP)
+			goto unlock_return;
+		if (reason & NMI_REASON_SERR)
+			pci_serr_error(reason, regs);
+		else if (reason & NMI_REASON_IOCHK)
+			io_check_error(reason, regs);
 #ifdef CONFIG_X86_32
-			/*
-			 * Reassert NMI in case it became active
-			 * meanwhile as it's edge-triggered:
-			 */
-			reassert_nmi();
+		/*
+		 * Reassert NMI in case it became active
+		 * meanwhile as it's edge-triggered:
+		 */
+		reassert_nmi();
 #endif
-			return;
-		}
+unlock_return:
+		raw_spin_unlock(&nmi_reason_lock);
+		return;
 	}
+	raw_spin_unlock(&nmi_reason_lock);
 
 	if (notify_die(DIE_NMI, "nmi", regs, 0, 2, SIGINT) == NOTIFY_STOP)
 		return;
-- 
1.7.2.3

[tip:perf/core] x86, NMI: Allow NMI reason io port (0x61) to be processed on any CPU

[tip-bot for Huang Ying]

Commit-ID:  4a45b5a06f61b46e7bce1aaf9331e2dc58704fa5
Gitweb:     http://git.kernel.org/tip/4a45b5a06f61b46e7bce1aaf9331e2dc58704fa5
Author:     Huang Ying <ying.huang@intel.com>
AuthorDate: Fri, 15 Oct 2010 22:22:17 -0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sat, 16 Oct 2010 15:01:28 +0200

x86, NMI: Allow NMI reason io port (0x61) to be processed on any CPU

In original NMI handler, NMI reason io port (0x61) is only
processed on BSP. This makes it impossible to hot-remove BSP. To
solve the issue, a raw spinlock is used to make the port can be
processed on any CPU.

Signed-off-by: Huang Ying <ying.huang@intel.com>
Signed-off-by: Don Zickus <dzickus@redhat.com>
Cc: andi@firstfloor.org
Cc: robert.richter@amd.com
Cc: peterz@infradead.org
LKML-Reference: <1287195738-3136-5-git-send-email-dzickus@redhat.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/traps.c |   45 +++++++++++++++++++++++++--------------------
 1 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index d8acab3..accb2f4 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -83,6 +83,12 @@ EXPORT_SYMBOL_GPL(used_vectors);
 
 static int ignore_nmis;
 
+/*
+ * Prevent NMI reason port (0x61) being accessed simultaneously, can
+ * only be used in NMI handler.
+ */
+static DEFINE_RAW_SPINLOCK(nmi_reason_lock);
+
 static inline void conditional_sti(struct pt_regs *regs)
 {
 	if (regs->flags & X86_EFLAGS_IF)
@@ -383,7 +389,6 @@ unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
 static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
 {
 	unsigned char reason = 0;
-	int cpu;
 
 	/*
 	 * CPU-specific NMI must be processed before non-CPU-specific
@@ -400,28 +405,28 @@ static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
 		return;
 
 	/* Non-CPU-specific NMI: NMI sources can be processed on any CPU */
-	cpu = smp_processor_id();
-	/* Only the BSP gets external NMIs from the system. */
-	if (!cpu) {
-		reason = get_nmi_reason();
-		if (reason & NMI_REASON_MASK) {
-			if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT)
-			    == NOTIFY_STOP)
-				return;
-			if (reason & NMI_REASON_SERR)
-				pci_serr_error(reason, regs);
-			else if (reason & NMI_REASON_IOCHK)
-				io_check_error(reason, regs);
+	raw_spin_lock(&nmi_reason_lock);
+	reason = get_nmi_reason();
+	if (reason & NMI_REASON_MASK) {
+		if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT)
+		    == NOTIFY_STOP)
+			goto unlock_return;
+		if (reason & NMI_REASON_SERR)
+			pci_serr_error(reason, regs);
+		else if (reason & NMI_REASON_IOCHK)
+			io_check_error(reason, regs);
 #ifdef CONFIG_X86_32
-			/*
-			 * Reassert NMI in case it became active
-			 * meanwhile as it's edge-triggered:
-			 */
-			reassert_nmi();
+		/*
+		 * Reassert NMI in case it became active
+		 * meanwhile as it's edge-triggered:
+		 */
+		reassert_nmi();
 #endif
-			return;
-		}
+unlock_return:
+		raw_spin_unlock(&nmi_reason_lock);
+		return;
 	}
+	raw_spin_unlock(&nmi_reason_lock);
 
 	if (notify_die(DIE_NMI, "nmi", regs, 0, 2, SIGINT) == NOTIFY_STOP)
 		return;

Re: [PATCH 4/5] x86, NMI: Allow NMI reason io port (0x61) to be processed on any CPU

[Robert Richter]

On 15.10.10 22:22:17, Don Zickus wrote:
> From: Huang Ying <ying.huang@intel.com>
> 
> In original NMI handler, NMI reason io port (0x61) is only processed
> on BSP. This makes it impossible to hot-remove BSP. To solve the
> issue, a raw spinlock is used to make the port can be processed on any
> CPU.
> 
> Signed-off-by: Huang Ying <ying.huang@intel.com>
> Signed-off-by: Don Zickus <dzickus@redhat.com>
> ---
>  arch/x86/kernel/traps.c |   45 +++++++++++++++++++++++++--------------------
>  1 files changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index d8acab3..accb2f4 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -83,6 +83,12 @@ EXPORT_SYMBOL_GPL(used_vectors);
>  
>  static int ignore_nmis;
>  
> +/*
> + * Prevent NMI reason port (0x61) being accessed simultaneously, can
> + * only be used in NMI handler.
> + */
> +static DEFINE_RAW_SPINLOCK(nmi_reason_lock);
> +
>  static inline void conditional_sti(struct pt_regs *regs)
>  {
>  	if (regs->flags & X86_EFLAGS_IF)
> @@ -383,7 +389,6 @@ unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
>  static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
>  {
>  	unsigned char reason = 0;
> -	int cpu;
>  
>  	/*
>  	 * CPU-specific NMI must be processed before non-CPU-specific
> @@ -400,28 +405,28 @@ static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
>  		return;
>  
>  	/* Non-CPU-specific NMI: NMI sources can be processed on any CPU */
> -	cpu = smp_processor_id();
> -	/* Only the BSP gets external NMIs from the system. */
> -	if (!cpu) {
> -		reason = get_nmi_reason();
> -		if (reason & NMI_REASON_MASK) {
> -			if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT)
> -			    == NOTIFY_STOP)
> -				return;
> -			if (reason & NMI_REASON_SERR)
> -				pci_serr_error(reason, regs);
> -			else if (reason & NMI_REASON_IOCHK)
> -				io_check_error(reason, regs);
> +	raw_spin_lock(&nmi_reason_lock);

What about using raw_spin_trylock() instead? We don't have to wait
here since we are already processing it by another cpu.

-Robert

> +	reason = get_nmi_reason();
> +	if (reason & NMI_REASON_MASK) {
> +		if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT)
> +		    == NOTIFY_STOP)
> +			goto unlock_return;
> +		if (reason & NMI_REASON_SERR)
> +			pci_serr_error(reason, regs);
> +		else if (reason & NMI_REASON_IOCHK)
> +			io_check_error(reason, regs);
>  #ifdef CONFIG_X86_32
> -			/*
> -			 * Reassert NMI in case it became active
> -			 * meanwhile as it's edge-triggered:
> -			 */
> -			reassert_nmi();
> +		/*
> +		 * Reassert NMI in case it became active
> +		 * meanwhile as it's edge-triggered:
> +		 */
> +		reassert_nmi();
>  #endif
> -			return;
> -		}
> +unlock_return:
> +		raw_spin_unlock(&nmi_reason_lock);
> +		return;
>  	}
> +	raw_spin_unlock(&nmi_reason_lock);
>  
>  	if (notify_die(DIE_NMI, "nmi", regs, 0, 2, SIGINT) == NOTIFY_STOP)
>  		return;
> -- 
> 1.7.2.3
> 
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 4/5] x86, NMI: Allow NMI reason io port (0x61) to be processed on any CPU

[Robert Richter]

On 19.10.10 17:07:01, Robert Richter wrote:
> On 15.10.10 22:22:17, Don Zickus wrote:
> > From: Huang Ying <ying.huang@intel.com>
> > 
> > In original NMI handler, NMI reason io port (0x61) is only processed
> > on BSP. This makes it impossible to hot-remove BSP. To solve the
> > issue, a raw spinlock is used to make the port can be processed on any
> > CPU.
> > 
> > Signed-off-by: Huang Ying <ying.huang@intel.com>
> > Signed-off-by: Don Zickus <dzickus@redhat.com>
> > ---
> >  arch/x86/kernel/traps.c |   45 +++++++++++++++++++++++++--------------------
> >  1 files changed, 25 insertions(+), 20 deletions(-)

> > @@ -400,28 +405,28 @@ static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
> >  		return;
> >  
> >  	/* Non-CPU-specific NMI: NMI sources can be processed on any CPU */
> > -	cpu = smp_processor_id();
> > -	/* Only the BSP gets external NMIs from the system. */
> > -	if (!cpu) {
> > -		reason = get_nmi_reason();
> > -		if (reason & NMI_REASON_MASK) {
> > -			if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT)
> > -			    == NOTIFY_STOP)
> > -				return;
> > -			if (reason & NMI_REASON_SERR)
> > -				pci_serr_error(reason, regs);
> > -			else if (reason & NMI_REASON_IOCHK)
> > -				io_check_error(reason, regs);
> > +	raw_spin_lock(&nmi_reason_lock);
> 
> What about using raw_spin_trylock() instead? We don't have to wait
> here since we are already processing it by another cpu.

This would avoid a global lock and also deadlocking in case of a
potential #gp in the nmi handler.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 4/5] x86, NMI: Allow NMI reason io port (0x61) to be processed on any CPU

[Don Zickus]

On Tue, Oct 19, 2010 at 06:25:07PM +0200, Robert Richter wrote:
> On 19.10.10 17:07:01, Robert Richter wrote:
> > On 15.10.10 22:22:17, Don Zickus wrote:
> > > From: Huang Ying <ying.huang@intel.com>
> > > 
> > > In original NMI handler, NMI reason io port (0x61) is only processed
> > > on BSP. This makes it impossible to hot-remove BSP. To solve the
> > > issue, a raw spinlock is used to make the port can be processed on any
> > > CPU.
> > > 
> > > Signed-off-by: Huang Ying <ying.huang@intel.com>
> > > Signed-off-by: Don Zickus <dzickus@redhat.com>
> > > ---
> > >  arch/x86/kernel/traps.c |   45 +++++++++++++++++++++++++--------------------
> > >  1 files changed, 25 insertions(+), 20 deletions(-)
> 
> > > @@ -400,28 +405,28 @@ static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
> > >  		return;
> > >  
> > >  	/* Non-CPU-specific NMI: NMI sources can be processed on any CPU */
> > > -	cpu = smp_processor_id();
> > > -	/* Only the BSP gets external NMIs from the system. */
> > > -	if (!cpu) {
> > > -		reason = get_nmi_reason();
> > > -		if (reason & NMI_REASON_MASK) {
> > > -			if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT)
> > > -			    == NOTIFY_STOP)
> > > -				return;
> > > -			if (reason & NMI_REASON_SERR)
> > > -				pci_serr_error(reason, regs);
> > > -			else if (reason & NMI_REASON_IOCHK)
> > > -				io_check_error(reason, regs);
> > > +	raw_spin_lock(&nmi_reason_lock);
> > 
> > What about using raw_spin_trylock() instead? We don't have to wait
> > here since we are already processing it by another cpu.
> 
> This would avoid a global lock and also deadlocking in case of a
> potential #gp in the nmi handler.

I would feel more comfortable with it too.  I can't find a reason where
trylock would do harm.

Cheers,
Don

Re: [PATCH 4/5] x86, NMI: Allow NMI reason io port (0x61) to be processed on any CPU

[Huang Ying]

On Wed, 2010-10-20 at 02:37 +0800, Don Zickus wrote:
> On Tue, Oct 19, 2010 at 06:25:07PM +0200, Robert Richter wrote:
> > On 19.10.10 17:07:01, Robert Richter wrote:
> > > On 15.10.10 22:22:17, Don Zickus wrote:
> > > > From: Huang Ying <ying.huang@intel.com>
> > > > 
> > > > In original NMI handler, NMI reason io port (0x61) is only processed
> > > > on BSP. This makes it impossible to hot-remove BSP. To solve the
> > > > issue, a raw spinlock is used to make the port can be processed on any
> > > > CPU.
> > > > 
> > > > Signed-off-by: Huang Ying <ying.huang@intel.com>
> > > > Signed-off-by: Don Zickus <dzickus@redhat.com>
> > > > ---
> > > >  arch/x86/kernel/traps.c |   45 +++++++++++++++++++++++++--------------------
> > > >  1 files changed, 25 insertions(+), 20 deletions(-)
> > 
> > > > @@ -400,28 +405,28 @@ static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
> > > >  		return;
> > > >  
> > > >  	/* Non-CPU-specific NMI: NMI sources can be processed on any CPU */
> > > > -	cpu = smp_processor_id();
> > > > -	/* Only the BSP gets external NMIs from the system. */
> > > > -	if (!cpu) {
> > > > -		reason = get_nmi_reason();
> > > > -		if (reason & NMI_REASON_MASK) {
> > > > -			if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT)
> > > > -			    == NOTIFY_STOP)
> > > > -				return;
> > > > -			if (reason & NMI_REASON_SERR)
> > > > -				pci_serr_error(reason, regs);
> > > > -			else if (reason & NMI_REASON_IOCHK)
> > > > -				io_check_error(reason, regs);
> > > > +	raw_spin_lock(&nmi_reason_lock);
> > > 
> > > What about using raw_spin_trylock() instead? We don't have to wait
> > > here since we are already processing it by another cpu.
> > 
> > This would avoid a global lock and also deadlocking in case of a
> > potential #gp in the nmi handler.
> 
> I would feel more comfortable with it too.  I can't find a reason where
> trylock would do harm.

One possible issue can be as follow:

- PCI SERR NMI raised on CPU 0
- IOCHK NMI raised on CPU 1

If we use try lock, we may get unknown NMI on one CPU. Do you guys think
so?

Best Regards,
Huang Ying

Re: [PATCH 4/5] x86, NMI: Allow NMI reason io port (0x61) to be processed on any CPU

[Robert Richter]

On 19.10.10 20:23:12, Huang Ying wrote:
> On Wed, 2010-10-20 at 02:37 +0800, Don Zickus wrote:
> > On Tue, Oct 19, 2010 at 06:25:07PM +0200, Robert Richter wrote:
> > > On 19.10.10 17:07:01, Robert Richter wrote:
> > > > On 15.10.10 22:22:17, Don Zickus wrote:
> > > > > From: Huang Ying <ying.huang@intel.com>
> > > > > 
> > > > > In original NMI handler, NMI reason io port (0x61) is only processed
> > > > > on BSP. This makes it impossible to hot-remove BSP. To solve the
> > > > > issue, a raw spinlock is used to make the port can be processed on any
> > > > > CPU.
> > > > > 
> > > > > Signed-off-by: Huang Ying <ying.huang@intel.com>
> > > > > Signed-off-by: Don Zickus <dzickus@redhat.com>
> > > > > ---
> > > > >  arch/x86/kernel/traps.c |   45 +++++++++++++++++++++++++--------------------
> > > > >  1 files changed, 25 insertions(+), 20 deletions(-)
> > > 
> > > > > @@ -400,28 +405,28 @@ static notrace __kprobes void default_do_nmi(struct pt_regs *regs)
> > > > >  		return;
> > > > >  
> > > > >  	/* Non-CPU-specific NMI: NMI sources can be processed on any CPU */
> > > > > -	cpu = smp_processor_id();
> > > > > -	/* Only the BSP gets external NMIs from the system. */
> > > > > -	if (!cpu) {
> > > > > -		reason = get_nmi_reason();
> > > > > -		if (reason & NMI_REASON_MASK) {
> > > > > -			if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT)
> > > > > -			    == NOTIFY_STOP)
> > > > > -				return;
> > > > > -			if (reason & NMI_REASON_SERR)
> > > > > -				pci_serr_error(reason, regs);
> > > > > -			else if (reason & NMI_REASON_IOCHK)
> > > > > -				io_check_error(reason, regs);
> > > > > +	raw_spin_lock(&nmi_reason_lock);
> > > > 
> > > > What about using raw_spin_trylock() instead? We don't have to wait
> > > > here since we are already processing it by another cpu.
> > > 
> > > This would avoid a global lock and also deadlocking in case of a
> > > potential #gp in the nmi handler.
> > 
> > I would feel more comfortable with it too.  I can't find a reason where
> > trylock would do harm.
> 
> One possible issue can be as follow:
> 
> - PCI SERR NMI raised on CPU 0
> - IOCHK NMI raised on CPU 1
> 
> If we use try lock, we may get unknown NMI on one CPU. Do you guys think
> so?

This could be a valid point. On the other side the former
implementation to let only handle cpu #0 i/o interrupts didn't trigger
unknown nmis, so try_lock wouldn't change much compared to this. To be
sure we might do a NOTIFY_STOP in the unknown path if we don't get the
lock.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 4/5] x86, NMI: Allow NMI reason io port (0x61) to be processed on any CPU

[Huang Ying]

On Wed, 2010-10-20 at 18:03 +0800, Robert Richter wrote:
> > > > > What about using raw_spin_trylock() instead? We don't have to wait
> > > > > here since we are already processing it by another cpu.
> > > > 
> > > > This would avoid a global lock and also deadlocking in case of a
> > > > potential #gp in the nmi handler.
> > > 
> > > I would feel more comfortable with it too.  I can't find a reason where
> > > trylock would do harm.
> > 
> > One possible issue can be as follow:
> > 
> > - PCI SERR NMI raised on CPU 0
> > - IOCHK NMI raised on CPU 1
> > 
> > If we use try lock, we may get unknown NMI on one CPU. Do you guys think
> > so?
> 
> This could be a valid point. On the other side the former
> implementation to let only handle cpu #0 i/o interrupts didn't trigger
> unknown nmis, so try_lock wouldn't change much compared to this.

Because we want to support BSP hot-remove, these NMIs may be redirected
to other CPUs. I think it may be possible after we hot-remove the BSP
PCI SERR NMI is routed to CPU 1, while IOCHK NMI is routed to CPU 2. The
raw_spin_lock() here is for that.

Best Regards,
Huang Ying

Re: [PATCH 4/5] x86, NMI: Allow NMI reason io port (0x61) to be processed on any CPU

[Don Zickus]

On Wed, Oct 20, 2010 at 08:23:12AM +0800, Huang Ying wrote:
> > > > What about using raw_spin_trylock() instead? We don't have to wait
> > > > here since we are already processing it by another cpu.
> > > 
> > > This would avoid a global lock and also deadlocking in case of a
> > > potential #gp in the nmi handler.
> > 
> > I would feel more comfortable with it too.  I can't find a reason where
> > trylock would do harm.
> 
> One possible issue can be as follow:
> 
> - PCI SERR NMI raised on CPU 0
> - IOCHK NMI raised on CPU 1
> 
> If we use try lock, we may get unknown NMI on one CPU. Do you guys think
> so?

I thought both PCI SERR and IOCK NMI's were external and routed through
the IOAPIC, which means only one cpu could receive those (unless the
IOAPIC was updated to route them elsewhere).  This would make the issue
moot.  Unless I am misunderstanding where those NMIs come from?

Also as Robert said, we used to handle them on the bsp cpu only before
without any issues.  I believed that was because everything in the IOAPIC
was routed that way.

I thought the point of this patch was to remove that restriction in the
nmi handler, which would allow future patches to re-route these NMIs to
another cpu, thus finally allowing people to hot-remove the bsp cpu, no?

Cheers,
Don

Re: [PATCH 4/5] x86, NMI: Allow NMI reason io port (0x61) to be processed on any CPU

[Huang Ying]

On Wed, 2010-10-20 at 22:27 +0800, Don Zickus wrote:
> On Wed, Oct 20, 2010 at 08:23:12AM +0800, Huang Ying wrote:
> > > > > What about using raw_spin_trylock() instead? We don't have to wait
> > > > > here since we are already processing it by another cpu.
> > > > 
> > > > This would avoid a global lock and also deadlocking in case of a
> > > > potential #gp in the nmi handler.
> > > 
> > > I would feel more comfortable with it too.  I can't find a reason where
> > > trylock would do harm.
> > 
> > One possible issue can be as follow:
> > 
> > - PCI SERR NMI raised on CPU 0
> > - IOCHK NMI raised on CPU 1
> > 
> > If we use try lock, we may get unknown NMI on one CPU. Do you guys think
> > so?
> 
> I thought both PCI SERR and IOCK NMI's were external and routed through
> the IOAPIC, which means only one cpu could receive those (unless the
> IOAPIC was updated to route them elsewhere).  This would make the issue
> moot.  Unless I am misunderstanding where those NMIs come from?
> 
> Also as Robert said, we used to handle them on the bsp cpu only before
> without any issues.  I believed that was because everything in the IOAPIC
> was routed that way.
> 
> I thought the point of this patch was to remove that restriction in the
> nmi handler, which would allow future patches to re-route these NMIs to
> another cpu, thus finally allowing people to hot-remove the bsp cpu, no?

Yes. We just want to make it possible to hot-remove the bsp cpu. Because
IOAPIC is configurable, I think it is possible to configure IOAPIC to
send PCI SERR NMI to one CPU while IOCK NMI to another CPU. Why not
support this situation too? It does not harm anything but performance to
use raw_spin_lock() instead of raw_spin_trylock() here. And for hardware
error processing, performance is not so important in fact.

Best Regards,
Huang Ying

Re: [PATCH 4/5] x86, NMI: Allow NMI reason io port (0x61) to be processed on any CPU

[Don Zickus]

On Thu, Oct 21, 2010 at 08:40:07AM +0800, Huang Ying wrote:
> On Wed, 2010-10-20 at 22:27 +0800, Don Zickus wrote:
> > I thought the point of this patch was to remove that restriction in the
> > nmi handler, which would allow future patches to re-route these NMIs to
> > another cpu, thus finally allowing people to hot-remove the bsp cpu, no?
> 
> Yes. We just want to make it possible to hot-remove the bsp cpu. Because
> IOAPIC is configurable, I think it is possible to configure IOAPIC to
> send PCI SERR NMI to one CPU while IOCK NMI to another CPU. Why not
> support this situation too? It does not harm anything but performance to

Why would we want to?  It seems simpler to have one cpu dedicated to
handling the external NMIs.

> use raw_spin_lock() instead of raw_spin_trylock() here. And for hardware
> error processing, performance is not so important in fact.

I don't know.  I was always a little uncomfortable with a spin_lock there,
so I am more supportive of a trylock.

Cheers,
Don

Re: [PATCH 4/5] x86, NMI: Allow NMI reason io port (0x61) to be processed on any CPU

[Huang Ying]

On Thu, 2010-10-21 at 09:18 +0800, Don Zickus wrote:
> On Thu, Oct 21, 2010 at 08:40:07AM +0800, Huang Ying wrote:
> > On Wed, 2010-10-20 at 22:27 +0800, Don Zickus wrote:
> > > I thought the point of this patch was to remove that restriction in the
> > > nmi handler, which would allow future patches to re-route these NMIs to
> > > another cpu, thus finally allowing people to hot-remove the bsp cpu, no?
> > 
> > Yes. We just want to make it possible to hot-remove the bsp cpu. Because
> > IOAPIC is configurable, I think it is possible to configure IOAPIC to
> > send PCI SERR NMI to one CPU while IOCK NMI to another CPU. Why not
> > support this situation too? It does not harm anything but performance to
> 
> Why would we want to?  It seems simpler to have one cpu dedicated to
> handling the external NMIs.

If we can guarantee that these NMIs will be only sent to one CPU, I am
fine with trylock.

> > use raw_spin_lock() instead of raw_spin_trylock() here. And for hardware
> > error processing, performance is not so important in fact.
> 
> I don't know.  I was always a little uncomfortable with a spin_lock there,
> so I am more supportive of a trylock.

Best Regards,
Huang Ying

Re: [PATCH 4/5] x86, NMI: Allow NMI reason io port (0x61) to be processed on any CPU

[Don Zickus]

On Thu, Oct 21, 2010 at 09:25:09AM +0800, Huang Ying wrote:
> On Thu, 2010-10-21 at 09:18 +0800, Don Zickus wrote:
> > On Thu, Oct 21, 2010 at 08:40:07AM +0800, Huang Ying wrote:
> > > On Wed, 2010-10-20 at 22:27 +0800, Don Zickus wrote:
> > > > I thought the point of this patch was to remove that restriction in the
> > > > nmi handler, which would allow future patches to re-route these NMIs to
> > > > another cpu, thus finally allowing people to hot-remove the bsp cpu, no?
> > > 
> > > Yes. We just want to make it possible to hot-remove the bsp cpu. Because
> > > IOAPIC is configurable, I think it is possible to configure IOAPIC to
> > > send PCI SERR NMI to one CPU while IOCK NMI to another CPU. Why not
> > > support this situation too? It does not harm anything but performance to
> > 
> > Why would we want to?  It seems simpler to have one cpu dedicated to
> > handling the external NMIs.
> 
> If we can guarantee that these NMIs will be only sent to one CPU, I am
> fine with trylock.

I guess I assumed the BIOS would only use the bsp CPU for the NMI, which
would be passed on to the kernel.  Otherwise what happens if an NMI
happens while the kernel is still bringing up the first cpu and it is
routed to another CPU or if we boot with MAX_CPUS=1?

Then again people keep saying BIOS writers are crazy, so my assumption
could be completely wrong. ;-p

Cheers,
Don

Re: [PATCH 4/5] x86, NMI: Allow NMI reason io port (0x61) to be processed on any CPU

[Huang Ying]

On Thu, 2010-10-21 at 10:37 +0800, Don Zickus wrote:
> On Thu, Oct 21, 2010 at 09:25:09AM +0800, Huang Ying wrote:
> > On Thu, 2010-10-21 at 09:18 +0800, Don Zickus wrote:
> > > On Thu, Oct 21, 2010 at 08:40:07AM +0800, Huang Ying wrote:
> > > > On Wed, 2010-10-20 at 22:27 +0800, Don Zickus wrote:
> > > > > I thought the point of this patch was to remove that restriction in the
> > > > > nmi handler, which would allow future patches to re-route these NMIs to
> > > > > another cpu, thus finally allowing people to hot-remove the bsp cpu, no?
> > > > 
> > > > Yes. We just want to make it possible to hot-remove the bsp cpu. Because
> > > > IOAPIC is configurable, I think it is possible to configure IOAPIC to
> > > > send PCI SERR NMI to one CPU while IOCK NMI to another CPU. Why not
> > > > support this situation too? It does not harm anything but performance to
> > > 
> > > Why would we want to?  It seems simpler to have one cpu dedicated to
> > > handling the external NMIs.
> > 
> > If we can guarantee that these NMIs will be only sent to one CPU, I am
> > fine with trylock.
> 
> I guess I assumed the BIOS would only use the bsp CPU for the NMI, which
> would be passed on to the kernel.  Otherwise what happens if an NMI
> happens while the kernel is still bringing up the first cpu and it is
> routed to another CPU or if we boot with MAX_CPUS=1?

After CPU hot-remove (should go through BIOS), BIOS may route the NMI to
other CPU I think. That should have no above issues. The question here
is that, BIOS will route different NMIs to one CPU or several CPUs?

Best Regards,
Huang Ying

[PATCH 5/5] x86, NMI: Remove do_nmi_callback logic

[Don Zickus]

From: Huang Ying <ying.huang@intel.com>

do_nmi_callback related logic is removed, because it is useless, just
adds code complexity.

unknown_nmi_panic sysctl is reserved to keep kernel ABI unchanged.

Signed-off-by: Huang Ying <ying.huang@intel.com>
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 arch/x86/include/asm/nmi.h    |   10 +++++++++-
 arch/x86/kernel/apic/hw_nmi.c |    1 -
 arch/x86/kernel/apic/nmi.c    |   29 +----------------------------
 arch/x86/kernel/traps.c       |   17 +++++++++++------
 4 files changed, 21 insertions(+), 36 deletions(-)

diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
index 932f0f8..d5a5793 100644
--- a/arch/x86/include/asm/nmi.h
+++ b/arch/x86/include/asm/nmi.h
@@ -30,9 +30,17 @@ extern void setup_apic_nmi_watchdog(void *);
 extern void stop_apic_nmi_watchdog(void *);
 extern void disable_timer_nmi_watchdog(void);
 extern void enable_timer_nmi_watchdog(void);
-extern int nmi_watchdog_tick(struct pt_regs *regs, unsigned reason);
 extern void cpu_nmi_set_wd_enabled(void);
 
+#if defined(CONFIG_X86_LOCAL_APIC) && !defined(CONFIG_LOCKUP_DETECTOR)
+extern int nmi_watchdog_tick(struct pt_regs *regs);
+#else
+static inline int nmi_watchdog_tick(struct pt_regs *regs)
+{
+	return 0;
+}
+#endif
+
 extern atomic_t nmi_active;
 extern unsigned int nmi_watchdog;
 #define NMI_NONE	0
diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
index cefd694..e66b16d 100644
--- a/arch/x86/kernel/apic/hw_nmi.c
+++ b/arch/x86/kernel/apic/hw_nmi.c
@@ -100,7 +100,6 @@ void acpi_nmi_disable(void) { return; }
 #endif
 atomic_t nmi_active = ATOMIC_INIT(0);           /* oprofile uses this */
 EXPORT_SYMBOL(nmi_active);
-int unknown_nmi_panic;
 void cpu_nmi_set_wd_enabled(void) { return; }
 void stop_apic_nmi_watchdog(void *unused) { return; }
 void setup_apic_nmi_watchdog(void *unused) { return; }
diff --git a/arch/x86/kernel/apic/nmi.c b/arch/x86/kernel/apic/nmi.c
index a43f71c..f407b6b 100644
--- a/arch/x86/kernel/apic/nmi.c
+++ b/arch/x86/kernel/apic/nmi.c
@@ -37,7 +37,6 @@
 
 #include <asm/mach_traps.h>
 
-int unknown_nmi_panic;
 int nmi_watchdog_enabled;
 
 /* For reliability, we're prepared to waste bits here. */
@@ -389,7 +388,7 @@ void touch_nmi_watchdog(void)
 EXPORT_SYMBOL(touch_nmi_watchdog);
 
 notrace __kprobes int
-nmi_watchdog_tick(struct pt_regs *regs, unsigned reason)
+nmi_watchdog_tick(struct pt_regs *regs)
 {
 	/*
 	 * Since current_thread_info()-> is always on the stack, and we
@@ -483,23 +482,6 @@ static void disable_ioapic_nmi_watchdog(void)
 	on_each_cpu(stop_apic_nmi_watchdog, NULL, 1);
 }
 
-static int __init setup_unknown_nmi_panic(char *str)
-{
-	unknown_nmi_panic = 1;
-	return 1;
-}
-__setup("unknown_nmi_panic", setup_unknown_nmi_panic);
-
-static int unknown_nmi_panic_callback(struct pt_regs *regs, int cpu)
-{
-	unsigned char reason = get_nmi_reason();
-	char buf[64];
-
-	sprintf(buf, "NMI received for unknown reason %02x\n", reason);
-	die_nmi(buf, regs, 1); /* Always panic here */
-	return 0;
-}
-
 /*
  * proc handler for /proc/sys/kernel/nmi
  */
@@ -540,15 +522,6 @@ int proc_nmi_enabled(struct ctl_table *table, int write,
 
 #endif /* CONFIG_SYSCTL */
 
-int do_nmi_callback(struct pt_regs *regs, int cpu)
-{
-#ifdef CONFIG_SYSCTL
-	if (unknown_nmi_panic)
-		return unknown_nmi_panic_callback(regs, cpu);
-#endif
-	return 0;
-}
-
 void arch_trigger_all_cpu_backtrace(void)
 {
 	int i;
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index accb2f4..9ef8e15 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -83,6 +83,8 @@ EXPORT_SYMBOL_GPL(used_vectors);
 
 static int ignore_nmis;
 
+int unknown_nmi_panic;
+
 /*
  * Prevent NMI reason port (0x61) being accessed simultaneously, can
  * only be used in NMI handler.
@@ -306,6 +308,13 @@ gp_in_kernel:
 	die("general protection fault", regs, error_code);
 }
 
+static int __init setup_unknown_nmi_panic(char *str)
+{
+	unknown_nmi_panic = 1;
+	return 1;
+}
+__setup("unknown_nmi_panic", setup_unknown_nmi_panic);
+
 static notrace __kprobes void
 pci_serr_error(unsigned char reason, struct pt_regs *regs)
 {
@@ -380,7 +389,7 @@ unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
 		 reason, smp_processor_id());
 
 	pr_emerg("Do you have a strange power saving mode enabled?\n");
-	if (panic_on_unrecovered_nmi)
+	if (unknown_nmi_panic || panic_on_unrecovered_nmi)
 		panic("NMI: Not continuing");
 
 	pr_emerg("Dazed and confused, but trying to continue\n");
@@ -431,12 +440,8 @@ unlock_return:
 	if (notify_die(DIE_NMI, "nmi", regs, 0, 2, SIGINT) == NOTIFY_STOP)
 		return;
 
-#if defined(CONFIG_X86_LOCAL_APIC) && !defined(CONFIG_LOCKUP_DETECTOR)
-	if (nmi_watchdog_tick(regs, reason))
-		return;
-	if (do_nmi_callback(regs, cpu))
+	if (nmi_watchdog_tick(regs))
 		return;
-#endif
 
 	unknown_nmi_error(reason, regs);
 }
-- 
1.7.2.3

[tip:perf/core] x86, NMI: Remove do_nmi_callback logic

[tip-bot for Huang Ying]

Commit-ID:  c218868fcc8f80c34bead7e887003b79faa89c48
Gitweb:     http://git.kernel.org/tip/c218868fcc8f80c34bead7e887003b79faa89c48
Author:     Huang Ying <ying.huang@intel.com>
AuthorDate: Fri, 15 Oct 2010 22:22:18 -0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sat, 16 Oct 2010 15:01:28 +0200

x86, NMI: Remove do_nmi_callback logic

do_nmi_callback related logic is removed, because it is useless,
just adds code complexity.

unknown_nmi_panic sysctl is reserved to keep kernel ABI
unchanged.

Signed-off-by: Huang Ying <ying.huang@intel.com>
Signed-off-by: Don Zickus <dzickus@redhat.com>
Cc: andi@firstfloor.org
Cc: robert.richter@amd.com
Cc: peterz@infradead.org
LKML-Reference: <1287195738-3136-6-git-send-email-dzickus@redhat.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/include/asm/nmi.h    |   10 +++++++++-
 arch/x86/kernel/apic/hw_nmi.c |    1 -
 arch/x86/kernel/apic/nmi.c    |   29 +----------------------------
 arch/x86/kernel/traps.c       |   17 +++++++++++------
 4 files changed, 21 insertions(+), 36 deletions(-)

diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
index 932f0f8..d5a5793 100644
--- a/arch/x86/include/asm/nmi.h
+++ b/arch/x86/include/asm/nmi.h
@@ -30,9 +30,17 @@ extern void setup_apic_nmi_watchdog(void *);
 extern void stop_apic_nmi_watchdog(void *);
 extern void disable_timer_nmi_watchdog(void);
 extern void enable_timer_nmi_watchdog(void);
-extern int nmi_watchdog_tick(struct pt_regs *regs, unsigned reason);
 extern void cpu_nmi_set_wd_enabled(void);
 
+#if defined(CONFIG_X86_LOCAL_APIC) && !defined(CONFIG_LOCKUP_DETECTOR)
+extern int nmi_watchdog_tick(struct pt_regs *regs);
+#else
+static inline int nmi_watchdog_tick(struct pt_regs *regs)
+{
+	return 0;
+}
+#endif
+
 extern atomic_t nmi_active;
 extern unsigned int nmi_watchdog;
 #define NMI_NONE	0
diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
index cefd694..e66b16d 100644
--- a/arch/x86/kernel/apic/hw_nmi.c
+++ b/arch/x86/kernel/apic/hw_nmi.c
@@ -100,7 +100,6 @@ void acpi_nmi_disable(void) { return; }
 #endif
 atomic_t nmi_active = ATOMIC_INIT(0);           /* oprofile uses this */
 EXPORT_SYMBOL(nmi_active);
-int unknown_nmi_panic;
 void cpu_nmi_set_wd_enabled(void) { return; }
 void stop_apic_nmi_watchdog(void *unused) { return; }
 void setup_apic_nmi_watchdog(void *unused) { return; }
diff --git a/arch/x86/kernel/apic/nmi.c b/arch/x86/kernel/apic/nmi.c
index a43f71c..f407b6b 100644
--- a/arch/x86/kernel/apic/nmi.c
+++ b/arch/x86/kernel/apic/nmi.c
@@ -37,7 +37,6 @@
 
 #include <asm/mach_traps.h>
 
-int unknown_nmi_panic;
 int nmi_watchdog_enabled;
 
 /* For reliability, we're prepared to waste bits here. */
@@ -389,7 +388,7 @@ void touch_nmi_watchdog(void)
 EXPORT_SYMBOL(touch_nmi_watchdog);
 
 notrace __kprobes int
-nmi_watchdog_tick(struct pt_regs *regs, unsigned reason)
+nmi_watchdog_tick(struct pt_regs *regs)
 {
 	/*
 	 * Since current_thread_info()-> is always on the stack, and we
@@ -483,23 +482,6 @@ static void disable_ioapic_nmi_watchdog(void)
 	on_each_cpu(stop_apic_nmi_watchdog, NULL, 1);
 }
 
-static int __init setup_unknown_nmi_panic(char *str)
-{
-	unknown_nmi_panic = 1;
-	return 1;
-}
-__setup("unknown_nmi_panic", setup_unknown_nmi_panic);
-
-static int unknown_nmi_panic_callback(struct pt_regs *regs, int cpu)
-{
-	unsigned char reason = get_nmi_reason();
-	char buf[64];
-
-	sprintf(buf, "NMI received for unknown reason %02x\n", reason);
-	die_nmi(buf, regs, 1); /* Always panic here */
-	return 0;
-}
-
 /*
  * proc handler for /proc/sys/kernel/nmi
  */
@@ -540,15 +522,6 @@ int proc_nmi_enabled(struct ctl_table *table, int write,
 
 #endif /* CONFIG_SYSCTL */
 
-int do_nmi_callback(struct pt_regs *regs, int cpu)
-{
-#ifdef CONFIG_SYSCTL
-	if (unknown_nmi_panic)
-		return unknown_nmi_panic_callback(regs, cpu);
-#endif
-	return 0;
-}
-
 void arch_trigger_all_cpu_backtrace(void)
 {
 	int i;
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index accb2f4..9ef8e15 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -83,6 +83,8 @@ EXPORT_SYMBOL_GPL(used_vectors);
 
 static int ignore_nmis;
 
+int unknown_nmi_panic;
+
 /*
  * Prevent NMI reason port (0x61) being accessed simultaneously, can
  * only be used in NMI handler.
@@ -306,6 +308,13 @@ gp_in_kernel:
 	die("general protection fault", regs, error_code);
 }
 
+static int __init setup_unknown_nmi_panic(char *str)
+{
+	unknown_nmi_panic = 1;
+	return 1;
+}
+__setup("unknown_nmi_panic", setup_unknown_nmi_panic);
+
 static notrace __kprobes void
 pci_serr_error(unsigned char reason, struct pt_regs *regs)
 {
@@ -380,7 +389,7 @@ unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
 		 reason, smp_processor_id());
 
 	pr_emerg("Do you have a strange power saving mode enabled?\n");
-	if (panic_on_unrecovered_nmi)
+	if (unknown_nmi_panic || panic_on_unrecovered_nmi)
 		panic("NMI: Not continuing");
 
 	pr_emerg("Dazed and confused, but trying to continue\n");
@@ -431,12 +440,8 @@ unlock_return:
 	if (notify_die(DIE_NMI, "nmi", regs, 0, 2, SIGINT) == NOTIFY_STOP)
 		return;
 
-#if defined(CONFIG_X86_LOCAL_APIC) && !defined(CONFIG_LOCKUP_DETECTOR)
-	if (nmi_watchdog_tick(regs, reason))
-		return;
-	if (do_nmi_callback(regs, cpu))
+	if (nmi_watchdog_tick(regs))
 		return;
-#endif
 
 	unknown_nmi_error(reason, regs);
 }

Re: [PATCH 5/5] x86, NMI: Remove do_nmi_callback logic

[Robert Richter]

On 15.10.10 22:22:18, Don Zickus wrote:
> From: Huang Ying <ying.huang@intel.com>
> 
> do_nmi_callback related logic is removed, because it is useless, just
> adds code complexity.
> 
> unknown_nmi_panic sysctl is reserved to keep kernel ABI unchanged.
> 
> Signed-off-by: Huang Ying <ying.huang@intel.com>
> Signed-off-by: Don Zickus <dzickus@redhat.com>
> ---
>  arch/x86/include/asm/nmi.h    |   10 +++++++++-
>  arch/x86/kernel/apic/hw_nmi.c |    1 -
>  arch/x86/kernel/apic/nmi.c    |   29 +----------------------------
>  arch/x86/kernel/traps.c       |   17 +++++++++++------
>  4 files changed, 21 insertions(+), 36 deletions(-)

> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index accb2f4..9ef8e15 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -83,6 +83,8 @@ EXPORT_SYMBOL_GPL(used_vectors);
>  
>  static int ignore_nmis;
>  
> +int unknown_nmi_panic;

I didn't apply the patches, so I am not really sure, but I think this
could be static now.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 5/5] x86, NMI: Remove do_nmi_callback logic

[Don Zickus]

On Tue, Oct 19, 2010 at 05:03:03PM +0200, Robert Richter wrote:
> > +++ b/arch/x86/kernel/traps.c
> > @@ -83,6 +83,8 @@ EXPORT_SYMBOL_GPL(used_vectors);
> >  
> >  static int ignore_nmis;
> >  
> > +int unknown_nmi_panic;
> 
> I didn't apply the patches, so I am not really sure, but I think this
> could be static now.

It is used in kernel/sysctl.c so I don't think it can.

Cheers,
Don

Re: [PATCH 5/5] x86, NMI: Remove do_nmi_callback logic

[Robert Richter]

On 19.10.10 12:01:58, Don Zickus wrote:
> On Tue, Oct 19, 2010 at 05:03:03PM +0200, Robert Richter wrote:
> > > +++ b/arch/x86/kernel/traps.c
> > > @@ -83,6 +83,8 @@ EXPORT_SYMBOL_GPL(used_vectors);
> > >  
> > >  static int ignore_nmis;
> > >  
> > > +int unknown_nmi_panic;
> > 
> > I didn't apply the patches, so I am not really sure, but I think this
> > could be static now.
> 
> It is used in kernel/sysctl.c so I don't think it can.

Ah, ok. Was only looking in arch/x86.

Thanks,

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 0/5] x86, NMI: give NMI handler a face-lift

[Robert Richter]

On 15.10.10 22:22:13, Don Zickus wrote:
> Restructuring the nmi handler to be more readable and simpler.
> 
> This is just laying the ground work for future improvements in this area.
> 
> I know people were complaining about having DIE_NMI and DIE_NMI_IPI, but
> for now it is consistent with what was there before.  Hopefully, we can
> modify the notifier chain to make it smarter and combine the two events.
> 
> I also left out one of Huang's patch until we figure out how we are going
> to proceed with a new notifier.
> 
> Tested 32-bit and 64-bit on AMD and Intel machines.
> 
> Cheers,
> Don
> 
> Huang Ying (5):
>   x86, NMI: Add NMI symbol constants and rename memory parity to PCI
>     SERR
>   x86, NMI: Add touch_nmi_watchdog to io_check_error delay
>   x86, NMI: Rewrite NMI handler
>   x86, NMI: Allow NMI reason io port (0x61) to be processed on any CPU
>   x86, NMI: Remove do_nmi_callback logic

I am fine with applying the patches, thanks Huang and Don.

I have some 2 comments on them and will reply directly.

-Robert

> 
>  arch/x86/include/asm/mach_traps.h |   12 +++-
>  arch/x86/include/asm/nmi.h        |   10 +++-
>  arch/x86/kernel/apic/hw_nmi.c     |    1 -
>  arch/x86/kernel/apic/nmi.c        |   29 +-------
>  arch/x86/kernel/cpu/perf_event.c  |    1 -
>  arch/x86/kernel/traps.c           |  141 ++++++++++++++++++++----------------
>  arch/x86/oprofile/nmi_int.c       |    1 -
>  arch/x86/oprofile/nmi_timer_int.c |    2 +-
>  drivers/char/ipmi/ipmi_watchdog.c |    2 +-
>  drivers/watchdog/hpwdt.c          |    2 +-
>  10 files changed, 102 insertions(+), 99 deletions(-)
> 
> -- 
> 1.7.2.3
> 
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center