[U-Boot] [RFC PATCH v1 2/2] arm:kgdb add KGDB support for arm platform
Simon Glass
sjg at chromium.org
Tue Nov 4 07:58:06 CET 2014
HI Peng,
On 31 October 2014 23:12, Peng Fan <Peng.Fan at freescale.com> wrote:
> The register save/restore:
> Use get_bad_stack and bad_save_user_regs to save regs.
> Introduce und_restore_regs to restore the previous regs before
> trigger a breakpoint.
>
> Signed-off-by: Peng Fan <Peng.Fan at freescale.com>
Looks good so far as I understand it. A few nits below and maybe you
can integrate better with the ptrace register numbering.
> ---
> arch/arm/include/asm/proc-armv/ptrace.h | 2 +-
> arch/arm/include/asm/signal.h | 1 +
> arch/arm/lib/Makefile | 3 +
> arch/arm/lib/crt0.S | 4 +
> arch/arm/lib/interrupts.c | 24 ++++
> arch/arm/lib/kgdb/kgdb.c | 231 ++++++++++++++++++++++++++++++++
> arch/arm/lib/kgdb/setjmp.S | 20 +++
> arch/arm/lib/vectors.S | 28 ++++
> 8 files changed, 312 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/include/asm/proc-armv/ptrace.h b/arch/arm/include/asm/proc-armv/ptrace.h
> index 71df5a9..33fe587 100644
> --- a/arch/arm/include/asm/proc-armv/ptrace.h
> +++ b/arch/arm/include/asm/proc-armv/ptrace.h
> @@ -58,7 +58,7 @@ struct pt_regs {
> stack during a system call. */
>
> struct pt_regs {
> - long uregs[18];
> + unsigned long uregs[18];
> };
>
> #define ARM_cpsr uregs[16]
> diff --git a/arch/arm/include/asm/signal.h b/arch/arm/include/asm/signal.h
> new file mode 100644
> index 0000000..7b1573c
> --- /dev/null
> +++ b/arch/arm/include/asm/signal.h
> @@ -0,0 +1 @@
> +#include <asm-generic/signal.h>
> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
> index 1ef2400..c543563 100644
> --- a/arch/arm/lib/Makefile
> +++ b/arch/arm/lib/Makefile
> @@ -52,3 +52,6 @@ endif
> ifneq (,$(findstring -mabi=aapcs-linux,$(PLATFORM_CPPFLAGS)))
> extra-y += eabi_compat.o
> endif
> +
> +obj-$(CONFIG_CMD_KGDB) += kgdb/setjmp.o
> +obj-$(CONFIG_CMD_KGDB) += kgdb/kgdb.o
> diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S
> index 29cdad0..d96e70b 100644
> --- a/arch/arm/lib/crt0.S
> +++ b/arch/arm/lib/crt0.S
> @@ -99,9 +99,13 @@ clr_gd:
> sub r9, r9, #GD_SIZE /* new GD is below bd */
>
> adr lr, here
> +#ifndef CONFIG_CMD_KGDB
> ldr r0, [r9, #GD_RELOC_OFF] /* r0 = gd->reloc_off */
> add lr, lr, r0
> ldr r0, [r9, #GD_RELOCADDR] /* r0 = gd->relocaddr */
> +#else
> + ldr r0, =__image_copy_start
> +#endif
> b relocate_code
> here:
>
> diff --git a/arch/arm/lib/interrupts.c b/arch/arm/lib/interrupts.c
> index 4dacfd9..d16bd58 100644
> --- a/arch/arm/lib/interrupts.c
> +++ b/arch/arm/lib/interrupts.c
> @@ -22,6 +22,7 @@
> #include <common.h>
> #include <asm/proc-armv/ptrace.h>
> #include <asm/u-boot-arm.h>
> +#include <kgdb.h>
>
> DECLARE_GLOBAL_DATA_PTR;
>
> @@ -160,9 +161,32 @@ void show_regs (struct pt_regs *regs)
>
> void do_undefined_instruction (struct pt_regs *pt_regs)
> {
> +#if defined(CONFIG_CMD_KGDB)
> + unsigned long *tmp_pc = NULL;
> +
> + pt_regs->ARM_pc -= 4;
> + tmp_pc = (unsigned long *)pt_regs->ARM_pc;
> +
> + if (*tmp_pc == 0xe7ffdeff) {
> + pt_regs->ARM_pc += 4;
> + if (debugger_exception_handler &&
> + debugger_exception_handler(pt_regs)) {
> + return;
> + }
> + } else if (*tmp_pc == 0xe7ffdefe) {
> + if (debugger_exception_handler &&
> + debugger_exception_handler(pt_regs)) {
> + return;
> + }
> + } else {
> + printf("DCache/ICACHE May need flush\n");
> + return;
> + }
> +#else
> printf ("undefined instruction\n");
> show_regs (pt_regs);
> bad_mode ();
> +#endif
> }
>
> void do_software_interrupt (struct pt_regs *pt_regs)
> diff --git a/arch/arm/lib/kgdb/kgdb.c b/arch/arm/lib/kgdb/kgdb.c
> new file mode 100644
> index 0000000..6b2e542
> --- /dev/null
> +++ b/arch/arm/lib/kgdb/kgdb.c
> @@ -0,0 +1,231 @@
> +#include <common.h>
> +#include <command.h>
> +#include <kgdb.h>
> +#include <asm/signal.h>
> +#include <asm/processor.h>
> +
> +#define KGDB_ARM_GP_REGS 16
> +#define KGDB_ARM_FP_REGS 8
> +#define KGDB_ARM_EXTRA_REGS 2
> +#define KGDB_ARM_MAX_REGS (KGDB_ARM_GP_REGS +\
> + (KGDB_ARM_FP_REGS * 3) +\
> + KGDB_ARM_EXTRA_REGS)
> +#define KGDB_NUMREGBYTES (KGDB_ARM_MAX_REGS << 2)
> +
> +enum arm_regs {
> + KGDB_ARM_R0,
> + KGDB_ARM_R1,
> + KGDB_ARM_R2,
> + KGDB_ARM_R3,
> + KGDB_ARM_R4,
> + KGDB_ARM_R5,
> + KGDB_ARM_R6,
> + KGDB_ARM_R7,
> + KGDB_ARM_R8,
> + KGDB_ARM_R9,
> + KGDB_ARM_R10,
> + KGDB_ARM_FP,
> + KGDB_ARM_IP,
> + KGDB_ARM_SP,
> + KGDB_ARM_LR,
> + KGDB_ARM_PC,
So in here there are the FP and EXTRA regs? If you added them to this
enum it would be clearer.
Also these mirror the numbers in ptrace.h so I wonder if somehow they
could be linked?
> + KGDB_ARM_CPSR = KGDB_ARM_MAX_REGS - 1
> +};
> +
> +void kgdb_enter(struct pt_regs *regs, kgdb_data *kdp)
> +{
> + disable_interrupts();
> +
> + kdp->sigval = kgdb_trap(regs);
> + kdp->nregs = 2;
> + kdp->regs[0].num = KGDB_ARM_PC;
> + kdp->regs[0].val = regs->ARM_pc;
> +
> + kdp->regs[1].num = KGDB_ARM_SP;
> + kdp->regs[1].val = regs->ARM_sp;
> +
> + return;
> +}
> +
> +void kgdb_exit(struct pt_regs *regs, kgdb_data *kdp)
> +{
> + /* Mark Not sure ??? */
What does this mean?
> +
> + if (kdp->extype & KGDBEXIT_WITHADDR) {
> + regs->ARM_pc = kdp->exaddr;
> + printf("KGDBEXIT_WITHADDR 0x%lx\n", regs->ARM_pc);
> + }
> +
> + switch (kdp->extype & KGDBEXIT_TYPEMASK) {
> + case KGDBEXIT_KILL:
> + printf("KGDBEXIT_KILL:\n");
> + break;
> + case KGDBEXIT_CONTINUE:
> + printf("KGDBEXIT_CONTINUE\n");
> + break;
> + case KGDBEXIT_SINGLE:
> + printf("KGDBEXIT_SINGLE\n");
> + break;
> + default:
> + printf("KGDBEXIT : %d\n", kdp->extype);
> + }
> +
> + enable_interrupts();
> +}
> +
> +int kgdb_trap(struct pt_regs *regs)
> +{
> + /* Mark Not sure ??? */
And this?
> + return SIGTRAP;
> +}
> +
> +int kgdb_getregs(struct pt_regs *regs, char *buf, int max)
> +{
> + unsigned long *gdb_regs = (unsigned long *)buf;
> +
> + if (max < KGDB_NUMREGBYTES)
> + kgdb_error(KGDBERR_NOSPACE);
> +
> + if ((unsigned long)gdb_regs & 3)
> + kgdb_error(KGDBERR_ALIGNFAULT);
> +
> + gdb_regs[KGDB_ARM_R0] = regs->ARM_r0;
> + gdb_regs[KGDB_ARM_R1] = regs->ARM_r1;
> + gdb_regs[KGDB_ARM_R2] = regs->ARM_r2;
> + gdb_regs[KGDB_ARM_R3] = regs->ARM_r3;
> + gdb_regs[KGDB_ARM_R4] = regs->ARM_r4;
> + gdb_regs[KGDB_ARM_R5] = regs->ARM_r5;
> + gdb_regs[KGDB_ARM_R6] = regs->ARM_r6;
> + gdb_regs[KGDB_ARM_R7] = regs->ARM_r7;
> + gdb_regs[KGDB_ARM_R8] = regs->ARM_r8;
> + gdb_regs[KGDB_ARM_R9] = regs->ARM_r9;
> + gdb_regs[KGDB_ARM_R10] = regs->ARM_r10;
> + gdb_regs[KGDB_ARM_IP] = regs->ARM_ip;
> + gdb_regs[KGDB_ARM_FP] = regs->ARM_fp;
> + /* set sp */
I think you can remove that comment as it's pretty obvious
> + gdb_regs[KGDB_ARM_SP] = (unsigned long)regs;
> + gdb_regs[KGDB_ARM_LR] = regs->ARM_lr;
> + gdb_regs[KGDB_ARM_PC] = regs->ARM_pc;
> + gdb_regs[KGDB_ARM_CPSR] = regs->ARM_cpsr;
> +
> + return KGDB_NUMREGBYTES;
> +}
> +
> +void kgdb_putreg(struct pt_regs *regs, int regno, char *buf, int length)
> +{
> + unsigned long *ptr = (unsigned long *)buf;
> +
> + if (regno < 0 || regno >= 16)
Should that be regno > KGDB_ARM_PC?
> + kgdb_error(KGDBERR_BADPARAMS);
> +
> + if (length < 4)
> + kgdb_error(KGDBERR_NOSPACE);
> +
> + if ((unsigned long)ptr & 3)
> + kgdb_error(KGDBERR_ALIGNFAULT);
> +
> + switch (regno) {
> + case KGDB_ARM_R0:
> + regs->ARM_r0 = *ptr;
> + break;
Would it be valid to drop this switch and use:
regs->uregs[regno] = *ptr
> + case KGDB_ARM_R1:
> + regs->ARM_r1 = *ptr;
> + break;
> + case KGDB_ARM_R2:
> + regs->ARM_r2 = *ptr;
> + break;
> + case KGDB_ARM_R3:
> + regs->ARM_r3 = *ptr;
> + break;
> + case KGDB_ARM_R4:
> + regs->ARM_r4 = *ptr;
> + break;
> + case KGDB_ARM_R5:
> + regs->ARM_r5 = *ptr;
> + break;
> + case KGDB_ARM_R6:
> + regs->ARM_r6 = *ptr;
> + break;
> + case KGDB_ARM_R7:
> + regs->ARM_r7 = *ptr;
> + break;
> + case KGDB_ARM_R8:
> + regs->ARM_r8 = *ptr;
> + break;
> + case KGDB_ARM_R9:
> + regs->ARM_r9 = *ptr;
> + break;
> + case KGDB_ARM_R10:
> + regs->ARM_r10 = *ptr;
> + break;
> + case KGDB_ARM_FP:
> + regs->ARM_fp = *ptr;
> + break;
> + case KGDB_ARM_IP:
> + regs->ARM_ip = *ptr;
> + break;
> + case KGDB_ARM_SP:
> + regs->ARM_sp = *ptr;
> + break;
> + case KGDB_ARM_LR:
> + regs->ARM_lr = *ptr;
> + break;
> + case KGDB_ARM_PC:
> + regs->ARM_pc = *ptr;
> + break;
> + case KGDB_ARM_CPSR:
> + regs->ARM_cpsr = *ptr;
> + break;
> + default:
> + kgdb_error(KGDBERR_BADPARAMS);
> + break;
> + }
> +}
> +
> +void kgdb_putregs(struct pt_regs *regs, char *buf, int length)
> +{
> + unsigned long *kgdb_regs = (unsigned long *)buf;
> +
> + if (length != KGDB_ARM_MAX_REGS)
Is length the number of registers rather than the number of bytes?
> + kgdb_error(KGDBERR_NOSPACE);
> +
> + if ((unsigned long)kgdb_regs & 3)
> + kgdb_error(KGDBERR_ALIGNFAULT);
> +
> + regs->ARM_r0 = kgdb_regs[KGDB_ARM_R0];
> + regs->ARM_r1 = kgdb_regs[KGDB_ARM_R1];
> + regs->ARM_r2 = kgdb_regs[KGDB_ARM_R2];
> + regs->ARM_r3 = kgdb_regs[KGDB_ARM_R3];
> + regs->ARM_r4 = kgdb_regs[KGDB_ARM_R4];
> + regs->ARM_r5 = kgdb_regs[KGDB_ARM_R5];
> + regs->ARM_r6 = kgdb_regs[KGDB_ARM_R6];
> + regs->ARM_r7 = kgdb_regs[KGDB_ARM_R7];
> + regs->ARM_r8 = kgdb_regs[KGDB_ARM_R8];
> + regs->ARM_r9 = kgdb_regs[KGDB_ARM_R9];
> + regs->ARM_r10 = kgdb_regs[KGDB_ARM_R10];
> + regs->ARM_ip = kgdb_regs[KGDB_ARM_IP];
> + regs->ARM_fp = kgdb_regs[KGDB_ARM_FP];
> + regs->ARM_sp = kgdb_regs[KGDB_ARM_SP];
> + regs->ARM_lr = kgdb_regs[KGDB_ARM_LR];
> + regs->ARM_pc = kgdb_regs[KGDB_ARM_PC];
> + regs->ARM_cpsr = kgdb_regs[KGDB_ARM_CPSR];
Feels like this could be a for() loop.
> +}
> +
> +void kgdb_flush_cache_all(void)
> +{
> + invalidate_icache_all();
> + flush_dcache_all();
Do you need to flush the write buffer here?
> +}
> +
> +/*
> + * This function will generate a breakpoint exception.
> + * It is used at the beginning of a program to sync up
> + * with a debugger and can be used otherwise as a quick
> + * means to stop program execution and "break" into
> + * the debugger.
> + */
> +
> +void kgdb_breakpoint(int argc, char * const argv[])
> +{
> + asm(".word 0xe7ffdeff");
> +}
> diff --git a/arch/arm/lib/kgdb/setjmp.S b/arch/arm/lib/kgdb/setjmp.S
> new file mode 100644
> index 0000000..e17f959
> --- /dev/null
> +++ b/arch/arm/lib/kgdb/setjmp.S
> @@ -0,0 +1,20 @@
> + .text
> + .balign 4
> + .global kgdb_setjmp
> + .type kgdb_setjmp, #function
> +kgdb_setjmp:
> + stmia r0, {r4, r5, r6, r7, r8, r9, r10, fp, sp, lr}
{r4-r10, fp, sp, lr}
and below
> + mov r0, #0
> + mov pc, lr
> + .size kgdb_setjmp,.-kgdb_setjmp
> +
> + .text
> + .balign 4
> + .global kgdb_longjmp
> + .type kgdb_longjmp, #function
> +kgdb_longjmp:
> + ldmia r0, {r4, r5, r6, r7, r8, r9, r10, fp, sp, lr}
> + movs r0, r1
> + moveq r0, #1
> + mov pc, lr
> + .size kgdb_longjmp,.-kgdb_longjmp
> diff --git a/arch/arm/lib/vectors.S b/arch/arm/lib/vectors.S
> index 49238ed..8abad3d 100644
> --- a/arch/arm/lib/vectors.S
> +++ b/arch/arm/lib/vectors.S
> @@ -221,15 +221,43 @@ FIQ_STACK_START:
> ldr sp, FIQ_STACK_START
> .endm
>
> + .macro get_und_stack
> + get_bad_stack
> + .endm
> +
> + .macro und_save_regs
> + bad_save_user_regs
> + .endm
> +
> + /* In svc mode */
> + .macro und_restore_regs
> + add r7, sp, #S_PC
> + ldr r6, [r7, #4] @load spsr into r6
> + msr cpsr, r6 @use und spsr to overwrite svc cpsr
> + ldmdb r7, {r1 - r2} @load sp lr into r1 r2
> + mov lr, r2
> + ldmia sp, {r0 - r12}
> + mov r0, r0
> + add sp, sp, #S_FRAME_SIZE
> + ldr pc, [sp, #-12]
> + .endm
> +
> /*
> * exception handlers
> */
>
> .align 5
> undefined_instruction:
> +#if defined(CONFIG_CMD_KGDB)
> + get_und_stack
> + und_save_regs
> + bl do_undefined_instruction
> + und_restore_regs
> +#else
> get_bad_stack
> bad_save_user_regs
> bl do_undefined_instruction
> +#endif
>
> .align 5
> software_interrupt:
> --
> 1.8.4.5
>
Regards,
Simon
More information about the U-Boot
mailing list