* [RFC PATCH 0/2] arm64 kgdb fixes for single stepping @ 2020-02-13 3:11 ` minyard 0 siblings, 0 replies; 10+ messages in thread From: minyard @ 2020-02-13 3:11 UTC (permalink / raw) To: Catalin Marinas, Will Deacon, linux-arm-kernel; +Cc: linux-kernel I got a bug report about using kgdb on arm64, and it turns out it was fairly broken. Patch 2 has a description of what was going on. I am using a Marvell 8100 board. The following patches fix the problem, but probably not in the best way. They are what I hacked out to show the problems. I am not quite sure how this will interact with kprobes and hardware breakpoints which use the same code, but they would have been broken, too, so this is not making them any worse. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFC PATCH 0/2] arm64 kgdb fixes for single stepping @ 2020-02-13 3:11 ` minyard 0 siblings, 0 replies; 10+ messages in thread From: minyard @ 2020-02-13 3:11 UTC (permalink / raw) To: Catalin Marinas, Will Deacon, linux-arm-kernel; +Cc: linux-kernel I got a bug report about using kgdb on arm64, and it turns out it was fairly broken. Patch 2 has a description of what was going on. I am using a Marvell 8100 board. The following patches fix the problem, but probably not in the best way. They are what I hacked out to show the problems. I am not quite sure how this will interact with kprobes and hardware breakpoints which use the same code, but they would have been broken, too, so this is not making them any worse. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFC PATCH 1/2] arm64: Pass registers to all single-step handling routines 2020-02-13 3:11 ` minyard @ 2020-02-13 3:11 ` minyard -1 siblings, 0 replies; 10+ messages in thread From: minyard @ 2020-02-13 3:11 UTC (permalink / raw) To: Catalin Marinas, Will Deacon, linux-arm-kernel Cc: Corey Minyard, linux-kernel From: Corey Minyard <cminyard@mvista.com> Get ready to set the SS bit in the MDSCR register in the kernel restore handler. Signed-off-by: Corey Minyard <cminyard@mvista.com> --- arch/arm64/include/asm/debug-monitors.h | 4 ++-- arch/arm64/kernel/debug-monitors.c | 4 ++-- arch/arm64/kernel/hw_breakpoint.c | 6 +++--- arch/arm64/kernel/kgdb.c | 6 +++--- arch/arm64/kernel/probes/kprobes.c | 4 ++-- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h index 7619f473155f..73ce974bf754 100644 --- a/arch/arm64/include/asm/debug-monitors.h +++ b/arch/arm64/include/asm/debug-monitors.h @@ -111,8 +111,8 @@ void user_rewind_single_step(struct task_struct *task); void user_fastforward_single_step(struct task_struct *task); void kernel_enable_single_step(struct pt_regs *regs); -void kernel_disable_single_step(void); -int kernel_active_single_step(void); +void kernel_disable_single_step(struct pt_regs *regs); +int kernel_active_single_step(struct pt_regs *regs); #ifdef CONFIG_HAVE_HW_BREAKPOINT int reinstall_suspended_bps(struct pt_regs *regs); diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index 48222a4760c2..2a0dfd8b1265 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -414,7 +414,7 @@ void kernel_enable_single_step(struct pt_regs *regs) } NOKPROBE_SYMBOL(kernel_enable_single_step); -void kernel_disable_single_step(void) +void kernel_disable_single_step(struct pt_regs *regs) { WARN_ON(!irqs_disabled()); mdscr_write(mdscr_read() & ~DBG_MDSCR_SS); @@ -422,7 +422,7 @@ void kernel_disable_single_step(void) } NOKPROBE_SYMBOL(kernel_disable_single_step); -int kernel_active_single_step(void) +int kernel_active_single_step(struct pt_regs *regs) { WARN_ON(!irqs_disabled()); return mdscr_read() & DBG_MDSCR_SS; diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c index 0b727edf4104..785c9a5060a6 100644 --- a/arch/arm64/kernel/hw_breakpoint.c +++ b/arch/arm64/kernel/hw_breakpoint.c @@ -682,7 +682,7 @@ static int breakpoint_handler(unsigned long unused, unsigned int esr, if (*kernel_step != ARM_KERNEL_STEP_NONE) return 0; - if (kernel_active_single_step()) { + if (kernel_active_single_step(regs)) { *kernel_step = ARM_KERNEL_STEP_SUSPEND; } else { *kernel_step = ARM_KERNEL_STEP_ACTIVE; @@ -825,7 +825,7 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr, if (*kernel_step != ARM_KERNEL_STEP_NONE) return 0; - if (kernel_active_single_step()) { + if (kernel_active_single_step(regs)) { *kernel_step = ARM_KERNEL_STEP_SUSPEND; } else { *kernel_step = ARM_KERNEL_STEP_ACTIVE; @@ -882,7 +882,7 @@ int reinstall_suspended_bps(struct pt_regs *regs) toggle_bp_registers(AARCH64_DBG_REG_WCR, DBG_ACTIVE_EL0, 1); if (*kernel_step != ARM_KERNEL_STEP_SUSPEND) { - kernel_disable_single_step(); + kernel_disable_single_step(regs); handled_exception = 1; } else { handled_exception = 0; diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c index 43119922341f..220fe8fd6ace 100644 --- a/arch/arm64/kernel/kgdb.c +++ b/arch/arm64/kernel/kgdb.c @@ -200,8 +200,8 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, /* * Received continue command, disable single step */ - if (kernel_active_single_step()) - kernel_disable_single_step(); + if (kernel_active_single_step(linux_regs)) + kernel_disable_single_step(linux_regs); err = 0; break; @@ -221,7 +221,7 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, /* * Enable single step handling */ - if (!kernel_active_single_step()) + if (!kernel_active_single_step(linux_regs)) kernel_enable_single_step(linux_regs); err = 0; break; diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c index d1c95dcf1d78..3082dfc3cd99 100644 --- a/arch/arm64/kernel/probes/kprobes.c +++ b/arch/arm64/kernel/probes/kprobes.c @@ -308,7 +308,7 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr) if (!instruction_pointer(regs)) BUG(); - kernel_disable_single_step(); + kernel_disable_single_step(regs); if (kcb->kprobe_status == KPROBE_REENTER) restore_previous_kprobe(kcb); @@ -415,7 +415,7 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr) if (retval == DBG_HOOK_HANDLED) { kprobes_restore_local_irqflag(kcb, regs); - kernel_disable_single_step(); + kernel_disable_single_step(regs); post_kprobe_handler(kcb, regs); } -- 2.17.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [RFC PATCH 1/2] arm64: Pass registers to all single-step handling routines @ 2020-02-13 3:11 ` minyard 0 siblings, 0 replies; 10+ messages in thread From: minyard @ 2020-02-13 3:11 UTC (permalink / raw) To: Catalin Marinas, Will Deacon, linux-arm-kernel Cc: linux-kernel, Corey Minyard From: Corey Minyard <cminyard@mvista.com> Get ready to set the SS bit in the MDSCR register in the kernel restore handler. Signed-off-by: Corey Minyard <cminyard@mvista.com> --- arch/arm64/include/asm/debug-monitors.h | 4 ++-- arch/arm64/kernel/debug-monitors.c | 4 ++-- arch/arm64/kernel/hw_breakpoint.c | 6 +++--- arch/arm64/kernel/kgdb.c | 6 +++--- arch/arm64/kernel/probes/kprobes.c | 4 ++-- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h index 7619f473155f..73ce974bf754 100644 --- a/arch/arm64/include/asm/debug-monitors.h +++ b/arch/arm64/include/asm/debug-monitors.h @@ -111,8 +111,8 @@ void user_rewind_single_step(struct task_struct *task); void user_fastforward_single_step(struct task_struct *task); void kernel_enable_single_step(struct pt_regs *regs); -void kernel_disable_single_step(void); -int kernel_active_single_step(void); +void kernel_disable_single_step(struct pt_regs *regs); +int kernel_active_single_step(struct pt_regs *regs); #ifdef CONFIG_HAVE_HW_BREAKPOINT int reinstall_suspended_bps(struct pt_regs *regs); diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index 48222a4760c2..2a0dfd8b1265 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -414,7 +414,7 @@ void kernel_enable_single_step(struct pt_regs *regs) } NOKPROBE_SYMBOL(kernel_enable_single_step); -void kernel_disable_single_step(void) +void kernel_disable_single_step(struct pt_regs *regs) { WARN_ON(!irqs_disabled()); mdscr_write(mdscr_read() & ~DBG_MDSCR_SS); @@ -422,7 +422,7 @@ void kernel_disable_single_step(void) } NOKPROBE_SYMBOL(kernel_disable_single_step); -int kernel_active_single_step(void) +int kernel_active_single_step(struct pt_regs *regs) { WARN_ON(!irqs_disabled()); return mdscr_read() & DBG_MDSCR_SS; diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c index 0b727edf4104..785c9a5060a6 100644 --- a/arch/arm64/kernel/hw_breakpoint.c +++ b/arch/arm64/kernel/hw_breakpoint.c @@ -682,7 +682,7 @@ static int breakpoint_handler(unsigned long unused, unsigned int esr, if (*kernel_step != ARM_KERNEL_STEP_NONE) return 0; - if (kernel_active_single_step()) { + if (kernel_active_single_step(regs)) { *kernel_step = ARM_KERNEL_STEP_SUSPEND; } else { *kernel_step = ARM_KERNEL_STEP_ACTIVE; @@ -825,7 +825,7 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr, if (*kernel_step != ARM_KERNEL_STEP_NONE) return 0; - if (kernel_active_single_step()) { + if (kernel_active_single_step(regs)) { *kernel_step = ARM_KERNEL_STEP_SUSPEND; } else { *kernel_step = ARM_KERNEL_STEP_ACTIVE; @@ -882,7 +882,7 @@ int reinstall_suspended_bps(struct pt_regs *regs) toggle_bp_registers(AARCH64_DBG_REG_WCR, DBG_ACTIVE_EL0, 1); if (*kernel_step != ARM_KERNEL_STEP_SUSPEND) { - kernel_disable_single_step(); + kernel_disable_single_step(regs); handled_exception = 1; } else { handled_exception = 0; diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c index 43119922341f..220fe8fd6ace 100644 --- a/arch/arm64/kernel/kgdb.c +++ b/arch/arm64/kernel/kgdb.c @@ -200,8 +200,8 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, /* * Received continue command, disable single step */ - if (kernel_active_single_step()) - kernel_disable_single_step(); + if (kernel_active_single_step(linux_regs)) + kernel_disable_single_step(linux_regs); err = 0; break; @@ -221,7 +221,7 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, /* * Enable single step handling */ - if (!kernel_active_single_step()) + if (!kernel_active_single_step(linux_regs)) kernel_enable_single_step(linux_regs); err = 0; break; diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c index d1c95dcf1d78..3082dfc3cd99 100644 --- a/arch/arm64/kernel/probes/kprobes.c +++ b/arch/arm64/kernel/probes/kprobes.c @@ -308,7 +308,7 @@ int __kprobes kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr) if (!instruction_pointer(regs)) BUG(); - kernel_disable_single_step(); + kernel_disable_single_step(regs); if (kcb->kprobe_status == KPROBE_REENTER) restore_previous_kprobe(kcb); @@ -415,7 +415,7 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr) if (retval == DBG_HOOK_HANDLED) { kprobes_restore_local_irqflag(kcb, regs); - kernel_disable_single_step(); + kernel_disable_single_step(regs); post_kprobe_handler(kcb, regs); } -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [RFC PATCH 2/2] arm64:kgdb: Fix kernel single-stepping 2020-02-13 3:11 ` minyard @ 2020-02-13 3:11 ` minyard -1 siblings, 0 replies; 10+ messages in thread From: minyard @ 2020-02-13 3:11 UTC (permalink / raw) To: Catalin Marinas, Will Deacon, linux-arm-kernel Cc: Corey Minyard, linux-kernel From: Corey Minyard <cminyard@mvista.com> Single stepping on arm64 in kernel mode was just broken. Here are the problems: If single step is set and an interrupt is pending, the interrupt is taken. The interrupt should run and then return and the single stepped instruction run. However, instead, as soon as the kernel calls enable_dbg, the single step happens right there. To fix this, disable SS in the MDSCR register on entry and save it's value for later use. The SS bit in the MDSCR register is set globally for the CPU, not for the task being single stepped. If the task migrates, that could cause the bit to be set on the wrong core. So instead, store the value of SS in the thread structure and set it properly on exit back to the kernel. If a single step occurs, it clears the SS bit in PSTATE. So subsiquent single steps will not work. The bit needs to be reset on each single step operation. Signed-off-by: Corey Minyard <cminyard@mvista.com> --- arch/arm64/include/asm/ptrace.h | 4 ++-- arch/arm64/kernel/asm-offsets.c | 1 + arch/arm64/kernel/debug-monitors.c | 7 ++++--- arch/arm64/kernel/entry.S | 21 +++++++++++++++++++++ arch/arm64/kernel/kgdb.c | 3 +++ 5 files changed, 31 insertions(+), 5 deletions(-) diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h index fbebb411ae20..a07847deff8f 100644 --- a/arch/arm64/include/asm/ptrace.h +++ b/arch/arm64/include/asm/ptrace.h @@ -168,11 +168,11 @@ struct pt_regs { }; u64 orig_x0; #ifdef __AARCH64EB__ - u32 unused2; + u32 ss_enable; /* Kernel single-step for a task */ s32 syscallno; #else s32 syscallno; - u32 unused2; + u32 ss_enable; #endif u64 orig_addr_limit; diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c index a5bdce8af65b..038e76d2f0c4 100644 --- a/arch/arm64/kernel/asm-offsets.c +++ b/arch/arm64/kernel/asm-offsets.c @@ -62,6 +62,7 @@ int main(void) DEFINE(S_PSTATE, offsetof(struct pt_regs, pstate)); DEFINE(S_PC, offsetof(struct pt_regs, pc)); DEFINE(S_SYSCALLNO, offsetof(struct pt_regs, syscallno)); + DEFINE(S_SS_ENABLE, offsetof(struct pt_regs, ss_enable)); DEFINE(S_ORIG_ADDR_LIMIT, offsetof(struct pt_regs, orig_addr_limit)); DEFINE(S_PMR_SAVE, offsetof(struct pt_regs, pmr_save)); DEFINE(S_STACKFRAME, offsetof(struct pt_regs, stackframe)); diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index 2a0dfd8b1265..d4085cfef5a7 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -409,7 +409,7 @@ void kernel_enable_single_step(struct pt_regs *regs) { WARN_ON(!irqs_disabled()); set_regs_spsr_ss(regs); - mdscr_write(mdscr_read() | DBG_MDSCR_SS); + regs->ss_enable = DBG_MDSCR_SS; enable_debug_monitors(DBG_ACTIVE_EL1); } NOKPROBE_SYMBOL(kernel_enable_single_step); @@ -417,7 +417,8 @@ NOKPROBE_SYMBOL(kernel_enable_single_step); void kernel_disable_single_step(struct pt_regs *regs) { WARN_ON(!irqs_disabled()); - mdscr_write(mdscr_read() & ~DBG_MDSCR_SS); + regs->ss_enable = 0; + clear_regs_spsr_ss(regs); disable_debug_monitors(DBG_ACTIVE_EL1); } NOKPROBE_SYMBOL(kernel_disable_single_step); @@ -425,7 +426,7 @@ NOKPROBE_SYMBOL(kernel_disable_single_step); int kernel_active_single_step(struct pt_regs *regs) { WARN_ON(!irqs_disabled()); - return mdscr_read() & DBG_MDSCR_SS; + return regs->ss_enable; } NOKPROBE_SYMBOL(kernel_active_single_step); diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 7c6a0a41676f..fcf979b17d94 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -174,6 +174,17 @@ alternative_cb_end apply_ssbd 1, x22, x23 .else + /* + * If single-step was enabled, save it off and disable it, + * or it will trap on clearing the D bit in PSTATE. + * The restore code will re-enable it if necessary. + */ + mrs x20, mdscr_el1 + bic x21, x20, #1 + msr mdscr_el1, x21 + and x20, x20, #1 + str w20, [sp, #S_SS_ENABLE] + add x21, sp, #S_FRAME_SIZE get_current_task tsk /* Save the task's original addr_limit and set USER_DS */ @@ -349,6 +360,16 @@ alternative_else_nop_endif msr elr_el1, x21 // set up the return data msr spsr_el1, x22 + + .if \el != 0 + /* Restore the single-step bit. */ + ldr w21, [sp, #S_SS_ENABLE] + mrs x20, mdscr_el1 + orr x20, x20, x21 + msr mdscr_el1, x20 + /* PSTATE.D and PSTATE.SS will be restored from SPSR_EL1. */ + .endif + ldp x0, x1, [sp, #16 * 0] ldp x2, x3, [sp, #16 * 1] ldp x4, x5, [sp, #16 * 2] diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c index 220fe8fd6ace..260f12c76b6e 100644 --- a/arch/arm64/kernel/kgdb.c +++ b/arch/arm64/kernel/kgdb.c @@ -223,6 +223,9 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, */ if (!kernel_active_single_step(linux_regs)) kernel_enable_single_step(linux_regs); + else + /* Doing a single-step clears ss, reset it. */ + linux_regs->pstate |= DBG_SPSR_SS; err = 0; break; default: -- 2.17.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [RFC PATCH 2/2] arm64:kgdb: Fix kernel single-stepping @ 2020-02-13 3:11 ` minyard 0 siblings, 0 replies; 10+ messages in thread From: minyard @ 2020-02-13 3:11 UTC (permalink / raw) To: Catalin Marinas, Will Deacon, linux-arm-kernel Cc: linux-kernel, Corey Minyard From: Corey Minyard <cminyard@mvista.com> Single stepping on arm64 in kernel mode was just broken. Here are the problems: If single step is set and an interrupt is pending, the interrupt is taken. The interrupt should run and then return and the single stepped instruction run. However, instead, as soon as the kernel calls enable_dbg, the single step happens right there. To fix this, disable SS in the MDSCR register on entry and save it's value for later use. The SS bit in the MDSCR register is set globally for the CPU, not for the task being single stepped. If the task migrates, that could cause the bit to be set on the wrong core. So instead, store the value of SS in the thread structure and set it properly on exit back to the kernel. If a single step occurs, it clears the SS bit in PSTATE. So subsiquent single steps will not work. The bit needs to be reset on each single step operation. Signed-off-by: Corey Minyard <cminyard@mvista.com> --- arch/arm64/include/asm/ptrace.h | 4 ++-- arch/arm64/kernel/asm-offsets.c | 1 + arch/arm64/kernel/debug-monitors.c | 7 ++++--- arch/arm64/kernel/entry.S | 21 +++++++++++++++++++++ arch/arm64/kernel/kgdb.c | 3 +++ 5 files changed, 31 insertions(+), 5 deletions(-) diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h index fbebb411ae20..a07847deff8f 100644 --- a/arch/arm64/include/asm/ptrace.h +++ b/arch/arm64/include/asm/ptrace.h @@ -168,11 +168,11 @@ struct pt_regs { }; u64 orig_x0; #ifdef __AARCH64EB__ - u32 unused2; + u32 ss_enable; /* Kernel single-step for a task */ s32 syscallno; #else s32 syscallno; - u32 unused2; + u32 ss_enable; #endif u64 orig_addr_limit; diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c index a5bdce8af65b..038e76d2f0c4 100644 --- a/arch/arm64/kernel/asm-offsets.c +++ b/arch/arm64/kernel/asm-offsets.c @@ -62,6 +62,7 @@ int main(void) DEFINE(S_PSTATE, offsetof(struct pt_regs, pstate)); DEFINE(S_PC, offsetof(struct pt_regs, pc)); DEFINE(S_SYSCALLNO, offsetof(struct pt_regs, syscallno)); + DEFINE(S_SS_ENABLE, offsetof(struct pt_regs, ss_enable)); DEFINE(S_ORIG_ADDR_LIMIT, offsetof(struct pt_regs, orig_addr_limit)); DEFINE(S_PMR_SAVE, offsetof(struct pt_regs, pmr_save)); DEFINE(S_STACKFRAME, offsetof(struct pt_regs, stackframe)); diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c index 2a0dfd8b1265..d4085cfef5a7 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -409,7 +409,7 @@ void kernel_enable_single_step(struct pt_regs *regs) { WARN_ON(!irqs_disabled()); set_regs_spsr_ss(regs); - mdscr_write(mdscr_read() | DBG_MDSCR_SS); + regs->ss_enable = DBG_MDSCR_SS; enable_debug_monitors(DBG_ACTIVE_EL1); } NOKPROBE_SYMBOL(kernel_enable_single_step); @@ -417,7 +417,8 @@ NOKPROBE_SYMBOL(kernel_enable_single_step); void kernel_disable_single_step(struct pt_regs *regs) { WARN_ON(!irqs_disabled()); - mdscr_write(mdscr_read() & ~DBG_MDSCR_SS); + regs->ss_enable = 0; + clear_regs_spsr_ss(regs); disable_debug_monitors(DBG_ACTIVE_EL1); } NOKPROBE_SYMBOL(kernel_disable_single_step); @@ -425,7 +426,7 @@ NOKPROBE_SYMBOL(kernel_disable_single_step); int kernel_active_single_step(struct pt_regs *regs) { WARN_ON(!irqs_disabled()); - return mdscr_read() & DBG_MDSCR_SS; + return regs->ss_enable; } NOKPROBE_SYMBOL(kernel_active_single_step); diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 7c6a0a41676f..fcf979b17d94 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -174,6 +174,17 @@ alternative_cb_end apply_ssbd 1, x22, x23 .else + /* + * If single-step was enabled, save it off and disable it, + * or it will trap on clearing the D bit in PSTATE. + * The restore code will re-enable it if necessary. + */ + mrs x20, mdscr_el1 + bic x21, x20, #1 + msr mdscr_el1, x21 + and x20, x20, #1 + str w20, [sp, #S_SS_ENABLE] + add x21, sp, #S_FRAME_SIZE get_current_task tsk /* Save the task's original addr_limit and set USER_DS */ @@ -349,6 +360,16 @@ alternative_else_nop_endif msr elr_el1, x21 // set up the return data msr spsr_el1, x22 + + .if \el != 0 + /* Restore the single-step bit. */ + ldr w21, [sp, #S_SS_ENABLE] + mrs x20, mdscr_el1 + orr x20, x20, x21 + msr mdscr_el1, x20 + /* PSTATE.D and PSTATE.SS will be restored from SPSR_EL1. */ + .endif + ldp x0, x1, [sp, #16 * 0] ldp x2, x3, [sp, #16 * 1] ldp x4, x5, [sp, #16 * 2] diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c index 220fe8fd6ace..260f12c76b6e 100644 --- a/arch/arm64/kernel/kgdb.c +++ b/arch/arm64/kernel/kgdb.c @@ -223,6 +223,9 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, */ if (!kernel_active_single_step(linux_regs)) kernel_enable_single_step(linux_regs); + else + /* Doing a single-step clears ss, reset it. */ + linux_regs->pstate |= DBG_SPSR_SS; err = 0; break; default: -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 0/2] arm64 kgdb fixes for single stepping 2020-02-13 3:11 ` minyard @ 2020-02-13 10:10 ` Will Deacon -1 siblings, 0 replies; 10+ messages in thread From: Will Deacon @ 2020-02-13 10:10 UTC (permalink / raw) To: minyard; +Cc: Catalin Marinas, linux-kernel, linux-arm-kernel On Wed, Feb 12, 2020 at 09:11:29PM -0600, minyard@acm.org wrote: > I got a bug report about using kgdb on arm64, and it turns out it was > fairly broken. Patch 2 has a description of what was going on. I am > using a Marvell 8100 board. > > The following patches fix the problem, but probably not in the > best way. They are what I hacked out to show the problems. > > I am not quite sure how this will interact with kprobes and hardware > breakpoints which use the same code, but they would have been broken, > too, so this is not making them any worse. This should all be handled by kgdb itself, not by changing the low-level debug exception handling. For example, the '&kgdb_step_hook' can take care of re-arming the step state machine and kgdb can also simply disable interrupts during the step if it doesn't want to step into the handler. Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 0/2] arm64 kgdb fixes for single stepping @ 2020-02-13 10:10 ` Will Deacon 0 siblings, 0 replies; 10+ messages in thread From: Will Deacon @ 2020-02-13 10:10 UTC (permalink / raw) To: minyard; +Cc: Catalin Marinas, linux-arm-kernel, linux-kernel On Wed, Feb 12, 2020 at 09:11:29PM -0600, minyard@acm.org wrote: > I got a bug report about using kgdb on arm64, and it turns out it was > fairly broken. Patch 2 has a description of what was going on. I am > using a Marvell 8100 board. > > The following patches fix the problem, but probably not in the > best way. They are what I hacked out to show the problems. > > I am not quite sure how this will interact with kprobes and hardware > breakpoints which use the same code, but they would have been broken, > too, so this is not making them any worse. This should all be handled by kgdb itself, not by changing the low-level debug exception handling. For example, the '&kgdb_step_hook' can take care of re-arming the step state machine and kgdb can also simply disable interrupts during the step if it doesn't want to step into the handler. Will ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 0/2] arm64 kgdb fixes for single stepping 2020-02-13 10:10 ` Will Deacon @ 2020-02-13 15:57 ` Corey Minyard -1 siblings, 0 replies; 10+ messages in thread From: Corey Minyard @ 2020-02-13 15:57 UTC (permalink / raw) To: Will Deacon; +Cc: Catalin Marinas, linux-kernel, linux-arm-kernel On Thu, Feb 13, 2020 at 10:10:58AM +0000, Will Deacon wrote: > On Wed, Feb 12, 2020 at 09:11:29PM -0600, minyard@acm.org wrote: > > I got a bug report about using kgdb on arm64, and it turns out it was > > fairly broken. Patch 2 has a description of what was going on. I am > > using a Marvell 8100 board. > > > > The following patches fix the problem, but probably not in the > > best way. They are what I hacked out to show the problems. > > > > I am not quite sure how this will interact with kprobes and hardware > > breakpoints which use the same code, but they would have been broken, > > too, so this is not making them any worse. > > This should all be handled by kgdb itself, not by changing the low-level > debug exception handling. For example, the '&kgdb_step_hook' can take > care of re-arming the step state machine and kgdb can also simply disable > interrupts during the step if it doesn't want to step into the handler. How can kgdb disable the SS bit in MDSRC, or re-enable it on the right CPU, without doing this in the exception handling? I'm actually thinking that this may be a hardware bug. Looking at the ARMv8 manual, it looks like PSTATE.SS should be set to 0 if the processor takes an exception. That's definitely not happening; if I do an instruction step from, say, sys_sync(), it gets the single-step trap on the instruction after the PSTATE.D bit is disabled in el1_irq. Even so, I think the migration issue is still a problem. If you do an eret set up for single-step, and interrupts are on, and you get a timer interrupt, it could migrate the task to a different CPU if PREEMPT_ENABLE is set, right? If so, the MDSRC.SS bit will be set on the wrong CPU and the single step trap won't happen. That will break kprobes, too. You mention turning off interrupts in kgdb when single-stepping, which you could do and it would solve this problem. But it wouldn't solve the problem of taking a paging exception, which you want to take in this case. And you could still migrate on a paging exception. So I don't think disabling interrupts is a good solution. I don't see a solution besides clearing MDSCR.SS on an el1 exception entry and conditionally setting it on an el1 exception return. It might be better to have a thread flag to do this instead of depending on the setting of that bit; I'm not sure how expensive accessing the MDSRC register is. Setting SPSR.SS on subsequent single steps is definitely an issue, but I can split that out into a separate patch. -corey > > Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 0/2] arm64 kgdb fixes for single stepping @ 2020-02-13 15:57 ` Corey Minyard 0 siblings, 0 replies; 10+ messages in thread From: Corey Minyard @ 2020-02-13 15:57 UTC (permalink / raw) To: Will Deacon; +Cc: Catalin Marinas, linux-arm-kernel, linux-kernel On Thu, Feb 13, 2020 at 10:10:58AM +0000, Will Deacon wrote: > On Wed, Feb 12, 2020 at 09:11:29PM -0600, minyard@acm.org wrote: > > I got a bug report about using kgdb on arm64, and it turns out it was > > fairly broken. Patch 2 has a description of what was going on. I am > > using a Marvell 8100 board. > > > > The following patches fix the problem, but probably not in the > > best way. They are what I hacked out to show the problems. > > > > I am not quite sure how this will interact with kprobes and hardware > > breakpoints which use the same code, but they would have been broken, > > too, so this is not making them any worse. > > This should all be handled by kgdb itself, not by changing the low-level > debug exception handling. For example, the '&kgdb_step_hook' can take > care of re-arming the step state machine and kgdb can also simply disable > interrupts during the step if it doesn't want to step into the handler. How can kgdb disable the SS bit in MDSRC, or re-enable it on the right CPU, without doing this in the exception handling? I'm actually thinking that this may be a hardware bug. Looking at the ARMv8 manual, it looks like PSTATE.SS should be set to 0 if the processor takes an exception. That's definitely not happening; if I do an instruction step from, say, sys_sync(), it gets the single-step trap on the instruction after the PSTATE.D bit is disabled in el1_irq. Even so, I think the migration issue is still a problem. If you do an eret set up for single-step, and interrupts are on, and you get a timer interrupt, it could migrate the task to a different CPU if PREEMPT_ENABLE is set, right? If so, the MDSRC.SS bit will be set on the wrong CPU and the single step trap won't happen. That will break kprobes, too. You mention turning off interrupts in kgdb when single-stepping, which you could do and it would solve this problem. But it wouldn't solve the problem of taking a paging exception, which you want to take in this case. And you could still migrate on a paging exception. So I don't think disabling interrupts is a good solution. I don't see a solution besides clearing MDSCR.SS on an el1 exception entry and conditionally setting it on an el1 exception return. It might be better to have a thread flag to do this instead of depending on the setting of that bit; I'm not sure how expensive accessing the MDSRC register is. Setting SPSR.SS on subsequent single steps is definitely an issue, but I can split that out into a separate patch. -corey > > Will ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-02-13 15:57 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-02-13 3:11 [RFC PATCH 0/2] arm64 kgdb fixes for single stepping minyard 2020-02-13 3:11 ` minyard 2020-02-13 3:11 ` [RFC PATCH 1/2] arm64: Pass registers to all single-step handling routines minyard 2020-02-13 3:11 ` minyard 2020-02-13 3:11 ` [RFC PATCH 2/2] arm64:kgdb: Fix kernel single-stepping minyard 2020-02-13 3:11 ` minyard 2020-02-13 10:10 ` [RFC PATCH 0/2] arm64 kgdb fixes for single stepping Will Deacon 2020-02-13 10:10 ` Will Deacon 2020-02-13 15:57 ` Corey Minyard 2020-02-13 15:57 ` Corey Minyard
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.