[PATCH v5 0/2] arm64: Fix pending single-step debugging issues

[PATCH v5 0/2] arm64: Fix pending single-step debugging issues

[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 v5:
- Incorporated misc. comments from Mark.

Changes in v4:
- Rebased to the tip of mainline.
- Picked up Doug's Tested-by tag.

Changes in v3:
- Reword commit descriptions as per Daniel's suggestions.

Changes in v2:
- Replace patch #1 to rather follow Will's suggestion.

Sumit Garg (2):
  arm64: entry: Skip single stepping into 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        | 22 ++++++++++++++++++++--
 arch/arm64/kernel/kgdb.c                |  2 ++
 4 files changed, 28 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v5 1/2] arm64: entry: Skip single stepping into interrupt handlers

[Sumit Garg]

Currently on systems where 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.

The common user's goal 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 into 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>
Tested-by: Douglas Anderson <dianders@chromium.org>
---
 arch/arm64/kernel/entry-common.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index cce1167199e3..688d1ef8e864 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -231,11 +231,15 @@ DEFINE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
 #define need_irq_preemption()	(IS_ENABLED(CONFIG_PREEMPTION))
 #endif
 
-static void __sched arm64_preempt_schedule_irq(void)
+static void __sched arm64_preempt_schedule_irq(struct pt_regs *regs)
 {
 	if (!need_irq_preemption())
 		return;
 
+	/* Don't reschedule in case we are single stepping */
+	if (!(regs->pstate & DBG_SPSR_SS))
+		return;
+
 	/*
 	 * Note: thread_info::preempt_count includes both thread_info::count
 	 * and thread_info::need_resched, and is not equivalent to
@@ -471,19 +475,33 @@ static __always_inline void __el1_irq(struct pt_regs *regs,
 	do_interrupt_handler(regs, handler);
 	irq_exit_rcu();
 
-	arm64_preempt_schedule_irq();
+	arm64_preempt_schedule_irq(regs);
 
 	exit_to_kernel_mode(regs);
 }
+
 static void noinstr el1_interrupt(struct pt_regs *regs,
 				  void (*handler)(struct pt_regs *))
 {
+	unsigned long mdscr;
+
+	/* Disable single stepping within interrupt handler */
+	if (regs->pstate & DBG_SPSR_SS) {
+		mdscr = read_sysreg(mdscr_el1);
+		write_sysreg(mdscr & ~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(mdscr, mdscr_el1);
+	}
 }
 
 asmlinkage void noinstr el1h_64_irq_handler(struct pt_regs *regs)
-- 
2.34.1

[PATCH v5 2/2] arm64: kgdb: Set PSTATE.SS to 1 to re-enable single-step

[Sumit Garg]

Currently only the first attempt to single-step has any effect. After
that all further stepping remains "stuck" at the same program counter
value.

Refer to the ARM Architecture Reference Manual (ARM DDI 0487E.a) D2.12,
PSTATE.SS=1 should be set at each step before 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>
Tested-by: Douglas Anderson <dianders@chromium.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 7b7e05c02691..ce3875ad5cd3 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 3da09778267e..9af898b22ed4 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 cda9c1e9864f..51f204bbcf87 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.34.1

Re: [PATCH v5 0/2] arm64: Fix pending single-step debugging issues

[Sumit Garg]

Hi Will, Catalin,

On Mon, 19 Dec 2022 at 15:55, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> 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 v5:
> - Incorporated misc. comments from Mark.
>

Since patch #1 has already been reviewed/acked by Mark and the
complete patchset has been tested by Doug, would it be fine for you to
pick up this patchset? It fixes a real single stepping problem for
kgdb on arm64.

-Sumit

> Changes in v4:
> - Rebased to the tip of mainline.
> - Picked up Doug's Tested-by tag.
>
> Changes in v3:
> - Reword commit descriptions as per Daniel's suggestions.
>
> Changes in v2:
> - Replace patch #1 to rather follow Will's suggestion.
>
> Sumit Garg (2):
>   arm64: entry: Skip single stepping into 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        | 22 ++++++++++++++++++++--
>  arch/arm64/kernel/kgdb.c                |  2 ++
>  4 files changed, 28 insertions(+), 2 deletions(-)
>
> --
> 2.34.1
>

Re: [PATCH v5 0/2] arm64: Fix pending single-step debugging issues

[Daniel Thompson]

On Thu, Jan 12, 2023 at 02:52:49PM +0530, Sumit Garg wrote:
> Hi Will, Catalin,
>
> On Mon, 19 Dec 2022 at 15:55, Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > 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 v5:
> > - Incorporated misc. comments from Mark.
> >
>
> Since patch #1 has already been reviewed/acked by Mark and the
> complete patchset has been tested by Doug, would it be fine for you to
> pick up this patchset? It fixes a real single stepping problem for
> kgdb on arm64.

Sorry to be quiet for so long.

Testing this patch set has proven to be a little difficult.

It certainly fixes the single step tests in the kgdbtest suite.
That's a good start.

Unfortunately when testing using qemu/KVM (hosted on NXP
2k/Solidrun Honeycomb) the patch set is resulting in instability
running the built-in self tests (specifically this one:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/kgdbts.c#n74 ). Running this test using the kgdbtest harness
results in the test failing roughly a third of the time.

The error reported is that the trap handler tried to unlock a spinlock
that isn't currently locked. To be honest I suspect this is a generic
problem that the new feature happens to tickle (this test has
historically been unreliable on x86 too... and x86 is noteworthy for
being the only other platform I test using KVM rather than pure qemu).
Of course the only way to prove that would be to find and fix the
problem in the trap handler (which probably involves rewriting it) and I
haven't managed to do that yet.

In short, I think the debugger is more useful with this patchset than
without so, although it is caveated by the above, I'd call this:

Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
Tested-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

Re: [PATCH v5 0/2] arm64: Fix pending single-step debugging issues

[Luis Machado]

Hi,

Is this expected to change single-stepping operation in usespace for debuggers (gdb/lldb)? If so, it would be nice to at least
test it a little to make sure it works.

On 1/24/23 18:04, Daniel Thompson wrote:
> On Thu, Jan 12, 2023 at 02:52:49PM +0530, Sumit Garg wrote:
>> Hi Will, Catalin,
>>
>> On Mon, 19 Dec 2022 at 15:55, Sumit Garg <sumit.garg@linaro.org> wrote:
>>>
>>> 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 v5:
>>> - Incorporated misc. comments from Mark.
>>>
>>
>> Since patch #1 has already been reviewed/acked by Mark and the
>> complete patchset has been tested by Doug, would it be fine for you to
>> pick up this patchset? It fixes a real single stepping problem for
>> kgdb on arm64.
> 
> Sorry to be quiet for so long.
> 
> Testing this patch set has proven to be a little difficult.
> 
> It certainly fixes the single step tests in the kgdbtest suite.
> That's a good start.
> 
> Unfortunately when testing using qemu/KVM (hosted on NXP
> 2k/Solidrun Honeycomb) the patch set is resulting in instability
> running the built-in self tests (specifically this one:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/kgdbts.c#n74 ). Running this test using the kgdbtest harness
> results in the test failing roughly a third of the time.
> 
> The error reported is that the trap handler tried to unlock a spinlock
> that isn't currently locked. To be honest I suspect this is a generic
> problem that the new feature happens to tickle (this test has
> historically been unreliable on x86 too... and x86 is noteworthy for
> being the only other platform I test using KVM rather than pure qemu).
> Of course the only way to prove that would be to find and fix the
> problem in the trap handler (which probably involves rewriting it) and I
> haven't managed to do that yet.
> 
> In short, I think the debugger is more useful with this patchset than
> without so, although it is caveated by the above, I'd call this:
> 
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> Tested-by: Daniel Thompson <daniel.thompson@linaro.org>
> 
> 
> Daniel.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 1/2] arm64: entry: Skip single stepping into interrupt handlers

[Will Deacon]

On Mon, Dec 19, 2022 at 03:54:51PM +0530, Sumit Garg wrote:
> Currently on systems where 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.
> 
> The common user's goal 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 into 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>
> Tested-by: Douglas Anderson <dianders@chromium.org>
> ---
>  arch/arm64/kernel/entry-common.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index cce1167199e3..688d1ef8e864 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -231,11 +231,15 @@ DEFINE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
>  #define need_irq_preemption()	(IS_ENABLED(CONFIG_PREEMPTION))
>  #endif
>  
> -static void __sched arm64_preempt_schedule_irq(void)
> +static void __sched arm64_preempt_schedule_irq(struct pt_regs *regs)
>  {
>  	if (!need_irq_preemption())
>  		return;
>  
> +	/* Don't reschedule in case we are single stepping */
> +	if (!(regs->pstate & DBG_SPSR_SS))
> +		return;

Hmm, isn't this the common case? PSTATE.SS will usually be clear, no?

>  	 * Note: thread_info::preempt_count includes both thread_info::count
>  	 * and thread_info::need_resched, and is not equivalent to
> @@ -471,19 +475,33 @@ static __always_inline void __el1_irq(struct pt_regs *regs,
>  	do_interrupt_handler(regs, handler);
>  	irq_exit_rcu();
>  
> -	arm64_preempt_schedule_irq();
> +	arm64_preempt_schedule_irq(regs);
>  
>  	exit_to_kernel_mode(regs);
>  }
> +
>  static void noinstr el1_interrupt(struct pt_regs *regs,
>  				  void (*handler)(struct pt_regs *))
>  {
> +	unsigned long mdscr;
> +
> +	/* Disable single stepping within interrupt handler */
> +	if (regs->pstate & DBG_SPSR_SS) {
> +		mdscr = read_sysreg(mdscr_el1);
> +		write_sysreg(mdscr & ~DBG_MDSCR_SS, mdscr_el1);
> +	}

I think this will break the implicit handling of kernel {break,watch}points.

Sadly, I think any attempts to workaround the issues here are likely just
to push the problems around. We really need to overhaul the debug exception
handling logic we have, which means I need to get back to writing up a
proposal.

Will

Re: [PATCH v5 0/2] arm64: Fix pending single-step debugging issues

[Sumit Garg]

On Tue, 24 Jan 2023 at 23:34, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Thu, Jan 12, 2023 at 02:52:49PM +0530, Sumit Garg wrote:
> > Hi Will, Catalin,
> >
> > On Mon, 19 Dec 2022 at 15:55, Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > 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 v5:
> > > - Incorporated misc. comments from Mark.
> > >
> >
> > Since patch #1 has already been reviewed/acked by Mark and the
> > complete patchset has been tested by Doug, would it be fine for you to
> > pick up this patchset? It fixes a real single stepping problem for
> > kgdb on arm64.
>
> Sorry to be quiet for so long.
>
> Testing this patch set has proven to be a little difficult.
>
> It certainly fixes the single step tests in the kgdbtest suite.
> That's a good start.
>
> Unfortunately when testing using qemu/KVM (hosted on NXP
> 2k/Solidrun Honeycomb) the patch set is resulting in instability
> running the built-in self tests (specifically this one:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/kgdbts.c#n74 ). Running this test using the kgdbtest harness
> results in the test failing roughly a third of the time.
>
> The error reported is that the trap handler tried to unlock a spinlock
> that isn't currently locked. To be honest I suspect this is a generic
> problem that the new feature happens to tickle (this test has
> historically been unreliable on x86 too... and x86 is noteworthy for
> being the only other platform I test using KVM rather than pure qemu).
> Of course the only way to prove that would be to find and fix the
> problem in the trap handler (which probably involves rewriting it) and I
> haven't managed to do that yet.
>
> In short, I think the debugger is more useful with this patchset than
> without so, although it is caveated by the above, I'd call this:
>
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> Tested-by: Daniel Thompson <daniel.thompson@linaro.org>
>

Thanks Daniel for the in-depth testing.

-Sumit

>
> Daniel.

Re: [PATCH v5 0/2] arm64: Fix pending single-step debugging issues

[Sumit Garg]

Hi Luis,

On Wed, 25 Jan 2023 at 14:48, Luis Machado <luis.machado@arm.com> wrote:
>
> Hi,
>
> Is this expected to change single-stepping operation in usespace for debuggers (gdb/lldb)?

No it won't affect user-space debuggers as we are only touching the
interrupt path in EL1 mode.

-Sumit

> If so, it would be nice to at least
> test it a little to make sure it works.
>
> On 1/24/23 18:04, Daniel Thompson wrote:
> > On Thu, Jan 12, 2023 at 02:52:49PM +0530, Sumit Garg wrote:
> >> Hi Will, Catalin,
> >>
> >> On Mon, 19 Dec 2022 at 15:55, Sumit Garg <sumit.garg@linaro.org> wrote:
> >>>
> >>> 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 v5:
> >>> - Incorporated misc. comments from Mark.
> >>>
> >>
> >> Since patch #1 has already been reviewed/acked by Mark and the
> >> complete patchset has been tested by Doug, would it be fine for you to
> >> pick up this patchset? It fixes a real single stepping problem for
> >> kgdb on arm64.
> >
> > Sorry to be quiet for so long.
> >
> > Testing this patch set has proven to be a little difficult.
> >
> > It certainly fixes the single step tests in the kgdbtest suite.
> > That's a good start.
> >
> > Unfortunately when testing using qemu/KVM (hosted on NXP
> > 2k/Solidrun Honeycomb) the patch set is resulting in instability
> > running the built-in self tests (specifically this one:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/kgdbts.c#n74 ). Running this test using the kgdbtest harness
> > results in the test failing roughly a third of the time.
> >
> > The error reported is that the trap handler tried to unlock a spinlock
> > that isn't currently locked. To be honest I suspect this is a generic
> > problem that the new feature happens to tickle (this test has
> > historically been unreliable on x86 too... and x86 is noteworthy for
> > being the only other platform I test using KVM rather than pure qemu).
> > Of course the only way to prove that would be to find and fix the
> > problem in the trap handler (which probably involves rewriting it) and I
> > haven't managed to do that yet.
> >
> > In short, I think the debugger is more useful with this patchset than
> > without so, although it is caveated by the above, I'd call this:
> >
> > Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> > Tested-by: Daniel Thompson <daniel.thompson@linaro.org>
> >
> >
> > Daniel.
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Re: [PATCH v5 1/2] arm64: entry: Skip single stepping into interrupt handlers

[Sumit Garg]

Hi Will,

Thanks for your review.

On Thu, 26 Jan 2023 at 19:09, Will Deacon <will@kernel.org> wrote:
>
> On Mon, Dec 19, 2022 at 03:54:51PM +0530, Sumit Garg wrote:
> > Currently on systems where 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.
> >
> > The common user's goal 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 into 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>
> > Tested-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >  arch/arm64/kernel/entry-common.c | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> > index cce1167199e3..688d1ef8e864 100644
> > --- a/arch/arm64/kernel/entry-common.c
> > +++ b/arch/arm64/kernel/entry-common.c
> > @@ -231,11 +231,15 @@ DEFINE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
> >  #define need_irq_preemption()        (IS_ENABLED(CONFIG_PREEMPTION))
> >  #endif
> >
> > -static void __sched arm64_preempt_schedule_irq(void)
> > +static void __sched arm64_preempt_schedule_irq(struct pt_regs *regs)
> >  {
> >       if (!need_irq_preemption())
> >               return;
> >
> > +     /* Don't reschedule in case we are single stepping */
> > +     if (!(regs->pstate & DBG_SPSR_SS))
> > +             return;
>
> Hmm, isn't this the common case? PSTATE.SS will usually be clear, no?
>

Ah I see, looks like a copy paste error from v4. This check should be instead:

     /* Don't reschedule in case we are single stepping */
     if (regs->pstate & DBG_SPSR_SS)
             return;

Thanks for catching this, I will correct it in the next version.

> >        * Note: thread_info::preempt_count includes both thread_info::count
> >        * and thread_info::need_resched, and is not equivalent to
> > @@ -471,19 +475,33 @@ static __always_inline void __el1_irq(struct pt_regs *regs,
> >       do_interrupt_handler(regs, handler);
> >       irq_exit_rcu();
> >
> > -     arm64_preempt_schedule_irq();
> > +     arm64_preempt_schedule_irq(regs);
> >
> >       exit_to_kernel_mode(regs);
> >  }
> > +
> >  static void noinstr el1_interrupt(struct pt_regs *regs,
> >                                 void (*handler)(struct pt_regs *))
> >  {
> > +     unsigned long mdscr;
> > +
> > +     /* Disable single stepping within interrupt handler */
> > +     if (regs->pstate & DBG_SPSR_SS) {
> > +             mdscr = read_sysreg(mdscr_el1);
> > +             write_sysreg(mdscr & ~DBG_MDSCR_SS, mdscr_el1);
> > +     }
>
> I think this will break the implicit handling of kernel {break,watch}points.
>

Can you please elaborate here? AFAICS, this change will only omit the
interrupt handler while stepping.

> Sadly, I think any attempts to workaround the issues here are likely just
> to push the problems around. We really need to overhaul the debug exception
> handling logic we have, which means I need to get back to writing up a
> proposal.
>

I will be very happy to assist you if you can help me understand the
problem here.

BTW, patch #2 should be an independent fix from patch #1. Can you pull
that alone?

-Sumit

> Will

Re: [PATCH v5 1/2] arm64: entry: Skip single stepping into interrupt handlers

[Ard Biesheuvel]

On Thu, 26 Jan 2023 at 14:40, Will Deacon <will@kernel.org> wrote:
>
> On Mon, Dec 19, 2022 at 03:54:51PM +0530, Sumit Garg wrote:
> > Currently on systems where 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.
> >
> > The common user's goal 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 into 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>
> > Tested-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >  arch/arm64/kernel/entry-common.c | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> > index cce1167199e3..688d1ef8e864 100644
> > --- a/arch/arm64/kernel/entry-common.c
> > +++ b/arch/arm64/kernel/entry-common.c
> > @@ -231,11 +231,15 @@ DEFINE_STATIC_KEY_TRUE(sk_dynamic_irqentry_exit_cond_resched);
> >  #define need_irq_preemption()        (IS_ENABLED(CONFIG_PREEMPTION))
> >  #endif
> >
> > -static void __sched arm64_preempt_schedule_irq(void)
> > +static void __sched arm64_preempt_schedule_irq(struct pt_regs *regs)
> >  {
> >       if (!need_irq_preemption())
> >               return;
> >
> > +     /* Don't reschedule in case we are single stepping */
> > +     if (!(regs->pstate & DBG_SPSR_SS))
> > +             return;
>
> Hmm, isn't this the common case? PSTATE.SS will usually be clear, no?
>
> >        * Note: thread_info::preempt_count includes both thread_info::count
> >        * and thread_info::need_resched, and is not equivalent to
> > @@ -471,19 +475,33 @@ static __always_inline void __el1_irq(struct pt_regs *regs,
> >       do_interrupt_handler(regs, handler);
> >       irq_exit_rcu();
> >
> > -     arm64_preempt_schedule_irq();
> > +     arm64_preempt_schedule_irq(regs);
> >
> >       exit_to_kernel_mode(regs);
> >  }
> > +
> >  static void noinstr el1_interrupt(struct pt_regs *regs,
> >                                 void (*handler)(struct pt_regs *))
> >  {
> > +     unsigned long mdscr;
> > +
> > +     /* Disable single stepping within interrupt handler */
> > +     if (regs->pstate & DBG_SPSR_SS) {
> > +             mdscr = read_sysreg(mdscr_el1);
> > +             write_sysreg(mdscr & ~DBG_MDSCR_SS, mdscr_el1);
> > +     }
>
> I think this will break the implicit handling of kernel {break,watch}points.
>
> Sadly, I think any attempts to workaround the issues here are likely just
> to push the problems around. We really need to overhaul the debug exception
> handling logic we have, which means I need to get back to writing up a
> proposal.
>

That would be much appreciated.

This patch makes single step debugging of VMs running under QEMU much
more useful (using QEMU gdbstub), for the same reason as with kdb, as
otherwise, there's a 50/50 chance (in my experience) that doing a
single step will take you the IRQ handler instead of to the next
instruction in program order.

FWIW, I tested this patch with that scenario, and it seems to work
much better, but not 100%: I still end up in the IRQ handler
occasionally, but considerably less often.

Re: [PATCH v5 2/2] arm64: kgdb: Set PSTATE.SS to 1 to re-enable single-step

[Will Deacon]

On Mon, Dec 19, 2022 at 03:54:52PM +0530, Sumit Garg wrote:
> Currently only the first attempt to single-step has any effect. After
> that all further stepping remains "stuck" at the same program counter
> value.
> 
> Refer to the ARM Architecture Reference Manual (ARM DDI 0487E.a) D2.12,
> PSTATE.SS=1 should be set at each step before 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>
> Tested-by: Douglas Anderson <dianders@chromium.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 7b7e05c02691..ce3875ad5cd3 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 3da09778267e..9af898b22ed4 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);
> +}

Just a nit on the naming here, but please can this be
kernel_rewind_single_step() instead? I think it's closer to the rewind
function we have for user tasks than the reset function.

Cheers,

Will

Re: [PATCH v5 2/2] arm64: kgdb: Set PSTATE.SS to 1 to re-enable single-step

[Sumit Garg]

On Fri, 27 Jan 2023 at 17:20, Will Deacon <will@kernel.org> wrote:
>
> On Mon, Dec 19, 2022 at 03:54:52PM +0530, Sumit Garg wrote:
> > Currently only the first attempt to single-step has any effect. After
> > that all further stepping remains "stuck" at the same program counter
> > value.
> >
> > Refer to the ARM Architecture Reference Manual (ARM DDI 0487E.a) D2.12,
> > PSTATE.SS=1 should be set at each step before 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>
> > Tested-by: Douglas Anderson <dianders@chromium.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 7b7e05c02691..ce3875ad5cd3 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 3da09778267e..9af898b22ed4 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);
> > +}
>
> Just a nit on the naming here, but please can this be
> kernel_rewind_single_step() instead? I think it's closer to the rewind
> function we have for user tasks than the reset function.
>

Sure, I will do the renaming in the next version.

-Sumit

> Cheers,
>
> Will