Re: [U-Boot] [RFC PATCH v1 1/2] kgdb: add breakpoint support

Hi Peng,

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

Add Z/z protocal support for breakpoint set/remove.

Signed-off-by: Peng Fan Peng.Fan@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

#undef KGDB_DEBUG

Where is this used?

/* @@ -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 */
  

?

•   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?

+{

•   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?

+{

•   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. */

•   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