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

Peng Fan B51431 at freescale.com
Tue Nov 4 14:09:21 CET 2014


Hi Simon,

在 11/4/2014 2:58 PM, Simon Glass 写道:
> HI Peng,
>
> On 31 October 2014 23:12, Peng Fan <Peng.Fan at 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 at 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.


More information about the U-Boot mailing list