| Message ID | 20220413065458.88541-3-sumit.garg@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Fix pending single-step debugging issues | expand |
On Wed, Apr 13, 2022 at 12:24:58PM +0530, Sumit Garg wrote: > After fixing wrongly single-stepping into the irq handler, when we execute > single-step in kdb/kgdb, we can see only the first step can work. I might be nitpicking since, again, I've no problems with the code but... I'd rather this patch description focused on what this patch does rather than what the patch before it does! Something more like: Currently only the first attempt to single-step has any effect. After that all further stepping remains "stuck" at the same program counter value. Daniel. > Refer to the ARM Architecture Reference Manual (ARM DDI 0487E.a) D2.12, > i think PSTATE.SS=1 should be set each step for transferring the PE to the > 'Active-not-pending' state. The problem here is PSTATE.SS=1 is not set > since the second single-step. > > After the first single-step, the PE transferes to the 'Inactive' state, > with PSTATE.SS=0 and MDSCR.SS=1, thus PSTATE.SS won't be set to 1 due to > kernel_active_single_step()=true. Then the PE transferes to the > 'Active-pending' state when ERET and returns to the debugger by step > exception. > > Before this patch: > ================== > Entering kdb (current=0xffff3376039f0000, pid 1) on processor 0 due to Keyboard Entry > [0]kdb> > > [0]kdb> > [0]kdb> bp write_sysrq_trigger > Instruction(i) BP #0 at 0xffffa45c13d09290 (write_sysrq_trigger) > is enabled addr at ffffa45c13d09290, hardtype=0 installed=0 > > [0]kdb> go > $ echo h > /proc/sysrq-trigger > > Entering kdb (current=0xffff4f7e453f8000, pid 175) on processor 1 due to Breakpoint @ 0xffffad651a309290 > [1]kdb> ss > > Entering kdb (current=0xffff4f7e453f8000, pid 175) on processor 1 due to SS trap @ 0xffffad651a309294 > [1]kdb> ss > > Entering kdb (current=0xffff4f7e453f8000, pid 175) on processor 1 due to SS trap @ 0xffffad651a309294 > [1]kdb> > > After this patch: > ================= > Entering kdb (current=0xffff6851c39f0000, pid 1) on processor 0 due to Keyboard Entry > [0]kdb> bp write_sysrq_trigger > Instruction(i) BP #0 at 0xffffc02d2dd09290 (write_sysrq_trigger) > is enabled addr at ffffc02d2dd09290, hardtype=0 installed=0 > > [0]kdb> go > $ echo h > /proc/sysrq-trigger > > Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to Breakpoint @ 0xffffc02d2dd09290 > [1]kdb> ss > > Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to SS trap @ 0xffffc02d2dd09294 > [1]kdb> ss > > Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to SS trap @ 0xffffc02d2dd09298 > [1]kdb> ss > > Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to SS trap @ 0xffffc02d2dd0929c > [1]kdb> > > Fixes: 44679a4f142b ("arm64: KGDB: Add step debugging support") > Co-developed-by: Wei Li <liwei391@huawei.com> > Signed-off-by: Wei Li <liwei391@huawei.com> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > --- > arch/arm64/include/asm/debug-monitors.h | 1 + > arch/arm64/kernel/debug-monitors.c | 5 +++++ > arch/arm64/kernel/kgdb.c | 2 ++ > 3 files changed, 8 insertions(+) > > diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h > index 00c291067e57..9e1e864d6440 100644 > --- a/arch/arm64/include/asm/debug-monitors.h > +++ b/arch/arm64/include/asm/debug-monitors.h > @@ -104,6 +104,7 @@ void user_regs_reset_single_step(struct user_pt_regs *regs, > void kernel_enable_single_step(struct pt_regs *regs); > void kernel_disable_single_step(void); > int kernel_active_single_step(void); > +void kernel_regs_reset_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 4f3661eeb7ec..ea3f410aa385 100644 > --- a/arch/arm64/kernel/debug-monitors.c > +++ b/arch/arm64/kernel/debug-monitors.c > @@ -438,6 +438,11 @@ int kernel_active_single_step(void) > } > NOKPROBE_SYMBOL(kernel_active_single_step); > > +void kernel_regs_reset_single_step(struct pt_regs *regs) > +{ > + set_regs_spsr_ss(regs); > +} > + > /* ptrace API */ > void user_enable_single_step(struct task_struct *task) > { > diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c > index 2aede780fb80..acf2196b1e9b 100644 > --- a/arch/arm64/kernel/kgdb.c > +++ b/arch/arm64/kernel/kgdb.c > @@ -224,6 +224,8 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, > */ > if (!kernel_active_single_step()) > kernel_enable_single_step(linux_regs); > + else > + kernel_regs_reset_single_step(linux_regs); > err = 0; > break; > default: > -- > 2.25.1
On Fri, 6 May 2022 at 21:43, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Wed, Apr 13, 2022 at 12:24:58PM +0530, Sumit Garg wrote: > > After fixing wrongly single-stepping into the irq handler, when we execute > > single-step in kdb/kgdb, we can see only the first step can work. > > I might be nitpicking since, again, I've no problems with the code > but... I'd rather this patch description focused on what this patch > does rather than what the patch before it does! > > Something more like: > > Currently only the first attempt to single-step has any effect. > After that all further stepping remains "stuck" at the same program > counter value. > Ack, I will use this instead. -Sumit > > Daniel. > > > > > > Refer to the ARM Architecture Reference Manual (ARM DDI 0487E.a) D2.12, > > i think PSTATE.SS=1 should be set each step for transferring the PE to the > > 'Active-not-pending' state. The problem here is PSTATE.SS=1 is not set > > since the second single-step. > > > > After the first single-step, the PE transferes to the 'Inactive' state, > > with PSTATE.SS=0 and MDSCR.SS=1, thus PSTATE.SS won't be set to 1 due to > > kernel_active_single_step()=true. Then the PE transferes to the > > 'Active-pending' state when ERET and returns to the debugger by step > > exception. > > > > Before this patch: > > ================== > > Entering kdb (current=0xffff3376039f0000, pid 1) on processor 0 due to Keyboard Entry > > [0]kdb> > > > > [0]kdb> > > [0]kdb> bp write_sysrq_trigger > > Instruction(i) BP #0 at 0xffffa45c13d09290 (write_sysrq_trigger) > > is enabled addr at ffffa45c13d09290, hardtype=0 installed=0 > > > > [0]kdb> go > > $ echo h > /proc/sysrq-trigger > > > > Entering kdb (current=0xffff4f7e453f8000, pid 175) on processor 1 due to Breakpoint @ 0xffffad651a309290 > > [1]kdb> ss > > > > Entering kdb (current=0xffff4f7e453f8000, pid 175) on processor 1 due to SS trap @ 0xffffad651a309294 > > [1]kdb> ss > > > > Entering kdb (current=0xffff4f7e453f8000, pid 175) on processor 1 due to SS trap @ 0xffffad651a309294 > > [1]kdb> > > > > After this patch: > > ================= > > Entering kdb (current=0xffff6851c39f0000, pid 1) on processor 0 due to Keyboard Entry > > [0]kdb> bp write_sysrq_trigger > > Instruction(i) BP #0 at 0xffffc02d2dd09290 (write_sysrq_trigger) > > is enabled addr at ffffc02d2dd09290, hardtype=0 installed=0 > > > > [0]kdb> go > > $ echo h > /proc/sysrq-trigger > > > > Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to Breakpoint @ 0xffffc02d2dd09290 > > [1]kdb> ss > > > > Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to SS trap @ 0xffffc02d2dd09294 > > [1]kdb> ss > > > > Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to SS trap @ 0xffffc02d2dd09298 > > [1]kdb> ss > > > > Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to SS trap @ 0xffffc02d2dd0929c > > [1]kdb> > > > > Fixes: 44679a4f142b ("arm64: KGDB: Add step debugging support") > > Co-developed-by: Wei Li <liwei391@huawei.com> > > Signed-off-by: Wei Li <liwei391@huawei.com> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > --- > > arch/arm64/include/asm/debug-monitors.h | 1 + > > arch/arm64/kernel/debug-monitors.c | 5 +++++ > > arch/arm64/kernel/kgdb.c | 2 ++ > > 3 files changed, 8 insertions(+) > > > > diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h > > index 00c291067e57..9e1e864d6440 100644 > > --- a/arch/arm64/include/asm/debug-monitors.h > > +++ b/arch/arm64/include/asm/debug-monitors.h > > @@ -104,6 +104,7 @@ void user_regs_reset_single_step(struct user_pt_regs *regs, > > void kernel_enable_single_step(struct pt_regs *regs); > > void kernel_disable_single_step(void); > > int kernel_active_single_step(void); > > +void kernel_regs_reset_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 4f3661eeb7ec..ea3f410aa385 100644 > > --- a/arch/arm64/kernel/debug-monitors.c > > +++ b/arch/arm64/kernel/debug-monitors.c > > @@ -438,6 +438,11 @@ int kernel_active_single_step(void) > > } > > NOKPROBE_SYMBOL(kernel_active_single_step); > > > > +void kernel_regs_reset_single_step(struct pt_regs *regs) > > +{ > > + set_regs_spsr_ss(regs); > > +} > > + > > /* ptrace API */ > > void user_enable_single_step(struct task_struct *task) > > { > > diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c > > index 2aede780fb80..acf2196b1e9b 100644 > > --- a/arch/arm64/kernel/kgdb.c > > +++ b/arch/arm64/kernel/kgdb.c > > @@ -224,6 +224,8 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, > > */ > > if (!kernel_active_single_step()) > > kernel_enable_single_step(linux_regs); > > + else > > + kernel_regs_reset_single_step(linux_regs); > > err = 0; > > break; > > default: > > -- > > 2.25.1
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h index 00c291067e57..9e1e864d6440 100644 --- a/arch/arm64/include/asm/debug-monitors.h +++ b/arch/arm64/include/asm/debug-monitors.h @@ -104,6 +104,7 @@ void user_regs_reset_single_step(struct user_pt_regs *regs, void kernel_enable_single_step(struct pt_regs *regs); void kernel_disable_single_step(void); int kernel_active_single_step(void); +void kernel_regs_reset_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 4f3661eeb7ec..ea3f410aa385 100644 --- a/arch/arm64/kernel/debug-monitors.c +++ b/arch/arm64/kernel/debug-monitors.c @@ -438,6 +438,11 @@ int kernel_active_single_step(void) } NOKPROBE_SYMBOL(kernel_active_single_step); +void kernel_regs_reset_single_step(struct pt_regs *regs) +{ + set_regs_spsr_ss(regs); +} + /* ptrace API */ void user_enable_single_step(struct task_struct *task) { diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c index 2aede780fb80..acf2196b1e9b 100644 --- a/arch/arm64/kernel/kgdb.c +++ b/arch/arm64/kernel/kgdb.c @@ -224,6 +224,8 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, */ if (!kernel_active_single_step()) kernel_enable_single_step(linux_regs); + else + kernel_regs_reset_single_step(linux_regs); err = 0; break; default:
After fixing wrongly single-stepping into the irq handler, when we execute single-step in kdb/kgdb, we can see only the first step can work. Refer to the ARM Architecture Reference Manual (ARM DDI 0487E.a) D2.12, i think PSTATE.SS=1 should be set each step for transferring the PE to the 'Active-not-pending' state. The problem here is PSTATE.SS=1 is not set since the second single-step. After the first single-step, the PE transferes to the 'Inactive' state, with PSTATE.SS=0 and MDSCR.SS=1, thus PSTATE.SS won't be set to 1 due to kernel_active_single_step()=true. Then the PE transferes to the 'Active-pending' state when ERET and returns to the debugger by step exception. Before this patch: ================== Entering kdb (current=0xffff3376039f0000, pid 1) on processor 0 due to Keyboard Entry [0]kdb> [0]kdb> [0]kdb> bp write_sysrq_trigger Instruction(i) BP #0 at 0xffffa45c13d09290 (write_sysrq_trigger) is enabled addr at ffffa45c13d09290, hardtype=0 installed=0 [0]kdb> go $ echo h > /proc/sysrq-trigger Entering kdb (current=0xffff4f7e453f8000, pid 175) on processor 1 due to Breakpoint @ 0xffffad651a309290 [1]kdb> ss Entering kdb (current=0xffff4f7e453f8000, pid 175) on processor 1 due to SS trap @ 0xffffad651a309294 [1]kdb> ss Entering kdb (current=0xffff4f7e453f8000, pid 175) on processor 1 due to SS trap @ 0xffffad651a309294 [1]kdb> After this patch: ================= Entering kdb (current=0xffff6851c39f0000, pid 1) on processor 0 due to Keyboard Entry [0]kdb> bp write_sysrq_trigger Instruction(i) BP #0 at 0xffffc02d2dd09290 (write_sysrq_trigger) is enabled addr at ffffc02d2dd09290, hardtype=0 installed=0 [0]kdb> go $ echo h > /proc/sysrq-trigger Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to Breakpoint @ 0xffffc02d2dd09290 [1]kdb> ss Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to SS trap @ 0xffffc02d2dd09294 [1]kdb> ss Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to SS trap @ 0xffffc02d2dd09298 [1]kdb> ss Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to SS trap @ 0xffffc02d2dd0929c [1]kdb> Fixes: 44679a4f142b ("arm64: KGDB: Add step debugging support") Co-developed-by: Wei Li <liwei391@huawei.com> Signed-off-by: Wei Li <liwei391@huawei.com> Signed-off-by: Sumit Garg <sumit.garg@linaro.org> --- arch/arm64/include/asm/debug-monitors.h | 1 + arch/arm64/kernel/debug-monitors.c | 5 +++++ arch/arm64/kernel/kgdb.c | 2 ++ 3 files changed, 8 insertions(+)