x86: merging nmi 32/64 to nmi.c v2

This is a second attempt to merge nmi-32/64 to nmi.c Summary changes:

- a few helper functions added (placed on top part of nmi.c)

- nmi_watchdog_default() now handles both 32/64 modes

- check_nmi_watchdog() uses for_each_online_cpu() even in 32bit mode since cpu_online_map is set up after cpu_callin_map anyway

- last_irq_sums and alert_counter was defined as static arrays in 32bit mode, so they were changed to per_cpu variables

- and the most notable change is that I've changed logic a bit for procedures touch_nmi_watchdog() and nmi_watchdog_tick() used in 32bit mode (to be familiar with 64bit mode).

- die_nmi interface is changed in traps_32.c to be the same as it is in traps_64.c. By this we also able (and we do) add an ability to pass "panic" for nmi_watchdog in 32bit mode too

- in both mode apic_write_around is used since in 64bit mode it will be exanded to apic_write anyway but allow us to save from #ifdef

The helper functions (on top of nmi.c) still contain #ifdef sections. There is no simple way to get them more elegant since they hide different types. So to eliminate this we need to bring much more specific changes which is not that easy at once. At least this could be a first step in further code simplification.

Actually I would like to get a somehow final word about this process and my efforts on it - I mean if it becomes annoying and silly from my side to try to merge them I would better digg in some other area ;)

This patch is based on commit

commit 8f40f672e6bb071812f61bfbd30efc3fc1263ad1
Merge: 8978a31... 887b3ec...
Author: Linus Torvalds <[email protected]>
Date: Wed May 14 19:30:13 2008 -0700

Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/ericvh/v9fs

Any comments are quite welcome.
And thanks for all comments about first version of the patch.

---

 arch/x86/kernel/Makefile   |    2 +-
 arch/x86/kernel/nmi.c      |  551 ++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/nmi_32.c   |  472 -------------------------------------
 arch/x86/kernel/nmi_64.c   |  482 --------------------------------------
 arch/x86/kernel/traps_32.c |    8 +-
 arch/x86/kernel/traps_64.c |    5 +-
 include/asm-x86/nmi.h      |    6 +-
 7 files changed, 563 insertions(+), 963 deletions(-)

index 5e618c3..d5dd666 100644
@@ -53,7 +53,7 @@ obj-$(CONFIG_X86_32_SMP)	+= smpcommon.o
 obj-$(CONFIG_X86_64_SMP)	+= tsc_sync.o smpcommon.o
 obj-$(CONFIG_X86_TRAMPOLINE)	+= trampoline_$(BITS).o
 obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
-obj-$(CONFIG_X86_LOCAL_APIC)	+= apic_$(BITS).o nmi_$(BITS).o
+obj-$(CONFIG_X86_LOCAL_APIC)	+= apic_$(BITS).o nmi.o
 obj-$(CONFIG_X86_IO_APIC)	+= io_apic_$(BITS).o
 obj-$(CONFIG_X86_REBOOTFIXUPS)	+= reboot_fixups_32.o
 obj-$(CONFIG_KEXEC)		+= machine_kexec_$(BITS).o

index bde6f63..f31e665 100644
@@ -755,9 +755,9 @@ unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
 
 static DEFINE_SPINLOCK(nmi_print_lock);
 
