[PATCH v6 0/3] AArch64: KGDB support

[PATCH v6 0/3] 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, 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

Tested with ARM64 simulator

v6:
 - Change pstate register to 8 bytes to make endian nuetral.
   Use GDB below GDB patch to display pstate in Big endian mode.
   https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html
   Thanks to Andrew.

v5:
 - Updated BRK #imm16 value to 0x400 & 0x401 as per recommendation
   as per Marcus recommendataion
   http://patchwork.ozlabs.org/patch/290801/
 - Rebased to 3.13 AArch64 kernel

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

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 |   47 +++++
 arch/arm64/include/asm/kgdb.h           |   86 ++++++++
 arch/arm64/kernel/Makefile              |    1 +
 arch/arm64/kernel/kgdb.c                |  341 +++++++++++++++++++++++++++++++
 kernel/debug/debug_core.c               |    2 +-
 5 files changed, 476 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/include/asm/kgdb.h
 create mode 100644 arch/arm64/kernel/kgdb.c

-- 
1.7.9.5

[PATCH v6 1/3] 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>
Reviewed-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/include/asm/debug-monitors.h |   47 +++++
 arch/arm64/include/asm/kgdb.h           |   86 +++++++++
 arch/arm64/kernel/Makefile              |    1 +
 arch/arm64/kernel/kgdb.c                |  292 +++++++++++++++++++++++++++++++
 4 files changed, 426 insertions(+)

diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 6231479..6926497 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -26,6 +26,53 @@
 #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
+ * Allowed values for kgbd are 0x400 - 0x7ff
+ * 0x400: for dynamic BRK instruction
+ * 0x401: for compile time BRK instruction
+ */
+#define KGDB_DYN_DGB_BRK_IMM		0x400
+#define KDBG_COMPILED_DBG_BRK_IMM	0x401
+
+/*
+ * 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 5ba2fd4..b9b87fa 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
 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..8530584
--- /dev/null
+++ b/arch/arm64/kernel/kgdb.c
@@ -0,0 +1,292 @@
+/*
+ * 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", 8, 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,
+	/*
+	 * Want to be lowest priority
+	 */
+	.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 v6 1/3] AArch64: KGDB: Add Basic KGDB support

[Catalin Marinas]

On Thu, Dec 19, 2013 at 11:50:49AM +0000, vijay.kilari at gmail.com wrote:
> --- /dev/null
> +++ b/arch/arm64/include/asm/kgdb.h
[...]
> +#ifndef __ARM_KGDB_H__
> +#define __ARM_KGDB_H__
[...]
> +#endif /* __ASM_KGDB_H__ */

Nitpick (while you are addressing the other issues): please change
header guards to __ASM_KGDB_H for consistency with most of the other
files.

Thanks.

-- 
Catalin

[PATCH v6 2/3] 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 in ELR.

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

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
Reviewed-by: Will Deacon <will.deacon@arm.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 8530584..aeef75b 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());
@@ -262,7 +310,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;
 }
 
@@ -275,6 +323,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 v6 3/3] 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>
Acked-by: Will Deacon <will.deacon@arm.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 7d2f35e..cf04798 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -1034,7 +1034,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 v6 3/3] KGDB: make kgdb_breakpoint() as noinline

[Jason Wessel]

On 12/19/2013 05:50 AM, 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>
> Acked-by: Will Deacon <will.deacon@arm.com>

Acked-by: Jason Wessel <jason.wessel@windriver.com>


Cheers,
Jason.

> ---
>  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 7d2f35e..cf04798 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -1034,7 +1034,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 */

[PATCH v6 3/3] KGDB: make kgdb_breakpoint() as noinline

[Will Deacon]

On Wed, Jan 15, 2014 at 04:44:27PM +0000, Jason Wessel wrote:
> On 12/19/2013 05:50 AM, 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>
> > Acked-by: Will Deacon <will.deacon@arm.com>
> 
> Acked-by: Jason Wessel <jason.wessel@windriver.com>

Thanks Jason! We'll take this series via the arm64 tree.

Will

