* [U-Boot] [RFC PATCH v1 0/2] kgdb:add breakpoint and arm support @ 2014-11-01 5:12 Peng Fan 2014-11-01 5:12 ` [U-Boot] [RFC PATCH v1 1/2] kgdb: add breakpoint support Peng Fan 2014-11-01 5:12 ` [U-Boot] [RFC PATCH v1 2/2] arm:kgdb add KGDB support for arm platform Peng Fan 0 siblings, 2 replies; 7+ messages in thread From: Peng Fan @ 2014-11-01 5:12 UTC (permalink / raw) To: u-boot There were patches about this part before, such as http://lists.denx.de/pipermail/u-boot/2010-April/069592.html https://www.mail-archive.com/u-boot at lists.denx.de/msg13464.html And I reference that piece of code. I have verified this patch set on freescale mx6sxsabresd revb board. Because the board related kgdb serial code is not fine for patch review, I just sent out the common code for review. I am not sure whether this patch set will break other platform's kgdb support or not, so request for comment. Peng Fan (2): kgdb: add breakpoint support arm:kgdb add KGDB support for arm platform 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 ++++ common/kgdb.c | 273 ++++++++++++++++++++++++++++++++ include/kgdb.h | 35 ++++ 10 files changed, 620 insertions(+), 1 deletion(-) create mode 100644 arch/arm/include/asm/signal.h create mode 100644 arch/arm/lib/kgdb/kgdb.c create mode 100644 arch/arm/lib/kgdb/setjmp.S -- 1.8.4.5 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [RFC PATCH v1 1/2] kgdb: add breakpoint support 2014-11-01 5:12 [U-Boot] [RFC PATCH v1 0/2] kgdb:add breakpoint and arm support Peng Fan @ 2014-11-01 5:12 ` Peng Fan 2014-11-04 6:46 ` Simon Glass 2014-11-01 5:12 ` [U-Boot] [RFC PATCH v1 2/2] arm:kgdb add KGDB support for arm platform Peng Fan 1 sibling, 1 reply; 7+ messages in thread From: Peng Fan @ 2014-11-01 5:12 UTC (permalink / raw) To: u-boot Add Z/z protocal support for breakpoint set/remove. Signed-off-by: Peng Fan <Peng.Fan@freescale.com> --- common/kgdb.c | 273 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ include/kgdb.h | 35 ++++++++ 2 files changed, 308 insertions(+) diff --git a/common/kgdb.c b/common/kgdb.c index d357463..fd83ccd 100644 --- a/common/kgdb.c +++ b/common/kgdb.c @@ -92,6 +92,8 @@ #include <kgdb.h> #include <command.h> +#include <asm-generic/errno.h> + #undef KGDB_DEBUG /* @@ -111,6 +113,17 @@ static int longjmp_on_fault = 0; static int kdebug = 1; #endif +struct kgdb_bkpt kgdb_break[KGDB_MAX_BREAKPOINTS] = { + [0 ... KGDB_MAX_BREAKPOINTS-1] = { .state = BP_UNDEFINED } +}; + +#ifdef CONFIG_ARM +unsigned char gdb_bpt_instr[4] = {0xfe, 0xde, 0xff, 0xe7}; +#else +#error "Please implement gdb_bpt_instr!" +#endif + + static const char hexchars[]="0123456789abcdef"; /* Convert ch from a hex digit to an int */ @@ -309,6 +322,200 @@ putpacket(unsigned char *buffer) } while ((recv & 0x7f) != '+'); } +int kgdb_validate_break_address(unsigned addr) +{ + /* Need More */ + return 0; +} + +static int probe_kernel_read(unsigned char *dst, void *src, size_t size) +{ + int i; + unsigned char *dst_ptr = dst; + unsigned char *src_ptr = src; + + for (i = 0; i < size; i++) + *dst_ptr++ = *src_ptr++; + + return 0; +} + +static int probe_kernel_write(char *dst, void *src, size_t size) +{ + int i; + char *dst_ptr = dst; + char *src_ptr = src; + + for (i = 0; i < size; i++) + *dst_ptr++ = *src_ptr++; + + return 0; +} + +__weak int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt) +{ + int err; + + err = probe_kernel_read(bpt->saved_instr, (char *)bpt->bpt_addr, + BREAK_INSTR_SIZE); + if (err) + return err; + + err = probe_kernel_write((char *)bpt->bpt_addr, gdb_bpt_instr, + BREAK_INSTR_SIZE); + + return err; +} + +/* + * Set the breakpoints whose state is BP_SET to BP_ACTIVE + */ +int kgdb_active_sw_breakpoints(void) +{ + int err; + int ret = 0; + int i; + + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { + if (kgdb_break[i].state != BP_SET) + continue; + + err = kgdb_arch_set_breakpoint(&kgdb_break[i]); + if (err) { + ret = err; + printf("KGDB: BP install failed: %lx\n", + kgdb_break[i].bpt_addr); + continue; + } + + kgdb_break[i].state = BP_ACTIVE; + + /* + * kgdb_arch_set_breakpoint touched dcache and memory. + * cache should be flushed to let icache can see the updated + * inst. + */ + /* flush work is done in do_exit */ + kgdb_flush_cache_all(); + } + + return ret; +} + +/* + * Set state from BP_SET to BP_REMOVED + */ +int kgdb_remove_sw_breakpoint(unsigned int addr) +{ + int i; + + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { + if ((kgdb_break[i].state == BP_SET) && + (kgdb_break[i].bpt_addr == addr)) { + kgdb_break[i].state = BP_REMOVED; + return 0; + } + } + + return -ENOENT; +} + +int kgdb_set_sw_breakpoint(unsigned int addr) +{ + int err = kgdb_validate_break_address(addr); + int breakno = -1; + int i; + + if (err) + return err; + + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { + if ((kgdb_break[i].state == BP_SET) && + (kgdb_break[i].bpt_addr == addr)) + return -EEXIST; + } + + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { + /* removed or unused, use it */ + if ((kgdb_break[i].state == BP_REMOVED) || + (kgdb_break[i].state == BP_UNDEFINED)) { + breakno = i; + break; + } + } + + if (breakno == -1) + return -E2BIG; + + kgdb_break[breakno].state = BP_SET; + kgdb_break[breakno].type = BP_BREAKPOINT; + kgdb_break[breakno].bpt_addr = addr; + + return 0; +} + +__weak int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt) +{ + return probe_kernel_write((char *)bpt->bpt_addr, + (char *)bpt->saved_instr, BREAK_INSTR_SIZE); +} + +/* + * set breakpoints whose state is BP_ACTIVE to BP_SET + */ +int kgdb_deactivate_sw_breakpoints(void) +{ + int err; + int ret = 0; + int i; + + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { + if (kgdb_break[i].state != BP_ACTIVE) + continue; + + err = kgdb_arch_remove_breakpoint(&kgdb_break[i]); + if (err) { + printf("KGDB: BP remove failed: %lx\n", + kgdb_break[i].bpt_addr); + ret = err; + } + + kgdb_break[i].state = BP_SET; + + /* + * kgdb_arch_remove_breakpoint touched dcache and memory. + * cache should be flushed to let icache can see the updated + * inst. + */ + kgdb_flush_cache_all(); + } + + return ret; +} + +static int kgdb_remove_all_break(void) +{ + int err; + int i; + + /* Clear memory breakpoints. */ + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { + if (kgdb_break[i].state != BP_ACTIVE) + goto setundefined; + err = kgdb_arch_remove_breakpoint(&kgdb_break[i]); + if (err) + printf("KGDB: breakpoint remove failed: %lx\n", + kgdb_break[i].bpt_addr); +setundefined: + kgdb_break[i].state = BP_UNDEFINED; + } + + /* clear hardware breakpoints. */ + /* ToDo in future. */ + + return 0; +} + /* * This function does all command processing for interfacing to gdb. */ @@ -318,6 +525,7 @@ handle_exception (struct pt_regs *regs) int addr; int length; char *ptr; + char *bpt_type; kgdb_data kd; int i; @@ -376,6 +584,12 @@ handle_exception (struct pt_regs *regs) putpacket((unsigned char *)&remcomOutBuffer); + /* + * Each time trigger a kgdb break, first deactive all active + * breakpoints. + */ + kgdb_deactivate_sw_breakpoints(); + while (1) { volatile int errnum; @@ -394,6 +608,8 @@ handle_exception (struct pt_regs *regs) if (errnum == 0) switch (remcomInBuffer[0]) { case '?': /* report most recent signal */ + kgdb_remove_all_break(); + remcomOutBuffer[0] = 'S'; remcomOutBuffer[1] = hexchars[kd.sigval >> 4]; remcomOutBuffer[2] = hexchars[kd.sigval & 0xf]; @@ -465,6 +681,8 @@ handle_exception (struct pt_regs *regs) kd.extype |= KGDBEXIT_WITHADDR; } + kgdb_active_sw_breakpoints(); + goto doexit; case 'S': /* SSS single step with signal SS */ @@ -479,6 +697,8 @@ handle_exception (struct pt_regs *regs) kd.extype |= KGDBEXIT_WITHADDR; } + kgdb_active_sw_breakpoints(); + doexit: /* Need to flush the instruction cache here, as we may have deposited a * breakpoint, and the icache probably has no way of knowing that a data ref to @@ -506,6 +726,59 @@ handle_exception (struct pt_regs *regs) kgdb_error(KGDBERR_BADPARAMS); } break; + case 'z': + /* + * Break point remove + * packet: zt,addr,length + */ + case 'Z': + /* + * Break point set + * packet: Zt,addr,length + * + * t is type: `0' - software breakpoint, + * `1' - hardware breakpoint, `2' - write watchpoint, + * `3' - read watchpoint, `4' - access watchpoint; + * addr is address; length is in bytes. For a software + * breakpoint, length specifies the size of the + * instruction to be patched. For hardware breakpoints + * and watchpoints length specifies the memory region + * to be monitored. To avoid potential problems with + * duplicate packets, the operations should be + * implemented in an idempotent way. + */ + bpt_type = &remcomInBuffer[1]; + ptr = &remcomInBuffer[2]; + + if (*(ptr++) != ',') { + errnum = -EINVAL; + break; + } + + if (!hexToInt(&ptr, &addr)) { + errnum = -EINVAL; + break; + } + + if (*ptr++ != ',') { + errnum = -EINVAL; + break; + } + + /* only software breakpoint is implemented */ + if ((remcomInBuffer[0] == 'Z') && (*bpt_type == '0')) { + errnum = kgdb_set_sw_breakpoint(addr); + } else if ((remcomInBuffer[0] == 'z') && + (*bpt_type == '0')) { + errnum = kgdb_remove_sw_breakpoint(addr); + } else { + /* Unsupported */ + errnum = -EINVAL; + } + + if (errnum == 0) + strcpy(remcomOutBuffer, "OK"); + break; } /* switch */ if (errnum != 0) diff --git a/include/kgdb.h b/include/kgdb.h index b6ba742..f9152b5 100644 --- a/include/kgdb.h +++ b/include/kgdb.h @@ -20,6 +20,12 @@ #define KGDBEXIT_WITHADDR 0x100 +#if defined(CONFIG_ARM) +#define BREAK_INSTR_SIZE 4 +#else +#error "BREAK_INSTR_SIZE not set" +#endif + typedef struct { int num; @@ -38,6 +44,29 @@ typedef } kgdb_data; +enum kgdb_bptype { + BP_BREAKPOINT = 0, + BP_HARDWARE_BREAKPOINT, + BP_WRITE_WATCHPOINT, + BP_READ_WATCHPOINT, + BP_ACCESS_WATCHPOINT, + BP_POKE_BREAKPOINT, +}; + +enum kgdb_bpstate { + BP_UNDEFINED = 0, + BP_REMOVED, + BP_SET, + BP_ACTIVE +}; + +struct kgdb_bkpt { + unsigned long bpt_addr; + unsigned char saved_instr[BREAK_INSTR_SIZE]; + enum kgdb_bptype type; + enum kgdb_bpstate state; +}; + /* these functions are provided by the generic kgdb support */ extern void kgdb_init(void); extern void kgdb_error(int); @@ -67,4 +96,10 @@ extern void kgdb_interruptible(int); /* this is referenced in the trap handler for the platform */ extern int (*debugger_exception_handler)(struct pt_regs *); +int kgdb_set_sw_break(unsigned int addr); +int kgdb_remove_sw_break(unsigned int addr); +int kgdb_validate_break_address(unsigned int addr); + +#define KGDB_MAX_BREAKPOINTS 32 + #endif /* __KGDB_H__ */ -- 1.8.4.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [U-Boot] [RFC PATCH v1 1/2] kgdb: add breakpoint support 2014-11-01 5:12 ` [U-Boot] [RFC PATCH v1 1/2] kgdb: add breakpoint support Peng Fan @ 2014-11-04 6:46 ` Simon Glass 2014-11-04 12:23 ` Peng Fan 0 siblings, 1 reply; 7+ messages in thread From: Simon Glass @ 2014-11-04 6:46 UTC (permalink / raw) To: u-boot Hi Peng, On 31 October 2014 23:12, Peng Fan <Peng.Fan@freescale.com> wrote: > Add Z/z protocal support for breakpoint set/remove. > > Signed-off-by: Peng Fan <Peng.Fan@freescale.com> This looks good to me - I just have a few bits below. > --- > common/kgdb.c | 273 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > include/kgdb.h | 35 ++++++++ > 2 files changed, 308 insertions(+) > > diff --git a/common/kgdb.c b/common/kgdb.c > index d357463..fd83ccd 100644 > --- a/common/kgdb.c > +++ b/common/kgdb.c > @@ -92,6 +92,8 @@ > #include <kgdb.h> > #include <command.h> > > +#include <asm-generic/errno.h> #include <errno.h> would do > + > #undef KGDB_DEBUG Where is this used? > > /* > @@ -111,6 +113,17 @@ static int longjmp_on_fault = 0; > static int kdebug = 1; > #endif > > +struct kgdb_bkpt kgdb_break[KGDB_MAX_BREAKPOINTS] = { > + [0 ... KGDB_MAX_BREAKPOINTS-1] = { .state = BP_UNDEFINED } > +}; > + > +#ifdef CONFIG_ARM > +unsigned char gdb_bpt_instr[4] = {0xfe, 0xde, 0xff, 0xe7}; > +#else > +#error "Please implement gdb_bpt_instr!" > +#endif > + > + > static const char hexchars[]="0123456789abcdef"; > > /* Convert ch from a hex digit to an int */ > @@ -309,6 +322,200 @@ putpacket(unsigned char *buffer) > } while ((recv & 0x7f) != '+'); > } > > +int kgdb_validate_break_address(unsigned addr) > +{ > + /* Need More */ ? > + return 0; > +} > + > +static int probe_kernel_read(unsigned char *dst, void *src, size_t size) > +{ > + int i; > + unsigned char *dst_ptr = dst; > + unsigned char *src_ptr = src; > + > + for (i = 0; i < size; i++) > + *dst_ptr++ = *src_ptr++; > + > + return 0; > +} > + > +static int probe_kernel_write(char *dst, void *src, size_t size) These two above are strange function names - why 'kernel' - what does it mean in this context? Also could you must use memcpy(), either in the functions or instead of them? > +{ > + int i; > + char *dst_ptr = dst; > + char *src_ptr = src; > + > + for (i = 0; i < size; i++) > + *dst_ptr++ = *src_ptr++; > + > + return 0; > +} > + > +__weak int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt) > +{ > + int err; > + > + err = probe_kernel_read(bpt->saved_instr, (char *)bpt->bpt_addr, > + BREAK_INSTR_SIZE); > + if (err) > + return err; > + > + err = probe_kernel_write((char *)bpt->bpt_addr, gdb_bpt_instr, > + BREAK_INSTR_SIZE); > + > + return err; > +} > + > +/* > + * Set the breakpoints whose state is BP_SET to BP_ACTIVE > + */ > +int kgdb_active_sw_breakpoints(void) > +{ > + int err; > + int ret = 0; > + int i; > + > + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { > + if (kgdb_break[i].state != BP_SET) > + continue; > + > + err = kgdb_arch_set_breakpoint(&kgdb_break[i]); > + if (err) { > + ret = err; > + printf("KGDB: BP install failed: %lx\n", > + kgdb_break[i].bpt_addr); > + continue; > + } > + > + kgdb_break[i].state = BP_ACTIVE; > + > + /* > + * kgdb_arch_set_breakpoint touched dcache and memory. > + * cache should be flushed to let icache can see the updated > + * inst. instruction > + */ > + /* flush work is done in do_exit */ > + kgdb_flush_cache_all(); > + } > + > + return ret; > +} > + > +/* > + * Set state from BP_SET to BP_REMOVED > + */ > +int kgdb_remove_sw_breakpoint(unsigned int addr) > +{ > + int i; > + > + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { > + if ((kgdb_break[i].state == BP_SET) && > + (kgdb_break[i].bpt_addr == addr)) { > + kgdb_break[i].state = BP_REMOVED; > + return 0; > + } > + } > + > + return -ENOENT; > +} > + > +int kgdb_set_sw_breakpoint(unsigned int addr) > +{ > + int err = kgdb_validate_break_address(addr); > + int breakno = -1; > + int i; > + > + if (err) > + return err; > + > + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { > + if ((kgdb_break[i].state == BP_SET) && > + (kgdb_break[i].bpt_addr == addr)) > + return -EEXIST; > + } > + > + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { > + /* removed or unused, use it */ > + if ((kgdb_break[i].state == BP_REMOVED) || > + (kgdb_break[i].state == BP_UNDEFINED)) { > + breakno = i; > + break; > + } > + } > + > + if (breakno == -1) > + return -E2BIG; > + > + kgdb_break[breakno].state = BP_SET; > + kgdb_break[breakno].type = BP_BREAKPOINT; > + kgdb_break[breakno].bpt_addr = addr; > + > + return 0; > +} > + > +__weak int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt) > +{ > + return probe_kernel_write((char *)bpt->bpt_addr, > + (char *)bpt->saved_instr, BREAK_INSTR_SIZE); > +} > + > +/* > + * set breakpoints whose state is BP_ACTIVE to BP_SET > + */ > +int kgdb_deactivate_sw_breakpoints(void) > +{ > + int err; > + int ret = 0; > + int i; > + > + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { > + if (kgdb_break[i].state != BP_ACTIVE) > + continue; > + > + err = kgdb_arch_remove_breakpoint(&kgdb_break[i]); > + if (err) { > + printf("KGDB: BP remove failed: %lx\n", > + kgdb_break[i].bpt_addr); > + ret = err; > + } > + > + kgdb_break[i].state = BP_SET; > + > + /* > + * kgdb_arch_remove_breakpoint touched dcache and memory. > + * cache should be flushed to let icache can see the updated > + * inst. > + */ > + kgdb_flush_cache_all(); > + } > + > + return ret; > +} > + > +static int kgdb_remove_all_break(void) kgdb_remove_all_breakpoints() to be consistent? > +{ > + int err; > + int i; > + > + /* Clear memory breakpoints. */ > + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { > + if (kgdb_break[i].state != BP_ACTIVE) > + goto setundefined; > + err = kgdb_arch_remove_breakpoint(&kgdb_break[i]); > + if (err) > + printf("KGDB: breakpoint remove failed: %lx\n", > + kgdb_break[i].bpt_addr); > +setundefined: > + kgdb_break[i].state = BP_UNDEFINED; > + } > + > + /* clear hardware breakpoints. */ > + /* ToDo in future. */ /* TODO: clear hardware breakpoints. */ > + > + return 0; > +} > + > /* > * This function does all command processing for interfacing to gdb. > */ > @@ -318,6 +525,7 @@ handle_exception (struct pt_regs *regs) > int addr; > int length; > char *ptr; > + char *bpt_type; > kgdb_data kd; > int i; > > @@ -376,6 +584,12 @@ handle_exception (struct pt_regs *regs) > > putpacket((unsigned char *)&remcomOutBuffer); > > + /* > + * Each time trigger a kgdb break, first deactive all active > + * breakpoints. > + */ > + kgdb_deactivate_sw_breakpoints(); > + > while (1) { > volatile int errnum; > > @@ -394,6 +608,8 @@ handle_exception (struct pt_regs *regs) > if (errnum == 0) switch (remcomInBuffer[0]) { > > case '?': /* report most recent signal */ > + kgdb_remove_all_break(); > + > remcomOutBuffer[0] = 'S'; > remcomOutBuffer[1] = hexchars[kd.sigval >> 4]; > remcomOutBuffer[2] = hexchars[kd.sigval & 0xf]; > @@ -465,6 +681,8 @@ handle_exception (struct pt_regs *regs) > kd.extype |= KGDBEXIT_WITHADDR; > } > > + kgdb_active_sw_breakpoints(); > + > goto doexit; > > case 'S': /* SSS single step with signal SS */ > @@ -479,6 +697,8 @@ handle_exception (struct pt_regs *regs) > kd.extype |= KGDBEXIT_WITHADDR; > } > > + kgdb_active_sw_breakpoints(); > + > doexit: > /* Need to flush the instruction cache here, as we may have deposited a > * breakpoint, and the icache probably has no way of knowing that a data ref to > @@ -506,6 +726,59 @@ handle_exception (struct pt_regs *regs) > kgdb_error(KGDBERR_BADPARAMS); > } > break; > + case 'z': > + /* > + * Break point remove > + * packet: zt,addr,length > + */ > + case 'Z': > + /* > + * Break point set > + * packet: Zt,addr,length > + * > + * t is type: `0' - software breakpoint, > + * `1' - hardware breakpoint, `2' - write watchpoint, > + * `3' - read watchpoint, `4' - access watchpoint; > + * addr is address; length is in bytes. For a software > + * breakpoint, length specifies the size of the > + * instruction to be patched. For hardware breakpoints > + * and watchpoints length specifies the memory region > + * to be monitored. To avoid potential problems with > + * duplicate packets, the operations should be > + * implemented in an idempotent way. > + */ > + bpt_type = &remcomInBuffer[1]; > + ptr = &remcomInBuffer[2]; > + > + if (*(ptr++) != ',') { > + errnum = -EINVAL; > + break; > + } > + > + if (!hexToInt(&ptr, &addr)) { > + errnum = -EINVAL; > + break; > + } > + > + if (*ptr++ != ',') { > + errnum = -EINVAL; > + break; > + } > + > + /* only software breakpoint is implemented */ > + if ((remcomInBuffer[0] == 'Z') && (*bpt_type == '0')) { > + errnum = kgdb_set_sw_breakpoint(addr); > + } else if ((remcomInBuffer[0] == 'z') && > + (*bpt_type == '0')) { > + errnum = kgdb_remove_sw_breakpoint(addr); > + } else { > + /* Unsupported */ > + errnum = -EINVAL; > + } > + > + if (errnum == 0) > + strcpy(remcomOutBuffer, "OK"); > + break; > } /* switch */ > > if (errnum != 0) > diff --git a/include/kgdb.h b/include/kgdb.h > index b6ba742..f9152b5 100644 > --- a/include/kgdb.h > +++ b/include/kgdb.h > @@ -20,6 +20,12 @@ > > #define KGDBEXIT_WITHADDR 0x100 > > +#if defined(CONFIG_ARM) > +#define BREAK_INSTR_SIZE 4 > +#else > +#error "BREAK_INSTR_SIZE not set" > +#endif > + > typedef > struct { > int num; > @@ -38,6 +44,29 @@ typedef > } > kgdb_data; > > +enum kgdb_bptype { > + BP_BREAKPOINT = 0, > + BP_HARDWARE_BREAKPOINT, > + BP_WRITE_WATCHPOINT, > + BP_READ_WATCHPOINT, > + BP_ACCESS_WATCHPOINT, > + BP_POKE_BREAKPOINT, > +}; > + > +enum kgdb_bpstate { > + BP_UNDEFINED = 0, > + BP_REMOVED, > + BP_SET, > + BP_ACTIVE > +}; > + > +struct kgdb_bkpt { > + unsigned long bpt_addr; > + unsigned char saved_instr[BREAK_INSTR_SIZE]; > + enum kgdb_bptype type; > + enum kgdb_bpstate state; > +}; > + > /* these functions are provided by the generic kgdb support */ > extern void kgdb_init(void); > extern void kgdb_error(int); > @@ -67,4 +96,10 @@ extern void kgdb_interruptible(int); > /* this is referenced in the trap handler for the platform */ > extern int (*debugger_exception_handler)(struct pt_regs *); > > +int kgdb_set_sw_break(unsigned int addr); > +int kgdb_remove_sw_break(unsigned int addr); > +int kgdb_validate_break_address(unsigned int addr); > + > +#define KGDB_MAX_BREAKPOINTS 32 > + > #endif /* __KGDB_H__ */ > -- > 1.8.4.5 > Regards, Simon ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [RFC PATCH v1 1/2] kgdb: add breakpoint support 2014-11-04 6:46 ` Simon Glass @ 2014-11-04 12:23 ` Peng Fan 0 siblings, 0 replies; 7+ messages in thread From: Peng Fan @ 2014-11-04 12:23 UTC (permalink / raw) To: u-boot Hi Simon, ? 11/4/2014 2:46 PM, Simon Glass ??: > Hi Peng, > > On 31 October 2014 23:12, Peng Fan <Peng.Fan@freescale.com> wrote: >> Add Z/z protocal support for breakpoint set/remove. >> >> Signed-off-by: Peng Fan <Peng.Fan@freescale.com> > > This looks good to me - I just have a few bits below. > >> --- >> common/kgdb.c | 273 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> include/kgdb.h | 35 ++++++++ >> 2 files changed, 308 insertions(+) >> >> diff --git a/common/kgdb.c b/common/kgdb.c >> index d357463..fd83ccd 100644 >> --- a/common/kgdb.c >> +++ b/common/kgdb.c >> @@ -92,6 +92,8 @@ >> #include <kgdb.h> >> #include <command.h> >> >> +#include <asm-generic/errno.h> > > #include <errno.h> would do Ok. > >> + >> #undef KGDB_DEBUG > > Where is this used? > You mean KGDB_DEBUG? >> >> /* >> @@ -111,6 +113,17 @@ static int longjmp_on_fault = 0; >> static int kdebug = 1; >> #endif >> >> +struct kgdb_bkpt kgdb_break[KGDB_MAX_BREAKPOINTS] = { >> + [0 ... KGDB_MAX_BREAKPOINTS-1] = { .state = BP_UNDEFINED } >> +}; >> + >> +#ifdef CONFIG_ARM >> +unsigned char gdb_bpt_instr[4] = {0xfe, 0xde, 0xff, 0xe7}; >> +#else >> +#error "Please implement gdb_bpt_instr!" >> +#endif >> + >> + >> static const char hexchars[]="0123456789abcdef"; >> >> /* Convert ch from a hex digit to an int */ >> @@ -309,6 +322,200 @@ putpacket(unsigned char *buffer) >> } while ((recv & 0x7f) != '+'); >> } >> >> +int kgdb_validate_break_address(unsigned addr) >> +{ >> + /* Need More */ > > ? I'll remove the comment. Actually i just want to validate whether the add parameter is fine or not using this function. > >> + return 0; >> +} >> + >> +static int probe_kernel_read(unsigned char *dst, void *src, size_t size) >> +{ >> + int i; >> + unsigned char *dst_ptr = dst; >> + unsigned char *src_ptr = src; >> + >> + for (i = 0; i < size; i++) >> + *dst_ptr++ = *src_ptr++; >> + >> + return 0; >> +} >> + >> +static int probe_kernel_write(char *dst, void *src, size_t size) > > These two above are strange function names - why 'kernel' - what does > it mean in this context? Also could you must use memcpy(), either in > the functions or instead of them? Ok. memcpy is better. I just use the function name in linux kernel, maybe misleading here. how about probe_mem_read? > >> +{ >> + int i; >> + char *dst_ptr = dst; >> + char *src_ptr = src; >> + >> + for (i = 0; i < size; i++) >> + *dst_ptr++ = *src_ptr++; >> + >> + return 0; >> +} >> + >> +__weak int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt) >> +{ >> + int err; >> + >> + err = probe_kernel_read(bpt->saved_instr, (char *)bpt->bpt_addr, >> + BREAK_INSTR_SIZE); >> + if (err) >> + return err; >> + >> + err = probe_kernel_write((char *)bpt->bpt_addr, gdb_bpt_instr, >> + BREAK_INSTR_SIZE); >> + >> + return err; >> +} >> + >> +/* >> + * Set the breakpoints whose state is BP_SET to BP_ACTIVE >> + */ >> +int kgdb_active_sw_breakpoints(void) >> +{ >> + int err; >> + int ret = 0; >> + int i; >> + >> + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { >> + if (kgdb_break[i].state != BP_SET) >> + continue; >> + >> + err = kgdb_arch_set_breakpoint(&kgdb_break[i]); >> + if (err) { >> + ret = err; >> + printf("KGDB: BP install failed: %lx\n", >> + kgdb_break[i].bpt_addr); >> + continue; >> + } >> + >> + kgdb_break[i].state = BP_ACTIVE; >> + >> + /* >> + * kgdb_arch_set_breakpoint touched dcache and memory. >> + * cache should be flushed to let icache can see the updated >> + * inst. > > instruction > >> + */ >> + /* flush work is done in do_exit */ >> + kgdb_flush_cache_all(); >> + } >> + >> + return ret; >> +} >> + >> +/* >> + * Set state from BP_SET to BP_REMOVED >> + */ >> +int kgdb_remove_sw_breakpoint(unsigned int addr) >> +{ >> + int i; >> + >> + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { >> + if ((kgdb_break[i].state == BP_SET) && >> + (kgdb_break[i].bpt_addr == addr)) { >> + kgdb_break[i].state = BP_REMOVED; >> + return 0; >> + } >> + } >> + >> + return -ENOENT; >> +} >> + >> +int kgdb_set_sw_breakpoint(unsigned int addr) >> +{ >> + int err = kgdb_validate_break_address(addr); >> + int breakno = -1; >> + int i; >> + >> + if (err) >> + return err; >> + >> + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { >> + if ((kgdb_break[i].state == BP_SET) && >> + (kgdb_break[i].bpt_addr == addr)) >> + return -EEXIST; >> + } >> + >> + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { >> + /* removed or unused, use it */ >> + if ((kgdb_break[i].state == BP_REMOVED) || >> + (kgdb_break[i].state == BP_UNDEFINED)) { >> + breakno = i; >> + break; >> + } >> + } >> + >> + if (breakno == -1) >> + return -E2BIG; >> + >> + kgdb_break[breakno].state = BP_SET; >> + kgdb_break[breakno].type = BP_BREAKPOINT; >> + kgdb_break[breakno].bpt_addr = addr; >> + >> + return 0; >> +} >> + >> +__weak int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt) >> +{ >> + return probe_kernel_write((char *)bpt->bpt_addr, >> + (char *)bpt->saved_instr, BREAK_INSTR_SIZE); >> +} >> + >> +/* >> + * set breakpoints whose state is BP_ACTIVE to BP_SET >> + */ >> +int kgdb_deactivate_sw_breakpoints(void) >> +{ >> + int err; >> + int ret = 0; >> + int i; >> + >> + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { >> + if (kgdb_break[i].state != BP_ACTIVE) >> + continue; >> + >> + err = kgdb_arch_remove_breakpoint(&kgdb_break[i]); >> + if (err) { >> + printf("KGDB: BP remove failed: %lx\n", >> + kgdb_break[i].bpt_addr); >> + ret = err; >> + } >> + >> + kgdb_break[i].state = BP_SET; >> + >> + /* >> + * kgdb_arch_remove_breakpoint touched dcache and memory. >> + * cache should be flushed to let icache can see the updated >> + * inst. >> + */ >> + kgdb_flush_cache_all(); >> + } >> + >> + return ret; >> +} >> + >> +static int kgdb_remove_all_break(void) > > kgdb_remove_all_breakpoints() to be consistent? kgdb_remove_all_breakpoints is better. > >> +{ >> + int err; >> + int i; >> + >> + /* Clear memory breakpoints. */ >> + for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) { >> + if (kgdb_break[i].state != BP_ACTIVE) >> + goto setundefined; >> + err = kgdb_arch_remove_breakpoint(&kgdb_break[i]); >> + if (err) >> + printf("KGDB: breakpoint remove failed: %lx\n", >> + kgdb_break[i].bpt_addr); >> +setundefined: >> + kgdb_break[i].state = BP_UNDEFINED; >> + } >> + >> + /* clear hardware breakpoints. */ >> + /* ToDo in future. */ > > /* TODO: clear hardware breakpoints. */ Ok. > >> + >> + return 0; >> +} >> + >> /* >> * This function does all command processing for interfacing to gdb. >> */ >> @@ -318,6 +525,7 @@ handle_exception (struct pt_regs *regs) >> int addr; >> int length; >> char *ptr; >> + char *bpt_type; >> kgdb_data kd; >> int i; >> >> @@ -376,6 +584,12 @@ handle_exception (struct pt_regs *regs) >> >> putpacket((unsigned char *)&remcomOutBuffer); >> >> + /* >> + * Each time trigger a kgdb break, first deactive all active >> + * breakpoints. >> + */ >> + kgdb_deactivate_sw_breakpoints(); >> + >> while (1) { >> volatile int errnum; >> >> @@ -394,6 +608,8 @@ handle_exception (struct pt_regs *regs) >> if (errnum == 0) switch (remcomInBuffer[0]) { >> >> case '?': /* report most recent signal */ >> + kgdb_remove_all_break(); >> + >> remcomOutBuffer[0] = 'S'; >> remcomOutBuffer[1] = hexchars[kd.sigval >> 4]; >> remcomOutBuffer[2] = hexchars[kd.sigval & 0xf]; >> @@ -465,6 +681,8 @@ handle_exception (struct pt_regs *regs) >> kd.extype |= KGDBEXIT_WITHADDR; >> } >> >> + kgdb_active_sw_breakpoints(); >> + >> goto doexit; >> >> case 'S': /* SSS single step with signal SS */ >> @@ -479,6 +697,8 @@ handle_exception (struct pt_regs *regs) >> kd.extype |= KGDBEXIT_WITHADDR; >> } >> >> + kgdb_active_sw_breakpoints(); >> + >> doexit: >> /* Need to flush the instruction cache here, as we may have deposited a >> * breakpoint, and the icache probably has no way of knowing that a data ref to >> @@ -506,6 +726,59 @@ handle_exception (struct pt_regs *regs) >> kgdb_error(KGDBERR_BADPARAMS); >> } >> break; >> + case 'z': >> + /* >> + * Break point remove >> + * packet: zt,addr,length >> + */ >> + case 'Z': >> + /* >> + * Break point set >> + * packet: Zt,addr,length >> + * >> + * t is type: `0' - software breakpoint, >> + * `1' - hardware breakpoint, `2' - write watchpoint, >> + * `3' - read watchpoint, `4' - access watchpoint; >> + * addr is address; length is in bytes. For a software >> + * breakpoint, length specifies the size of the >> + * instruction to be patched. For hardware breakpoints >> + * and watchpoints length specifies the memory region >> + * to be monitored. To avoid potential problems with >> + * duplicate packets, the operations should be >> + * implemented in an idempotent way. >> + */ >> + bpt_type = &remcomInBuffer[1]; >> + ptr = &remcomInBuffer[2]; >> + >> + if (*(ptr++) != ',') { >> + errnum = -EINVAL; >> + break; >> + } >> + >> + if (!hexToInt(&ptr, &addr)) { >> + errnum = -EINVAL; >> + break; >> + } >> + >> + if (*ptr++ != ',') { >> + errnum = -EINVAL; >> + break; >> + } >> + >> + /* only software breakpoint is implemented */ >> + if ((remcomInBuffer[0] == 'Z') && (*bpt_type == '0')) { >> + errnum = kgdb_set_sw_breakpoint(addr); >> + } else if ((remcomInBuffer[0] == 'z') && >> + (*bpt_type == '0')) { >> + errnum = kgdb_remove_sw_breakpoint(addr); >> + } else { >> + /* Unsupported */ >> + errnum = -EINVAL; >> + } >> + >> + if (errnum == 0) >> + strcpy(remcomOutBuffer, "OK"); >> + break; >> } /* switch */ >> >> if (errnum != 0) >> diff --git a/include/kgdb.h b/include/kgdb.h >> index b6ba742..f9152b5 100644 >> --- a/include/kgdb.h >> +++ b/include/kgdb.h >> @@ -20,6 +20,12 @@ >> >> #define KGDBEXIT_WITHADDR 0x100 >> >> +#if defined(CONFIG_ARM) >> +#define BREAK_INSTR_SIZE 4 >> +#else >> +#error "BREAK_INSTR_SIZE not set" >> +#endif >> + >> typedef >> struct { >> int num; >> @@ -38,6 +44,29 @@ typedef >> } >> kgdb_data; >> >> +enum kgdb_bptype { >> + BP_BREAKPOINT = 0, >> + BP_HARDWARE_BREAKPOINT, >> + BP_WRITE_WATCHPOINT, >> + BP_READ_WATCHPOINT, >> + BP_ACCESS_WATCHPOINT, >> + BP_POKE_BREAKPOINT, >> +}; >> + >> +enum kgdb_bpstate { >> + BP_UNDEFINED = 0, >> + BP_REMOVED, >> + BP_SET, >> + BP_ACTIVE >> +}; >> + >> +struct kgdb_bkpt { >> + unsigned long bpt_addr; >> + unsigned char saved_instr[BREAK_INSTR_SIZE]; >> + enum kgdb_bptype type; >> + enum kgdb_bpstate state; >> +}; >> + >> /* these functions are provided by the generic kgdb support */ >> extern void kgdb_init(void); >> extern void kgdb_error(int); >> @@ -67,4 +96,10 @@ extern void kgdb_interruptible(int); >> /* this is referenced in the trap handler for the platform */ >> extern int (*debugger_exception_handler)(struct pt_regs *); >> >> +int kgdb_set_sw_break(unsigned int addr); >> +int kgdb_remove_sw_break(unsigned int addr); >> +int kgdb_validate_break_address(unsigned int addr); >> + >> +#define KGDB_MAX_BREAKPOINTS 32 >> + >> #endif /* __KGDB_H__ */ >> -- >> 1.8.4.5 >> > > Regards, > Simon > Thanks for reviewing. Regards, Peng. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [RFC PATCH v1 2/2] arm:kgdb add KGDB support for arm platform 2014-11-01 5:12 [U-Boot] [RFC PATCH v1 0/2] kgdb:add breakpoint and arm support Peng Fan 2014-11-01 5:12 ` [U-Boot] [RFC PATCH v1 1/2] kgdb: add breakpoint support Peng Fan @ 2014-11-01 5:12 ` Peng Fan 2014-11-04 6:58 ` Simon Glass 1 sibling, 1 reply; 7+ messages in thread From: Peng Fan @ 2014-11-01 5:12 UTC (permalink / raw) To: u-boot 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@freescale.com> --- 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, + 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 ??? */ + + 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 ??? */ + 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 */ + 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) + 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; + 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) + 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]; +} + +void kgdb_flush_cache_all(void) +{ + invalidate_icache_all(); + flush_dcache_all(); +} + +/* + * 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} + 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [U-Boot] [RFC PATCH v1 2/2] arm:kgdb add KGDB support for arm platform 2014-11-01 5:12 ` [U-Boot] [RFC PATCH v1 2/2] arm:kgdb add KGDB support for arm platform Peng Fan @ 2014-11-04 6:58 ` Simon Glass 2014-11-04 13:09 ` Peng Fan 0 siblings, 1 reply; 7+ messages in thread From: Simon Glass @ 2014-11-04 6:58 UTC (permalink / raw) To: u-boot HI Peng, On 31 October 2014 23:12, Peng Fan <Peng.Fan@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@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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [RFC PATCH v1 2/2] arm:kgdb add KGDB support for arm platform 2014-11-04 6:58 ` Simon Glass @ 2014-11-04 13:09 ` Peng Fan 0 siblings, 0 replies; 7+ messages in thread From: Peng Fan @ 2014-11-04 13:09 UTC (permalink / raw) To: u-boot Hi Simon, ? 11/4/2014 2:58 PM, Simon Glass ??: > HI Peng, > > On 31 October 2014 23:12, Peng Fan <Peng.Fan@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@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. 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-11-04 13:09 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-01 5:12 [U-Boot] [RFC PATCH v1 0/2] kgdb:add breakpoint and arm support Peng Fan 2014-11-01 5:12 ` [U-Boot] [RFC PATCH v1 1/2] kgdb: add breakpoint support Peng Fan 2014-11-04 6:46 ` Simon Glass 2014-11-04 12:23 ` Peng Fan 2014-11-01 5:12 ` [U-Boot] [RFC PATCH v1 2/2] arm:kgdb add KGDB support for arm platform Peng Fan 2014-11-04 6:58 ` Simon Glass 2014-11-04 13:09 ` Peng Fan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox