[PATCH v4 0/4] AArch64: KGDB support

[PATCH v4 0/4] AArch64: KGDB support

[vijay.kilari at gmail.com]

From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

Based on the step-handler and break-handler hooks patch from
Sandeepa (Patch 1), KGDB debugging support is added for EL1
debug in AArch64 mode. Any updates that come for Patch 1 from
Sandeepa will be rebased in next version

With second patch,register layout is updated to be inline with GDB tool.
Basic GDB connection, break point set/clear and info commands
are supported except step/next debugging

With third patch, step/next debugging support is added, where in
pc is updated to point to the instruction to be stepped and
stopped.

with fourth patch, the compile time breakpoint instruction
reordering is fixed by making kgbd_breakpoint() as noinline

v4:
 - Updated kgdb_single_step and kgdb_cpu_doing_single_step
   variables properly based on gdb state

v3:
 - Rebased to v4 version of Sandeepa Prabhu's patch (patch 1)
 - Made dynamic break point instruction encoding generic
 - Made ESR value encoding generic for dynamic and compile break point
 - Used memcpy and memset to copy register contents to gdb buffer
 - Fixed reordering of break point instruction by compiler with
   patch 3
 - Rebased against AAach64 upstream kernel

v2:
 - Moved break instruction encoding to debug-monitors.h file
 - Fixed endianess of compile break instruction encoding
 - Updated I/O buffer sizes
 - Updated register buffer size
 - Remove changes to debug_exception handler in entry.S for
 - ELR update and step debugging with update pc instead of ELR
 - Rebased against AArch64 upstream kernel

v1:
 - Initial patch-set

Sandeepa Prabhu (1):
  arm64: support single-step and breakpoint handler hooks

Vijaya Kumar K (3):
  AArch64: KGDB: Add Basic KGDB support
  AArch64: KGDB: Add step debugging support
  KGDB: make kgdb_breakpoint() as noinline

 arch/arm64/include/asm/debug-monitors.h |   67 ++++++
 arch/arm64/include/asm/kgdb.h           |   86 ++++++++
 arch/arm64/kernel/Makefile              |    1 +
 arch/arm64/kernel/debug-monitors.c      |   86 +++++++-
 arch/arm64/kernel/entry.S               |    2 +
 arch/arm64/kernel/kgdb.c                |  338 +++++++++++++++++++++++++++++++
 kernel/debug/debug_core.c               |    2 +-
 7 files changed, 580 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm64/include/asm/kgdb.h
 create mode 100644 arch/arm64/kernel/kgdb.c

-- 
1.7.9.5

[PATCH v4 1/4] arm64: support single-step and breakpoint handler hooks

[vijay.kilari at gmail.com]

From: Sandeepa Prabhu <sandeepa.prabhu@linaro.org>

AArch64 Single Steping and Breakpoint debug exceptions will be
used by multiple debug framworks like kprobes & kgdb.

This patch implements the hooks for those frameworks to register
their own handlers for handling breakpoint and single step events.

Reworked the debug exception handler in entry.S: do_dbg to route
software breakpoint (BRK64) exception to do_debug_exception()

Signed-off-by: Sandeepa Prabhu <sandeepa.prabhu@linaro.org>
Signed-off-by: Deepak Saxena <dsaxena@linaro.org>
---
 arch/arm64/include/asm/debug-monitors.h |   21 ++++++++
 arch/arm64/kernel/debug-monitors.c      |   86 ++++++++++++++++++++++++++++++-
 arch/arm64/kernel/entry.S               |    2 +
 3 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index a2232d0..6231479 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -62,6 +62,27 @@ struct task_struct;
 
 #define DBG_ARCH_ID_RESERVED	0	/* In case of ptrace ABI updates. */
 
+#define DBG_HOOK_HANDLED	0
+#define DBG_HOOK_ERROR		1
+
+struct step_hook {
+	struct list_head node;
+	int (*fn)(struct pt_regs *regs, unsigned int esr);
+};
+
+void register_step_hook(struct step_hook *hook);
+void unregister_step_hook(struct step_hook *hook);
+
+struct break_hook {
+	struct list_head node;
+	u32 esr_val;
+	u32 esr_mask;
+	int (*fn)(struct pt_regs *regs, unsigned int esr);
+};
+
+void register_break_hook(struct break_hook *hook);
+void unregister_break_hook(struct break_hook *hook);
+
 u8 debug_monitors_arch(void);
 
 void enable_debug_monitors(enum debug_el el);
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index cbfacf7..e42ad5f 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -188,6 +188,48 @@ static void clear_regs_spsr_ss(struct pt_regs *regs)
 	regs->pstate = spsr;
 }
 