[PATCH v6 0/3] AArch64: KGDB support

[Will Deacon]

Hi Vijay,

On Thu, Dec 19, 2013 at 11:50:48AM +0000, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Based on the step-handler and break-handler hooks patch from
> Sandeepa, 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
> 
> Tested with ARM64 simulator
> 
> v6:
>  - Change pstate register to 8 bytes to make endian nuetral.
>    Use GDB below GDB patch to display pstate in Big endian mode.
>    https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html
>    Thanks to Andrew.

Thanks for getting this new version out. Just a few things before it can be
applied:

  1. Can you please use "arm64:" instead of "AArch64:" in the patch
     subjects?

  2. Can you get an ack from somebody like Jason on the final patch please?

  3. Can you rebase onto the for-next/core branch at the arm64 repo:

       git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git

     you'll get a trivial conflict against arch/arm64/kernel/Makefile.

  4. Can you make sure that the gdb patch you linked to actually gets
     merged? There is still some discussion by the looks of it.

Cheers,

Will

[PATCH v6 0/3] AArch64: KGDB support

[Catalin Marinas]

On Mon, Jan 06, 2014 at 06:12:01PM +0000, Will Deacon wrote:
> On Thu, Dec 19, 2013 at 11:50:48AM +0000, vijay.kilari at gmail.com wrote:
> > From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> > 
> > Based on the step-handler and break-handler hooks patch from
> > Sandeepa, 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
> > 
> > Tested with ARM64 simulator
> > 
> > v6:
> >  - Change pstate register to 8 bytes to make endian nuetral.
> >    Use GDB below GDB patch to display pstate in Big endian mode.
> >    https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html
> >    Thanks to Andrew.
> 
> Thanks for getting this new version out. Just a few things before it can be
> applied:
> 
>   1. Can you please use "arm64:" instead of "AArch64:" in the patch
>      subjects?
> 
>   2. Can you get an ack from somebody like Jason on the final patch please?
> 
>   3. Can you rebase onto the for-next/core branch at the arm64 repo:
> 
>        git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git
> 
>      you'll get a trivial conflict against arch/arm64/kernel/Makefile.

Just to be clear here, I would only recommend rebasing on top of this
branch if there are patches queued for upstream which are required by
the rebased patches and there isn't a topic branch already elsewhere. I
do *not* recommend this branch for any kind of development (though
testing is fine and we can fix the trivial Makefile conflicts
ourselves).

>   4. Can you make sure that the gdb patch you linked to actually gets
>      merged? There is still some discussion by the looks of it.

I guess the kernel patches will have to wait for the binutils patch to
go in. BTW, does this mean that kgdb on arm64 will only work with a
future version of gdb?

-- 
Catalin

[PATCH v6 0/3] AArch64: KGDB support

[Vijay Kilari]

On Tue, Jan 7, 2014 at 5:41 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Mon, Jan 06, 2014 at 06:12:01PM +0000, Will Deacon wrote:
>> On Thu, Dec 19, 2013 at 11:50:48AM +0000, vijay.kilari at gmail.com wrote:
>> > From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>> >
>> > Based on the step-handler and break-handler hooks patch from
>> > Sandeepa, 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
>> >
>> > Tested with ARM64 simulator
>> >
>> > v6:
>> >  - Change pstate register to 8 bytes to make endian nuetral.
>> >    Use GDB below GDB patch to display pstate in Big endian mode.
>> >    https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html
>> >    Thanks to Andrew.
>>
>> Thanks for getting this new version out. Just a few things before it can be
>> applied:
>>
>>   1. Can you please use "arm64:" instead of "AArch64:" in the patch
>>      subjects?
>>
      OK.

>>   2. Can you get an ack from somebody like Jason on the final patch please?
>>
      I have not received any ack from Jason.

>>   3. Can you rebase onto the for-next/core branch at the arm64 repo:
>>
>>        git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git
>>
>>      you'll get a trivial conflict against arch/arm64/kernel/Makefile.
>
> Just to be clear here, I would only recommend rebasing on top of this
> branch if there are patches queued for upstream which are required by
> the rebased patches and there isn't a topic branch already elsewhere. I
> do *not* recommend this branch for any kind of development (though
> testing is fine and we can fix the trivial Makefile conflicts
> ourselves).

