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

[PATCH v3 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 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        | 18 +++++++++++++++++-
 arch/arm64/kernel/kgdb.c                |  2 ++
 4 files changed, 25 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH v3 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>
---
 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

[PATCH v3 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,
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

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

[Doug Anderson]

Hi,

On Tue, May 10, 2022 at 11:05 PM 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 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        | 18 +++++++++++++++++-
>  arch/arm64/kernel/kgdb.c                |  2 ++
>  4 files changed, 25 insertions(+), 1 deletion(-)

Sorry it took so long for me to respond. I kept dreaming that I'd find
the time to really dig deep into this to understand it fully and I'm
finally giving up on it. I'm going to hope that Will and/or Catalin
knows this area of the code well and can give it a good review. If not
then I'll strive harder to make the time...

In any case, I poked around with this a bunch and it definitely
improved the stepping behavior a whole lot. I still got one case where
gdb hit an assertion while I was stepping, but I could believe that
was a problem with gdb? I couldn't reproduce it. Thus I can at least
give:

Tested-by: Douglas Anderson <dianders@chromium.org>

I'll also note that I _think_ I remember that with Wei's series that
the gdb function "call" started working. I tried that here and it
didn't seem so happy. To keep things simple, I created a dummy
function in my kernel that looked like:

void doug_test(void)
{
  pr_info("testing, 1 2 3\n");
}

I broke into the debugger by echoing "g" to /proc/sysrq-trigger and
then tried "call doug_test()". I guess my printout actually printed
but it wasn't so happy after that. Seems like it somehow ended up
returning to a bogus address after the call which then caused a crash.

  testing, 1 2 3
  BUG: sleeping function called from invalid context at
arch/arm64/mm/fault.c:593
  in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 3393, name: bash
  preempt_count: 0, expected: 0
  RCU nest depth: 1, expected: 0
  CPU: 6 PID: 3393 Comm: bash Not tainted 5.19.0-rc4+ #3
dbec0bdb8582e447bccdcf2e70d7fe04477b1aac
  Hardware name: Google Herobrine (rev1+) (DT)
  Call trace:
   dump_backtrace+0xf0/0x110
   show_stack+0x24/0x70
   dump_stack_lvl+0x64/0x7c
   dump_stack+0x18/0x38
   __might_resched+0x144/0x154
   __might_sleep+0x54/0x84
   do_page_fault+0x1d4/0x42c
   do_mem_abort+0x4c/0xb0
   el1_abort+0x3c/0x5c
   el1h_64_sync_handler+0x4c/0xc4
   el1h_64_sync+0x64/0x68
   0xffffffc008000000
   __handle_sysrq+0x15c/0x184
   write_sysrq_trigger+0x94/0x128
   proc_reg_write+0xbc/0xec
   vfs_write+0xf0/0x2c8
   ksys_write+0x84/0xf0
   __arm64_sys_write+0x28/0x34
   invoke_syscall+0x4c/0x120
   el0_svc_common+0x94/0xfc
   do_el0_svc+0x38/0xc0
   el0_svc+0x2c/0x7c
   el0t_64_sync_handler+0x48/0x114
   el0t_64_sync+0x18c/0x190
  Unable to handle kernel execute from non-executable memory at
virtual address ffffffc008000000
  Mem abort info:
    ESR = 0x000000008600000f
    EC = 0x21: IABT (current EL), IL = 32 bits
    SET = 0, FnV = 0
    EA = 0, S1PTW = 0
    FSC = 0x0f: level 3 permission fault
  swapper pgtable: 4k pages, 39-bit VAs, pgdp=0000000082863000
  [ffffffc008000000] pgd=100000027ffff003, p4d=100000027ffff003,
pud=100000027ffff003, pmd=100000027fffe003, pte=00680001001c3703
  Internal error: Oops: 8600000f [#1] PREEMPT SMP

I'm not sure if that's a sign that something is missing with your patch or not.

-Doug

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

[Will Deacon]

On Fri, Jul 01, 2022 at 03:14:16PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, May 10, 2022 at 11:05 PM 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 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        | 18 +++++++++++++++++-
> >  arch/arm64/kernel/kgdb.c                |  2 ++
> >  4 files changed, 25 insertions(+), 1 deletion(-)
> 
> Sorry it took so long for me to respond. I kept dreaming that I'd find
> the time to really dig deep into this to understand it fully and I'm
> finally giving up on it. I'm going to hope that Will and/or Catalin
> knows this area of the code well and can give it a good review. If not
> then I'll strive harder to make the time...

So the good news is that I spent a couple of days on this last week.

The bad news is that I'm nowhere done and about to disappear on holiday
for a week!

But anyway, I might be able to give this a review when I'm back. Failing
that, I wonder if enough of us have a little bit of time each that we
could hack on an agreed design together which covers the debug exception
behaviour beyond kgdb?

Will

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

[Doug Anderson]

Hi,

On Fri, Jul 8, 2022 at 9:31 AM Will Deacon <will@kernel.org> wrote:
>
> On Fri, Jul 01, 2022 at 03:14:16PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, May 10, 2022 at 11:05 PM 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 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        | 18 +++++++++++++++++-
> > >  arch/arm64/kernel/kgdb.c                |  2 ++
> > >  4 files changed, 25 insertions(+), 1 deletion(-)
> >
> > Sorry it took so long for me to respond. I kept dreaming that I'd find
> > the time to really dig deep into this to understand it fully and I'm
> > finally giving up on it. I'm going to hope that Will and/or Catalin
> > knows this area of the code well and can give it a good review. If not
> > then I'll strive harder to make the time...
>
> So the good news is that I spent a couple of days on this last week.

Excellent, thanks!


> The bad news is that I'm nowhere done and about to disappear on holiday
> for a week!

No worries. It's been broken for so many years and isn't the most
urgent thing, but it's also one of those things that I do eventually
want to get fixed so I just want to make sure it doesn't get put off
indefinitely...


> But anyway, I might be able to give this a review when I'm back. Failing
> that, I wonder if enough of us have a little bit of time each that we
> could hack on an agreed design together which covers the debug exception
> behaviour beyond kgdb?

If it will unblock this then I can figure out how to make time for it.
Definitely my biggest utility would be in testing, but I'm also happy
to knock around ideas. ...and as per above if the way to move forward
is to block off a day or two to learn more about all the status bits
and how they interact in the kernel then I can do that too. Barring
unforeseen circumstances I'll also be in Dublin in September if that's
somehow useful. ;-)

-Doug

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

[Sumit Garg]

Hi Will,

On Fri, 8 Jul 2022 at 22:01, Will Deacon <will@kernel.org> wrote:
>
> On Fri, Jul 01, 2022 at 03:14:16PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, May 10, 2022 at 11:05 PM 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 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        | 18 +++++++++++++++++-
> > >  arch/arm64/kernel/kgdb.c                |  2 ++
> > >  4 files changed, 25 insertions(+), 1 deletion(-)
> >
> > Sorry it took so long for me to respond. I kept dreaming that I'd find
> > the time to really dig deep into this to understand it fully and I'm
> > finally giving up on it. I'm going to hope that Will and/or Catalin
> > knows this area of the code well and can give it a good review. If not
> > then I'll strive harder to make the time...
>
> So the good news is that I spent a couple of days on this last week.
>

Thanks for spending time to review this.

> The bad news is that I'm nowhere done and about to disappear on holiday
> for a week!
>
> But anyway, I might be able to give this a review when I'm back. Failing
> that, I wonder if enough of us have a little bit of time each that we
> could hack on an agreed design together which covers the debug exception
> behaviour beyond kgdb?

Sure I will be happy to contribute to improving overall debug
exception behaviour. I look forward to any further
comments/suggestions.

-Sumit

>
> Will

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

[Sumit Garg]

Hi Doug,

On Sat, 2 Jul 2022 at 03:44, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, May 10, 2022 at 11:05 PM 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 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        | 18 +++++++++++++++++-
> >  arch/arm64/kernel/kgdb.c                |  2 ++
> >  4 files changed, 25 insertions(+), 1 deletion(-)
>
> Sorry it took so long for me to respond. I kept dreaming that I'd find
> the time to really dig deep into this to understand it fully and I'm
> finally giving up on it.

No worries and apologies on my part as well as I had to find some time
to reproduce the issue that you have reported below.

> I'm going to hope that Will and/or Catalin
> knows this area of the code well and can give it a good review. If not
> then I'll strive harder to make the time...
>
> In any case, I poked around with this a bunch and it definitely
> improved the stepping behavior a whole lot. I still got one case where
> gdb hit an assertion while I was stepping, but I could believe that
> was a problem with gdb? I couldn't reproduce it. Thus I can at least
> give:
>
> Tested-by: Douglas Anderson <dianders@chromium.org>
>

Thanks for the testing.

> I'll also note that I _think_ I remember that with Wei's series that
> the gdb function "call" started working. I tried that here and it
> didn't seem so happy. To keep things simple, I created a dummy
> function in my kernel that looked like:
>
> void doug_test(void)
> {
>   pr_info("testing, 1 2 3\n");
> }
>
> I broke into the debugger by echoing "g" to /proc/sysrq-trigger and
> then tried "call doug_test()". I guess my printout actually printed
> but it wasn't so happy after that. Seems like it somehow ended up
> returning to a bogus address after the call which then caused a crash.
>

I am able to reproduce this issue on my setup as well. But it doesn't
seem to be a regression caused by this patch-set over Wei's series. As
I could reproduce this issue with v1 [1] patch-set as well which was
just a forward port of pending patches from Wei's series to the latest
upstream.

Maybe it's a different regression caused by other changes? BTW, do you
remember the kernel version you tested with Wei's series applied?

[1] https://lore.kernel.org/linux-arm-kernel/20220411093819.1012583-1-sumit.garg@linaro.org/T/

-Sumit

>   testing, 1 2 3
>   BUG: sleeping function called from invalid context at
> arch/arm64/mm/fault.c:593
>   in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 3393, name: bash
>   preempt_count: 0, expected: 0
>   RCU nest depth: 1, expected: 0
>   CPU: 6 PID: 3393 Comm: bash Not tainted 5.19.0-rc4+ #3
> dbec0bdb8582e447bccdcf2e70d7fe04477b1aac
>   Hardware name: Google Herobrine (rev1+) (DT)
>   Call trace:
>    dump_backtrace+0xf0/0x110
>    show_stack+0x24/0x70
>    dump_stack_lvl+0x64/0x7c
>    dump_stack+0x18/0x38
>    __might_resched+0x144/0x154
>    __might_sleep+0x54/0x84
>    do_page_fault+0x1d4/0x42c
>    do_mem_abort+0x4c/0xb0
>    el1_abort+0x3c/0x5c
>    el1h_64_sync_handler+0x4c/0xc4
>    el1h_64_sync+0x64/0x68
>    0xffffffc008000000
>    __handle_sysrq+0x15c/0x184
>    write_sysrq_trigger+0x94/0x128
>    proc_reg_write+0xbc/0xec
>    vfs_write+0xf0/0x2c8
>    ksys_write+0x84/0xf0
>    __arm64_sys_write+0x28/0x34
>    invoke_syscall+0x4c/0x120
>    el0_svc_common+0x94/0xfc
>    do_el0_svc+0x38/0xc0
>    el0_svc+0x2c/0x7c
>    el0t_64_sync_handler+0x48/0x114
>    el0t_64_sync+0x18c/0x190
>   Unable to handle kernel execute from non-executable memory at
> virtual address ffffffc008000000
>   Mem abort info:
>     ESR = 0x000000008600000f
>     EC = 0x21: IABT (current EL), IL = 32 bits
>     SET = 0, FnV = 0
>     EA = 0, S1PTW = 0
>     FSC = 0x0f: level 3 permission fault
>   swapper pgtable: 4k pages, 39-bit VAs, pgdp=0000000082863000
>   [ffffffc008000000] pgd=100000027ffff003, p4d=100000027ffff003,
> pud=100000027ffff003, pmd=100000027fffe003, pte=00680001001c3703
>   Internal error: Oops: 8600000f [#1] PREEMPT SMP
>
> I'm not sure if that's a sign that something is missing with your patch or not.
>
> -Doug

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

[Doug Anderson]

Hi,

On Mon, Jul 11, 2022 at 5:44 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> > I'll also note that I _think_ I remember that with Wei's series that
> > the gdb function "call" started working. I tried that here and it
> > didn't seem so happy. To keep things simple, I created a dummy
> > function in my kernel that looked like:
> >
> > void doug_test(void)
> > {
> >   pr_info("testing, 1 2 3\n");
> > }
> >
> > I broke into the debugger by echoing "g" to /proc/sysrq-trigger and
> > then tried "call doug_test()". I guess my printout actually printed
> > but it wasn't so happy after that. Seems like it somehow ended up
> > returning to a bogus address after the call which then caused a crash.
> >
>
> I am able to reproduce this issue on my setup as well. But it doesn't
> seem to be a regression caused by this patch-set over Wei's series. As
> I could reproduce this issue with v1 [1] patch-set as well which was
> just a forward port of pending patches from Wei's series to the latest
> upstream.
>
> Maybe it's a different regression caused by other changes? BTW, do you
> remember the kernel version you tested with Wei's series applied?

Sorry, I don't remember! :( I can't even be 100% sure that I'm
remembering correctly that I tested it back in the day, so it's
possible that it simply never worked...

-Doug

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

[Sumit Garg]

On Mon, 11 Jul 2022 at 19:17, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Jul 11, 2022 at 5:44 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > > I'll also note that I _think_ I remember that with Wei's series that
> > > the gdb function "call" started working. I tried that here and it
> > > didn't seem so happy. To keep things simple, I created a dummy
> > > function in my kernel that looked like:
> > >
> > > void doug_test(void)
> > > {
> > >   pr_info("testing, 1 2 3\n");
> > > }
> > >
> > > I broke into the debugger by echoing "g" to /proc/sysrq-trigger and
> > > then tried "call doug_test()". I guess my printout actually printed
> > > but it wasn't so happy after that. Seems like it somehow ended up
> > > returning to a bogus address after the call which then caused a crash.
> > >
> >
> > I am able to reproduce this issue on my setup as well. But it doesn't
> > seem to be a regression caused by this patch-set over Wei's series. As
> > I could reproduce this issue with v1 [1] patch-set as well which was
> > just a forward port of pending patches from Wei's series to the latest
> > upstream.
> >
> > Maybe it's a different regression caused by other changes? BTW, do you
> > remember the kernel version you tested with Wei's series applied?
>
> Sorry, I don't remember! :( I can't even be 100% sure that I'm
> remembering correctly that I tested it back in the day, so it's
> possible that it simply never worked...

Okay, no worries. Let me see if I can come up with a separate fix for this.

-Sumit

>
> -Doug

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

[Sumit Garg]

On Mon, 11 Jul 2022 at 19:21, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Mon, 11 Jul 2022 at 19:17, Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Mon, Jul 11, 2022 at 5:44 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > > I'll also note that I _think_ I remember that with Wei's series that
> > > > the gdb function "call" started working. I tried that here and it
> > > > didn't seem so happy. To keep things simple, I created a dummy
> > > > function in my kernel that looked like:
> > > >
> > > > void doug_test(void)
> > > > {
> > > >   pr_info("testing, 1 2 3\n");
> > > > }
> > > >
> > > > I broke into the debugger by echoing "g" to /proc/sysrq-trigger and
> > > > then tried "call doug_test()". I guess my printout actually printed
> > > > but it wasn't so happy after that. Seems like it somehow ended up
> > > > returning to a bogus address after the call which then caused a crash.
> > > >
> > >
> > > I am able to reproduce this issue on my setup as well. But it doesn't
> > > seem to be a regression caused by this patch-set over Wei's series. As
> > > I could reproduce this issue with v1 [1] patch-set as well which was
> > > just a forward port of pending patches from Wei's series to the latest
> > > upstream.
> > >
> > > Maybe it's a different regression caused by other changes? BTW, do you
> > > remember the kernel version you tested with Wei's series applied?
> >
> > Sorry, I don't remember! :( I can't even be 100% sure that I'm
> > remembering correctly that I tested it back in the day, so it's
> > possible that it simply never worked...
>
> Okay, no worries. Let me see if I can come up with a separate fix for this.
>

After digging deep into GDB call function operations for aarch64, it
is certain that function calls simply never worked due to below
reasons:

1. On aarch64, GDB call function inserts a breakpoint at the
entrypoint of kernel (which is ffffffc008000000 from your dump) as
return address from function called. And since it refers to the
"_text" symbol which is marked non-executable as the actual text
section starts with the "_stext" symbol. I did a following hack that
makes it executable:

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 626ec32873c6..e39ad1a5f5d6 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -700,7 +700,7 @@ static bool arm64_early_this_cpu_has_bti(void)
 static void __init map_kernel(pgd_t *pgdp)
 {
        static struct vm_struct vmlinux_text, vmlinux_rodata, vmlinux_inittext,
-                               vmlinux_initdata, vmlinux_data;
+                               vmlinux_initdata, vmlinux_data, vmlinux_htext;

        /*
         * External debuggers may need to write directly to the text
@@ -721,6 +721,8 @@ static void __init map_kernel(pgd_t *pgdp)
         * Only rodata will be remapped with different permissions later on,
         * all other segments are allowed to use contiguous mappings.
         */
+       map_kernel_segment(pgdp, _text, _stext, text_prot, &vmlinux_htext, 0,
+                          VM_NO_GUARD);
        map_kernel_segment(pgdp, _stext, _etext, text_prot, &vmlinux_text, 0,
                           VM_NO_GUARD);
        map_kernel_segment(pgdp, __start_rodata, __inittext_begin, PAGE_KERNEL,

2. For the GDB function "call" to work, GDB inserts a dummy stack
frame. But in case of kernel on aarch64, the stack used is common
among the exception handler and the kernel threads. So it's not
trivial to insert a dummy stack frame and requires rework of exception
entry code as it pushes pt_regs onto the stack.

-Sumit

>
> >
> > -Doug