+/* EL1 Single Step Handler hooks */
+static LIST_HEAD(step_hook);
+DEFINE_RWLOCK(step_hook_lock);
+
+void register_step_hook(struct step_hook *hook)
+{
+	write_lock(&step_hook_lock);
+	list_add(&hook->node, &step_hook);
+	write_unlock(&step_hook_lock);
+}
+
+void unregister_step_hook(struct step_hook *hook)
+{
+	write_lock(&step_hook_lock);
+	list_del(&hook->node);
+	write_unlock(&step_hook_lock);
+}
+
+/*
+ * Call registered single step handers
+ * There is no Syndrome info to check for determining the handler.
+ * So we call all the registered handlers, until the right handler is
+ * found which returns zero.
+ */
+static int call_step_hook(struct pt_regs *regs, unsigned int esr)
+{
+	struct step_hook *hook;
+	int retval = DBG_HOOK_ERROR;
+
+	read_lock(&step_hook_lock);
+
+	list_for_each_entry(hook, &step_hook, node)	{
+		retval = hook->fn(regs, esr);
+		if (retval == DBG_HOOK_HANDLED)
+			break;
+	}
+
+	read_unlock(&step_hook_lock);
+
+	return retval;
+}
+
 static int single_step_handler(unsigned long addr, unsigned int esr,
 			       struct pt_regs *regs)
 {
@@ -215,7 +257,10 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
 		 */
 		user_rewind_single_step(current);
 	} else {
-		/* TODO: route to KGDB */
+		/* call registered single step handlers */
+		if (call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
+			return 0;
+
 		pr_warning("Unexpected kernel single-step exception at EL1\n");
 		/*
 		 * Re-enable stepping since we know that we will be
@@ -227,11 +272,50 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
 	return 0;
 }
 
+
+static LIST_HEAD(break_hook);
+DEFINE_RWLOCK(break_hook_lock);
+
+void register_break_hook(struct break_hook *hook)
+{
+	write_lock(&break_hook_lock);
+	list_add(&hook->node, &break_hook);
+	write_unlock(&break_hook_lock);
+}
+
+void unregister_break_hook(struct break_hook *hook)
+{
+	write_lock(&break_hook_lock);
+	list_del(&hook->node);
+	write_unlock(&break_hook_lock);
+}
+
+static int call_break_hook(struct pt_regs *regs, unsigned int esr)
+{
+	struct break_hook *hook;
+	int (*fn)(struct pt_regs *regs, unsigned int esr) = NULL;
+
+	read_lock(&break_hook_lock);
+	list_for_each_entry(hook, &break_hook, node)
+		if ((esr & hook->esr_mask) == hook->esr_val)
+			fn = hook->fn;
+	read_unlock(&break_hook_lock);
+
+	return fn ? fn(regs, esr) : DBG_HOOK_ERROR;
+}
+
 static int brk_handler(unsigned long addr, unsigned int esr,
 		       struct pt_regs *regs)
 {
 	siginfo_t info;
 
+	/* call the registered breakpoint handler */
+	if (call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
+		return 0;
+
+	pr_warn("unexpected brk exception at %lx, esr=0x%x\n",
+			(long)instruction_pointer(regs), esr);
+
 	if (!user_mode(regs))
 		return -EFAULT;
 
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 3881fd1..7fbc510 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -288,6 +288,8 @@ el1_dbg:
 	/*
 	 * Debug exception handling
 	 */
+	cmp	x24, #ESR_EL1_EC_BRK64		// if BRK64
+	cinc	x24, x24, eq			// set bit '0'
 	tbz	x24, #0, el1_inv		// EL1 only
 	mrs	x0, far_el1
 	mov	x2, sp				// struct pt_regs
-- 
1.7.9.5

[PATCH v4 2/4] AArch64: KGDB: Add Basic KGDB support

[vijay.kilari at gmail.com]

From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

Add KGDB debug support for kernel debugging.
With this patch, basic KGDB debugging is possible.GDB register
layout is updated and GDB tool can establish connection with
target and can set/clear breakpoints.

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
---
 arch/arm64/include/asm/debug-monitors.h |   46 +++++
 arch/arm64/include/asm/kgdb.h           |   86 +++++++++
 arch/arm64/kernel/Makefile              |    1 +
 arch/arm64/kernel/kgdb.c                |  289 +++++++++++++++++++++++++++++++
 4 files changed, 422 insertions(+)

diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 6231479..d00c440 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -26,6 +26,52 @@
 #define DBG_ESR_EVT_HWWP	0x2
 #define DBG_ESR_EVT_BRK		0x6
 
+/*
+ * Break point instruction encoding
+ */
+#define BREAK_INSTR_SIZE		4
+
+/*
+ * ESR values expected for dynamic and compile time BRK instruction
+ */
+#define DBG_ESR_VAL_BRK(x)	(0xf2000000 | ((x) & 0xfffff))
+
+/*
+ * #imm16 values used for BRK instruction generation
+ * 0: for dynamic BRK instruction
+ * 1: for compile time BRK instruction
+ */
+#define KGDB_DYN_DGB_BRK_IMM		0
+#define KDBG_COMPILED_DBG_BRK_IMM	1
+
+/*
+ * BRK instruction encoding
+ * The #imm16 value should be placed at bits[20:5] within BRK ins
+ */
+#define AARCH64_BREAK_MON	0xd4200000
+
+/*
+ * Extract byte from BRK instruction
+ */
+#define KGDB_DYN_DGB_BRK_INS_BYTE(x) \
+	((((AARCH64_BREAK_MON) & 0xffe0001f) >> (x * 8)) & 0xff)
+
+/*
+ * Extract byte from BRK #imm16
+ */
+#define KGBD_DYN_DGB_BRK_IMM_BYTE(x) \
+	(((((KGDB_DYN_DGB_BRK_IMM) & 0xffff) << 5) >> (x * 8)) & 0xff)
+
+#define KGDB_DYN_DGB_BRK_BYTE(x) \
+	(KGDB_DYN_DGB_BRK_INS_BYTE(x) | KGBD_DYN_DGB_BRK_IMM_BYTE(x))
+
+#define  KGDB_DYN_BRK_INS_BYTE0  KGDB_DYN_DGB_BRK_BYTE(0)
+#define  KGDB_DYN_BRK_INS_BYTE1  KGDB_DYN_DGB_BRK_BYTE(1)
+#define  KGDB_DYN_BRK_INS_BYTE2  KGDB_DYN_DGB_BRK_BYTE(2)
+#define  KGDB_DYN_BRK_INS_BYTE3  KGDB_DYN_DGB_BRK_BYTE(3)
+
+#define CACHE_FLUSH_IS_SAFE		1
+
 enum debug_el {
 	DBG_ACTIVE_EL0 = 0,
 	DBG_ACTIVE_EL1,
diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
new file mode 100644
index 0000000..5c9faa9
--- /dev/null
+++ b/arch/arm64/include/asm/kgdb.h
@@ -0,0 +1,86 @@
+/*
+ * AArch64 KGDB support
+ *
+ * Based on arch/arm/include/kgdb.h
+ *
+ * Copyright (C) 2013 Cavium Inc.
+ * Author: Vijaya Kumar K <vijaya.kumar@caviumnetworks.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __ARM_KGDB_H__
+#define __ARM_KGDB_H__
+
+#include <linux/ptrace.h>
+#include <asm/debug-monitors.h>
+
+#ifndef	__ASSEMBLY__
+
+static inline void arch_kgdb_breakpoint(void)
+{
+	asm ("brk %0" :: "I" (KDBG_COMPILED_DBG_BRK_IMM));
+}
+
+extern void kgdb_handle_bus_error(void);
+extern int kgdb_fault_expected;
+
+#endif /* !__ASSEMBLY__ */
+
+/*
+ * gdb is expecting the following registers layout.
+ *
+ * General purpose regs:
+ *     r0-r30: 64 bit
+ *     sp,pc : 64 bit
+ *     pstate  : 32 bit
+ *     Total: 34
+ * FPU regs:
+ *     f0-f31: 128 bit
+ *     Total: 32
+ * Extra regs
+ *     fpsr & fpcr: 32 bit
+ *     Total: 2
+ *
+ */
+
+#define _GP_REGS		34
+#define _FP_REGS		32
+#define _EXTRA_REGS		2
+/*
+ * general purpose registers size in bytes.
+ * pstate is only 4 bytes. subtract 4 bytes
+ */
+#define GP_REG_BYTES		((_GP_REGS * 8) - 4)
+#define DBG_MAX_REG_NUM		(_GP_REGS + _FP_REGS + _EXTRA_REGS)
+
+/*
+ * Size of I/O buffer for gdb packet.
+ * considering to hold all register contents, size is set to 1024
+ */
+
+#define BUFMAX			1024
+
+/*
+ * Number of bytes required for gdb_regs buffer.
+ * _GP_REGS: 8 bytes, _FP_REGS: 16 bytes and _EXTRA_REGS: 4 bytes each
+ * pstate reg is only 4 bytes. subtract 4 from size contributed
+ * by _GP_REGS.
+ * GDB fails to connect for size beyond this with error
+ * "'g' packet reply is too long"
+ */
+
+#define NUMREGBYTES	(((_GP_REGS * 8) - 4) + (_FP_REGS * 16) + \
+			(_EXTRA_REGS * 4))
+
+#endif /* __ASM_KGDB_H__ */
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 7b4b564..69613bb 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -18,6 +18,7 @@ arm64-obj-$(CONFIG_SMP)			+= smp.o smp_spin_table.o smp_psci.o
 arm64-obj-$(CONFIG_HW_PERF_EVENTS)	+= perf_event.o
 arm64-obj-$(CONFIG_HAVE_HW_BREAKPOINT)+= hw_breakpoint.o
 arm64-obj-$(CONFIG_EARLY_PRINTK)	+= early_printk.o
+arm64-obj-$(CONFIG_KGDB)		+= kgdb.o
 
 obj-y					+= $(arm64-obj-y) vdso/
 obj-m					+= $(arm64-obj-m)
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
new file mode 100644
index 0000000..ec624bc
--- /dev/null
+++ b/arch/arm64/kernel/kgdb.c
@@ -0,0 +1,289 @@
+/*
+ * AArch64 KGDB support
+ *
+ * Based on arch/arm/kernel/kgdb.c
+ *
+ * Copyright (C) 2013 Cavium Inc.
+ * Author: Vijaya Kumar K <vijaya.kumar@caviumnetworks.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/irq.h>
+#include <linux/kdebug.h>
+#include <linux/kgdb.h>
+#include <asm/traps.h>
+
+struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = {
+	{ "x0", 8, offsetof(struct pt_regs, regs[0])},
+	{ "x1", 8, offsetof(struct pt_regs, regs[1])},
+	{ "x2", 8, offsetof(struct pt_regs, regs[2])},
+	{ "x3", 8, offsetof(struct pt_regs, regs[3])},
+	{ "x4", 8, offsetof(struct pt_regs, regs[4])},
+	{ "x5", 8, offsetof(struct pt_regs, regs[5])},
+	{ "x6", 8, offsetof(struct pt_regs, regs[6])},
+	{ "x7", 8, offsetof(struct pt_regs, regs[7])},
+	{ "x8", 8, offsetof(struct pt_regs, regs[8])},
+	{ "x9", 8, offsetof(struct pt_regs, regs[9])},
+	{ "x10", 8, offsetof(struct pt_regs, regs[10])},
+	{ "x11", 8, offsetof(struct pt_regs, regs[11])},
+	{ "x12", 8, offsetof(struct pt_regs, regs[12])},
+	{ "x13", 8, offsetof(struct pt_regs, regs[13])},
+	{ "x14", 8, offsetof(struct pt_regs, regs[14])},
+	{ "x15", 8, offsetof(struct pt_regs, regs[15])},
+	{ "x16", 8, offsetof(struct pt_regs, regs[16])},
+	{ "x17", 8, offsetof(struct pt_regs, regs[17])},
+	{ "x18", 8, offsetof(struct pt_regs, regs[18])},
+	{ "x19", 8, offsetof(struct pt_regs, regs[19])},
+	{ "x20", 8, offsetof(struct pt_regs, regs[20])},
+	{ "x21", 8, offsetof(struct pt_regs, regs[21])},
+	{ "x22", 8, offsetof(struct pt_regs, regs[22])},
+	{ "x23", 8, offsetof(struct pt_regs, regs[23])},
+	{ "x24", 8, offsetof(struct pt_regs, regs[24])},
+	{ "x25", 8, offsetof(struct pt_regs, regs[25])},
+	{ "x26", 8, offsetof(struct pt_regs, regs[26])},
+	{ "x27", 8, offsetof(struct pt_regs, regs[27])},
+	{ "x28", 8, offsetof(struct pt_regs, regs[28])},
+	{ "x29", 8, offsetof(struct pt_regs, regs[29])},
+	{ "x30", 8, offsetof(struct pt_regs, regs[30])},
+	{ "sp", 8, offsetof(struct pt_regs, sp)},
+	{ "pc", 8, offsetof(struct pt_regs, pc)},
+	{ "pstate", 4, offsetof(struct pt_regs, pstate)},
+	{ "v0", 16, -1 },
+	{ "v1", 16, -1 },
+	{ "v2", 16, -1 },
+	{ "v3", 16, -1 },
+	{ "v4", 16, -1 },
+	{ "v5", 16, -1 },
+	{ "v6", 16, -1 },
+	{ "v7", 16, -1 },
+	{ "v8", 16, -1 },
+	{ "v9", 16, -1 },
+	{ "v10", 16, -1 },
+	{ "v11", 16, -1 },
+	{ "v12", 16, -1 },
+	{ "v13", 16, -1 },
+	{ "v14", 16, -1 },
+	{ "v15", 16, -1 },
+	{ "v16", 16, -1 },
+	{ "v17", 16, -1 },
+	{ "v18", 16, -1 },
+	{ "v19", 16, -1 },
+	{ "v20", 16, -1 },
+	{ "v21", 16, -1 },
+	{ "v22", 16, -1 },
+	{ "v23", 16, -1 },
+	{ "v24", 16, -1 },
+	{ "v25", 16, -1 },
+	{ "v26", 16, -1 },
+	{ "v27", 16, -1 },
+	{ "v28", 16, -1 },
+	{ "v29", 16, -1 },
+	{ "v30", 16, -1 },
+	{ "v31", 16, -1 },
+	{ "fpsr", 4, -1 },
+	{ "fpcr", 4, -1 },
+};
+
+char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs)
+{
+	if (regno >= DBG_MAX_REG_NUM || regno < 0)
+		return NULL;
+
+	if (dbg_reg_def[regno].offset != -1)
+		memcpy(mem, (void *)regs + dbg_reg_def[regno].offset,
+		       dbg_reg_def[regno].size);
+	else
+		memset(mem, 0, dbg_reg_def[regno].size);
+	return dbg_reg_def[regno].name;
+}
+
+int dbg_set_reg(int regno, void *mem, struct pt_regs *regs)
+{
+	if (regno >= DBG_MAX_REG_NUM || regno < 0)
+		return -EINVAL;
+
+	if (dbg_reg_def[regno].offset != -1)
+		memcpy((void *)regs + dbg_reg_def[regno].offset, mem,
+		       dbg_reg_def[regno].size);
+	return 0;
+}
+
+void
+sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *task)
+{
+	struct pt_regs *thread_regs;
+
+	/* Initialize to zero */
+	memset((char *)gdb_regs, 0, NUMREGBYTES);
+	thread_regs = task_pt_regs(task);
+	memcpy((void *)gdb_regs, (void *)thread_regs->regs, GP_REG_BYTES);
+}
+
+void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc)
+{
+	regs->pc = pc;
+}
+
+static int compiled_break;
+
+int kgdb_arch_handle_exception(int exception_vector, int signo,
+			       int err_code, char *remcom_in_buffer,
+			       char *remcom_out_buffer,
+			       struct pt_regs *linux_regs)
+{
+	unsigned long addr;
+	char *ptr;
+	int err;
+
+	switch (remcom_in_buffer[0]) {
+	case 'D':
+	case 'k':
+		/*
+		 * Packet D (Detach), k (kill). No special handling
+		 * is required here
+		 */
+		err = 0;
+		break;
+	case 'c':
+		/*
+		 * Packet c (Continue) to continue executing.
+		 * Set pc to required address.
+		 * Try to read optional parameter and set pc.
+		 * If this was a compiled breakpoint, we need to move
+		 * to the next instruction else we will just breakpoint
+		 * over and over again.
+		 */
+		ptr = &remcom_in_buffer[1];
+		if (kgdb_hex2long(&ptr, &addr))
+			kgdb_arch_set_pc(linux_regs, addr);
+		else if (compiled_break == 1)
+			kgdb_arch_set_pc(linux_regs, linux_regs->pc + 4);
+
+		compiled_break = 0;
+
+		err = 0;
+		break;
+	default:
+		err = -1;
+	}
+	return err;
+}
+
+static int kgdb_brk_fn(struct pt_regs *regs, unsigned int esr)
+{
+	kgdb_handle_exception(1, SIGTRAP, 0, regs);
+	return 0;
+}
+
+static int kgdb_compiled_brk_fn(struct pt_regs *regs, unsigned int esr)
+{
+	compiled_break = 1;
+	kgdb_handle_exception(1, SIGTRAP, 0, regs);
+
+	return 0;
+}
+
+static struct break_hook kgdb_brkpt_hook = {
+	.esr_mask	= 0xffffffff,
+	.esr_val	= DBG_ESR_VAL_BRK(KGDB_DYN_DGB_BRK_IMM),
+	.fn		= kgdb_brk_fn
+};
+
+static struct break_hook kgdb_compiled_brkpt_hook = {
+	.esr_mask	= 0xffffffff,
+	.esr_val	= DBG_ESR_VAL_BRK(KDBG_COMPILED_DBG_BRK_IMM),
+	.fn		= kgdb_compiled_brk_fn
+};
+
+static void kgdb_call_nmi_hook(void *ignored)
+{
+	kgdb_nmicallback(raw_smp_processor_id(), get_irq_regs());
+}
+
+void kgdb_roundup_cpus(unsigned long flags)
+{
+	local_irq_enable();
+	smp_call_function(kgdb_call_nmi_hook, NULL, 0);
+	local_irq_disable();
+}
+
+static int __kgdb_notify(struct die_args *args, unsigned long cmd)
+{
+	struct pt_regs *regs = args->regs;
+
+	if (kgdb_handle_exception(1, args->signr, cmd, regs))
+		return NOTIFY_DONE;
+	return NOTIFY_STOP;
+}
+
+static int
+kgdb_notify(struct notifier_block *self, unsigned long cmd, void *ptr)
+{
+	unsigned long flags;
+	int ret;
+
+	local_irq_save(flags);
+	ret = __kgdb_notify(ptr, cmd);
+	local_irq_restore(flags);
+
+	return ret;
+}
+
+static struct notifier_block kgdb_notifier = {
+	.notifier_call	= kgdb_notify,
+	.priority	= -INT_MAX,
+};
+
+/*
+ * kgdb_arch_init - Perform any architecture specific initalization.
+ * This function will handle the initalization of any architecture
+ * specific callbacks.
+ */
+int kgdb_arch_init(void)
+{
+	int ret = register_die_notifier(&kgdb_notifier);
+
+	if (ret != 0)
+		return ret;
+
+	register_break_hook(&kgdb_brkpt_hook);
+	register_break_hook(&kgdb_compiled_brkpt_hook);
+
+	return 0;
+}
+
+/*
+ * kgdb_arch_exit - Perform any architecture specific uninitalization.
+ * This function will handle the uninitalization of any architecture
+ * specific callbacks, for dynamic registration and unregistration.
+ */
+void kgdb_arch_exit(void)
+{
+	unregister_break_hook(&kgdb_brkpt_hook);
+	unregister_break_hook(&kgdb_compiled_brkpt_hook);
+	unregister_die_notifier(&kgdb_notifier);
+}
+
+/*
+ * ARM instructions are always in LE.
+ * Break instruction is encoded in LE format
+ */
+struct kgdb_arch arch_kgdb_ops = {
+	.gdb_bpt_instr = {
+		KGDB_DYN_BRK_INS_BYTE0,
+		KGDB_DYN_BRK_INS_BYTE1,
+		KGDB_DYN_BRK_INS_BYTE2,
+		KGDB_DYN_BRK_INS_BYTE3,
+	}
+};
-- 
1.7.9.5

[PATCH v4 3/4] AArch64: KGDB: Add step debugging support

[vijay.kilari at gmail.com]

From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

Add KGDB software step debugging support for EL1 debug
in AArch64 mode.

KGDB registers step debug handler with debug monitor.
On receiving 'step' command from GDB tool, target enables
software step debugging and step address is updated.
If no step address is received from GDB tool, target assumes
next step address is current PC.

Software Step debugging is disabled when 'continue' command
is received

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
---
 arch/arm64/kernel/kgdb.c |   67 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 58 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index ec624bc..f10f2ba 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -137,13 +137,26 @@ void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc)
 
 static int compiled_break;
 
+static void kgdb_arch_update_addr(struct pt_regs *regs,
+				char *remcom_in_buffer)
+{
+	unsigned long addr;
+	char *ptr;
+
+	ptr = &remcom_in_buffer[1];
+	if (kgdb_hex2long(&ptr, &addr))
+		kgdb_arch_set_pc(regs, addr);
+	else if (compiled_break == 1)
+		kgdb_arch_set_pc(regs, regs->pc + 4);
+
+	compiled_break = 0;
+}
+
 int kgdb_arch_handle_exception(int exception_vector, int signo,
 			       int err_code, char *remcom_in_buffer,
 			       char *remcom_out_buffer,
 			       struct pt_regs *linux_regs)
 {
-	unsigned long addr;
-	char *ptr;
 	int err;
 
 	switch (remcom_in_buffer[0]) {
@@ -153,6 +166,8 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
 		 * Packet D (Detach), k (kill). No special handling
 		 * is required here
 		 */
+		atomic_set(&kgdb_cpu_doing_single_step, -1);
+		kgdb_single_step =  0;
 		err = 0;
 		break;
 	case 'c':
@@ -164,16 +179,39 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
 		 * to the next instruction else we will just breakpoint
 		 * over and over again.
 		 */
-		ptr = &remcom_in_buffer[1];
-		if (kgdb_hex2long(&ptr, &addr))
-			kgdb_arch_set_pc(linux_regs, addr);
-		else if (compiled_break == 1)
-			kgdb_arch_set_pc(linux_regs, linux_regs->pc + 4);
+		kgdb_arch_update_addr(linux_regs, remcom_in_buffer);
+		atomic_set(&kgdb_cpu_doing_single_step, -1);
+		kgdb_single_step =  0;
 
-		compiled_break = 0;
+		/*
+		 * Received continue command, disable single step
+		 */
+		if (kernel_active_single_step())
+			kernel_disable_single_step();
 
 		err = 0;
 		break;
+	case 's':
+		/*
+		 * Update step address value with address passed
+		 * with step packet.
+		 * On debug exception return PC is copied to ELR
+		 * So just update PC.
+		 * If no step address is passed, resume from the address
+		 * pointed by PC. Do not update PC
+		 */
+		kgdb_arch_update_addr(linux_regs, remcom_in_buffer);
+		atomic_set(&kgdb_cpu_doing_single_step, raw_smp_processor_id());
+		kgdb_single_step =  1;
+
+		/*
+		 * Enable single step handling
+		 */
+		if (!kernel_active_single_step())
+			kernel_enable_single_step(linux_regs);
+		err = 0;
+		break;
+
 	default:
 		err = -1;
 	}
@@ -194,6 +232,12 @@ static int kgdb_compiled_brk_fn(struct pt_regs *regs, unsigned int esr)
 	return 0;
 }
 
