* [PATCH v2 0/2] arm64: Fix pending single-step debugging issues @ 2022-04-13 6:54 Sumit Garg 2022-04-13 6:54 ` [PATCH v2 1/2] arm64: entry: Skip single stepping interrupt handlers Sumit Garg 2022-04-13 6:54 ` [PATCH v2 2/2] arm64: kgdb: Set PSTATE.SS to 1 to re-enable single-step Sumit Garg 0 siblings, 2 replies; 9+ messages in thread From: Sumit Garg @ 2022-04-13 6:54 UTC (permalink / raw) To: linux-arm-kernel, dianders, will, liwei391 Cc: catalin.marinas, mark.rutland, mhiramat, daniel.thompson, jason.wessel, maz, linux-kernel, Sumit Garg This patch-set reworks pending fixes from Wei's series [1] to make single-step debugging via kgdb/kdb on arm64 work as expected. There was a prior discussion on ML [2] regarding if we should keep the interrupts enabled during single-stepping. So patch #1 follows suggestion from Will [3] to not disable interrupts during single stepping but rather skip single stepping within interrupt handler. [1] https://lore.kernel.org/all/20200509214159.19680-1-liwei391@huawei.com/ [2] https://lore.kernel.org/all/CAD=FV=Voyfq3Qz0T3RY+aYWYJ0utdH=P_AweB=13rcV8GDBeyQ@mail.gmail.com/ [3] https://lore.kernel.org/all/20200626095551.GA9312@willie-the-truck/ Changes in v2: - Replace patch #1 to rather follow Will's suggestion. Sumit Garg (2): arm64: entry: Skip single stepping interrupt handlers arm64: kgdb: Set PSTATE.SS to 1 to re-enable single-step arch/arm64/include/asm/debug-monitors.h | 1 + arch/arm64/kernel/debug-monitors.c | 5 +++++ arch/arm64/kernel/entry-common.c | 18 +++++++++++++++++- arch/arm64/kernel/kgdb.c | 2 ++ 4 files changed, 25 insertions(+), 1 deletion(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2] arm64: entry: Skip single stepping interrupt handlers 2022-04-13 6:54 [PATCH v2 0/2] arm64: Fix pending single-step debugging issues Sumit Garg @ 2022-04-13 6:54 ` Sumit Garg 2022-05-06 16:06 ` Daniel Thompson 2022-04-13 6:54 ` [PATCH v2 2/2] arm64: kgdb: Set PSTATE.SS to 1 to re-enable single-step Sumit Garg 1 sibling, 1 reply; 9+ messages in thread From: Sumit Garg @ 2022-04-13 6:54 UTC (permalink / raw) To: linux-arm-kernel, dianders, will, liwei391 Cc: catalin.marinas, mark.rutland, mhiramat, daniel.thompson, jason.wessel, maz, linux-kernel, Sumit Garg Current implementation allows single stepping into interrupt handlers for interrupts that were received during single stepping. But interrupt handlers aren't something that the user expect to debug. Moreover single stepping interrupt handlers is risky as it may sometimes leads to unbalanced locking when we resume from single-step debug. Fix broken single-step implementation via skipping single-step over interrupt handlers. The methodology is when we receive an interrupt from EL1, check if we are single stepping (pstate.SS). If yes then we save MDSCR_EL1.SS and clear the register bit if it was set. Then unmask only D and leave I set. On return from the interrupt, set D and restore MDSCR_EL1.SS. Along with this skip reschedule if we were stepping. Suggested-by: Will Deacon <will@kernel.org> Signed-off-by: Sumit Garg <sumit.garg@linaro.org> --- arch/arm64/kernel/entry-common.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c index 878c65aa7206..dd2d3af615de 100644 --- a/arch/arm64/kernel/entry-common.c +++ b/arch/arm64/kernel/entry-common.c @@ -458,19 +458,35 @@ static __always_inline void __el1_irq(struct pt_regs *regs, do_interrupt_handler(regs, handler); irq_exit_rcu(); - arm64_preempt_schedule_irq(); + /* Don't reschedule in case we are single stepping */ + if (!(regs->pstate & DBG_SPSR_SS)) + arm64_preempt_schedule_irq(); exit_to_kernel_mode(regs); } + static void noinstr el1_interrupt(struct pt_regs *regs, void (*handler)(struct pt_regs *)) { + unsigned long reg; + + /* Disable single stepping within interrupt handler */ + if (regs->pstate & DBG_SPSR_SS) { + reg = read_sysreg(mdscr_el1); + write_sysreg(reg & ~DBG_MDSCR_SS, mdscr_el1); + } + write_sysreg(DAIF_PROCCTX_NOIRQ, daif); if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs)) __el1_pnmi(regs, handler); else __el1_irq(regs, handler); + + if (regs->pstate & DBG_SPSR_SS) { + write_sysreg(DAIF_PROCCTX_NOIRQ | PSR_D_BIT, daif); + write_sysreg(reg, mdscr_el1); + } } asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs) -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] arm64: entry: Skip single stepping interrupt handlers 2022-04-13 6:54 ` [PATCH v2 1/2] arm64: entry: Skip single stepping interrupt handlers Sumit Garg @ 2022-05-06 16:06 ` Daniel Thompson 2022-05-09 7:55 ` Sumit Garg 0 siblings, 1 reply; 9+ messages in thread From: Daniel Thompson @ 2022-05-06 16:06 UTC (permalink / raw) To: Sumit Garg Cc: linux-arm-kernel, dianders, will, liwei391, catalin.marinas, mark.rutland, mhiramat, jason.wessel, maz, linux-kernel On Wed, Apr 13, 2022 at 12:24:57PM +0530, Sumit Garg wrote: > Current implementation allows single stepping into interrupt handlers > for interrupts that were received during single stepping. But interrupt > handlers aren't something that the user expect to debug. Moreover single > stepping interrupt handlers is risky as it may sometimes leads to > unbalanced locking when we resume from single-step debug. Why does single stepping interrupt handlers cause unbalanced locking (with the current code)? > Fix broken single-step implementation via skipping single-step over > interrupt handlers. The methodology is when we receive an interrupt from > EL1, check if we are single stepping (pstate.SS). If yes then we save > MDSCR_EL1.SS and clear the register bit if it was set. Then unmask only > D and leave I set. On return from the interrupt, set D and restore > MDSCR_EL1.SS. Along with this skip reschedule if we were stepping. Does this description really explains the motivation here. Isn't the goal to meet the user's expectation that when they step then the system will stop at PC+4 relative the instruction they are stepping (or PC+I for a branch that gets taken)? To be clear, I've no objections to leaving interrupt handling enabled when single stepping (AFAIK it is similar to what x86 does) but I think this patch description will be difficult for future adventurers to comprehend. Daniel. > Suggested-by: Will Deacon <will@kernel.org> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > --- > arch/arm64/kernel/entry-common.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c > index 878c65aa7206..dd2d3af615de 100644 > --- a/arch/arm64/kernel/entry-common.c > +++ b/arch/arm64/kernel/entry-common.c > @@ -458,19 +458,35 @@ static __always_inline void __el1_irq(struct pt_regs *regs, > do_interrupt_handler(regs, handler); > irq_exit_rcu(); > > - arm64_preempt_schedule_irq(); > + /* Don't reschedule in case we are single stepping */ > + if (!(regs->pstate & DBG_SPSR_SS)) > + arm64_preempt_schedule_irq(); > > exit_to_kernel_mode(regs); > } > + > static void noinstr el1_interrupt(struct pt_regs *regs, > void (*handler)(struct pt_regs *)) > { > + unsigned long reg; > + > + /* Disable single stepping within interrupt handler */ > + if (regs->pstate & DBG_SPSR_SS) { > + reg = read_sysreg(mdscr_el1); > + write_sysreg(reg & ~DBG_MDSCR_SS, mdscr_el1); > + } > + > write_sysreg(DAIF_PROCCTX_NOIRQ, daif); > > if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs)) > __el1_pnmi(regs, handler); > else > __el1_irq(regs, handler); > + > + if (regs->pstate & DBG_SPSR_SS) { > + write_sysreg(DAIF_PROCCTX_NOIRQ | PSR_D_BIT, daif); > + write_sysreg(reg, mdscr_el1); > + } > } > > asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs) > -- > 2.25.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] arm64: entry: Skip single stepping interrupt handlers 2022-05-06 16:06 ` Daniel Thompson @ 2022-05-09 7:55 ` Sumit Garg 2022-05-09 9:25 ` Daniel Thompson 0 siblings, 1 reply; 9+ messages in thread From: Sumit Garg @ 2022-05-09 7:55 UTC (permalink / raw) To: Daniel Thompson Cc: linux-arm-kernel, dianders, will, liwei391, catalin.marinas, mark.rutland, mhiramat, jason.wessel, maz, linux-kernel Hi Daniel, On Fri, 6 May 2022 at 21:36, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Wed, Apr 13, 2022 at 12:24:57PM +0530, Sumit Garg wrote: > > Current implementation allows single stepping into interrupt handlers > > for interrupts that were received during single stepping. But interrupt > > handlers aren't something that the user expect to debug. Moreover single > > stepping interrupt handlers is risky as it may sometimes leads to > > unbalanced locking when we resume from single-step debug. > > Why does single stepping interrupt handlers cause unbalanced locking > (with the current code)? > I have to dig deeper to completely answer this question but this is based on observation from following warning splats seen while single stepping interrupt handlers: [ 300.328300] WARNING: bad unlock balance detected! [ 300.328608] 5.18.0-rc1-00016-g3e732ebf7316-dirty #6 Not tainted [ 300.329058] ------------------------------------- [ 300.329298] sh/173 is trying to release lock (dbg_slave_lock) at: [ 300.329718] [<ffffd57c951c016c>] kgdb_cpu_enter+0x7ac/0x820 [ 300.330029] but there are no more locks to release! [ 300.330265] [ 300.330265] other info that might help us debug this: [ 300.330668] 4 locks held by sh/173: [ 300.330891] #0: ffff4f5e454d8438 (sb_writers#3){.+.+}-{0:0}, at: vfs_write+0x98/0x204 [ 300.331735] #1: ffffd57c973bc2f0 (dbg_slave_lock){+.+.}-{2:2}, at: kgdb_cpu_enter+0x5b4/0x820 [ 300.332259] #2: ffffd57c973a9460 (rcu_read_lock){....}-{1:2}, at: kgdb_cpu_enter+0xe0/0x820 [ 300.332717] #3: ffffd57c973bc2a8 (dbg_master_lock){....}-{2:2}, at: kgdb_cpu_enter+0x1ec/0x820 > > > Fix broken single-step implementation via skipping single-step over > > interrupt handlers. The methodology is when we receive an interrupt from > > EL1, check if we are single stepping (pstate.SS). If yes then we save > > MDSCR_EL1.SS and clear the register bit if it was set. Then unmask only > > D and leave I set. On return from the interrupt, set D and restore > > MDSCR_EL1.SS. Along with this skip reschedule if we were stepping. > > Does this description really explains the motivation here. > > Isn't the goal to meet the user's expectation that when they step then > the system will stop at PC+4 relative the instruction they are stepping > (or PC+I for a branch that gets taken)? > Yeah this matches my understanding as well. > To be clear, I've no objections to leaving interrupt handling enabled > when single stepping (AFAIK it is similar to what x86 does) but I think > this patch description will be difficult for future adventurers to > comprehend. > Okay, so does the below patch description sound apt? " Current implementation allows single stepping into interrupt handlers for interrupts that were received during single stepping. But interrupt handlers aren't something that the user expect to debug. Moreover single stepping interrupt handlers is risky as it may sometimes leads to unbalanced locking when we resume from single-step debug: [ 300.328300] WARNING: bad unlock balance detected! [ 300.328608] 5.18.0-rc1-00016-g3e732ebf7316-dirty #6 Not tainted [ 300.329058] ------------------------------------- [ 300.329298] sh/173 is trying to release lock (dbg_slave_lock) at: [ 300.329718] [<ffffd57c951c016c>] kgdb_cpu_enter+0x7ac/0x820 [ 300.330029] but there are no more locks to release! [ 300.330265] [ 300.330265] other info that might help us debug this: [ 300.330668] 4 locks held by sh/173: [ 300.330891] #0: ffff4f5e454d8438 (sb_writers#3){.+.+}-{0:0}, at: vfs_write+0x98/0x204 [ 300.331735] #1: ffffd57c973bc2f0 (dbg_slave_lock){+.+.}-{2:2}, at: kgdb_cpu_enter+0x5b4/0x820 [ 300.332259] #2: ffffd57c973a9460 (rcu_read_lock){....}-{1:2}, at: kgdb_cpu_enter+0xe0/0x820 [ 300.332717] #3: ffffd57c973bc2a8 (dbg_master_lock){....}-{2:2}, at: kgdb_cpu_enter+0x1ec/0x820 The common user's expectation while single stepping is that when they step then the system will stop at PC+4 or PC+I for a branch that gets taken relative to the instruction they are stepping. So, fix broken single step implementation via skipping single stepping interrupt handlers. The methodology is when we receive an interrupt from EL1, check if we are single stepping (pstate.SS). If yes then we save MDSCR_EL1.SS and clear the register bit if it was set. Then unmask only D and leave I set. On return from the interrupt, set D and restore MDSCR_EL1.SS. Along with this skip reschedule if we were stepping. " -Sumit > > Daniel. > > > > Suggested-by: Will Deacon <will@kernel.org> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > --- > > arch/arm64/kernel/entry-common.c | 18 +++++++++++++++++- > > 1 file changed, 17 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c > > index 878c65aa7206..dd2d3af615de 100644 > > --- a/arch/arm64/kernel/entry-common.c > > +++ b/arch/arm64/kernel/entry-common.c > > @@ -458,19 +458,35 @@ static __always_inline void __el1_irq(struct pt_regs *regs, > > do_interrupt_handler(regs, handler); > > irq_exit_rcu(); > > > > - arm64_preempt_schedule_irq(); > > + /* Don't reschedule in case we are single stepping */ > > + if (!(regs->pstate & DBG_SPSR_SS)) > > + arm64_preempt_schedule_irq(); > > > > exit_to_kernel_mode(regs); > > } > > + > > static void noinstr el1_interrupt(struct pt_regs *regs, > > void (*handler)(struct pt_regs *)) > > { > > + unsigned long reg; > > + > > + /* Disable single stepping within interrupt handler */ > > + if (regs->pstate & DBG_SPSR_SS) { > > + reg = read_sysreg(mdscr_el1); > > + write_sysreg(reg & ~DBG_MDSCR_SS, mdscr_el1); > > + } > > + > > write_sysreg(DAIF_PROCCTX_NOIRQ, daif); > > > > if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs)) > > __el1_pnmi(regs, handler); > > else > > __el1_irq(regs, handler); > > + > > + if (regs->pstate & DBG_SPSR_SS) { > > + write_sysreg(DAIF_PROCCTX_NOIRQ | PSR_D_BIT, daif); > > + write_sysreg(reg, mdscr_el1); > > + } > > } > > > > asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs) > > -- > > 2.25.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] arm64: entry: Skip single stepping interrupt handlers 2022-05-09 7:55 ` Sumit Garg @ 2022-05-09 9:25 ` Daniel Thompson 2022-05-10 5:11 ` Sumit Garg 0 siblings, 1 reply; 9+ messages in thread From: Daniel Thompson @ 2022-05-09 9:25 UTC (permalink / raw) To: Sumit Garg Cc: linux-arm-kernel, dianders, will, liwei391, catalin.marinas, mark.rutland, mhiramat, jason.wessel, maz, linux-kernel On Mon, May 09, 2022 at 01:25:21PM +0530, Sumit Garg wrote: > Hi Daniel, > > On Fri, 6 May 2022 at 21:36, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > > > On Wed, Apr 13, 2022 at 12:24:57PM +0530, Sumit Garg wrote: > > > Current implementation allows single stepping into interrupt handlers > > > for interrupts that were received during single stepping. But interrupt > > > handlers aren't something that the user expect to debug. Moreover single > > > stepping interrupt handlers is risky as it may sometimes leads to > > > unbalanced locking when we resume from single-step debug. > > > > Why does single stepping interrupt handlers cause unbalanced locking > > (with the current code)? > > > > I have to dig deeper to completely answer this question but this is > based on observation from following warning splats seen while single > stepping interrupt handlers: > > [ 300.328300] WARNING: bad unlock balance detected! > [ 300.328608] 5.18.0-rc1-00016-g3e732ebf7316-dirty #6 Not tainted > [ 300.329058] ------------------------------------- > [ 300.329298] sh/173 is trying to release lock (dbg_slave_lock) at: > [ 300.329718] [<ffffd57c951c016c>] kgdb_cpu_enter+0x7ac/0x820 > [ 300.330029] but there are no more locks to release! > [ 300.330265] > [ 300.330265] other info that might help us debug this: > [ 300.330668] 4 locks held by sh/173: > [ 300.330891] #0: ffff4f5e454d8438 (sb_writers#3){.+.+}-{0:0}, at: > vfs_write+0x98/0x204 > [ 300.331735] #1: ffffd57c973bc2f0 (dbg_slave_lock){+.+.}-{2:2}, at: > kgdb_cpu_enter+0x5b4/0x820 > [ 300.332259] #2: ffffd57c973a9460 (rcu_read_lock){....}-{1:2}, at: > kgdb_cpu_enter+0xe0/0x820 > [ 300.332717] #3: ffffd57c973bc2a8 (dbg_master_lock){....}-{2:2}, > at: kgdb_cpu_enter+0x1ec/0x820 This splat tells us we entered the debugger from a regular task (echo g > /proc/sysrq-trigger by the looks of it) and exited the debugger from interrupt. As such I'd be inclined to describe this as a false positive: it occurs because we have not taught lockdep that the locks are effectively owned by the debug trap handler rather than the task/interrupt we trapped from. To be honest, for a very long time been inclined to replace dbg_slave_lock with an atomic flag instead. dbg_slave_lock isn't a lock in any meaningful sense since it is never contended for and literally only exists so that other cores can use raw_spin_is_locked() on it. However sorting out dbg_master_lock is a bit more complex though which is why I've never found the time for it (at least not yet). BTW I suspect this patch only avoids the simplest reproduction of this splat: I think a temporary breakpoint in the ISR that runs during the single step would also cause the single step to complete early, yielding the same splat. > > > Fix broken single-step implementation via skipping single-step over > > > interrupt handlers. The methodology is when we receive an interrupt from > > > EL1, check if we are single stepping (pstate.SS). If yes then we save > > > MDSCR_EL1.SS and clear the register bit if it was set. Then unmask only > > > D and leave I set. On return from the interrupt, set D and restore > > > MDSCR_EL1.SS. Along with this skip reschedule if we were stepping. > > > > Does this description really explains the motivation here. > > > > Isn't the goal to meet the user's expectation that when they step then > > the system will stop at PC+4 relative the instruction they are stepping > > (or PC+I for a branch that gets taken)? > > > > Yeah this matches my understanding as well. > > > To be clear, I've no objections to leaving interrupt handling enabled > > when single stepping (AFAIK it is similar to what x86 does) but I think > > this patch description will be difficult for future adventurers to > > comprehend. > > > > Okay, so does the below patch description sound apt? Not entirely. The lockdep splat still distracts from any explanation about why the patch is "the right thing to do". That the patch partially conceals a (false-positive) lockdep splat is nice but I don't think it should dominate the description of why single-step is unusable if a single step ends during interrupt handling. Surely a far more powerful motivation is that (currently) on systems when the timer interrupt (or any other fast-at-human-scale periodic interrupt) is active then it is impossible to step any code with interrupts unlocked because we will always end up stepping into the timer interrupt instead of stepping the user code. > " > Current implementation allows single stepping into interrupt handlers > for interrupts that were received during single stepping. But interrupt > handlers aren't something that the user expect to debug. As above, I think it is more than user expectation. With a 100Hz timer humans cannot react to the debugger fast enough to step before the next timer interrupt is due. > Moreover single > stepping interrupt handlers is risky as it may sometimes leads to > unbalanced locking when we resume from single-step debug: If you want to keep the comments about lockdep as an aside at the bottom the it is important to: s/stepping interrupt handlers/stepping into interrupt handlers/ ^^^^^^ > [ 300.328300] WARNING: bad unlock balance detected! > [ 300.328608] 5.18.0-rc1-00016-g3e732ebf7316-dirty #6 Not tainted > [ 300.329058] ------------------------------------- > [ 300.329298] sh/173 is trying to release lock (dbg_slave_lock) at: > [ 300.329718] [<ffffd57c951c016c>] kgdb_cpu_enter+0x7ac/0x820 > [ 300.330029] but there are no more locks to release! > [ 300.330265] > [ 300.330265] other info that might help us debug this: > [ 300.330668] 4 locks held by sh/173: > [ 300.330891] #0: ffff4f5e454d8438 (sb_writers#3){.+.+}-{0:0}, > at: vfs_write+0x98/0x204 > [ 300.331735] #1: ffffd57c973bc2f0 (dbg_slave_lock){+.+.}-{2:2}, > at: kgdb_cpu_enter+0x5b4/0x820 > [ 300.332259] #2: ffffd57c973a9460 (rcu_read_lock){....}-{1:2}, > at: kgdb_cpu_enter+0xe0/0x820 > [ 300.332717] #3: ffffd57c973bc2a8 > (dbg_master_lock){....}-{2:2}, at: kgdb_cpu_enter+0x1ec/0x820 > > The common user's expectation while single stepping is that when they > step then the system will stop at PC+4 or PC+I for a branch that gets > taken relative to the instruction they are stepping. So, fix broken single > step implementation via skipping single stepping interrupt handlers. > > The methodology is when we receive an interrupt from EL1, check if we > are single stepping (pstate.SS). If yes then we save MDSCR_EL1.SS and > clear the register bit if it was set. Then unmask only D and leave I set. On > return from the interrupt, set D and restore MDSCR_EL1.SS. Along with this > skip reschedule if we were stepping. > " > > -Sumit > > > > > Daniel. > > > > > > > Suggested-by: Will Deacon <will@kernel.org> > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > > --- > > > arch/arm64/kernel/entry-common.c | 18 +++++++++++++++++- > > > 1 file changed, 17 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c > > > index 878c65aa7206..dd2d3af615de 100644 > > > --- a/arch/arm64/kernel/entry-common.c > > > +++ b/arch/arm64/kernel/entry-common.c > > > @@ -458,19 +458,35 @@ static __always_inline void __el1_irq(struct pt_regs *regs, > > > do_interrupt_handler(regs, handler); > > > irq_exit_rcu(); > > > > > > - arm64_preempt_schedule_irq(); > > > + /* Don't reschedule in case we are single stepping */ > > > + if (!(regs->pstate & DBG_SPSR_SS)) > > > + arm64_preempt_schedule_irq(); > > > > > > exit_to_kernel_mode(regs); > > > } > > > + > > > static void noinstr el1_interrupt(struct pt_regs *regs, > > > void (*handler)(struct pt_regs *)) > > > { > > > + unsigned long reg; > > > + > > > + /* Disable single stepping within interrupt handler */ > > > + if (regs->pstate & DBG_SPSR_SS) { > > > + reg = read_sysreg(mdscr_el1); > > > + write_sysreg(reg & ~DBG_MDSCR_SS, mdscr_el1); > > > + } > > > + > > > write_sysreg(DAIF_PROCCTX_NOIRQ, daif); > > > > > > if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs)) > > > __el1_pnmi(regs, handler); > > > else > > > __el1_irq(regs, handler); > > > + > > > + if (regs->pstate & DBG_SPSR_SS) { > > > + write_sysreg(DAIF_PROCCTX_NOIRQ | PSR_D_BIT, daif); > > > + write_sysreg(reg, mdscr_el1); > > > + } > > > } > > > > > > asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs) > > > -- > > > 2.25.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] arm64: entry: Skip single stepping interrupt handlers 2022-05-09 9:25 ` Daniel Thompson @ 2022-05-10 5:11 ` Sumit Garg 0 siblings, 0 replies; 9+ messages in thread From: Sumit Garg @ 2022-05-10 5:11 UTC (permalink / raw) To: Daniel Thompson Cc: linux-arm-kernel, dianders, will, liwei391, catalin.marinas, mark.rutland, mhiramat, jason.wessel, maz, linux-kernel On Mon, 9 May 2022 at 14:55, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Mon, May 09, 2022 at 01:25:21PM +0530, Sumit Garg wrote: > > Hi Daniel, > > > > On Fri, 6 May 2022 at 21:36, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > > > > > On Wed, Apr 13, 2022 at 12:24:57PM +0530, Sumit Garg wrote: > > > > Current implementation allows single stepping into interrupt handlers > > > > for interrupts that were received during single stepping. But interrupt > > > > handlers aren't something that the user expect to debug. Moreover single > > > > stepping interrupt handlers is risky as it may sometimes leads to > > > > unbalanced locking when we resume from single-step debug. > > > > > > Why does single stepping interrupt handlers cause unbalanced locking > > > (with the current code)? > > > > > > > I have to dig deeper to completely answer this question but this is > > based on observation from following warning splats seen while single > > stepping interrupt handlers: > > > > [ 300.328300] WARNING: bad unlock balance detected! > > [ 300.328608] 5.18.0-rc1-00016-g3e732ebf7316-dirty #6 Not tainted > > [ 300.329058] ------------------------------------- > > [ 300.329298] sh/173 is trying to release lock (dbg_slave_lock) at: > > [ 300.329718] [<ffffd57c951c016c>] kgdb_cpu_enter+0x7ac/0x820 > > [ 300.330029] but there are no more locks to release! > > [ 300.330265] > > [ 300.330265] other info that might help us debug this: > > [ 300.330668] 4 locks held by sh/173: > > [ 300.330891] #0: ffff4f5e454d8438 (sb_writers#3){.+.+}-{0:0}, at: > > vfs_write+0x98/0x204 > > [ 300.331735] #1: ffffd57c973bc2f0 (dbg_slave_lock){+.+.}-{2:2}, at: > > kgdb_cpu_enter+0x5b4/0x820 > > [ 300.332259] #2: ffffd57c973a9460 (rcu_read_lock){....}-{1:2}, at: > > kgdb_cpu_enter+0xe0/0x820 > > [ 300.332717] #3: ffffd57c973bc2a8 (dbg_master_lock){....}-{2:2}, > > at: kgdb_cpu_enter+0x1ec/0x820 > > This splat tells us we entered the debugger from a regular task > (echo g > /proc/sysrq-trigger by the looks of it) and exited the > debugger from interrupt. > > As such I'd be inclined to describe this as a false positive: it occurs > because we have not taught lockdep that the locks are effectively > owned by the debug trap handler rather than the task/interrupt we trapped > from. > > To be honest, for a very long time been inclined to replace > dbg_slave_lock with an atomic flag instead. dbg_slave_lock isn't a lock > in any meaningful sense since it is never contended for and literally > only exists so that other cores can use raw_spin_is_locked() on it. > However sorting out dbg_master_lock is a bit more complex though which > is why I've never found the time for it (at least not yet). > > BTW I suspect this patch only avoids the simplest reproduction of this > splat: I think a temporary breakpoint in the ISR that runs during the > single step would also cause the single step to complete early, yielding > the same splat. > Okay, this sounds reasonable to me. I will remove reference to these false positive warning splats. > > > > > Fix broken single-step implementation via skipping single-step over > > > > interrupt handlers. The methodology is when we receive an interrupt from > > > > EL1, check if we are single stepping (pstate.SS). If yes then we save > > > > MDSCR_EL1.SS and clear the register bit if it was set. Then unmask only > > > > D and leave I set. On return from the interrupt, set D and restore > > > > MDSCR_EL1.SS. Along with this skip reschedule if we were stepping. > > > > > > Does this description really explains the motivation here. > > > > > > Isn't the goal to meet the user's expectation that when they step then > > > the system will stop at PC+4 relative the instruction they are stepping > > > (or PC+I for a branch that gets taken)? > > > > > > > Yeah this matches my understanding as well. > > > > > To be clear, I've no objections to leaving interrupt handling enabled > > > when single stepping (AFAIK it is similar to what x86 does) but I think > > > this patch description will be difficult for future adventurers to > > > comprehend. > > > > > > > Okay, so does the below patch description sound apt? > > Not entirely. > > The lockdep splat still distracts from any explanation about why the > patch is "the right thing to do". > > That the patch partially conceals a (false-positive) lockdep splat is > nice but I don't think it should dominate the description of why > single-step is unusable if a single step ends during interrupt handling. > > Surely a far more powerful motivation is that (currently) on systems when > the timer interrupt (or any other fast-at-human-scale periodic interrupt) > is active then it is impossible to step any code with interrupts unlocked > because we will always end up stepping into the timer interrupt instead of > stepping the user code. > Agree. > > > " > > Current implementation allows single stepping into interrupt handlers > > for interrupts that were received during single stepping. But interrupt > > handlers aren't something that the user expect to debug. > > As above, I think it is more than user expectation. With a 100Hz timer > humans cannot react to the debugger fast enough to step before the next timer > interrupt is due. > Thanks for your inputs, I will update the commit description in v3. -Sumit > > > Moreover single > > stepping interrupt handlers is risky as it may sometimes leads to > > unbalanced locking when we resume from single-step debug: > > If you want to keep the comments about lockdep as an aside at the bottom > the it is important to: > > s/stepping interrupt handlers/stepping into interrupt handlers/ > ^^^^^^ > > > [ 300.328300] WARNING: bad unlock balance detected! > > [ 300.328608] 5.18.0-rc1-00016-g3e732ebf7316-dirty #6 Not tainted > > [ 300.329058] ------------------------------------- > > [ 300.329298] sh/173 is trying to release lock (dbg_slave_lock) at: > > [ 300.329718] [<ffffd57c951c016c>] kgdb_cpu_enter+0x7ac/0x820 > > [ 300.330029] but there are no more locks to release! > > [ 300.330265] > > [ 300.330265] other info that might help us debug this: > > [ 300.330668] 4 locks held by sh/173: > > [ 300.330891] #0: ffff4f5e454d8438 (sb_writers#3){.+.+}-{0:0}, > > at: vfs_write+0x98/0x204 > > [ 300.331735] #1: ffffd57c973bc2f0 (dbg_slave_lock){+.+.}-{2:2}, > > at: kgdb_cpu_enter+0x5b4/0x820 > > [ 300.332259] #2: ffffd57c973a9460 (rcu_read_lock){....}-{1:2}, > > at: kgdb_cpu_enter+0xe0/0x820 > > [ 300.332717] #3: ffffd57c973bc2a8 > > (dbg_master_lock){....}-{2:2}, at: kgdb_cpu_enter+0x1ec/0x820 > > > > The common user's expectation while single stepping is that when they > > step then the system will stop at PC+4 or PC+I for a branch that gets > > taken relative to the instruction they are stepping. So, fix broken single > > step implementation via skipping single stepping interrupt handlers. > > > > The methodology is when we receive an interrupt from EL1, check if we > > are single stepping (pstate.SS). If yes then we save MDSCR_EL1.SS and > > clear the register bit if it was set. Then unmask only D and leave I set. On > > return from the interrupt, set D and restore MDSCR_EL1.SS. Along with this > > skip reschedule if we were stepping. > > " > > > > -Sumit > > > > > > > > Daniel. > > > > > > > > > > Suggested-by: Will Deacon <will@kernel.org> > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > > > --- > > > > arch/arm64/kernel/entry-common.c | 18 +++++++++++++++++- > > > > 1 file changed, 17 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c > > > > index 878c65aa7206..dd2d3af615de 100644 > > > > --- a/arch/arm64/kernel/entry-common.c > > > > +++ b/arch/arm64/kernel/entry-common.c > > > > @@ -458,19 +458,35 @@ static __always_inline void __el1_irq(struct pt_regs *regs, > > > > do_interrupt_handler(regs, handler); > > > > irq_exit_rcu(); > > > > > > > > - arm64_preempt_schedule_irq(); > > > > + /* Don't reschedule in case we are single stepping */ > > > > + if (!(regs->pstate & DBG_SPSR_SS)) > > > > + arm64_preempt_schedule_irq(); > > > > > > > > exit_to_kernel_mode(regs); > > > > } > > > > + > > > > static void noinstr el1_interrupt(struct pt_regs *regs, > > > > void (*handler)(struct pt_regs *)) > > > > { > > > > + unsigned long reg; > > > > + > > > > + /* Disable single stepping within interrupt handler */ > > > > + if (regs->pstate & DBG_SPSR_SS) { > > > > + reg = read_sysreg(mdscr_el1); > > > > + write_sysreg(reg & ~DBG_MDSCR_SS, mdscr_el1); > > > > + } > > > > + > > > > write_sysreg(DAIF_PROCCTX_NOIRQ, daif); > > > > > > > > if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs)) > > > > __el1_pnmi(regs, handler); > > > > else > > > > __el1_irq(regs, handler); > > > > + > > > > + if (regs->pstate & DBG_SPSR_SS) { > > > > + write_sysreg(DAIF_PROCCTX_NOIRQ | PSR_D_BIT, daif); > > > > + write_sysreg(reg, mdscr_el1); > > > > + } > > > > } > > > > > > > > asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs) > > > > -- > > > > 2.25.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] arm64: kgdb: Set PSTATE.SS to 1 to re-enable single-step 2022-04-13 6:54 [PATCH v2 0/2] arm64: Fix pending single-step debugging issues Sumit Garg 2022-04-13 6:54 ` [PATCH v2 1/2] arm64: entry: Skip single stepping interrupt handlers Sumit Garg @ 2022-04-13 6:54 ` Sumit Garg 2022-05-06 16:12 ` Daniel Thompson 1 sibling, 1 reply; 9+ messages in thread From: Sumit Garg @ 2022-04-13 6:54 UTC (permalink / raw) To: linux-arm-kernel, dianders, will, liwei391 Cc: catalin.marinas, mark.rutland, mhiramat, daniel.thompson, jason.wessel, maz, linux-kernel, Sumit Garg 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(+) 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] arm64: kgdb: Set PSTATE.SS to 1 to re-enable single-step 2022-04-13 6:54 ` [PATCH v2 2/2] arm64: kgdb: Set PSTATE.SS to 1 to re-enable single-step Sumit Garg @ 2022-05-06 16:12 ` Daniel Thompson 2022-05-09 7:56 ` Sumit Garg 0 siblings, 1 reply; 9+ messages in thread From: Daniel Thompson @ 2022-05-06 16:12 UTC (permalink / raw) To: Sumit Garg Cc: linux-arm-kernel, dianders, will, liwei391, catalin.marinas, mark.rutland, mhiramat, jason.wessel, maz, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] arm64: kgdb: Set PSTATE.SS to 1 to re-enable single-step 2022-05-06 16:12 ` Daniel Thompson @ 2022-05-09 7:56 ` Sumit Garg 0 siblings, 0 replies; 9+ messages in thread From: Sumit Garg @ 2022-05-09 7:56 UTC (permalink / raw) To: Daniel Thompson Cc: linux-arm-kernel, dianders, will, liwei391, catalin.marinas, mark.rutland, mhiramat, jason.wessel, maz, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-05-10 5:12 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-13 6:54 [PATCH v2 0/2] arm64: Fix pending single-step debugging issues Sumit Garg 2022-04-13 6:54 ` [PATCH v2 1/2] arm64: entry: Skip single stepping interrupt handlers Sumit Garg 2022-05-06 16:06 ` Daniel Thompson 2022-05-09 7:55 ` Sumit Garg 2022-05-09 9:25 ` Daniel Thompson 2022-05-10 5:11 ` Sumit Garg 2022-04-13 6:54 ` [PATCH v2 2/2] arm64: kgdb: Set PSTATE.SS to 1 to re-enable single-step Sumit Garg 2022-05-06 16:12 ` Daniel Thompson 2022-05-09 7:56 ` Sumit Garg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).