From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peng Fan Date: Tue, 4 Nov 2014 21:09:21 +0800 Subject: [U-Boot] [RFC PATCH v1 2/2] arm:kgdb add KGDB support for arm platform In-Reply-To: References: <1414818776-4074-1-git-send-email-Peng.Fan@freescale.com> <1414818776-4074-3-git-send-email-Peng.Fan@freescale.com> Message-ID: <5458D001.4040307@freescale.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon, ? 11/4/2014 2:58 PM, Simon Glass ??: > HI Peng, > > On 31 October 2014 23:12, Peng Fan 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 > > 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 >> 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 >> #include >> #include >> +#include >> >> 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 >> +#include >> +#include >> +#include >> +#include >> + >> +#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. I did not test FP and other extra regs. Actually in uboot, the FP and extra regs are used? I am not sure. > > Also these mirror the numbers in ptrace.h so I wonder if somehow they > could be linked? hmm, I'll try this. > >> + 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? Missed to remove this. This is marked when I beginning wrote this piece of code. >> + >> + 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? I'll remove 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 Ok. > >> + 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? Yeah. > >> + 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 Hah. regs->uregs[regno] = *ptr is better. Thanks. > >> + 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? you are right, should be 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. I'll try to use for loop or memcpy. > >> +} >> + >> +void kgdb_flush_cache_all(void) >> +{ >> + invalidate_icache_all(); >> + flush_dcache_all(); > > Do you need to flush the write buffer here? Actually, the inst 'x' is in DRAM, we modify it to a break instruction. If not flush dcache, the modified instruction may still exists in dcache. Then icache may not see the break instruction. So flush the write buffer. > >> +} >> + >> +/* >> + * 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 Ok. > >> + 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 > Thanks for reviewing. I'll fix and test the patch set and send v2 out for review. Regards, Peng.