HI Peng, On 31 October 2014 23:12, Peng Fan <peng....@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....@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot