Re: [U-Boot] [RFC PATCH v1 2/2] arm:kgdb add KGDB support for arm platform

Hi Simon,

在 11/4/2014 2:58 PM, Simon Glass 写道:

HI Peng,

On 31 October 2014 23:12, Peng Fan Peng.Fan@freescale.com wrote:

...

The register save/restore: Use get_bad_stack and bad_save_user_regs to save regs. Introduce und_restore_regs to restore the previous regs before trigger a breakpoint.

Signed-off-by: Peng Fan Peng.Fan@freescale.com

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

...

---

arch/arm/include/asm/proc-armv/ptrace.h | 2 +- arch/arm/include/asm/signal.h | 1 + arch/arm/lib/Makefile | 3 + arch/arm/lib/crt0.S | 4 + arch/arm/lib/interrupts.c | 24 ++++ arch/arm/lib/kgdb/kgdb.c | 231 ++++++++++++++++++++++++++++++++ arch/arm/lib/kgdb/setjmp.S | 20 +++ arch/arm/lib/vectors.S | 28 ++++ 8 files changed, 312 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/proc-armv/ptrace.h b/arch/arm/include/asm/proc-armv/ptrace.h index 71df5a9..33fe587 100644 --- a/arch/arm/include/asm/proc-armv/ptrace.h +++ b/arch/arm/include/asm/proc-armv/ptrace.h @@ -58,7 +58,7 @@ struct pt_regs { stack during a system call. */

struct pt_regs {

•   long uregs[18];
  

•   unsigned long uregs[18];
  

  };

  #define ARM_cpsr uregs[16]

diff --git a/arch/arm/include/asm/signal.h b/arch/arm/include/asm/signal.h new file mode 100644 index 0000000..7b1573c --- /dev/null +++ b/arch/arm/include/asm/signal.h @@ -0,0 +1 @@ +#include <asm-generic/signal.h> diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile index 1ef2400..c543563 100644 --- a/arch/arm/lib/Makefile +++ b/arch/arm/lib/Makefile @@ -52,3 +52,6 @@ endif ifneq (,$(findstring -mabi=aapcs-linux,$(PLATFORM_CPPFLAGS))) extra-y += eabi_compat.o endif

+obj-$(CONFIG_CMD_KGDB) += kgdb/setjmp.o +obj-$(CONFIG_CMD_KGDB) += kgdb/kgdb.o diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S index 29cdad0..d96e70b 100644 --- a/arch/arm/lib/crt0.S +++ b/arch/arm/lib/crt0.S @@ -99,9 +99,13 @@ clr_gd: sub r9, r9, #GD_SIZE /* new GD is below bd */

     adr     lr, here

+#ifndef CONFIG_CMD_KGDB ldr r0, [r9, #GD_RELOC_OFF] /* r0 = gd->reloc_off */ add lr, lr, r0 ldr r0, [r9, #GD_RELOCADDR] /* r0 = gd->relocaddr */ +#else

• ldr r0, =__image_copy_start

+#endif b relocate_code here:

diff --git a/arch/arm/lib/interrupts.c b/arch/arm/lib/interrupts.c index 4dacfd9..d16bd58 100644 --- a/arch/arm/lib/interrupts.c +++ b/arch/arm/lib/interrupts.c @@ -22,6 +22,7 @@ #include <common.h> #include <asm/proc-armv/ptrace.h> #include <asm/u-boot-arm.h> +#include <kgdb.h>

DECLARE_GLOBAL_DATA_PTR;

@@ -160,9 +161,32 @@ void show_regs (struct pt_regs *regs)

void do_undefined_instruction (struct pt_regs *pt_regs) { +#if defined(CONFIG_CMD_KGDB)

•   unsigned long *tmp_pc = NULL;
  

•   pt_regs->ARM_pc -= 4;
  

•   tmp_pc = (unsigned long *)pt_regs->ARM_pc;
  

•   if (*tmp_pc == 0xe7ffdeff) {
  

•           pt_regs->ARM_pc += 4;
  

•           if (debugger_exception_handler &&
  

•               debugger_exception_handler(pt_regs)) {
  

•                   return;
  

•           }
  

•   } else if (*tmp_pc == 0xe7ffdefe) {
  

•           if (debugger_exception_handler &&
  

•               debugger_exception_handler(pt_regs)) {
  

•                   return;
  

•           }
  

•   } else {
  

•           printf("DCache/ICACHE May need flush\n");
  

•           return;
  

•   }
  

+#else printf ("undefined instruction\n"); show_regs (pt_regs); bad_mode (); +#endif }

void do_software_interrupt (struct pt_regs *pt_regs) diff --git a/arch/arm/lib/kgdb/kgdb.c b/arch/arm/lib/kgdb/kgdb.c new file mode 100644 index 0000000..6b2e542 --- /dev/null +++ b/arch/arm/lib/kgdb/kgdb.c @@ -0,0 +1,231 @@ +#include <common.h> +#include <command.h> +#include <kgdb.h> +#include <asm/signal.h> +#include <asm/processor.h>

+#define KGDB_ARM_GP_REGS 16 +#define KGDB_ARM_FP_REGS 8 +#define KGDB_ARM_EXTRA_REGS 2 +#define KGDB_ARM_MAX_REGS (KGDB_ARM_GP_REGS +\

•                            (KGDB_ARM_FP_REGS * 3) +\
  

•                             KGDB_ARM_EXTRA_REGS)
  

+#define KGDB_NUMREGBYTES (KGDB_ARM_MAX_REGS << 2)

+enum arm_regs {

•   KGDB_ARM_R0,
  

•   KGDB_ARM_R1,
  

•   KGDB_ARM_R2,
  

•   KGDB_ARM_R3,
  

•   KGDB_ARM_R4,
  

•   KGDB_ARM_R5,
  

•   KGDB_ARM_R6,
  

•   KGDB_ARM_R7,
  

•   KGDB_ARM_R8,
  

•   KGDB_ARM_R9,
  

•   KGDB_ARM_R10,
  

•   KGDB_ARM_FP,
  

•   KGDB_ARM_IP,
  

•   KGDB_ARM_SP,
  

•   KGDB_ARM_LR,
  

•   KGDB_ARM_PC,
  

So in here there are the FP and EXTRA regs? If you added them to this enum it would be clearer.

I did not test FP and other extra regs. Actually in uboot, the FP and extra regs are used? I am not sure.

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

hmm, I'll try this.

...

•   KGDB_ARM_CPSR = KGDB_ARM_MAX_REGS - 1
  

+};

+void kgdb_enter(struct pt_regs *regs, kgdb_data *kdp) +{

•   disable_interrupts();
  

•   kdp->sigval = kgdb_trap(regs);
  

•   kdp->nregs = 2;
  

•   kdp->regs[0].num = KGDB_ARM_PC;
  

•   kdp->regs[0].val = regs->ARM_pc;
  

•   kdp->regs[1].num = KGDB_ARM_SP;
  

•   kdp->regs[1].val = regs->ARM_sp;
  

•   return;
  

+}

+void kgdb_exit(struct pt_regs *regs, kgdb_data *kdp) +{

•   /* Mark Not sure ??? */
  

What does this mean?

Missed to remove this. This is marked when I beginning wrote this piece of code.

...

•   if (kdp->extype & KGDBEXIT_WITHADDR) {
  

•           regs->ARM_pc = kdp->exaddr;
  

•           printf("KGDBEXIT_WITHADDR 0x%lx\n", regs->ARM_pc);
  

•   }
  

•   switch (kdp->extype & KGDBEXIT_TYPEMASK) {
  

•   case KGDBEXIT_KILL:
  

•           printf("KGDBEXIT_KILL:\n");
  

•           break;
  

•   case KGDBEXIT_CONTINUE:
  

•           printf("KGDBEXIT_CONTINUE\n");
  

•           break;
  

•   case KGDBEXIT_SINGLE:
  

•           printf("KGDBEXIT_SINGLE\n");
  

•           break;
  

•   default:
  

•           printf("KGDBEXIT : %d\n", kdp->extype);
  

•   }
  

•   enable_interrupts();
  

+}

+int kgdb_trap(struct pt_regs *regs) +{

•   /* Mark Not sure ??? */
  

And this?

I'll remove this.

...

•   return SIGTRAP;
  

+}

+int kgdb_getregs(struct pt_regs *regs, char *buf, int max) +{

•   unsigned long *gdb_regs = (unsigned long *)buf;
  

•   if (max < KGDB_NUMREGBYTES)
  

•           kgdb_error(KGDBERR_NOSPACE);
  

•   if ((unsigned long)gdb_regs & 3)
  

•           kgdb_error(KGDBERR_ALIGNFAULT);
  

•   gdb_regs[KGDB_ARM_R0] = regs->ARM_r0;
  

•   gdb_regs[KGDB_ARM_R1] = regs->ARM_r1;
  

•   gdb_regs[KGDB_ARM_R2] = regs->ARM_r2;
  

•   gdb_regs[KGDB_ARM_R3] = regs->ARM_r3;
  

•   gdb_regs[KGDB_ARM_R4] = regs->ARM_r4;
  

•   gdb_regs[KGDB_ARM_R5] = regs->ARM_r5;
  

•   gdb_regs[KGDB_ARM_R6] = regs->ARM_r6;
  

•   gdb_regs[KGDB_ARM_R7] = regs->ARM_r7;
  

•   gdb_regs[KGDB_ARM_R8] = regs->ARM_r8;
  

•   gdb_regs[KGDB_ARM_R9] = regs->ARM_r9;
  

•   gdb_regs[KGDB_ARM_R10] = regs->ARM_r10;
  

•   gdb_regs[KGDB_ARM_IP] = regs->ARM_ip;
  

•   gdb_regs[KGDB_ARM_FP] = regs->ARM_fp;
  

•   /* set sp */
  

I think you can remove that comment as it's pretty obvious

Ok.

...

•   gdb_regs[KGDB_ARM_SP] = (unsigned long)regs;
  

•   gdb_regs[KGDB_ARM_LR] = regs->ARM_lr;
  

•   gdb_regs[KGDB_ARM_PC] = regs->ARM_pc;
  

•   gdb_regs[KGDB_ARM_CPSR] = regs->ARM_cpsr;
  

•   return KGDB_NUMREGBYTES;
  

+}

+void kgdb_putreg(struct pt_regs *regs, int regno, char *buf, int length) +{

•   unsigned long *ptr = (unsigned long *)buf;
  

•   if (regno < 0 || regno >= 16)
  

Should that be regno > KGDB_ARM_PC?

Yeah.

...

•           kgdb_error(KGDBERR_BADPARAMS);
  

•   if (length < 4)
  

•           kgdb_error(KGDBERR_NOSPACE);
  

•   if ((unsigned long)ptr & 3)
  

•           kgdb_error(KGDBERR_ALIGNFAULT);
  

•   switch (regno) {
  

•   case KGDB_ARM_R0:
  

•           regs->ARM_r0 = *ptr;
  

•           break;
  

Would it be valid to drop this switch and use:

regs->uregs[regno] = *ptr

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

...

•   case KGDB_ARM_R1:
  

•           regs->ARM_r1 = *ptr;
  

•           break;
  

•   case KGDB_ARM_R2:
  

•           regs->ARM_r2 = *ptr;
  

•           break;
  

•   case KGDB_ARM_R3:
  

•           regs->ARM_r3 = *ptr;
  

•           break;
  

•   case KGDB_ARM_R4:
  

•           regs->ARM_r4 = *ptr;
  

•           break;
  

•   case KGDB_ARM_R5:
  

•           regs->ARM_r5 = *ptr;
  

•           break;
  

•   case KGDB_ARM_R6:
  

•           regs->ARM_r6 = *ptr;
  

•           break;
  

•   case KGDB_ARM_R7:
  

•           regs->ARM_r7 = *ptr;
  

•           break;
  

•   case KGDB_ARM_R8:
  

•           regs->ARM_r8 = *ptr;
  

•           break;
  

•   case KGDB_ARM_R9:
  

•           regs->ARM_r9 = *ptr;
  

•           break;
  

•   case KGDB_ARM_R10:
  

•           regs->ARM_r10 = *ptr;
  

•           break;
  

•   case KGDB_ARM_FP:
  

•           regs->ARM_fp = *ptr;
  

•           break;
  

•   case KGDB_ARM_IP:
  

•           regs->ARM_ip = *ptr;
  

•           break;
  

•   case KGDB_ARM_SP:
  

•           regs->ARM_sp = *ptr;
  

•           break;
  

•   case KGDB_ARM_LR:
  

•           regs->ARM_lr = *ptr;
  

•           break;
  

•   case KGDB_ARM_PC:
  

•           regs->ARM_pc = *ptr;
  

•           break;
  

•   case KGDB_ARM_CPSR:
  

•           regs->ARM_cpsr = *ptr;
  

•           break;
  

•   default:
  

•           kgdb_error(KGDBERR_BADPARAMS);
  

•           break;
  

•   }
  

+}

+void kgdb_putregs(struct pt_regs *regs, char *buf, int length) +{

•   unsigned long *kgdb_regs = (unsigned long *)buf;
  

•   if (length != KGDB_ARM_MAX_REGS)
  

Is length the number of registers rather than the number of bytes?

you are right, should be the number of bytes.

...

•           kgdb_error(KGDBERR_NOSPACE);
  

•   if ((unsigned long)kgdb_regs & 3)
  

•           kgdb_error(KGDBERR_ALIGNFAULT);
  

•   regs->ARM_r0 = kgdb_regs[KGDB_ARM_R0];
  

•   regs->ARM_r1 = kgdb_regs[KGDB_ARM_R1];
  

•   regs->ARM_r2 = kgdb_regs[KGDB_ARM_R2];
  

•   regs->ARM_r3 = kgdb_regs[KGDB_ARM_R3];
  

•   regs->ARM_r4 = kgdb_regs[KGDB_ARM_R4];
  

•   regs->ARM_r5 = kgdb_regs[KGDB_ARM_R5];
  

•   regs->ARM_r6 = kgdb_regs[KGDB_ARM_R6];
  

•   regs->ARM_r7 = kgdb_regs[KGDB_ARM_R7];
  

•   regs->ARM_r8 = kgdb_regs[KGDB_ARM_R8];
  

•   regs->ARM_r9 = kgdb_regs[KGDB_ARM_R9];
  

•   regs->ARM_r10 = kgdb_regs[KGDB_ARM_R10];
  

•   regs->ARM_ip = kgdb_regs[KGDB_ARM_IP];
  

•   regs->ARM_fp = kgdb_regs[KGDB_ARM_FP];
  

•   regs->ARM_sp = kgdb_regs[KGDB_ARM_SP];
  

•   regs->ARM_lr = kgdb_regs[KGDB_ARM_LR];
  

•   regs->ARM_pc = kgdb_regs[KGDB_ARM_PC];
  

•   regs->ARM_cpsr = kgdb_regs[KGDB_ARM_CPSR];
  

Feels like this could be a for() loop.

I'll try to use for loop or memcpy.

...

+}

+void kgdb_flush_cache_all(void) +{

•   invalidate_icache_all();
  

•   flush_dcache_all();
  

Do you need to flush the write buffer here?

Actually, the inst 'x' is in DRAM, we modify it to a break instruction. If not flush dcache, the modified instruction may still exists in dcache. Then icache may not see the break instruction. So flush the write buffer.

...

+}

+/*

• • This function will generate a breakpoint exception.
• • It is used at the beginning of a program to sync up
• • with a debugger and can be used otherwise as a quick
• • means to stop program execution and "break" into
• • the debugger.
• */

+void kgdb_breakpoint(int argc, char * const argv[]) +{

•   asm(".word 0xe7ffdeff");
  

+} diff --git a/arch/arm/lib/kgdb/setjmp.S b/arch/arm/lib/kgdb/setjmp.S new file mode 100644 index 0000000..e17f959 --- /dev/null +++ b/arch/arm/lib/kgdb/setjmp.S @@ -0,0 +1,20 @@

•   .text
  

•   .balign 4
  

•   .global kgdb_setjmp
  

•   .type kgdb_setjmp, #function
  

+kgdb_setjmp:

•   stmia   r0, {r4, r5, r6, r7, r8, r9, r10, fp, sp, lr}
  

{r4-r10, fp, sp, lr}

and below

Ok.

...

•   mov r0, #0
  

•   mov pc, lr
  

•   .size kgdb_setjmp,.-kgdb_setjmp
  

•   .text
  

•   .balign 4
  

•   .global kgdb_longjmp
  

•   .type kgdb_longjmp, #function
  

+kgdb_longjmp:

•   ldmia   r0, {r4, r5, r6, r7, r8, r9, r10, fp, sp, lr}
  

•   movs r0, r1
  

•   moveq r0, #1
  

•   mov pc, lr
  

•   .size kgdb_longjmp,.-kgdb_longjmp
  

diff --git a/arch/arm/lib/vectors.S b/arch/arm/lib/vectors.S index 49238ed..8abad3d 100644 --- a/arch/arm/lib/vectors.S +++ b/arch/arm/lib/vectors.S @@ -221,15 +221,43 @@ FIQ_STACK_START: ldr sp, FIQ_STACK_START .endm

•   .macro get_und_stack
  

•   get_bad_stack
  

•   .endm
  

•   .macro und_save_regs
  

•   bad_save_user_regs
  

•   .endm
  

•   /* In svc mode */
  

•   .macro und_restore_regs
  

•   add     r7, sp, #S_PC
  

•   ldr     r6, [r7, #4] @load spsr into r6
  

•   msr     cpsr, r6 @use und spsr to overwrite svc cpsr
  

•   ldmdb   r7, {r1 - r2} @load sp lr into r1 r2
  

•   mov     lr, r2
  

•   ldmia   sp, {r0 - r12}
  

•   mov     r0, r0
  

•   add     sp, sp, #S_FRAME_SIZE
  

•   ldr     pc, [sp, #-12]
  

•   .endm
  

• /*

  • exception handlers

  */

     .align  5
  

  undefined_instruction:

+#if defined(CONFIG_CMD_KGDB)

•   get_und_stack
  

•   und_save_regs
  

•   bl      do_undefined_instruction
  

•   und_restore_regs
  

+#else get_bad_stack bad_save_user_regs bl do_undefined_instruction +#endif

     .align  5

software_interrupt:

1.8.4.5

Regards, Simon

Thanks for reviewing.

I'll fix and test the patch set and send v2 out for review.

Regards, Peng.