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

[Mike Travis <travis@sgi.com>]

On 9/5/2013 10:00 PM, Jason Wessel wrote:
> 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.

Good idea, I hadn't thought of using that field.  In fact, it
simplified the patch enough that I just folded into the other.

I'll address your other question separately.

Thanks!
Mike
> 
> 
>>  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",
>>
>> -- 
>>
>