[U-Boot] [RFC PATCH v1 1/2] kgdb: add breakpoint support
Peng Fan
B51431 at freescale.com
Tue Nov 4 13:23:37 CET 2014
Hi Simon,
在 11/4/2014 2:46 PM, Simon Glass 写道:
> Hi Peng,
>
> On 31 October 2014 23:12, Peng Fan <Peng.Fan at freescale.com> wrote:
>> Add Z/z protocal support for breakpoint set/remove.
>>
>> Signed-off-by: Peng Fan <Peng.Fan at freescale.com>
>
> This looks good to me - I just have a few bits below.
>
>> ---
>> common/kgdb.c | 273 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> include/kgdb.h | 35 ++++++++
>> 2 files changed, 308 insertions(+)
>>
>> diff --git a/common/kgdb.c b/common/kgdb.c
>> index d357463..fd83ccd 100644
>> --- a/common/kgdb.c
>> +++ b/common/kgdb.c
>> @@ -92,6 +92,8 @@
>> #include <kgdb.h>
>> #include <command.h>
>>
>> +#include <asm-generic/errno.h>
>
> #include <errno.h> would do
Ok.
>
>> +
>> #undef KGDB_DEBUG
>
> Where is this used?
>
You mean 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 */
>
> ?
I'll remove the comment. Actually i just want to validate whether the
add parameter is fine or not using this function.
>
>> + 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)
>
> 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?
>
>> +{
>> + 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.
>
> instruction
>
>> + */
>> + /* 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)
>
> kgdb_remove_all_breakpoints() to be consistent?
kgdb_remove_all_breakpoints is better.
>
>> +{
>> + 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. */
>
> /* TODO: clear hardware breakpoints. */
Ok.
>
>> +
>> + 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)
>> diff --git a/include/kgdb.h b/include/kgdb.h
>> index b6ba742..f9152b5 100644
>> --- a/include/kgdb.h
>> +++ b/include/kgdb.h
>> @@ -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__ */
>> --
>> 1.8.4.5
>>
>
> Regards,
> Simon
>
Thanks for reviewing.
Regards,
Peng.
More information about the U-Boot
mailing list