Add KGDB support for ARM platforms

These patches add kgdb support for ARM platforms.

The add and invocation of the bad_restore_user_regs macro in cpu/arm720t/start.S should be made to other cpu/arm*/start.S files as well.

---
 common/kgdb.c          |   30 ++++++
 cpu/arm720t/start.S    |    7 ++
 include/asm-arm/kgdb.h |   67 ++++++++++++
 include/kgdb.h         |   33 ++++++
 lib_arm/Makefile       |    1 +
 lib_arm/board.c        |    6 +
 lib_arm/interrupts.c   |   10 ++
 lib_arm/kgdb.c         |  262 ++++++++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 416 insertions(+), 0 deletions(-)
 create mode 100644 include/asm-arm/kgdb.h
 create mode 100644 lib_arm/kgdb.c

index 0531452..15ff6e8 100644
@@ -220,6 +220,29 @@ hexToInt(char **ptr, int *intValue)
 	return (numChars);
 }
 
+/* Handle the 'z' or 'Z' breakpoint remove or set packets */
+static void gdb_cmd_break(kgdb_data *kdp)
+{
+	/*
+	 * Since GDB-5.3, it's been drafted that '0' is a software
+	 * breakpoint, '1' is a hardware breakpoint, so let's do that.
+	 */
+	char *bpt_type = &remcomInBuffer[1];
+	char *ptr = &remcomInBuffer[2];
+	int addr, length;
+
+	if (*bpt_type != '0')
+		return;		/* Unsupported. */
+	if (*ptr++ != ',' || !hexToInt(&ptr, &addr) ||
+	    *ptr++ != ',' || !hexToInt(&ptr, &length)) {
+		kgdb_error(KGDBERR_BADPARAMS);
+	}
+
+	if (kdp->hook_break &&
+	    !(*kdp->hook_break)(remcomInBuffer[0] == 'Z', addr))
+		strcpy(remcomOutBuffer, "OK");
+}
+
 /* scan for the sequence $<data>#<checksum>     */
 static void
 getpacket(char *buffer)

@@ -341,11 +364,14 @@ handle_exception (struct pt_regs *regs)
 
 	kgdb_interruptible(0);
 
+#ifdef KGDB_DEBUG
 	printf("kgdb: handle_exception; trap [0x%x]\n", kgdb_trap(regs));
+#endif
 
 	if (kgdb_setjmp(error_jmp_buf) != 0)
 		panic("kgdb: error or fault in entry init!\n");
 
+	memset(&kd, 0, sizeof(kgdb_data));
 	kgdb_enter(regs, &kd);
 
 	if (first_entry) {

@@ -516,6 +542,10 @@ handle_exception (struct pt_regs *regs)
 				kgdb_error(KGDBERR_BADPARAMS);
 			}
 			break;
+		case 'Z': /* [Z|z]N,AA..AA,LLLL Set/Remove breakpoint type N LLLL bytes at address AA.AA return OK */
+		case 'z':
+			gdb_cmd_break(&kd);
+			break;
 		}			/* switch */
 
 		if (errnum != 0)

index 022b873..a59e186 100644
@@ -455,6 +455,12 @@ lock_loop:
 	mov	r0, sp
 	.endm
 
+	.macro	bad_restore_user_regs
+	ldr	lr, [sp, #S_PSR]		@ Get SVC cpsr
+	msr	spsr_cxsf, lr
+	ldmia	sp, {r0 - pc}^			@ Restore SVC registers
+	.endm
+
 	.macro	irq_save_user_regs
 	sub	sp, sp, #S_FRAME_SIZE
 	stmia	sp, {r0 - r12}			@ Calling r0-r12

@@ -506,6 +512,7 @@ undefined_instruction:
 	get_bad_stack
 	bad_save_user_regs
 	bl	do_undefined_instruction
+	bad_restore_user_regs
 
 	.align	5
 software_interrupt:

index f543cd6..b4c5d81 100644
@@ -8,6 +8,38 @@
 #define KGDBERR_MEMFAULT	3
 #define KGDBERR_NOSPACE		4
 #define KGDBERR_ALIGNFAULT	5
+#define KGDBERR_BPEXIST		6
+#define KGDBERR_BPNOENT		7
+
+#ifndef BREAK_INSTR_SIZE
+#define BREAK_INSTR_SIZE	4
+#endif
+
+#ifndef KGDB_MAX_BREAKPOINTS
+#define KGDB_MAX_BREAKPOINTS	1000
+#endif
+
+enum kgdb_bptype {
+	BP_BREAKPOINT = 0,
+	BP_HARDWARE_BREAKPOINT,
+	BP_WRITE_WATCHPOINT,
+	BP_READ_WATCHPOINT,
+	BP_ACCESS_WATCHPOINT
+};
+
+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;
+};
 
 #define KGDBDATA_MAXREGS	8
 #define KGDBDATA_MAXPRIV	8

@@ -35,6 +67,7 @@ typedef
 		int nregs;
 		kgdb_reg regs[KGDBDATA_MAXREGS];
 		unsigned long private[KGDBDATA_MAXPRIV];
+		int (*hook_break)(int set, int addr);
 	}
 kgdb_data;
 

index 0293348..36fe528 100644
@@ -44,6 +44,7 @@ COBJS-y	+= cache-cp15.o
 endif
 COBJS-y	+= interrupts.o
 COBJS-y	+= reset.o
+COBJS-$(CONFIG_CMD_KGDB) += kgdb.o
 
 SRCS	:= $(GLSOBJS:.o=.S) $(GLCOBJS:.o=.c) \
 	   $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c)

index f5660a9..edc2120 100644
@@ -39,6 +39,7 @@
  */
 
 #include <common.h>
+#include <watchdog.h>
 #include <command.h>
 #include <malloc.h>
 #include <stdio_dev.h>

@@ -381,6 +382,11 @@ void start_armboot (void)
 	/* miscellaneous platform dependent initialisations */
 	misc_init_r ();
 #endif
+#if defined(CONFIG_CMD_KGDB)
+	WATCHDOG_RESET();
+	puts("KGDB:  ");
+	kgdb_init();
+#endif
 
 	/* enable exceptions */
 	enable_interrupts ();

index 1f2b815..d62b355 100644
@@ -37,6 +37,10 @@
 
 #include <common.h>
 #include <asm/proc-armv/ptrace.h>
+#include <kgdb.h>
+#ifdef CONFIG_CMD_KGDB
+#include <asm/kgdb.h>
+#endif
 
 #ifdef CONFIG_USE_IRQ
 DECLARE_GLOBAL_DATA_PTR;

@@ -137,6 +141,12 @@ void show_regs (struct pt_regs *regs)
 
 void do_undefined_instruction (struct pt_regs *pt_regs)
 {
+#ifdef CONFIG_CMD_KGDB
+	if (*(unsigned long *)(instruction_pointer(pt_regs) - 4) == KGDB_COMPILED_BREAK) {
+		(*debugger_exception_handler)(pt_regs);
+		return;
+	}
+#endif
 	printf ("undefined instruction\n");
 	show_regs (pt_regs);
 	bad_mode ();

1.6.0.6

These patches add kgdb support for ARM platforms.

The add and invocation of the bad_restore_user_regs macro in cpu/arm720t/start.S should be made to other cpu/arm*/start.S files as well.

---

?common/kgdb.c ? ? ? ? ?| ? 30 ++++++
?cpu/arm720t/start.S ? ?| ? ?7 ++
?include/asm-arm/kgdb.h | ? 67 ++++++++++++
?include/kgdb.h ? ? ? ? | ? 33 ++++++
?lib_arm/Makefile ? ? ? | ? ?1 +
?lib_arm/board.c ? ? ? ?| ? ?6 +
?lib_arm/interrupts.c ? | ? 10 ++
?lib_arm/kgdb.c ? ? ? ? | ?262 ++++++++++++++++++++++++++++++++++++++++++++++++

?8 files changed, 416 insertions(+), 0 deletions(-) ?create mode 100644 include/asm-arm/kgdb.h ?create mode 100644 lib_arm/kgdb.c

index 0531452..15ff6e8 100644
@@ -220,6 +220,29 @@ hexToInt(char **ptr, int *intValue)

? ? ? ?return (numChars);
?}

+/* Handle the 'z' or 'Z' breakpoint remove or set packets */ +static void gdb_cmd_break(kgdb_data *kdp)
+{
+ ? ? ? /*
+ ? ? ? ?* Since GDB-5.3, it's been drafted that '0' is a software
+ ? ? ? ?* breakpoint, '1' is a hardware breakpoint, so let's do that.
+ ? ? ? ?*/
+ ? ? ? char *bpt_type = &remcomInBuffer[1];
+ ? ? ? char *ptr = &remcomInBuffer[2];
+ ? ? ? int addr, length;
+
+ ? ? ? if (*bpt_type != '0')
+ ? ? ? ? ? ? ? return; ? ? ? ? /* Unsupported. */

+ ? ? ? if (*ptr++ != ',' || !hexToInt(&ptr, &addr) ||
+ ? ? ? ? ? *ptr++ != ',' || !hexToInt(&ptr, &length)) {

+ ? ? ? ? ? ? ? kgdb_error(KGDBERR_BADPARAMS);
+ ? ? ? }
+
+ ? ? ? if (kdp->hook_break &&
+ ? ? ? ? ? !(*kdp->hook_break)(remcomInBuffer[0] == 'Z', addr))
+ ? ? ? ? ? ? ? strcpy(remcomOutBuffer, "OK");
+}
+
?/* scan for the sequence $<data>#<checksum> ? ? */ ?static void
?getpacket(char *buffer)
@@ -341,11 +364,14 @@ handle_exception (struct pt_regs *regs)

