AArch64: KGDB support

arch/arm64/include/asm/kgdb.h new file +0 -0

arch/arm64/include/asm/kgdb.h new file +2 -0

+#ifndef __ARM_KGDB_H__
+#define __ARM_KGDB_H__

+#endif /* __ASM_KGDB_H__ */

From: Vijaya Kumar K <[email protected]>

The function kgdb_breakpoint() sets up break point at compile time by calling arch_kgdb_breakpoint(); Though this call is surrounded by wmb() barrier, the compile can still re-order the break point, because this scheduling barrier is not a code motion barrier in gcc.

Making kgdb_breakpoint() as noinline solves this problem of code reording around break point instruction and also avoids problem of being called as inline function from other places

More details about discussion on this can be found here
http://comments.gmane.org/gmane.linux.ports.arm.kernel/269732

Signed-off-by Vijaya Kumar K <[email protected]>

Acked-by Will Deacon <[email protected]>

---
 kernel/debug/debug_core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/debug/debug_core.c +1 -1

index 7d2f35e..cf04798 100644
@@ -1034,7 +1034,7 @@ int dbg_io_get_char(void)
  * otherwise as a quick means to stop program execution and "break" into
  * the debugger.
  */
-void kgdb_breakpoint(void)
+noinline void kgdb_breakpoint(void)
 {
 	atomic_inc(&kgdb_setting_breakpoint);
 	wmb(); /* Sync point before breakpoint */

Quoted 14 lines · vijay.kilari at gmail.com

From: Vijaya Kumar K <[email protected]>

The function kgdb_breakpoint() sets up break point at compile time by calling arch_kgdb_breakpoint(); Though this call is surrounded by wmb() barrier, the compile can still re-order the break point, because this scheduling barrier is not a code motion barrier in gcc.

Making kgdb_breakpoint() as noinline solves this problem of code reording around break point instruction and also avoids problem of being called as inline function from other places

Quoted 5 lines

More details about discussion on this can be found here
http://comments.gmane.org/gmane.linux.ports.arm.kernel/269732

Signed-off-by Vijaya Kumar K <[email protected]>

Acked-by Will Deacon <[email protected]>

Acked-by Jason Wessel <[email protected]>

From: Vijaya Kumar K <[email protected]>

Based on the step-handler and break-handler hooks patch from Sandeepa, KGDB debugging support is added for EL1 debug in AArch64 mode. Any updates that come for Patch 1 from Sandeepa will be rebased in next version

With second patch,register layout is updated to be inline with GDB tool. Basic GDB connection, break point set/clear and info commands are supported except step/next debugging

With third patch, step/next debugging support is added, where in pc is updated to point to the instruction to be stepped and stopped.

With fourth patch, the compile time breakpoint instruction reordering is fixed by making kgbd_breakpoint() as noinline

Tested with ARM64 simulator

v6:
- Change pstate register to 8 bytes to make endian nuetral. Use GDB below GDB patch to display pstate in Big endian mode.
https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html
Thanks to Andrew.

Quoted 18 lines · vijay.kilari at gmail.com

From: Vijaya Kumar K <[email protected]>

Based on the step-handler and break-handler hooks patch from Sandeepa, KGDB debugging support is added for EL1 debug in AArch64 mode. Any updates that come for Patch 1 from Sandeepa will be rebased in next version

With second patch,register layout is updated to be inline with GDB tool. Basic GDB connection, break point set/clear and info commands are supported except step/next debugging

With third patch, step/next debugging support is added, where in pc is updated to point to the instruction to be stepped and stopped.

With fourth patch, the compile time breakpoint instruction reordering is fixed by making kgbd_breakpoint() as noinline

Quoted 7 lines

Tested with ARM64 simulator

v6:
- Change pstate register to 8 bytes to make endian nuetral. Use GDB below GDB patch to display pstate in Big endian mode.
https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html
Thanks to Andrew.

Thanks for getting this new version out. Just a few things before it can be applied:

1. Can you please use "arm64:" instead of "AArch64:" in the patch subjects?

2. Can you get an ack from somebody like Jason on the final patch please?

3. Can you rebase onto the for-next/core branch at the arm64 repo:

git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git

you'll get a trivial conflict against arch/arm64/kernel/Makefile.

4. Can you make sure that the gdb patch you linked to actually gets merged? There is still some discussion by the looks of it.

Quoted 27 lines · Will Deacon

Quoted 25 lines · vijay.kilari at gmail.com

From: Vijaya Kumar K <[email protected]>

Based on the step-handler and break-handler hooks patch from Sandeepa, KGDB debugging support is added for EL1 debug in AArch64 mode. Any updates that come for Patch 1 from Sandeepa will be rebased in next version

With second patch,register layout is updated to be inline with GDB tool. Basic GDB connection, break point set/clear and info commands are supported except step/next debugging

With third patch, step/next debugging support is added, where in pc is updated to point to the instruction to be stepped and stopped.

With fourth patch, the compile time breakpoint instruction reordering is fixed by making kgbd_breakpoint() as noinline

Tested with ARM64 simulator

v6:
- Change pstate register to 8 bytes to make endian nuetral. Use GDB below GDB patch to display pstate in Big endian mode.
https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html
Thanks to Andrew.

Quoted 6 lines

Thanks for getting this new version out. Just a few things before it can be applied:

1. Can you please use "arm64:" instead of "AArch64:" in the patch subjects?

Quoted 2 lines

2. Can you get an ack from somebody like Jason on the final patch please?

Quoted 4 lines

3. Can you rebase onto the for-next/core branch at the arm64 repo:

git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git

Quoted 1 line

you'll get a trivial conflict against arch/arm64/kernel/Makefile.

Just to be clear here, I would only recommend rebasing on top of this branch if there are patches queued for upstream which are required by the rebased patches and there isn't a topic branch already elsewhere. I do *not* recommend this branch for any kind of development (though testing is fine and we can fix the trivial Makefile conflicts ourselves).

Quoted 3 lines

4. Can you make sure that the gdb patch you linked to actually gets merged? There is still some discussion by the looks of it.

I guess the kernel patches will have to wait for the binutils patch to go in. BTW, does this mean that kgdb on arm64 will only work with a future version of gdb?

--
Catalin

Hi Vijay,

Quoted 18 lines · vijay.kilari at gmail.com

From: Vijaya Kumar K <[email protected]>

Based on the step-handler and break-handler hooks patch from Sandeepa, KGDB debugging support is added for EL1 debug in AArch64 mode. Any updates that come for Patch 1 from Sandeepa will be rebased in next version

With second patch,register layout is updated to be inline with GDB tool. Basic GDB connection, break point set/clear and info commands are supported except step/next debugging

With third patch, step/next debugging support is added, where in pc is updated to point to the instruction to be stepped and stopped.

With fourth patch, the compile time breakpoint instruction reordering is fixed by making kgbd_breakpoint() as noinline

Quoted 7 lines

Tested with ARM64 simulator

v6:
- Change pstate register to 8 bytes to make endian nuetral. Use GDB below GDB patch to display pstate in Big endian mode.
https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html
Thanks to Andrew.

Thanks for getting this new version out. Just a few things before it can be applied:

1. Can you please use "arm64:" instead of "AArch64:" in the patch subjects?

2. Can you get an ack from somebody like Jason on the final patch please?

3. Can you rebase onto the for-next/core branch at the arm64 repo:

git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git

you'll get a trivial conflict against arch/arm64/kernel/Makefile.

4. Can you make sure that the gdb patch you linked to actually gets merged? There is still some discussion by the looks of it.

Cheers,

Will

Vijaya Kumar K (3):
AArch64: KGDB: Add Basic KGDB support
AArch64: KGDB: Add step debugging support
KGDB: make kgdb_breakpoint() as noinline

Quoted 4 lines · vijay.kilari at gmail.com

Vijaya Kumar K (3):
AArch64: KGDB: Add Basic KGDB support
AArch64: KGDB: Add step debugging support
KGDB: make kgdb_breakpoint() as noinline

I gave them a try and now I have some doubts about your testing methodology:

1. There is no HAVE_ARCH_KGDB anywhere in these patches, so the code here isn't compiled.

2. There are several compiler warnings that need to be addressed.

3. I enabled CONFIG_KGDB_TESTS and CONFIG_KGDB_TESTS_ON_BOOT and that's what really scared me.

Please address these before we can consider the patches for merging.

--
Catalin

Hi Catalin,

Quoted 6 lines · Catalin Marinas

Quoted 4 lines · vijay.kilari at gmail.com

Vijaya Kumar K (3):
AArch64: KGDB: Add Basic KGDB support
AArch64: KGDB: Add step debugging support
KGDB: make kgdb_breakpoint() as noinline

Quoted 6 lines

I gave them a try and now I have some doubts about your testing methodology:

1. There is no HAVE_ARCH_KGDB anywhere in these patches, so the code here isn't compiled.

OK, I will send a patch

Quoted 5 lines

2. There are several compiler warnings that need to be addressed.

3. I enabled CONFIG_KGDB_TESTS and CONFIG_KGDB_TESTS_ON_BOOT and that's what really scared me.

Are you refering to these warning?. I don't see any warning in the patched files. I will fix these.

Quoted 1 line · Vijay Kilari

Hi Will,

Hello Vijay,

Quoted 14 lines · Will Deacon

Quoted 5 lines

Well, the warnings are one thing but the 100 miles of backtrace output that appear on every boot (and I think end in an oops) are probably more important to fix. Please enable CONFIG_KGDB_TESTS and CONFIG_KGDB_TESTS_ON_BOOT and take a look.

I could manage to run KGDB boot tests if I run from sysfs after complete boot

echo V1F1000 > /sys/module/kgdbts/parameters/kgdbts

Here the value of PSTATE is 80000145, which means PSTATE.D is unmasked hence it works fine.

Quoted 8 lines

If I run during boot by enabling CONFIG_KGDB_TESTS_ON_BOOT, the step debug test fail because value of PSTATE is 80000345. If I force PSTATE.D to 0, it works fine

In debug_monitors.c file, only PSTATE.SS & MDSCR.KDE/MDE is managed but not PSTATE.D

Can you please let me know if where PSTATE.D is managed in arm64?

That's a good point: I think we wait until our first exception before they are unmasked. Perhaps we should:

(1) Move local_dbg_{save,restore} from debug-monitors.h into irqflags.h
(2) Add local_dbg_enable/local_dbg_disable macros
(3) Add a call to local_dbg_enable in the clear_os_lock function after the

     isb().

Does that work for you?

Will

Quoted 26 lines · Will Deacon

Quoted 1 line · Vijay Kilari

Hi Will,

Hello Vijay,

Quoted 21 lines · Will Deacon

Quoted 5 lines

Well, the warnings are one thing but the 100 miles of backtrace output that appear on every boot (and I think end in an oops) are probably more important to fix. Please enable CONFIG_KGDB_TESTS and CONFIG_KGDB_TESTS_ON_BOOT and take a look.

I could manage to run KGDB boot tests if I run from sysfs after complete boot

echo V1F1000 > /sys/module/kgdbts/parameters/kgdbts

Here the value of PSTATE is 80000145, which means PSTATE.D is unmasked hence it works fine.

If I run during boot by enabling CONFIG_KGDB_TESTS_ON_BOOT, the step debug test fail because value of PSTATE is 80000345. If I force PSTATE.D to 0, it works fine

In debug_monitors.c file, only PSTATE.SS & MDSCR.KDE/MDE is managed but not PSTATE.D

Quoted 6 lines

Quoted 1 line

Can you please let me know if where PSTATE.D is managed in arm64?

That's a good point: I think we wait until our first exception before they are unmasked. Perhaps we should:

(1) Move local_dbg_{save,restore} from debug-monitors.h into irqflags.h

Quoted 5 lines

(2) Add local_dbg_enable/local_dbg_disable macros
(3) Add a call to local_dbg_enable in the clear_os_lock function after the

     isb().

Does that work for you?

Yes, only after first exception occurs the PSTATE.D is unmasked.

I have patched (temp) as below and now KGDB boot tests pass

arch/arm64/include/asm/debug-monitors.h +4 -0

index aff3a76..ea2bc46 100644
@@ -64,6 +111,24 @@ struct task_struct;
 
 #define DBG_ARCH_ID_RESERVED   0       /* In case of ptrace ABI updates. */
 
+#define local_dbg_enable()
         \
+       do {
         \
+               asm volatile(
         \
+               "msr    daifclr, #9             //

arch_local_irq_disable" \

arch/arm64/include/asm/debug-monitors.h +9 -0

+               :
         \
+               :
         \
+               : "memory");
         \
+       } while (0)
+
+#define local_dbg_disable()
         \
+       do {
         \
+               asm volatile(
         \
+               "msr    daifset, #9             //

arch_local_irq_disable" \

arch/arm64/include/asm/debug-monitors.h +5 -0

+               :
         \
+               :
         \
+               : "memory");
         \
+       } while (0)
+
 struct step_hook {
        struct list_head node;
        int (*fn)(struct pt_regs *regs, unsigned int insn, unsigned long addr);

arch/arm64/kernel/debug-monitors.c +1 -0

index f8b90c0..d0e55f7 100644
@@ -139,6 +142,7 @@ static void clear_os_lock(void *unused)
 {
        asm volatile("msr oslar_el1, %0" : : "r" (0));
        isb();
+       local_dbg_enable();
 }
 
 

boot test:

[32927.161317] msgmni has been set to 1870
[32927.212747] alg: No test for stdrng (krng)
[32927.213953] Key type asymmetric registered
[32927.214899] Asymmetric key parser 'x509' registered
[32927.220029] Block layer SCSI generic (bsg) driver version 0.4
loaded (major 253)
[32927.225824] io scheduler noop registered
[32927.226764] io scheduler deadline registered
[32927.230714] io scheduler cfq registered (default)
[32927.237895] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
[32927.266066] kgdb: Registered I/O driver kgdbts.
[32927.266419] kgdb: Waiting for connection from remote gdb...
[32927.268598] kgdbts:RUN plant and detach test
[32927.270683] kgdbts:RUN sw breakpoint test
[32927.287659] kgdbts:RUN bad memory access test
[32927.290322] kgdbts:RUN singlestep test 1000 iterations
[32927.330342] kgdbts:RUN singlestep [0/1000]
[32931.286356] kgdbts:RUN singlestep [100/1000]
[32935.242536] kgdbts:RUN singlestep [200/1000]
[32939.205392] kgdbts:RUN singlestep [300/1000]
[32943.169522] kgdbts:RUN singlestep [400/1000]
[32947.231868] kgdbts:RUN singlestep [500/1000]
[32951.188008] kgdbts:RUN singlestep [600/1000]
[32955.332243] kgdbts:RUN singlestep [700/1000]
[32959.467109] kgdbts:RUN singlestep [800/1000]
[32963.430888] kgdbts:RUN singlestep [900/1000]
[32967.346992] kgdbts:RUN do_fork for 100 breakpoints

kgdb test using sysfs:

~ # echo V1F1000 > /sys/module/kgdbts/parameters/kgdbts

[33231.554237] kgdb: Registered I/O driver kgdbts.
[33231.554677] kgdbts:RUN plant and detach test
[33231.557072] kgdbts:RUN sw breakpoint test
[33231.576980] kgdbts:RUN bad memory access test
[33231.580022] kgdbts:RUN singlestep test 1000 iterations
[33231.627056] kgdbts:RUN singlestep [0/1000]
[33235.954027] kgdbts:RUN singlestep [100/1000]
[33240.429086] kgdbts:RUN singlestep [200/1000]
[33244.687118] kgdbts:RUN singlestep [300/1000]
[33248.945191] kgdbts:RUN singlestep [400/1000]
[33253.203751] kgdbts:RUN singlestep [500/1000]
[33257.462019] kgdbts:RUN singlestep [600/1000]
[33261.817809] kgdbts:RUN singlestep [700/1000]
[33266.081268] kgdbts:RUN singlestep [800/1000]
[33270.339813] kgdbts:RUN singlestep [900/1000]
[33274.712404] kgdbts:RUN do_fork for 1000 breakpoints

~ #

This works for me. Should I patch it or will you send a patch for this?

PS: cc to mailing list missed

Quoted 2 lines

Will

Quoted 5 lines · Vijay Kilari

Quoted 5 lines

Quoted 1 line · Will Deacon

That's a good point: I think we wait until our first exception before they are unmasked. Perhaps we should:

(1) Move local_dbg_{save,restore} from debug-monitors.h into irqflags.h

local_dbg_save is setting bit #8 it should be bit #9?

#define local_dbg_save(flags)
         \
        do {
         \
                typecheck(unsigned long, flags);
         \
                asm volatile(
         \
                "mrs    %0, daif                        //

local_dbg_save\n" \
"msr daifset, #8"

Quoted 19 lines

Quoted 5 lines

(2) Add local_dbg_enable/local_dbg_disable macros
(3) Add a call to local_dbg_enable in the clear_os_lock function after the

     isb().

Does that work for you?

Yes, only after first exception occurs the PSTATE.D is unmasked.

I have patched (temp) as below and now KGDB boot tests pass

arch/arm64/include/asm/debug-monitors.h +0 -0

index aff3a76..ea2bc46 100644
@@ -64,6 +111,24 @@ struct task_struct;
 
 #define DBG_ARCH_ID_RESERVED   0       /* In case of ptrace ABI updates. */
 

arch/arm64/include/asm/debug-monitors.h struct task_struct; · line 120

Quoted 7 lines

arch/arm64/include/asm/debug-monitors.h +4 -0

+#define local_dbg_enable()
         \
+       do {
         \
+               asm volatile(
         \
+               "msr    daifclr, #9             //

Hi Catalin,

Quoted 6 lines · Catalin Marinas

Quoted 4 lines · vijay.kilari at gmail.com

Vijaya Kumar K (3):
AArch64: KGDB: Add Basic KGDB support
AArch64: KGDB: Add step debugging support
KGDB: make kgdb_breakpoint() as noinline

Quoted 6 lines

I gave them a try and now I have some doubts about your testing methodology:

1. There is no HAVE_ARCH_KGDB anywhere in these patches, so the code here isn't compiled.

OK, I will send a patch

Quoted 5 lines

2. There are several compiler warnings that need to be addressed.

3. I enabled CONFIG_KGDB_TESTS and CONFIG_KGDB_TESTS_ON_BOOT and that's what really scared me.

Are you refering to these warning?. I don't see any warning in the patched files. I will fix these.

drivers/misc/kgdbts.c: In function 'check_single_step':
drivers/misc/kgdbts.c:353:2: warning: format '%lx' expects argument of type 'long unsigned int', but argument 2 has type 'u64' [-Wformat] drivers/misc/kgdbts.c:379:3: warning: format '%lx' expects argument of type 'long unsigned int', but argument 2 has type 'u64' [-Wformat] drivers/misc/kgdbts.c: In function 'emul_sstep_put':
drivers/misc/kgdbts.c:494:3: warning: format '%lx' expects argument of type 'long unsigned int', but argument 2 has type 'u64' [-Wformat] kernel/debug/debug_core.c: In function 'kgdb_cpu_enter':
kernel/debug/debug_core.c:498:4: warning: value computed is not used [-Wunused-value]
kernel/debug/debug_core.c:513:5: warning: value computed is not used [-Wunused-value]

Quoted 4 lines

Please address these before we can consider the patches for merging.

--
Catalin