[U-Boot] [RFC PATCH v1 0/2] kgdb:add breakpoint and arm support
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@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
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__ */
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
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:
- 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.
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:
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, #0mov 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, r1moveq r0, #1mov pc, lr.size kgdb_longjmp,.-kgdb_longjmpdiff --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_stackget_bad_stack.endm.macro und_save_regsbad_save_user_regs.endm/* In svc mode */.macro und_restore_regsadd r7, sp, #S_PCldr r6, [r7, #4] @load spsr into r6msr cpsr, r6 @use und spsr to overwrite svc cpsrldmdb r7, {r1 - r2} @load sp lr into r1 r2mov lr, r2ldmia sp, {r0 - r12}mov r0, r0add sp, sp, #S_FRAME_SIZEldr pc, [sp, #-12].endm/*
- exception handlers
*/
.align 5undefined_instruction: +#if defined(CONFIG_CMD_KGDB)
get_und_stackund_save_regsbl do_undefined_instructionund_restore_regs+#else get_bad_stack bad_save_user_regs bl do_undefined_instruction +#endif
.align 5software_interrupt:
1.8.4.5
Regards, Simon
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, #0mov 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, r1moveq r0, #1mov pc, lr.size kgdb_longjmp,.-kgdb_longjmpdiff --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_stackget_bad_stack.endm.macro und_save_regsbad_save_user_regs.endm/* In svc mode */.macro und_restore_regsadd r7, sp, #S_PCldr r6, [r7, #4] @load spsr into r6msr cpsr, r6 @use und spsr to overwrite svc cpsrldmdb r7, {r1 - r2} @load sp lr into r1 r2mov lr, r2ldmia sp, {r0 - r12}mov r0, r0add sp, sp, #S_FRAME_SIZEldr pc, [sp, #-12].endm/*
- exception handlers
*/
.align 5undefined_instruction:
+#if defined(CONFIG_CMD_KGDB)
get_und_stackund_save_regsbl do_undefined_instructionund_restore_regs+#else get_bad_stack bad_save_user_regs bl do_undefined_instruction +#endif
.align 5software_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.
participants (3)
-
Peng Fan -
Peng Fan -
Simon Glass