-void notrace __kprobes die_nmi(struct pt_regs *regs, const char *msg)
+void notrace __kprobes die_nmi(char *str, struct pt_regs *regs, int do_panic)
 {
-	if (notify_die(DIE_NMIWATCHDOG, msg, regs, 0, 2, SIGINT) == NOTIFY_STOP)
+	if (notify_die(DIE_NMIWATCHDOG, str, regs, 0, 2, SIGINT) == NOTIFY_STOP)
 		return;
 
 	spin_lock(&nmi_print_lock);

@@ -766,10 +766,12 @@ void notrace __kprobes die_nmi(struct pt_regs *regs, const char *msg)
 	* to get a message out:
 	*/
 	bust_spinlocks(1);
-	printk(KERN_EMERG "%s", msg);
+	printk(KERN_EMERG "%s", str);
 	printk(" on CPU%d, ip %08lx, registers:\n",
 		smp_processor_id(), regs->ip);
 	show_registers(regs);
+	if (do_panic)
+		panic("Non maskable interrupt");
 	console_silent();
 	spin_unlock(&nmi_print_lock);
 	bust_spinlocks(0);

index adff76e..6113df8 100644
@@ -76,6 +76,7 @@ asmlinkage void alignment_check(void);
 asmlinkage void machine_check(void);
 asmlinkage void spurious_interrupt_bug(void);
 
+int panic_on_unrecovered_nmi;
 static unsigned int code_bytes = 64;
 
 static inline void conditional_sti(struct pt_regs *regs)

@@ -614,7 +615,9 @@ die_nmi(char *str, struct pt_regs *regs, int do_panic)
 	 * We are in trouble anyway, lets at least try
 	 * to get a message out.
 	 */
-	printk(str, smp_processor_id());
+	printk(KERN_EMERG "%s", str);
+	printk(" on CPU%d, ip %08lx, registers:\n",
+		smp_processor_id(), regs->ip);
 	show_registers(regs);
 	if (kexec_should_crash(current))
 		crash_kexec(regs);

index 1e36302..2c5c4be 100644
@@ -38,12 +38,10 @@ static inline void unset_nmi_pm_callback(struct pm_dev *dev)
 
 #ifdef CONFIG_X86_64
 extern void default_do_nmi(struct pt_regs *);
-extern void die_nmi(char *str, struct pt_regs *regs, int do_panic);
-extern void nmi_watchdog_default(void);
-#else
-#define nmi_watchdog_default() do {} while (0)
 #endif
 
+extern void nmi_watchdog_default(void);
+extern void die_nmi(char *str, struct pt_regs *regs, int do_panic);
 extern int check_nmi_watchdog(void);
 extern int nmi_watchdog_enabled;
 extern int unknown_nmi_panic;

Hi Cyrill,

cool work!

...

This was already done, see http://lkml.org/lkml/2008/5/2/525.
But it doesn't affect the unified file.

[Hiroshi Shimamoto - Wed, May 21, 2008 at 11:20:17AM -0700]

| Cyrill Gorcunov wrote:
| > This is a second attempt to merge nmi-32/64 to nmi.c
| 
| Hi Cyrill,
| 
| cool work!
| 
| > Summary changes:
| > 
| ...
| > - last_irq_sums and alert_counter was defined as static arrays in 32bit
| >   mode, so they were changed to per_cpu variables
| 
| This was already done, see http://lkml.org/lkml/2008/5/2/525.
| But it doesn't affect the unified file.
| 
| > +#ifdef CONFIG_X86_64
| > +static unsigned ignore_nmis;
| > +
| > +asmlinkage notrace __kprobes void
| > +do_nmi(struct pt_regs *regs, long error_code)
| > +{
| > +	nmi_enter();
| > +	add_pda(__nmi_count,1);
| > +	if (!ignore_nmis)
| > +		default_do_nmi(regs);
| > +	nmi_exit();
| > +}
| > +
| > +void stop_nmi(void)
| > +{
| > +	acpi_nmi_disable();
| > +	ignore_nmis++;
| > +}
| > +
| > +void restart_nmi(void)
| > +{
| > +	ignore_nmis--;
| > +	acpi_nmi_enable();
| > +}
| > +#endif /* CONFIG_X86_64 */
| 
| I'm guessing we can move these functions to traps_64.c.
| I've tried to move these functions from traps_32.c to nmi_32.c,
| but it caused build error on some 32bit configs.
| 
| thanks,
| Hiroshi Shimamoto
| 

Hi Hiroshi,

thanks a lot for reply. I will check if it would be possible to move them to traps_64.c. Thanks ;)

- Cyrill -

[Hiroshi Shimamoto - Wed, May 21, 2008 at 11:20:17AM -0700]

| Cyrill Gorcunov wrote:
| > This is a second attempt to merge nmi-32/64 to nmi.c
| 
| Hi Cyrill,
| 
| cool work!
| 
| > Summary changes:
| > 
| ...
| > - last_irq_sums and alert_counter was defined as static arrays in 32bit
| >   mode, so they were changed to per_cpu variables
| 
| This was already done, see http://lkml.org/lkml/2008/5/2/525.
| But it doesn't affect the unified file.
| 
| > +#ifdef CONFIG_X86_64
| > +static unsigned ignore_nmis;
| > +
| > +asmlinkage notrace __kprobes void
| > +do_nmi(struct pt_regs *regs, long error_code)
| > +{
| > +	nmi_enter();
| > +	add_pda(__nmi_count,1);
| > +	if (!ignore_nmis)
| > +		default_do_nmi(regs);
| > +	nmi_exit();
| > +}
| > +
| > +void stop_nmi(void)
| > +{
| > +	acpi_nmi_disable();
| > +	ignore_nmis++;
| > +}
| > +
| > +void restart_nmi(void)
| > +{
| > +	ignore_nmis--;
| > +	acpi_nmi_enable();
| > +}
| > +#endif /* CONFIG_X86_64 */
| 
| I'm guessing we can move these functions to traps_64.c.
| I've tried to move these functions from traps_32.c to nmi_32.c,
| but it caused build error on some 32bit configs.
| 
| thanks,
| Hiroshi Shimamoto
| 

Here is updated version (thanks Hiroshi) - I've moved these funcs to traps_64.c and it's compiled just fine (so we are going closer to merge someday even traps-32/64 ;)

---

 arch/x86/kernel/Makefile   |    2 +-
 arch/x86/kernel/nmi.c      |  551 ++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/nmi_32.c   |  472 -------------------------------------
 arch/x86/kernel/nmi_64.c   |  482 --------------------------------------
 arch/x86/kernel/traps_32.c |    8 +-
 arch/x86/kernel/traps_64.c |   28 +++-
 include/asm-x86/nmi.h      |    6 +-
 7 files changed, 586 insertions(+), 963 deletions(-)

index 5e618c3..d5dd666 100644
@@ -53,7 +53,7 @@ obj-$(CONFIG_X86_32_SMP)	+= smpcommon.o
 obj-$(CONFIG_X86_64_SMP)	+= tsc_sync.o smpcommon.o
 obj-$(CONFIG_X86_TRAMPOLINE)	+= trampoline_$(BITS).o
 obj-$(CONFIG_X86_MPPARSE)	+= mpparse.o
-obj-$(CONFIG_X86_LOCAL_APIC)	+= apic_$(BITS).o nmi_$(BITS).o
+obj-$(CONFIG_X86_LOCAL_APIC)	+= apic_$(BITS).o nmi.o
 obj-$(CONFIG_X86_IO_APIC)	+= io_apic_$(BITS).o
 obj-$(CONFIG_X86_REBOOTFIXUPS)	+= reboot_fixups_32.o
 obj-$(CONFIG_KEXEC)		+= machine_kexec_$(BITS).o

index bde6f63..f31e665 100644
@@ -755,9 +755,9 @@ unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
 
 static DEFINE_SPINLOCK(nmi_print_lock);
 
-void notrace __kprobes die_nmi(struct pt_regs *regs, const char *msg)
+void notrace __kprobes die_nmi(char *str, struct pt_regs *regs, int do_panic)
 {
-	if (notify_die(DIE_NMIWATCHDOG, msg, regs, 0, 2, SIGINT) == NOTIFY_STOP)
+	if (notify_die(DIE_NMIWATCHDOG, str, regs, 0, 2, SIGINT) == NOTIFY_STOP)
 		return;
 
 	spin_lock(&nmi_print_lock);

@@ -766,10 +766,12 @@ void notrace __kprobes die_nmi(struct pt_regs *regs, const char *msg)
 	* to get a message out:
 	*/
 	bust_spinlocks(1);
-	printk(KERN_EMERG "%s", msg);
+	printk(KERN_EMERG "%s", str);
 	printk(" on CPU%d, ip %08lx, registers:\n",
 		smp_processor_id(), regs->ip);
 	show_registers(regs);
+	if (do_panic)
+		panic("Non maskable interrupt");
 	console_silent();
 	spin_unlock(&nmi_print_lock);
 	bust_spinlocks(0);

index adff76e..e4a3807 100644
@@ -76,7 +76,9 @@ asmlinkage void alignment_check(void);
 asmlinkage void machine_check(void);
 asmlinkage void spurious_interrupt_bug(void);
 
+int panic_on_unrecovered_nmi;
 static unsigned int code_bytes = 64;
+static unsigned ignore_nmis;
 
 static inline void conditional_sti(struct pt_regs *regs)
 {

@@ -614,7 +616,9 @@ die_nmi(char *str, struct pt_regs *regs, int do_panic)
 	 * We are in trouble anyway, lets at least try
 	 * to get a message out.
 	 */
-	printk(str, smp_processor_id());
+	printk(KERN_EMERG "%s", str);
+	printk(" on CPU%d, ip %08lx, registers:\n",
+		smp_processor_id(), regs->ip);
 	show_registers(regs);
 	if (kexec_should_crash(current))
 		crash_kexec(regs);

@@ -865,6 +869,28 @@ asmlinkage notrace  __kprobes void default_do_nmi(struct pt_regs *regs)
 		io_check_error(reason, regs);
 }
 
+asmlinkage notrace __kprobes void
+do_nmi(struct pt_regs *regs, long error_code)
+{
+	nmi_enter();
+	add_pda(__nmi_count, 1);
+	if (!ignore_nmis)
+		default_do_nmi(regs);
+	nmi_exit();
+}
+
+void stop_nmi(void)
+{
+	acpi_nmi_disable();
+	ignore_nmis++;
+}
+
+void restart_nmi(void)
+{
+	ignore_nmis--;
+	acpi_nmi_enable();
+}
+
 /* runs on IST stack. */
 asmlinkage void __kprobes do_int3(struct pt_regs * regs, long error_code)
 {

index 1e36302..2c5c4be 100644
@@ -38,12 +38,10 @@ static inline void unset_nmi_pm_callback(struct pm_dev *dev)
 
 #ifdef CONFIG_X86_64
 extern void default_do_nmi(struct pt_regs *);
-extern void die_nmi(char *str, struct pt_regs *regs, int do_panic);
-extern void nmi_watchdog_default(void);
-#else
-#define nmi_watchdog_default() do {} while (0)
 #endif
 
+extern void nmi_watchdog_default(void);
+extern void die_nmi(char *str, struct pt_regs *regs, int do_panic);
 extern int check_nmi_watchdog(void);
 extern int nmi_watchdog_enabled;
 extern int unknown_nmi_panic;

Cyrill,

I really like that patch, but I'm a bit wary about the doit in one go approach. Its pretty hard to review that way and in case of breakage bisecting will just point to one large diff which is hard to decode.

Can you please try to make this a more fine grained patch series ?

Gradually add/modify the two files so they are identical at the end. There are a couple of obvious steps, e.g. the move of function to traps_*.c and adding the inline functions like get_timer_irqs().

Thanks,
tglx

[Thomas Gleixner - Thu, May 22, 2008 at 11:37:34PM +0200]

| On Wed, 21 May 2008, Cyrill Gorcunov wrote:
| > 
| > Here is updated version (thanks Hiroshi) - I've moved these funcs
| > to traps_64.c and it's compiled just fine (so we are going closer
| > to merge someday even traps-32/64 ;)
| 
| Cyrill,
| 
| I really like that patch, but I'm a bit wary about the doit in one go
| approach. Its pretty hard to review that way and in case of breakage
| bisecting will just point to one large diff which is hard to decode.
| 
| Can you please try to make this a more fine grained patch series ?
| 
| Gradually add/modify the two files so they are identical at the
| end. There are a couple of obvious steps, e.g. the move of function to
| traps_*.c and adding the inline functions like get_timer_irqs().
| 
| Thanks,
| 	tglx
| 

Hi Thomas,

i think it was silly from my side to send such a *huge* patch at once, but original idea was to get an overwiew of the change in general, that is why I put changelog on top. So this patch (well RFC) was not to inclusion but for review only ;) And even if you/Ingo have it applied to a tree it would be totally undisectable anyway (as you pointed). So I will make quilt series this weekend. Thanks a lot!

- Cyrill -