OK. I will rebased on top of that branch and resend the patches soon.

>
>>   4. Can you make sure that the gdb patch you linked to actually gets
>>      merged? There is still some discussion by the looks of it.
>
> I guess the kernel patches will have to wait for the binutils patch to
> go in. BTW, does this mean that kgdb on arm64 will only work with a
> future version of gdb?
>
KGDB works with current gdb version as well, except that it only fails
to display
cpsr reg value if kernel is built in BE mode. In LE it works fine.

> --
> Catalin

[PATCH v6 0/3] AArch64: KGDB support

[Andrew Pinski]

On Mon, Jan 6, 2014 at 10:12 AM, Will Deacon <will.deacon@arm.com> wrote:
> Hi Vijay,
>
> On Thu, Dec 19, 2013 at 11:50:48AM +0000, vijay.kilari at gmail.com wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>
>> Based on the step-handler and break-handler hooks patch from
>> Sandeepa, 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
>>
>> Tested with ARM64 simulator
>>
>> v6:
>>  - Change pstate register to 8 bytes to make endian nuetral.
>>    Use GDB below GDB patch to display pstate in Big endian mode.
>>    https://sourceware.org/ml/gdb-patches/2013-12/msg00720.html
>>    Thanks to Andrew.
>
> Thanks for getting this new version out. Just a few things before it can be
> applied:
>
>   1. Can you please use "arm64:" instead of "AArch64:" in the patch
>      subjects?
>
>   2. Can you get an ack from somebody like Jason on the final patch please?
>
>   3. Can you rebase onto the for-next/core branch at the arm64 repo:
>
>        git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git
>
>      you'll get a trivial conflict against arch/arm64/kernel/Makefile.
>
>   4. Can you make sure that the gdb patch you linked to actually gets
>      merged? There is still some discussion by the looks of it.


I merged it last week:
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=a4d9ba85ec5597a6a556afe26b712e878374b9dd
.

Thanks,
Andrew Pinski

>
> Cheers,
>
> Will

[PATCH v6 0/3] AArch64: KGDB support

[Catalin Marinas]

On Thu, Dec 19, 2013 at 11:50:48AM +0000, vijay.kilari at gmail.com wrote:
> Vijaya Kumar K (3):
>   AArch64: KGDB: Add Basic KGDB support
>   AArch64: KGDB: Add step debugging support
>   KGDB: make kgdb_breakpoint() as noinline

I gave them a try and now I have some doubts about your testing
methodology:

1. There is no HAVE_ARCH_KGDB anywhere in these patches, so
the code here isn't compiled.

2. There are several compiler warnings that need to be addressed.

3. I enabled CONFIG_KGDB_TESTS and CONFIG_KGDB_TESTS_ON_BOOT and that's
what really scared me.

Please address these before we can consider the patches for merging.

-- 
Catalin

[PATCH v6 0/3] AArch64: KGDB support

[Vijay Kilari]

Hi Catalin,

On Thu, Jan 16, 2014 at 7:15 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Thu, Dec 19, 2013 at 11:50:48AM +0000, vijay.kilari at gmail.com wrote:
>> Vijaya Kumar K (3):
>>   AArch64: KGDB: Add Basic KGDB support
>>   AArch64: KGDB: Add step debugging support
>>   KGDB: make kgdb_breakpoint() as noinline
>
> I gave them a try and now I have some doubts about your testing
> methodology:
>
> 1. There is no HAVE_ARCH_KGDB anywhere in these patches, so
> the code here isn't compiled.
>
OK, I will send a patch

> 2. There are several compiler warnings that need to be addressed.
>
> 3. I enabled CONFIG_KGDB_TESTS and CONFIG_KGDB_TESTS_ON_BOOT and that's
> what really scared me.
>

Are you refering to these warning?. I don't see any warning in the
patched files. I will fix these.

