Re: [PATCH 01/11] kernel/debug: Mask KGDB NMI upon entry

[Jason Wessel]

On 09/13/2012 10:03 AM, Anton Vorontsov wrote:
> The new arch callback should manage NMIs that usually cause KGDB to
> enter. That is, not all NMIs should be enabled/disabled, but only
> those that issue kgdb_handle_exception().
> 
> We must mask it as serial-line interrupt can be used as an NMI, so
> if the original KGDB-entry cause was say a breakpoint, then every
> input to KDB console will cause KGDB to reenter, which we don't want.
> 
> Signed-off-by: Anton Vorontsov <anton.vorontsov@xxxxxxxxxx>
> ---
>  include/linux/kgdb.h      | 23 +++++++++++++++++++++++
>  kernel/debug/debug_core.c | 36 +++++++++++++++++++++++++++++++++---
>  2 files changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> index c4d2fc1..3b111a6 100644
> --- a/include/linux/kgdb.h
> +++ b/include/linux/kgdb.h
> @@ -221,6 +221,29 @@ extern int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt);
>   */
>  extern void kgdb_arch_late(void);
>  
> +/**
> + *	kgdb_arch_enable_nmi - Enable or disable KGDB-entry NMI
> + *	@on: Flag to either enable or disable an NMI
> + *
> + *	This is an architecture-specific "back-end" for kgdb_enable_nmi(). The
> + *	call does not count disable/enable requests, do not use it directly.
> + */
> +extern void kgdb_arch_enable_nmi(bool on);

I realize this is more of a clean up, and this doesn't necessarily
have to gate the acceptance of these patches to the mainline, but I
would like to not see us using the kgdb_arch_enable_nmi() as a weak
function.  This belongs in:

struct kgdb_arch {
	...
	void enable_nmi(bool on);
}


> +
> +/**
> + *	kgdb_enable_nmi - Enable or disable KGDB-entry NMI
> + *	@on: Flag to either enable or disable an NMI
> + *
> + *	This function manages NMIs that usually cause KGDB to enter. That is,
> + *	not all NMIs should be enabled or disabled, but only those that issue
> + *	kgdb_handle_exception().
> + *
> + *	The call counts disable requests, and thus allows to nest disables.
> + *	But trying to enable already enabled NMI is an error. The call returns
> + *	1 if NMI has been actually enabled after the call, and a value <= 0 if
> + *	it is still disabled.
> + */
> +extern int kgdb_enable_nmi(bool on);


Clearly you need something arch specific here.  With respect to the
atomic math, it is not clear that would be the case for this function
unilaterally accross other architectures.  This comment block and the
"implementation" code belong in the arch stub: arch/kernel/arm/kgdb.c
The whole kgdb_enable_nmi can go away if using the struct kgdb_arch
callbacks


> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -214,6 +214,30 @@ int __weak kgdb_skipexception(int exception, struct pt_regs *regs)
>  	return 0;
>  }
>  
> +void __weak kgdb_arch_enable_nmi(bool on)
> +{
> +}
> +
> +int kgdb_enable_nmi(bool on)
> +{
> +	static atomic_t cnt;
> +	int ret;
> +
> +	ret = atomic_add_return(on ? 1 : -1, &cnt);
> +	if (ret > 1 && on) {
> +		/*
> +		 * There should be only one instance that calls this function
> +		 * in "enable, disable" order. All other users must call
> +		 * disable first, then enable. If not, something is wrong.
> +		 */
> +		WARN_ON(1);
> +		return 1;
> +	}
> +
> +	kgdb_arch_enable_nmi(ret > 0);
> +	return ret;
> +}
> +
>  /*
>   * Some architectures need cache flushes when we set/clear a
>   * breakpoint:
> @@ -672,6 +696,9 @@ kgdb_handle_exception(int evector, int signo, int ecode, struct pt_regs *regs)
>  {
>  	struct kgdb_state kgdb_var;
>  	struct kgdb_state *ks = &kgdb_var;
> +	int ret = 0;
> +
> +	kgdb_enable_nmi(0);


With what I mentioned before this becomes:

	if (arch_kgdb_ops->enable_nmi)
		arch_kgdb_ops->enable_nmi(0)


>  
>  	ks->cpu			= raw_smp_processor_id();
>  	ks->ex_vector		= evector;
> @@ -681,11 +708,14 @@ kgdb_handle_exception(int evector, int signo, int ecode, struct pt_regs *regs)
>  	ks->linux_regs		= regs;
>  
>  	if (kgdb_reenter_check(ks))
> -		return 0; /* Ouch, double exception ! */
> +		goto out; /* Ouch, double exception ! */
>  	if (kgdb_info[ks->cpu].enter_kgdb != 0)
> -		return 0;
> +		goto out;
>  
> -	return kgdb_cpu_enter(ks, regs, DCPU_WANT_MASTER);
> +	ret = kgdb_cpu_enter(ks, regs, DCPU_WANT_MASTER);
> +out:
> +	kgdb_enable_nmi(1);


Becomes:
	if (arch_kgdb_ops->enable_nmi)
		arch_kgdb_ops->enable_nmi(1)


> +	return ret;
>  }
>  
>  int kgdb_nmicallback(int cpu, void *regs)
> 


Cheers,
Jason.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/