? ? ? ?kgdb_interruptible(0);

+#ifdef KGDB_DEBUG
? ? ? ?printf("kgdb: handle_exception; trap [0x%x]\n", kgdb_trap(regs)); +#endif

? ? ? ?if (kgdb_setjmp(error_jmp_buf) != 0)
? ? ? ? ? ? ? ?panic("kgdb: error or fault in entry init!\n");

+ ? ? ? memset(&kd, 0, sizeof(kgdb_data));
? ? ? ?kgdb_enter(regs, &kd);

? ? ? ?if (first_entry) {
@@ -516,6 +542,10 @@ handle_exception (struct pt_regs *regs) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?kgdb_error(KGDBERR_BADPARAMS); ? ? ? ? ? ? ? ? ? ? ? ?}
? ? ? ? ? ? ? ? ? ? ? ?break;
+ ? ? ? ? ? ? ? case 'Z': /* [Z|z]N,AA..AA,LLLL Set/Remove breakpoint type N LLLL bytes at address AA.AA return OK */
+ ? ? ? ? ? ? ? case 'z':
+ ? ? ? ? ? ? ? ? ? ? ? gdb_cmd_break(&kd);
+ ? ? ? ? ? ? ? ? ? ? ? break;
? ? ? ? ? ? ? ?} ? ? ? ? ? ? ? ? ? ? ? /* switch */

? ? ? ? ? ? ? ?if (errnum != 0)

index 022b873..a59e186 100644
@@ -455,6 +455,12 @@ lock_loop:

? ? ? ?mov ? ? r0, sp
? ? ? ?.endm

+ ? ? ? .macro ?bad_restore_user_regs
+ ? ? ? ldr ? ? lr, [sp, #S_PSR] ? ? ? ? ? ? ? ?@ Get SVC cpsr
+ ? ? ? msr ? ? spsr_cxsf, lr
+ ? ? ? ldmia ? sp, {r0 - pc}^ ? ? ? ? ? ? ? ? ?@ Restore SVC registers
+ ? ? ? .endm
+
? ? ? ?.macro ?irq_save_user_regs
? ? ? ?sub ? ? sp, sp, #S_FRAME_SIZE
? ? ? ?stmia ? sp, {r0 - r12} ? ? ? ? ? ? ? ? ?@ Calling r0-r12 @@ -506,6 +512,7 @@ undefined_instruction:
? ? ? ?get_bad_stack
? ? ? ?bad_save_user_regs
? ? ? ?bl ? ? ?do_undefined_instruction
+ ? ? ? bad_restore_user_regs

? ? ? ?.align ?5
?software_interrupt:

index f543cd6..b4c5d81 100644
@@ -8,6 +8,38 @@

?#define KGDBERR_MEMFAULT ? ? ? 3
?#define KGDBERR_NOSPACE ? ? ? ? ? ? ? ?4
?#define KGDBERR_ALIGNFAULT ? ? 5
+#define KGDBERR_BPEXIST ? ? ? ? ? ? ? ?6
+#define KGDBERR_BPNOENT ? ? ? ? ? ? ? ?7
+
+#ifndef BREAK_INSTR_SIZE
+#define BREAK_INSTR_SIZE ? ? ? 4
+#endif
+
+#ifndef KGDB_MAX_BREAKPOINTS
+#define KGDB_MAX_BREAKPOINTS ? 1000
+#endif
+
+enum kgdb_bptype {
+ ? ? ? BP_BREAKPOINT = 0,
+ ? ? ? BP_HARDWARE_BREAKPOINT,
+ ? ? ? BP_WRITE_WATCHPOINT,
+ ? ? ? BP_READ_WATCHPOINT,
+ ? ? ? BP_ACCESS_WATCHPOINT
+};
+
+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;
+};

?#define KGDBDATA_MAXREGS ? ? ? 8
?#define KGDBDATA_MAXPRIV ? ? ? 8
@@ -35,6 +67,7 @@ typedef
? ? ? ? ? ? ? ?int nregs;
? ? ? ? ? ? ? ?kgdb_reg regs[KGDBDATA_MAXREGS]; ? ? ? ? ? ? ? ?unsigned long private[KGDBDATA_MAXPRIV];
+ ? ? ? ? ? ? ? int (*hook_break)(int set, int addr); ? ? ? ?}
?kgdb_data;

index 0293348..36fe528 100644
@@ -44,6 +44,7 @@ COBJS-y ? ? ? += cache-cp15.o

?endif
?COBJS-y ? ? ? ?+= interrupts.o
?COBJS-y ? ? ? ?+= reset.o
+COBJS-$(CONFIG_CMD_KGDB) += kgdb.o

?SRCS ? := $(GLSOBJS:.o=.S) $(GLCOBJS:.o=.c) \
? ? ? ? ? $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c)

index f5660a9..edc2120 100644
@@ -39,6 +39,7 @@

?*/

?#include <common.h>
+#include <watchdog.h>
?#include <command.h>
?#include <malloc.h>
?#include <stdio_dev.h>
@@ -381,6 +382,11 @@ void start_armboot (void)
? ? ? ?/* miscellaneous platform dependent initialisations */ ? ? ? ?misc_init_r ();
?#endif
+#if defined(CONFIG_CMD_KGDB)
+ ? ? ? WATCHDOG_RESET();
+ ? ? ? puts("KGDB: ?");
+ ? ? ? kgdb_init();
+#endif

? ? ? ?/* enable exceptions */
? ? ? ?enable_interrupts ();

index 1f2b815..d62b355 100644
@@ -37,6 +37,10 @@
 

?#include <common.h>
?#include <asm/proc-armv/ptrace.h>
+#include <kgdb.h>
+#ifdef CONFIG_CMD_KGDB
+#include <asm/kgdb.h>
+#endif

?#ifdef CONFIG_USE_IRQ
?DECLARE_GLOBAL_DATA_PTR;
@@ -137,6 +141,12 @@ void show_regs (struct pt_regs *regs)

?void do_undefined_instruction (struct pt_regs *pt_regs) ?{
+#ifdef CONFIG_CMD_KGDB
+ ? ? ? if (*(unsigned long *)(instruction_pointer(pt_regs) - 4) == KGDB_COMPILED_BREAK) {
+ ? ? ? ? ? ? ? (*debugger_exception_handler)(pt_regs);
+ ? ? ? ? ? ? ? return;
+ ? ? ? }
+#endif
? ? ? ?printf ("undefined instruction\n");
? ? ? ?show_regs (pt_regs);
? ? ? ?bad_mode ();

--
1.6.0.6

Mike, thanks for your review. Here need your further clarifications, thanks in advance.

Yes, these 2 breakpoint commands are not specific to ARM, but I think they are mandatory required for debugging ARM kernel space, otherwise gdb will use M command to write a SWI instruction code to the trap location.

Anyway, agree with you that it might be more clear to separate these code to other patch, as this is going to modify to common/kgdb.c which is architecture / platform independent.

Since I added a (*hook_break)() to the kgdb_data structure, and in order to make it transparent to other architectures (e.g. ppc, blackfin), I believe we need to clear the entire kd here. But will separate this into other patchset as your suggestion.

Since this include/kgdb.h file has been referenced by other architectures, the existing code may not define BREAK_INSTR_SIZE, so this is to prevent the new added code break the KGDB support for other architectures.

These code are identical as ppc, m68k, and i386 architectures, and I check the serial driver of my board, it looks like some serial drivers may not call watchdog funcs, so I'd keep these changes. Any comment on this? Thanks.

Best Regards,
Tonny

hmm, kgdb_enter() is more for initializing state, not new hooks. my understanding is that either an arch always implements hook_break or it doesnt, it doesnt just do it some of the time.

that means the better way to add breakpoint support is via kgdb_stubs.c. add a new weak func to that where it always returns 0. then the remaining kgdb code can always assume the func exists and merely checks the return value.

although, now that i look at the breakpoint logic added to lib_arm/kgdb.c, it looks like most of that should be common code. the interface between common bp code and arch bp code should be the breakinst value (perhaps an unsigned char array encoding the opcode) and the size of the breakpoint array.

also, unless i missed something, your code that sets the bp is missing an icache flush.

i think an undefined size should be used as a tip that the arch doesnt yet support break points

historical warts should be fixed, not extended. in other words, any serial driver not calling WATCHDOG_RESET() in putc()/tstc() is broken. i know this isnt documented properly, but it's still true. look at the cpu/blackfin/serial.c driver for a simple example.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100409/cac18593/attachment.pgp