drivers/misc/kgdbts.c: In function 'check_single_step':
drivers/misc/kgdbts.c:353:2: warning: format '%lx' expects argument of
type 'long unsigned int', but argument 2 has type 'u64' [-Wformat]
drivers/misc/kgdbts.c:379:3: warning: format '%lx' expects argument of
type 'long unsigned int', but argument 2 has type 'u64' [-Wformat]
drivers/misc/kgdbts.c: In function 'emul_sstep_put':
drivers/misc/kgdbts.c:494:3: warning: format '%lx' expects argument of
type 'long unsigned int', but argument 2 has type 'u64' [-Wformat]
kernel/debug/debug_core.c: In function 'kgdb_cpu_enter':
kernel/debug/debug_core.c:498:4: warning: value computed is not used
[-Wunused-value]
kernel/debug/debug_core.c:513:5: warning: value computed is not used
[-Wunused-value]

> Please address these before we can consider the patches for merging.
>
> --
> Catalin

[PATCH v6 0/3] AArch64: KGDB support

[Will Deacon]

On Sat, Jan 18, 2014 at 08:02:21AM +0000, Vijay Kilari wrote:
> Hi Catalin,
> 
> On Thu, Jan 16, 2014 at 7:15 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Thu, Dec 19, 2013 at 11:50:48AM +0000, vijay.kilari at gmail.com wrote:
> >> Vijaya Kumar K (3):
> >>   AArch64: KGDB: Add Basic KGDB support
> >>   AArch64: KGDB: Add step debugging support
> >>   KGDB: make kgdb_breakpoint() as noinline
> >
> > I gave them a try and now I have some doubts about your testing
> > methodology:
> >
> > 1. There is no HAVE_ARCH_KGDB anywhere in these patches, so
> > the code here isn't compiled.
> >
> OK, I will send a patch
> 
> > 2. There are several compiler warnings that need to be addressed.
> >
> > 3. I enabled CONFIG_KGDB_TESTS and CONFIG_KGDB_TESTS_ON_BOOT and that's
> > what really scared me.
> >
> 
> Are you refering to these warning?. I don't see any warning in the
> patched files. I will fix these.

[...]

Well, the warnings are one thing but the 100 miles of backtrace output that
appear on every boot (and I think end in an oops) are probably more
important to fix. Please enable CONFIG_KGDB_TESTS and
CONFIG_KGDB_TESTS_ON_BOOT and take a look.

Will

[PATCH v6 0/3] AArch64: KGDB support

[Vijay Kilari]

On Wed, Jan 22, 2014 at 12:06 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jan 21, 2014 at 02:10:14PM +0000, Vijay Kilari wrote:
>> Hi Will,
>
> Hello Vijay,
>
>> On Mon, Jan 20, 2014 at 3:53 PM, Will Deacon <will.deacon@arm.com> wrote:
>> > Well, the warnings are one thing but the 100 miles of backtrace output that
>> > appear on every boot (and I think end in an oops) are probably more
>> > important to fix. Please enable CONFIG_KGDB_TESTS and
>> > CONFIG_KGDB_TESTS_ON_BOOT and take a look.
>> >
>>
>> I could manage to run KGDB boot tests if I run from sysfs after complete boot
>>
>> echo V1F1000 > /sys/module/kgdbts/parameters/kgdbts
>>
>> Here the value of PSTATE is 80000145, which means PSTATE.D is unmasked
>> hence it works fine.
>>
>> If I run during boot by enabling CONFIG_KGDB_TESTS_ON_BOOT,
>> the step debug test fail because value of PSTATE is 80000345.
>> If I force PSTATE.D to 0, it works fine
>>
>> In debug_monitors.c file, only PSTATE.SS & MDSCR.KDE/MDE is managed
>> but not PSTATE.D
>>
>> Can you please let me know if where PSTATE.D is managed in arm64?
>
> That's a good point: I think we wait until our first exception before they
> are unmasked. Perhaps we should:
>
>  (1) Move local_dbg_{save,restore} from debug-monitors.h into irqflags.h
>  (2) Add local_dbg_enable/local_dbg_disable macros
>  (3) Add a call to local_dbg_enable in the clear_os_lock function after the
>      isb().
>
> Does that work for you?