+static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr)
+{
+	kgdb_handle_exception(1, SIGTRAP, 0, regs);
+	return 0;
+}
+
 static struct break_hook kgdb_brkpt_hook = {
 	.esr_mask	= 0xffffffff,
 	.esr_val	= DBG_ESR_VAL_BRK(KGDB_DYN_DGB_BRK_IMM),
@@ -206,6 +250,10 @@ static struct break_hook kgdb_compiled_brkpt_hook = {
 	.fn		= kgdb_compiled_brk_fn
 };
 
+static struct step_hook kgdb_step_hook = {
+	.fn		= kgdb_step_brk_fn
+};
+
 static void kgdb_call_nmi_hook(void *ignored)
 {
 	kgdb_nmicallback(raw_smp_processor_id(), get_irq_regs());
@@ -259,7 +307,7 @@ int kgdb_arch_init(void)
 
 	register_break_hook(&kgdb_brkpt_hook);
 	register_break_hook(&kgdb_compiled_brkpt_hook);
-
+	register_step_hook(&kgdb_step_hook);
 	return 0;
 }
 
@@ -272,6 +320,7 @@ void kgdb_arch_exit(void)
 {
 	unregister_break_hook(&kgdb_brkpt_hook);
 	unregister_break_hook(&kgdb_compiled_brkpt_hook);
+	unregister_step_hook(&kgdb_step_hook);
 	unregister_die_notifier(&kgdb_notifier);
 }
 
-- 
1.7.9.5

[PATCH v4 4/4] KGDB: make kgdb_breakpoint() as noinline

[vijay.kilari at gmail.com]

From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

The function kgdb_breakpoint() sets up break point at
compile time by calling arch_kgdb_breakpoint();
Though this call is surrounded by wmb() barrier,
the compile can still re-order the break point,
because this scheduling barrier is not a code motion
barrier in gcc.

Making kgdb_breakpoint() as noinline solves this problem
of code reording around break point instruction and also
avoids problem of being called as inline function from
other places

More details about discussion on this can be found here
http://comments.gmane.org/gmane.linux.ports.arm.kernel/269732

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
---
 kernel/debug/debug_core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index 0506d44..359ce6a 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -1006,7 +1006,7 @@ int dbg_io_get_char(void)
  * otherwise as a quick means to stop program execution and "break" into
  * the debugger.
  */
-void kgdb_breakpoint(void)
+noinline void kgdb_breakpoint(void)
 {
 	atomic_inc(&kgdb_setting_breakpoint);
 	wmb(); /* Sync point before breakpoint */
-- 
1.7.9.5

[PATCH v4 4/4] KGDB: make kgdb_breakpoint() as noinline

[Will Deacon]

On Tue, Nov 05, 2013 at 08:45:46AM +0000, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> The function kgdb_breakpoint() sets up break point at
> compile time by calling arch_kgdb_breakpoint();
> Though this call is surrounded by wmb() barrier,
> the compile can still re-order the break point,
> because this scheduling barrier is not a code motion
> barrier in gcc.
> 
> Making kgdb_breakpoint() as noinline solves this problem
> of code reording around break point instruction and also
> avoids problem of being called as inline function from
> other places
> 
> More details about discussion on this can be found here
> http://comments.gmane.org/gmane.linux.ports.arm.kernel/269732
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

I thought I already acked this?

Will

[PATCH v4 2/4] AArch64: KGDB: Add Basic KGDB support

[Will Deacon]

On Tue, Nov 05, 2013 at 08:45:44AM +0000, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Add KGDB debug support for kernel debugging.
> With this patch, basic KGDB debugging is possible.GDB register
> layout is updated and GDB tool can establish connection with
> target and can set/clear breakpoints.

  Reviewed-by: Will Deacon <will.deacon@arm.com>

Will

[PATCH v4 3/4] AArch64: KGDB: Add step debugging support

[Will Deacon]

On Tue, Nov 05, 2013 at 08:45:45AM +0000, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Add KGDB software step debugging support for EL1 debug
> in AArch64 mode.
> 
> KGDB registers step debug handler with debug monitor.
> On receiving 'step' command from GDB tool, target enables
> software step debugging and step address is updated.
> If no step address is received from GDB tool, target assumes
> next step address is current PC.
> 
> Software Step debugging is disabled when 'continue' command
> is received
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  arch/arm64/kernel/kgdb.c |   67 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 58 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index ec624bc..f10f2ba 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -137,13 +137,26 @@ void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc)
>  
>  static int compiled_break;
>  
> +static void kgdb_arch_update_addr(struct pt_regs *regs,
> +				char *remcom_in_buffer)
> +{
> +	unsigned long addr;
> +	char *ptr;
> +
> +	ptr = &remcom_in_buffer[1];
> +	if (kgdb_hex2long(&ptr, &addr))
> +		kgdb_arch_set_pc(regs, addr);
> +	else if (compiled_break == 1)
> +		kgdb_arch_set_pc(regs, regs->pc + 4);
> +
> +	compiled_break = 0;
> +}
> +
>  int kgdb_arch_handle_exception(int exception_vector, int signo,
>  			       int err_code, char *remcom_in_buffer,
>  			       char *remcom_out_buffer,
>  			       struct pt_regs *linux_regs)
>  {
> -	unsigned long addr;
> -	char *ptr;
>  	int err;
>  
>  	switch (remcom_in_buffer[0]) {
> @@ -153,6 +166,8 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
>  		 * Packet D (Detach), k (kill). No special handling
>  		 * is required here
>  		 */
> +		atomic_set(&kgdb_cpu_doing_single_step, -1);
> +		kgdb_single_step =  0;

This looks really weird: you have two variables, which you attempt to keep
in sync, only one of them is an atomic access and you don't have any
locking or memory barriers between them, so it must be ok if they're not
in-sync. In which case, why do we have two variables?

Will

[PATCH v4 3/4] AArch64: KGDB: Add step debugging support

[Vijay Kilari]

On Fri, Nov 8, 2013 at 7:48 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Nov 05, 2013 at 08:45:45AM +0000, vijay.kilari at gmail.com wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>
>> Add KGDB software step debugging support for EL1 debug
>> in AArch64 mode.
>>
>> KGDB registers step debug handler with debug monitor.
>> On receiving 'step' command from GDB tool, target enables
>> software step debugging and step address is updated.
>> If no step address is received from GDB tool, target assumes
>> next step address is current PC.
>>
>> Software Step debugging is disabled when 'continue' command
>> is received
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>> ---
>>  arch/arm64/kernel/kgdb.c |   67 +++++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 58 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
>> index ec624bc..f10f2ba 100644
>> --- a/arch/arm64/kernel/kgdb.c
>> +++ b/arch/arm64/kernel/kgdb.c
>> @@ -137,13 +137,26 @@ void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc)
>>
>>  static int compiled_break;
>>
>> +static void kgdb_arch_update_addr(struct pt_regs *regs,
>> +                             char *remcom_in_buffer)
>> +{
>> +     unsigned long addr;
>> +     char *ptr;
>> +
>> +     ptr = &remcom_in_buffer[1];
>> +     if (kgdb_hex2long(&ptr, &addr))
>> +             kgdb_arch_set_pc(regs, addr);
>> +     else if (compiled_break == 1)
>> +             kgdb_arch_set_pc(regs, regs->pc + 4);
>> +
>> +     compiled_break = 0;
>> +}
>> +
>>  int kgdb_arch_handle_exception(int exception_vector, int signo,
>>                              int err_code, char *remcom_in_buffer,
>>                              char *remcom_out_buffer,
>>                              struct pt_regs *linux_regs)
>>  {
>> -     unsigned long addr;
>> -     char *ptr;
>>       int err;
>>
>>       switch (remcom_in_buffer[0]) {
>> @@ -153,6 +166,8 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
>>                * Packet D (Detach), k (kill). No special handling
>>                * is required here
>>                */
>> +             atomic_set(&kgdb_cpu_doing_single_step, -1);
>> +             kgdb_single_step =  0;
>
> This looks really weird: you have two variables, which you attempt to keep
> in sync, only one of them is an atomic access and you don't have any
> locking or memory barriers between them, so it must be ok if they're not
> in-sync. In which case, why do we have two variables?
>
IMHO, two variables are being used by kgdb framework
The variables 'kgdb_cpu_doing_single_step'  holds the cpu on which
step debugging
is happening. With this, when during step debugging, it ensures that on next
step debug exception only this CPU aquires master lock and step
debugging is performed.

The variable 'kgdb_single_step' is used to know if step debugging is
ongoing or not.
If so, the non-primary cpu's are not released untill the continue
command is received
So, with this the cpu's are not released and acquired for every step command

> Will

[PATCH v4 3/4] AArch64: KGDB: Add step debugging support

[Will Deacon]

On Mon, Nov 11, 2013 at 11:09:46AM +0000, Vijay Kilari wrote:
> On Fri, Nov 8, 2013 at 7:48 PM, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Nov 05, 2013 at 08:45:45AM +0000, vijay.kilari at gmail.com wrote:
> >> @@ -153,6 +166,8 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
> >>                * Packet D (Detach), k (kill). No special handling
> >>                * is required here
> >>                */
> >> +             atomic_set(&kgdb_cpu_doing_single_step, -1);
> >> +             kgdb_single_step =  0;
> >
> > This looks really weird: you have two variables, which you attempt to keep
> > in sync, only one of them is an atomic access and you don't have any
> > locking or memory barriers between them, so it must be ok if they're not
> > in-sync. In which case, why do we have two variables?
> >
> IMHO, two variables are being used by kgdb framework
> The variables 'kgdb_cpu_doing_single_step'  holds the cpu on which
> step debugging
> is happening. With this, when during step debugging, it ensures that on next
> step debug exception only this CPU aquires master lock and step
> debugging is performed.
> 
> The variable 'kgdb_single_step' is used to know if step debugging is
> ongoing or not.
> If so, the non-primary cpu's are not released untill the continue
> command is received
> So, with this the cpu's are not released and acquired for every step command

Ok, but in what situation would you have both kgdb_cpu_doing_single_step == -1
and kgdb_single_step == 1 (ignoring races between setting the two variables)?

In other words, can we reduce this to a single variable?

Will

[PATCH v4 3/4] AArch64: KGDB: Add step debugging support

[Vijay Kilari]

On Mon, Nov 11, 2013 at 8:30 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Mon, Nov 11, 2013 at 11:09:46AM +0000, Vijay Kilari wrote:
>> On Fri, Nov 8, 2013 at 7:48 PM, Will Deacon <will.deacon@arm.com> wrote:
>> > On Tue, Nov 05, 2013 at 08:45:45AM +0000, vijay.kilari at gmail.com wrote:
>> >> @@ -153,6 +166,8 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
>> >>                * Packet D (Detach), k (kill). No special handling
>> >>                * is required here
>> >>                */
>> >> +             atomic_set(&kgdb_cpu_doing_single_step, -1);
>> >> +             kgdb_single_step =  0;
>> >
>> > This looks really weird: you have two variables, which you attempt to keep
>> > in sync, only one of them is an atomic access and you don't have any
>> > locking or memory barriers between them, so it must be ok if they're not
>> > in-sync. In which case, why do we have two variables?
>> >
>> IMHO, two variables are being used by kgdb framework
>> The variables 'kgdb_cpu_doing_single_step'  holds the cpu on which
>> step debugging
>> is happening. With this, when during step debugging, it ensures that on next
>> step debug exception only this CPU aquires master lock and step
>> debugging is performed.
>>
>> The variable 'kgdb_single_step' is used to know if step debugging is
>> ongoing or not.
>> If so, the non-primary cpu's are not released untill the continue
>> command is received
>> So, with this the cpu's are not released and acquired for every step command
>
> Ok, but in what situation would you have both kgdb_cpu_doing_single_step == -1
> and kgdb_single_step == 1 (ignoring races between setting the two variables)?
>
> In other words, can we reduce this to a single variable?
>
More elaborately,
The kgdb_cpu_doing_single_step holds the cpu number which is used to
store the task information that is currently being step debugged in the
corresponding cpu structure. so we cannot get rid of this variable.
Also this variable tells which cpu should take the exception for step debugging
so it waits for this paricular cpu to get the chance to take exception in case
if exception has been taken by other cpu.

The kgdb_single_step variable, it only tells if cpu's that don't get
debug exception
should be released & acquired for every step debug or not

So, both variables are used for different purpose. we can make an attempt to
make to use one variable, but I think, the flexibility is lost.
Moreover, I see that these variables are declared in generic kgdb code and
different architectures use this based on their requirement.

The race will not happen because, only one cpu will be executing this
function at
any point of the time and all other cpu's will be spinning.

> Will