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
>
