Re: [PATCH v4 04/12] kgdb: Delay "kgdbwait" to dbg_late_init() by default

[Daniel Thompson]

On Thu, May 07, 2020 at 01:08:42PM -0700, Douglas Anderson wrote:
> Using kgdb requires at least some level of architecture-level
> initialization.  If nothing else, it relies on the architecture to
> pass breakpoints / crashes onto kgdb.
> 
> On some architectures this all works super early, specifically it
> starts working at some point in time before Linux parses
> early_params's.  On other architectures it doesn't.  A survey of a few
> platforms:
> 
> a) x86: Presumably it all works early since "ekgdboc" is documented to
>    work here.
> b) arm64: Catching crashes works; with a simple patch breakpoints can
>    also be made to work.
> c) arm: Nothing in kgdb works until
>    paging_init() -> devicemaps_init() -> early_trap_init()
> 
> Let's be conservative and, by default, process "kgdbwait" (which tells
> the kernel to drop into the debugger ASAP at boot) a bit later at
> dbg_late_init() time.  If an architecture has tested it and wants to
> re-enable super early debugging, they can select the
> ARCH_HAS_EARLY_DEBUG KConfig option.  We'll do this for x86 to start.
> It should be noted that dbg_late_init() is still called quite early in
> the system.
> 
> Note that this patch doesn't affect when kgdb runs its init.  If kgdb
> is set to initialize early it will still initialize when parsing
> early_param's.  This patch _only_ inhibits the initial breakpoint from
> "kgdbwait".  This means:
> 
> * Without any extra patches arm64 platforms will at least catch
>   crashes after kgdb inits.
> * arm platforms will catch crashes (and could handle a hardcoded
>   kgdb_breakpoint()) any time after early_trap_init() runs, even
>   before dbg_late_init().
> 
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>

I hope to pull this set into the kgdb tree shortly. Any objections to
the arch/x86 changes this would bring?


Daniel.


> ---
> 
> Changes in v4:
> - Add "if KGDB" to "select ARCH_HAS_EARLY_DEBUG" in Kconfig.
> 
> Changes in v3:
> - Change boolean weak function to KConfig.
> 
> Changes in v2: None
> 
>  arch/x86/Kconfig          |  1 +
>  kernel/debug/debug_core.c | 25 +++++++++++++++----------
>  lib/Kconfig.kgdb          | 18 ++++++++++++++++++
>  3 files changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 1197b5596d5a..5f44955ee21c 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -60,6 +60,7 @@ config X86
>  	select ARCH_HAS_ACPI_TABLE_UPGRADE	if ACPI
>  	select ARCH_HAS_DEBUG_VIRTUAL
>  	select ARCH_HAS_DEVMEM_IS_ALLOWED
> +	select ARCH_HAS_EARLY_DEBUG		if KGDB
>  	select ARCH_HAS_ELF_RANDOMIZE
>  	select ARCH_HAS_FAST_MULTIPLIER
>  	select ARCH_HAS_FILTER_PGPROT
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index 950dc667c823..503c1630ca76 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -950,6 +950,14 @@ void kgdb_panic(const char *msg)
>  	kgdb_breakpoint();
>  }
>  
> +static void kgdb_initial_breakpoint(void)
> +{
> +	kgdb_break_asap = 0;
> +
> +	pr_crit("Waiting for connection from remote gdb...\n");
> +	kgdb_breakpoint();
> +}
> +
>  void __weak kgdb_arch_late(void)
>  {
>  }
> @@ -960,6 +968,9 @@ void __init dbg_late_init(void)
>  	if (kgdb_io_module_registered)
>  		kgdb_arch_late();
>  	kdb_init(KDB_INIT_FULL);
> +
> +	if (kgdb_io_module_registered && kgdb_break_asap)
> +		kgdb_initial_breakpoint();
>  }
>  
>  static int
> @@ -1055,14 +1066,6 @@ void kgdb_schedule_breakpoint(void)
>  }
>  EXPORT_SYMBOL_GPL(kgdb_schedule_breakpoint);
>  
> -static void kgdb_initial_breakpoint(void)
> -{
> -	kgdb_break_asap = 0;
> -
> -	pr_crit("Waiting for connection from remote gdb...\n");
> -	kgdb_breakpoint();
> -}
> -
>  /**
>   *	kgdb_register_io_module - register KGDB IO module
>   *	@new_dbg_io_ops: the io ops vector
> @@ -1099,7 +1102,8 @@ int kgdb_register_io_module(struct kgdb_io *new_dbg_io_ops)
>  	/* Arm KGDB now. */
>  	kgdb_register_callbacks();
>  
> -	if (kgdb_break_asap)
> +	if (kgdb_break_asap &&
> +	    (!dbg_is_early || IS_ENABLED(CONFIG_ARCH_HAS_EARLY_DEBUG)))
>  		kgdb_initial_breakpoint();
>  
>  	return 0;
> @@ -1169,7 +1173,8 @@ static int __init opt_kgdb_wait(char *str)
>  	kgdb_break_asap = 1;
>  
>  	kdb_init(KDB_INIT_EARLY);
> -	if (kgdb_io_module_registered)
> +	if (kgdb_io_module_registered &&
> +	    IS_ENABLED(CONFIG_ARCH_HAS_EARLY_DEBUG))
>  		kgdb_initial_breakpoint();
>  
>  	return 0;
> diff --git a/lib/Kconfig.kgdb b/lib/Kconfig.kgdb
> index 933680b59e2d..ffa7a76de086 100644
> --- a/lib/Kconfig.kgdb
> +++ b/lib/Kconfig.kgdb
> @@ -124,4 +124,22 @@ config KDB_CONTINUE_CATASTROPHIC
>  	  CONFIG_KDB_CONTINUE_CATASTROPHIC == 2. KDB forces a reboot.
>  	  If you are not sure, say 0.
>  
> +config ARCH_HAS_EARLY_DEBUG
> +	bool
> +	default n
> +	help
> +	  If an architecture can definitely handle entering the debugger
> +	  when early_param's are parsed then it select this config.
> +	  Otherwise, if "kgdbwait" is passed on the kernel command line it
> +	  won't actually be processed until dbg_late_init() just after the
> +	  call to kgdb_arch_late() is made.
> +
> +	  NOTE: Even if this isn't selected by an architecture we will
> +	  still try to register kgdb to handle breakpoints and crashes
> +	  when early_param's are parsed, we just won't act on the
> +	  "kgdbwait" parameter until dbg_late_init().  If you get a
> +	  crash and try to drop into kgdb somewhere between these two
> +	  places you might or might not end up being able to use kgdb
> +	  depending on exactly how far along the architecture has initted.
> +
>  endif # KGDB
> -- 
> 2.26.2.645.ge9eca65c58-goog
>