Re: [PATCH 7/9] KGDB/KDB: add new system NMI entry code to KDB

[Jason Wessel <jason.wessel@windriver.com>]

On 09/05/2013 05:50 PM, Mike Travis wrote:
> This patch adds a new "KDB_REASON" code (KDB_REASON_SYSTEM_NMI).  This
> is purely cosmetic to distinguish it from the other various reasons that
> NMI may occur and are usually after an error occurred.  Also the dumping
> of registers is not done to more closely match what is displayed when KDB
> is entered manually via the sysreq 'g' key.


This patch is not quite right.   See below.


> 
> Signed-off-by: Mike Travis <travis@sgi.com>
> Reviewed-by: Dimitri Sivanich <sivanich@sgi.com>
> Reviewed-by: Hedi Berriche <hberrich@sgi.com>
> ---
>  include/linux/kdb.h             |    1 +
>  include/linux/kgdb.h            |    1 +
>  kernel/debug/debug_core.c       |    5 +++++
>  kernel/debug/kdb/kdb_debugger.c |    5 ++++-
>  kernel/debug/kdb/kdb_main.c     |    3 +++
>  5 files changed, 14 insertions(+), 1 deletion(-)
> 
> --- linux.orig/include/linux/kdb.h
> +++ linux/include/linux/kdb.h
> @@ -109,6 +109,7 @@ typedef enum {
>  	KDB_REASON_RECURSE,	/* Recursive entry to kdb;
>  				 * regs probably valid */
>  	KDB_REASON_SSTEP,	/* Single Step trap. - regs valid */
> +	KDB_REASON_SYSTEM_NMI,	/* In NMI due to SYSTEM cmd; regs valid */
>  } kdb_reason_t;
>  
>  extern int kdb_trap_printk;
> --- linux.orig/include/linux/kgdb.h
> +++ linux/include/linux/kgdb.h
> @@ -52,6 +52,7 @@ extern int kgdb_connected;
>  extern int kgdb_io_module_registered;
>  
>  extern atomic_t			kgdb_setting_breakpoint;
> +extern atomic_t			kgdb_system_nmi;


We don't need extra atomics.  You should add another variable to the kgdb_state which is processor specific in this case.

Better yet, just set the ks->err_code properly in your kgdb_nmicallin() or in the origination call to kgdb_nmicallback() from your nmi handler (remember I still have the question pending if we actually need kgdb_nmicallin() in the first place.  You already did the work of adding another NMI type to the enum.  We just need to use the ks->err_code variable as well.


>  extern atomic_t			kgdb_cpu_doing_single_step;
>  
>  extern struct task_struct	*kgdb_usethread;
> --- linux.orig/kernel/debug/debug_core.c
> +++ linux/kernel/debug/debug_core.c
> @@ -125,6 +125,7 @@ static atomic_t			masters_in_kgdb;
>  static atomic_t			slaves_in_kgdb;
>  static atomic_t			kgdb_break_tasklet_var;
>  atomic_t			kgdb_setting_breakpoint;
> +atomic_t			kgdb_system_nmi;
>  
>  struct task_struct		*kgdb_usethread;
>  struct task_struct		*kgdb_contthread;
> @@ -760,7 +761,11 @@ int kgdb_nmicallin(int cpu, int trapnr,
>  
>  		/* Indicate there are slaves waiting */
>  		kgdb_info[cpu].send_ready = send_ready;
> +
> +		/* Use new reason code "SYSTEM_NMI" */
> +		atomic_inc(&kgdb_system_nmi);
>  		kgdb_cpu_enter(ks, regs, DCPU_WANT_MASTER);
> +		atomic_dec(&kgdb_system_nmi);
>  		kgdb_do_roundup = save_kgdb_do_roundup;
>  		kgdb_info[cpu].send_ready = NULL;
>  
> --- linux.orig/kernel/debug/kdb/kdb_debugger.c
> +++ linux/kernel/debug/kdb/kdb_debugger.c
> @@ -69,7 +69,10 @@ int kdb_stub(struct kgdb_state *ks)
>  	if (atomic_read(&kgdb_setting_breakpoint))
>  		reason = KDB_REASON_KEYBOARD;
>  
> -	if (in_nmi())
> +	if (atomic_read(&kgdb_system_nmi))
> +		reason = KDB_REASON_SYSTEM_NMI;


This would get changed to if (ks->err == KDB_REASON_SYSNMI && ks->signo == SIGTRAP) ....

Cheers,
Jason.

> +
> +	else if (in_nmi())
>  		reason = KDB_REASON_NMI;
>  
>  	for (i = 0, bp = kdb_breakpoints; i < KDB_MAXBPT; i++, bp++) {
> --- linux.orig/kernel/debug/kdb/kdb_main.c
> +++ linux/kernel/debug/kdb/kdb_main.c
> @@ -1200,6 +1200,9 @@ static int kdb_local(kdb_reason_t reason
>  			   instruction_pointer(regs));
>  		kdb_dumpregs(regs);
>  		break;
> +	case KDB_REASON_SYSTEM_NMI:
> +		kdb_printf("due to System NonMaskable Interrupt\n");
> +		break;
>  	case KDB_REASON_NMI:
>  		kdb_printf("due to NonMaskable Interrupt @ "
>  			   kdb_machreg_fmt "\n",
> 
> -- 
>