kgdb:add breakpoint and arm support

RFC PATCH New Peng Fan 3 participants 7 messages Nov 1, 2014 – Nov 4, 2014 Arm (32-bit)

Share & tools

Peng Fan

author

2014-11-01 05:12 UTC #0 lore raw

cover 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 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

Signature · 1 line

1.8.4.5

Peng Fan

author

2014-11-01 05:12 UTC #1 lore raw

RFC PATCH 1/2 kgdb: add breakpoint support

Add Z/z protocal support for breakpoint set/remove.

Signed-off-by Peng Fan <[email protected]>

---
 common/kgdb.c  | 273 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/kgdb.h |  35 ++++++++
 2 files changed, 308 insertions(+)

common/kgdb.c +273 -0

index d357463..fd83ccd 100644
@@ -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)

include/kgdb.h +35 -0

index b6ba742..f9152b5 100644
@@ -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__ */

Signature · 1 line

1.8.4.5

Peng Fan

2014-11-04 12:23 UTC #3 lore raw

RFC PATCH 1/2 kgdb: add breakpoint support

Hi Simon,

? 11/4/2014 2:46 PM, Simon Glass ??:

Quoted 22 lines

Hi Peng,
Quoted 3 lines · Peng Fan
Add Z/z protocal support for breakpoint set/remove.
Signed-off-by Peng Fan <[email protected]>
This looks good to me - I just have a few bits below.
Quoted 13 lines
---
  common/kgdb.c  | 273 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  include/kgdb.h |  35 ++++++++
  2 files changed, 308 insertions(+)
common/kgdb.c +0 -0
index d357463..fd83ccd 100644
@@ -92,6 +92,8 @@
  #include <kgdb.h>
  #include <command.h>
 

Quoted 3 lines

common/kgdb.c line 95
Quoted 1 line
common/kgdb.c +1 -0
+#include <asm-generic/errno.h>
#include <errno.h> would do

Ok.

Quoted 6 lines

common/kgdb.c line 97
Quoted 3 lines
common/kgdb.c +1 -0
+
  #undef KGDB_DEBUG
Where is this used?

You mean KGDB_DEBUG?

Quoted 24 lines

common/kgdb.c +11 -0

/*

@@ -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) != '+');
  }

Quoted 5 lines

common/kgdb.c putpacket(unsigned char *buffer) · line 327
Quoted 3 lines
common/kgdb.c +3 -0
+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.

Quoted 13 lines

common/kgdb.c +12 -0

+       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++;
+

Quoted 8 lines

common/kgdb.c putpacket(unsigned char *buffer) · line 343
Quoted 4 lines
common/kgdb.c +4 -0
+       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?

Quoted 151 lines

Quoted 44 lines

common/kgdb.c +43 -0

+{
+       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",

common/kgdb.c putpacket(unsigned char *buffer) · line 396

Quoted 10 lines

common/kgdb.c +10 -0

+                              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

Quoted 94 lines

common/kgdb.c +94 -0

+                */
+               /* 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();

Quoted 8 lines

common/kgdb.c putpacket(unsigned char *buffer) · line 496
Quoted 6 lines
common/kgdb.c +6 -0
+       }
+
+       return ret;
+}
+
+static int kgdb_remove_all_break(void)
kgdb_remove_all_breakpoints() to be consistent?

kgdb_remove_all_breakpoints is better.

Quoted 13 lines

common/kgdb.c +12 -0

+{
+       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);

Quoted 8 lines

common/kgdb.c putpacket(unsigned char *buffer) · line 514
Quoted 6 lines
common/kgdb.c +6 -0
+setundefined:
+               kgdb_break[i].state = BP_UNDEFINED;
+       }
+
+       /* clear hardware breakpoints. */
+       /* ToDo in future. */
/* TODO: clear hardware breakpoints. */

Ok.

Quoted 167 lines

common/kgdb.c +70 -0

+
+       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)

include/kgdb.h +29 -0

index b6ba742..f9152b5 100644
@@ -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 *);

Quoted 14 lines

Quoted 10 lines

include/kgdb.h +6 -1

+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.

Peng Fan

author

2014-11-01 05:12 UTC #4 lore raw

RFC PATCH 2/2 arm:kgdb add KGDB support for arm platform

In reply to Peng Fan

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 <[email protected]>

---
 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(-)

arch/arm/include/asm/proc-armv/ptrace.h +1 -1

index 71df5a9..33fe587 100644
@@ -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]

arch/arm/include/asm/signal.h new file +1 -0

new file mode 100644
index 0000000..7b1573c
@@ -0,0 +1 @@
+#include <asm-generic/signal.h>

arch/arm/lib/Makefile +3 -0

index 1ef2400..c543563 100644
@@ -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

arch/arm/lib/crt0.S +4 -0

index 29cdad0..d96e70b 100644
@@ -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:

arch/arm/lib/interrupts.c +24 -0

index 4dacfd9..d16bd58 100644
@@ -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)

arch/arm/lib/kgdb/kgdb.c new file +231 -0

231 lines not on this page · raw message

arch/arm/lib/kgdb/setjmp.S new file +20 -0

new file mode 100644
index 0000000..e17f959
@@ -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

arch/arm/lib/vectors.S +28 -0

index 49238ed..8abad3d 100644
@@ -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:

Signature · 1 line

1.8.4.5

Peng Fan

2014-11-04 13:09 UTC #6 lore raw

RFC PATCH 2/2 arm:kgdb add KGDB support for arm platform

Hi Simon,

? 11/4/2014 2:58 PM, Simon Glass ??:

Quoted 150 lines

HI Peng,

Quoted 6 lines · Peng Fan

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 <[email protected]>

Looks good so far as I understand it. A few nits below and maybe you can integrate better with the ptrace register numbering.

Quoted 137 lines

---
  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(-)

arch/arm/include/asm/proc-armv/ptrace.h +1 -1

index 71df5a9..33fe587 100644
@@ -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]

arch/arm/include/asm/signal.h new file +1 -0

new file mode 100644
index 0000000..7b1573c
@@ -0,0 +1 @@
+#include <asm-generic/signal.h>

arch/arm/lib/Makefile +3 -0

index 1ef2400..c543563 100644
@@ -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

arch/arm/lib/crt0.S +4 -0

index 29cdad0..d96e70b 100644
@@ -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:

arch/arm/lib/interrupts.c +24 -0

index 4dacfd9..d16bd58 100644
@@ -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)

arch/arm/lib/kgdb/kgdb.c new file +26 -0

new file mode 100644
index 0000000..6b2e542
@@ -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,

Quoted 8 lines

arch/arm/lib/kgdb/kgdb.c line 31
Quoted 5 lines
arch/arm/lib/kgdb/kgdb.c new file +5 -0
+       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.

Quoted 3 lines

Also these mirror the numbers in ptrace.h so I wonder if somehow they could be linked?

hmm, I'll try this.

Quoted 16 lines

arch/arm/lib/kgdb/kgdb.c new file +15 -0

+       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;
+

Quoted 8 lines

arch/arm/lib/kgdb/kgdb.c line 52
Quoted 6 lines
arch/arm/lib/kgdb/kgdb.c new file +6 -0
+       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.

Quoted 20 lines

arch/arm/lib/kgdb/kgdb.c new file +20 -0

+
+       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);
+       }
+

Quoted 8 lines

arch/arm/lib/kgdb/kgdb.c line 78
Quoted 6 lines
arch/arm/lib/kgdb/kgdb.c new file +6 -0
+       enable_interrupts();
+}
+
+int kgdb_trap(struct pt_regs *regs)
+{
+       /* Mark Not sure ??? */
And this?

I'll remove this.

Quoted 22 lines

arch/arm/lib/kgdb/kgdb.c new file +21 -0

+       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;

Quoted 8 lines

arch/arm/lib/kgdb/kgdb.c line 105
Quoted 6 lines
arch/arm/lib/kgdb/kgdb.c new file +6 -0
+       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.

Quoted 16 lines

Quoted 4 lines

arch/arm/lib/kgdb/kgdb.c new file +3 -0

+       gdb_regs[KGDB_ARM_SP] = (unsigned long)regs;
+       gdb_regs[KGDB_ARM_LR] = regs->ARM_lr;
+       gdb_regs[KGDB_ARM_PC] = regs->ARM_pc;

arch/arm/lib/kgdb/kgdb.c line 118

Quoted 10 lines

arch/arm/lib/kgdb/kgdb.c new file +10 -0

+       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.

Quoted 16 lines

arch/arm/lib/kgdb/kgdb.c line 130

Quoted 13 lines

arch/arm/lib/kgdb/kgdb.c new file +12 -0

+               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:

Quoted 1 line

regs->uregs[regno] = *ptr

Hah. regs->uregs[regno] = *ptr is better. Thanks.

Quoted 54 lines

arch/arm/lib/kgdb/kgdb.c new file +53 -0

+       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;
+       }
+}

Quoted 8 lines

arch/arm/lib/kgdb/kgdb.c line 189
Quoted 6 lines
arch/arm/lib/kgdb/kgdb.c new file +6 -0
+
+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.

Quoted 17 lines

arch/arm/lib/kgdb/kgdb.c new file +16 -0

+               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];

Quoted 8 lines

arch/arm/lib/kgdb/kgdb.c line 211

Quoted 6 lines

arch/arm/lib/kgdb/kgdb.c new file +6 -0

+       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.

Quoted 9 lines

arch/arm/lib/kgdb/kgdb.c line 217
Quoted 7 lines
arch/arm/lib/kgdb/kgdb.c new file +6 -0
+}
+
+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.

Quoted 30 lines

Quoted 21 lines

arch/arm/lib/kgdb/kgdb.c new file +14 -0

+}
+
+/*
+ * 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");
+}

arch/arm/lib/kgdb/setjmp.S new file +0 -0

new file mode 100644
index 0000000..e17f959

arch/arm/lib/kgdb/setjmp.S line 6

Quoted 6 lines

arch/arm/lib/kgdb/setjmp.S new file +6 -0

@@ -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}

Quoted 1 line

and below

Ok.

Quoted 61 lines

arch/arm/lib/kgdb/setjmp.S new file +14 -0

+       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

arch/arm/lib/vectors.S +28 -0

index 49238ed..8abad3d 100644
@@ -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

Quoted 9 lines

Quoted 5 lines
arch/arm/lib/vectors.S +0 -1
         .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.