From: Peng Fan <B51431@freescale.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [RFC PATCH v1 2/2] arm:kgdb add KGDB support for arm platform
Date: Tue, 4 Nov 2014 21:09:21 +0800 [thread overview]
Message-ID: <5458D001.4040307@freescale.com> (raw)
In-Reply-To: <CAPnjgZ37BZzLgYP3EE-q_P6FAWr7Ldn0n1Rha_XbKcnHmN4B8Q@mail.gmail.com>
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.
prev parent reply other threads:[~2014-11-04 13:09 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-01 5:12 [U-Boot] [RFC PATCH v1 0/2] kgdb:add breakpoint and arm support Peng Fan
2014-11-01 5:12 ` [U-Boot] [RFC PATCH v1 1/2] kgdb: add breakpoint support Peng Fan
2014-11-04 6:46 ` Simon Glass
2014-11-04 12:23 ` Peng Fan
2014-11-01 5:12 ` [U-Boot] [RFC PATCH v1 2/2] arm:kgdb add KGDB support for arm platform Peng Fan
2014-11-04 6:58 ` Simon Glass
2014-11-04 13:09 ` Peng Fan [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5458D001.4040307@freescale.com \
--to=b51431@freescale.com \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.