Yes, only after first exception occurs the PSTATE.D is unmasked.

I have patched (temp) as below and now KGDB boot tests pass

diff --git a/arch/arm64/include/asm/debug-monitors.h
b/arch/arm64/include/asm/debug-monitors.h
index aff3a76..ea2bc46 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -64,6 +111,24 @@ struct task_struct;

 #define DBG_ARCH_ID_RESERVED   0       /* In case of ptrace ABI updates. */

+#define local_dbg_enable()
         \
+       do {
         \
+               asm volatile(
         \
+               "msr    daifclr, #9             //
arch_local_irq_disable"      \
+               :
         \
+               :
         \
+               : "memory");
         \
+       } while (0)
+
+#define local_dbg_disable()
         \
+       do {
         \
+               asm volatile(
         \
+               "msr    daifset, #9             //
arch_local_irq_disable"      \
+               :
         \
+               :
         \
+               : "memory");
         \
+       } while (0)
+
 struct step_hook {
        struct list_head node;
        int (*fn)(struct pt_regs *regs, unsigned int insn, unsigned long addr);

diff --git a/arch/arm64/kernel/debug-monitors.c
b/arch/arm64/kernel/debug-monitors.c
index f8b90c0..d0e55f7 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -139,6 +142,7 @@ static void clear_os_lock(void *unused)
 {
        asm volatile("msr oslar_el1, %0" : : "r" (0));
        isb();
+       local_dbg_enable();
 }


boot test:

[32927.161317] msgmni has been set to 1870
[32927.212747] alg: No test for stdrng (krng)
[32927.213953] Key type asymmetric registered
[32927.214899] Asymmetric key parser 'x509' registered
[32927.220029] Block layer SCSI generic (bsg) driver version 0.4
loaded (major 253)
[32927.225824] io scheduler noop registered
[32927.226764] io scheduler deadline registered
[32927.230714] io scheduler cfq registered (default)
[32927.237895] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
[32927.266066] kgdb: Registered I/O driver kgdbts.
[32927.266419] kgdb: Waiting for connection from remote gdb...
[32927.268598] kgdbts:RUN plant and detach test
[32927.270683] kgdbts:RUN sw breakpoint test
[32927.287659] kgdbts:RUN bad memory access test
[32927.290322] kgdbts:RUN singlestep test 1000 iterations
[32927.330342] kgdbts:RUN singlestep [0/1000]
[32931.286356] kgdbts:RUN singlestep [100/1000]
[32935.242536] kgdbts:RUN singlestep [200/1000]
[32939.205392] kgdbts:RUN singlestep [300/1000]
[32943.169522] kgdbts:RUN singlestep [400/1000]
[32947.231868] kgdbts:RUN singlestep [500/1000]
[32951.188008] kgdbts:RUN singlestep [600/1000]
[32955.332243] kgdbts:RUN singlestep [700/1000]
[32959.467109] kgdbts:RUN singlestep [800/1000]
[32963.430888] kgdbts:RUN singlestep [900/1000]
[32967.346992] kgdbts:RUN do_fork for 100 breakpoints

kgdb test using sysfs:

~ # echo V1F1000 > /sys/module/kgdbts/parameters/kgdbts
[33231.554237] kgdb: Registered I/O driver kgdbts.
[33231.554677] kgdbts:RUN plant and detach test
[33231.557072] kgdbts:RUN sw breakpoint test
[33231.576980] kgdbts:RUN bad memory access test
[33231.580022] kgdbts:RUN singlestep test 1000 iterations
[33231.627056] kgdbts:RUN singlestep [0/1000]
[33235.954027] kgdbts:RUN singlestep [100/1000]
[33240.429086] kgdbts:RUN singlestep [200/1000]
[33244.687118] kgdbts:RUN singlestep [300/1000]
[33248.945191] kgdbts:RUN singlestep [400/1000]
[33253.203751] kgdbts:RUN singlestep [500/1000]
[33257.462019] kgdbts:RUN singlestep [600/1000]
[33261.817809] kgdbts:RUN singlestep [700/1000]
[33266.081268] kgdbts:RUN singlestep [800/1000]
[33270.339813] kgdbts:RUN singlestep [900/1000]
[33274.712404] kgdbts:RUN do_fork for 1000 breakpoints
~ #

This works for me. Should I patch it or will you send a patch for this?

PS: cc to mailing list missed
>
> Will

[PATCH v6 0/3] AArch64: KGDB support

[Vijay Kilari]

On Wed, Jan 22, 2014 at 10:01 AM, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> On Wed, Jan 22, 2014 at 12:06 AM, Will Deacon <will.deacon@arm.com> wrote:
>> On Tue, Jan 21, 2014 at 02:10:14PM +0000, Vijay Kilari wrote:
>>> Hi Will,
>>
>> Hello Vijay,
>>
>>> On Mon, Jan 20, 2014 at 3:53 PM, Will Deacon <will.deacon@arm.com> wrote:
>>> > Well, the warnings are one thing but the 100 miles of backtrace output that
>>> > appear on every boot (and I think end in an oops) are probably more
>>> > important to fix. Please enable CONFIG_KGDB_TESTS and
>>> > CONFIG_KGDB_TESTS_ON_BOOT and take a look.
>>> >
>>>
>>> I could manage to run KGDB boot tests if I run from sysfs after complete boot
>>>
>>> echo V1F1000 > /sys/module/kgdbts/parameters/kgdbts
>>>
>>> Here the value of PSTATE is 80000145, which means PSTATE.D is unmasked
>>> hence it works fine.
>>>
>>> If I run during boot by enabling CONFIG_KGDB_TESTS_ON_BOOT,
>>> the step debug test fail because value of PSTATE is 80000345.
>>> If I force PSTATE.D to 0, it works fine
>>>
>>> In debug_monitors.c file, only PSTATE.SS & MDSCR.KDE/MDE is managed
>>> but not PSTATE.D
>>>
>>> Can you please let me know if where PSTATE.D is managed in arm64?
>>
>> That's a good point: I think we wait until our first exception before they
>> are unmasked. Perhaps we should:
>>
>>  (1) Move local_dbg_{save,restore} from debug-monitors.h into irqflags.h

local_dbg_save is setting bit #8 it should be bit #9?

#define local_dbg_save(flags)
         \
        do {
         \
                typecheck(unsigned long, flags);
         \
                asm volatile(
         \
                "mrs    %0, daif                        //
local_dbg_save\n"    \
                "msr    daifset, #8"
         \
                : "=r" (flags) : : "memory");
         \
        } while (0)


>>  (2) Add local_dbg_enable/local_dbg_disable macros
>>  (3) Add a call to local_dbg_enable in the clear_os_lock function after the
>>      isb().
>>
>> Does that work for you?
>
> Yes, only after first exception occurs the PSTATE.D is unmasked.
>
> I have patched (temp) as below and now KGDB boot tests pass
>
> diff --git a/arch/arm64/include/asm/debug-monitors.h
> b/arch/arm64/include/asm/debug-monitors.h
> index aff3a76..ea2bc46 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -64,6 +111,24 @@ struct task_struct;
>
>  #define DBG_ARCH_ID_RESERVED   0       /* In case of ptrace ABI updates. */
>
> +#define local_dbg_enable()
>          \
> +       do {
>          \
> +               asm volatile(
>          \
> +               "msr    daifclr, #9             //
> arch_local_irq_disable"      \
> +               :
>          \
> +               :
>          \
> +               : "memory");
>          \
> +       } while (0)
> +
> +#define local_dbg_disable()
>          \
> +       do {
>          \
> +               asm volatile(
>          \
> +               "msr    daifset, #9             //
> arch_local_irq_disable"      \
> +               :
>          \
> +               :
>          \
> +               : "memory");
>          \
> +       } while (0)
> +
>  struct step_hook {
>         struct list_head node;
>         int (*fn)(struct pt_regs *regs, unsigned int insn, unsigned long addr);
>
> diff --git a/arch/arm64/kernel/debug-monitors.c
> b/arch/arm64/kernel/debug-monitors.c
> index f8b90c0..d0e55f7 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -139,6 +142,7 @@ static void clear_os_lock(void *unused)
>  {
>         asm volatile("msr oslar_el1, %0" : : "r" (0));
>         isb();
> +       local_dbg_enable();
>  }
>
>
> boot test:
>
> [32927.161317] msgmni has been set to 1870
> [32927.212747] alg: No test for stdrng (krng)
> [32927.213953] Key type asymmetric registered
> [32927.214899] Asymmetric key parser 'x509' registered
> [32927.220029] Block layer SCSI generic (bsg) driver version 0.4
> loaded (major 253)
> [32927.225824] io scheduler noop registered
> [32927.226764] io scheduler deadline registered
> [32927.230714] io scheduler cfq registered (default)
> [32927.237895] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> [32927.266066] kgdb: Registered I/O driver kgdbts.
> [32927.266419] kgdb: Waiting for connection from remote gdb...
> [32927.268598] kgdbts:RUN plant and detach test
> [32927.270683] kgdbts:RUN sw breakpoint test
> [32927.287659] kgdbts:RUN bad memory access test
> [32927.290322] kgdbts:RUN singlestep test 1000 iterations
> [32927.330342] kgdbts:RUN singlestep [0/1000]
> [32931.286356] kgdbts:RUN singlestep [100/1000]
> [32935.242536] kgdbts:RUN singlestep [200/1000]
> [32939.205392] kgdbts:RUN singlestep [300/1000]
> [32943.169522] kgdbts:RUN singlestep [400/1000]
> [32947.231868] kgdbts:RUN singlestep [500/1000]
> [32951.188008] kgdbts:RUN singlestep [600/1000]
> [32955.332243] kgdbts:RUN singlestep [700/1000]
> [32959.467109] kgdbts:RUN singlestep [800/1000]
> [32963.430888] kgdbts:RUN singlestep [900/1000]
> [32967.346992] kgdbts:RUN do_fork for 100 breakpoints
>
> kgdb test using sysfs:
>
> ~ # echo V1F1000 > /sys/module/kgdbts/parameters/kgdbts
> [33231.554237] kgdb: Registered I/O driver kgdbts.
> [33231.554677] kgdbts:RUN plant and detach test
> [33231.557072] kgdbts:RUN sw breakpoint test
> [33231.576980] kgdbts:RUN bad memory access test
> [33231.580022] kgdbts:RUN singlestep test 1000 iterations
> [33231.627056] kgdbts:RUN singlestep [0/1000]
> [33235.954027] kgdbts:RUN singlestep [100/1000]
> [33240.429086] kgdbts:RUN singlestep [200/1000]
> [33244.687118] kgdbts:RUN singlestep [300/1000]
> [33248.945191] kgdbts:RUN singlestep [400/1000]
> [33253.203751] kgdbts:RUN singlestep [500/1000]
> [33257.462019] kgdbts:RUN singlestep [600/1000]
> [33261.817809] kgdbts:RUN singlestep [700/1000]
> [33266.081268] kgdbts:RUN singlestep [800/1000]
> [33270.339813] kgdbts:RUN singlestep [900/1000]
> [33274.712404] kgdbts:RUN do_fork for 1000 breakpoints
> ~ #
>
> This works for me. Should I patch it or will you send a patch for this?
>
> PS: cc to mailing list missed
>>
>> Will

[PATCH v6 0/3] AArch64: KGDB support

[Will Deacon]

On Wed, Jan 22, 2014 at 07:50:46AM +0000, Vijay Kilari wrote:
> On Wed, Jan 22, 2014 at 10:01 AM, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> >>> On Mon, Jan 20, 2014 at 3:53 PM, Will Deacon <will.deacon@arm.com> wrote:
> >> That's a good point: I think we wait until our first exception before they
> >> are unmasked. Perhaps we should:
> >>
> >>  (1) Move local_dbg_{save,restore} from debug-monitors.h into irqflags.h
> 
> local_dbg_save is setting bit #8 it should be bit #9?
> 
> #define local_dbg_save(flags)
>          \
>         do {
>          \
>                 typecheck(unsigned long, flags);
>          \
>                 asm volatile(
>          \
>                 "mrs    %0, daif                        //
> local_dbg_save\n"    \
>                 "msr    daifset, #8"

That uses daifset, so #8 is bit 3 which is D.

> >>  (2) Add local_dbg_enable/local_dbg_disable macros
> >>  (3) Add a call to local_dbg_enable in the clear_os_lock function after the
> >>      isb().
> >>
> >> Does that work for you?
> >
> > Yes, only after first exception occurs the PSTATE.D is unmasked.
> >
> > I have patched (temp) as below and now KGDB boot tests pass
> >
> > diff --git a/arch/arm64/include/asm/debug-monitors.h
> > b/arch/arm64/include/asm/debug-monitors.h
> > index aff3a76..ea2bc46 100644
> > --- a/arch/arm64/include/asm/debug-monitors.h
> > +++ b/arch/arm64/include/asm/debug-monitors.h
> > @@ -64,6 +111,24 @@ struct task_struct;
> >
> >  #define DBG_ARCH_ID_RESERVED   0       /* In case of ptrace ABI updates. */
> >
> > +#define local_dbg_enable()
> >          \
> > +       do {
> >          \
> > +               asm volatile(
> >          \
> > +               "msr    daifclr, #9             //

I think this is going to unmask fiq as well...

Will

[PATCH v6 0/3] AArch64: KGDB support

[Vijay Kilari]

Hi Catalin,

On Sat, Jan 18, 2014 at 1:32 PM, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> Hi Catalin,
>
> On Thu, Jan 16, 2014 at 7:15 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
>> On Thu, Dec 19, 2013 at 11:50:48AM +0000, vijay.kilari at gmail.com wrote:
>>> Vijaya Kumar K (3):
>>>   AArch64: KGDB: Add Basic KGDB support
>>>   AArch64: KGDB: Add step debugging support
>>>   KGDB: make kgdb_breakpoint() as noinline
>>
>> I gave them a try and now I have some doubts about your testing
>> methodology:
>>
>> 1. There is no HAVE_ARCH_KGDB anywhere in these patches, so
>> the code here isn't compiled.
>>
> OK, I will send a patch
>
>> 2. There are several compiler warnings that need to be addressed.
>>
>> 3. I enabled CONFIG_KGDB_TESTS and CONFIG_KGDB_TESTS_ON_BOOT and that's
>> what really scared me.
>>
>
> Are you refering to these warning?. I don't see any warning in the
> patched files. I will fix these.
>
> drivers/misc/kgdbts.c: In function 'check_single_step':
> drivers/misc/kgdbts.c:353:2: warning: format '%lx' expects argument of
> type 'long unsigned int', but argument 2 has type 'u64' [-Wformat]
> drivers/misc/kgdbts.c:379:3: warning: format '%lx' expects argument of
> type 'long unsigned int', but argument 2 has type 'u64' [-Wformat]
> drivers/misc/kgdbts.c: In function 'emul_sstep_put':
> drivers/misc/kgdbts.c:494:3: warning: format '%lx' expects argument of
> type 'long unsigned int', but argument 2 has type 'u64' [-Wformat]
> kernel/debug/debug_core.c: In function 'kgdb_cpu_enter':
> kernel/debug/debug_core.c:498:4: warning: value computed is not used
> [-Wunused-value]
> kernel/debug/debug_core.c:513:5: warning: value computed is not used
> [-Wunused-value]
>

  The last warnings in kernel/debug/debug_core.c is due to
atomic_xchg(&kgdb_active, cpu);
returns value which is not captured as it is not useful. Though we capture
to dummy variable again it is raises unused variable warning

>> Please address these before we can consider the patches for merging.
>>
>> --
>> Catalin