[RFC PATCH 0/2] Aarch64: KGDB: kernel debugging support

[RFC PATCH 0/2] Aarch64: KGDB: kernel debugging support

[vijay.kilari at gmail.com]

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

Based on the step-handler and break-handler hooks patch
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/189510.html
KGDB debugging support is added for EL1 debug in Aarch64 mode

With first 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 second patch, step/next debugging support is added, where in
ELR_EL1 is updated to point to the instruction to be stepped and
stopped.

kernel exception handler exit macro in entry.S is update to
retain ELR value written by KGDB.

Tested with Aarch64 GDB tool chain on simulator

Vijaya Kumar K (2):
  Aarch64: KGDB: Add Basic KGDB support
  Aarch64: KGDB: Add Step debugging support

 arch/arm64/include/asm/debug-monitors.h |    3 +
 arch/arm64/include/asm/kgdb.h           |   61 ++++++
 arch/arm64/kernel/Makefile              |    1 +
 arch/arm64/kernel/debug-monitors.c      |   15 ++
 arch/arm64/kernel/entry.S               |    9 +-
 arch/arm64/kernel/kgdb.c                |  323 +++++++++++++++++++++++++++++++
 6 files changed, 411 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/include/asm/kgdb.h
 create mode 100644 arch/arm64/kernel/kgdb.c

-- 
1.7.9.5

[RFC PATCH 1/2] 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 these changes, basic KGDB debugging is possible.
Target waits for GDB tool on hitting compile time inserted
break point and GDB tool can establish connection with target
and can set/clear breakpoints and continue.

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
---
 arch/arm64/include/asm/kgdb.h |   61 +++++++++
 arch/arm64/kernel/Makefile    |    1 +
 arch/arm64/kernel/kgdb.c      |  287 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 349 insertions(+)

diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
new file mode 100644
index 0000000..5b5f59e
--- /dev/null
+++ b/arch/arm64/include/asm/kgdb.h
@@ -0,0 +1,61 @@
+/*
+ * Aarch64 KGDB support
+ *
+ * Author: Vijaya Kumar <vijaya.kumar@caviumnetworks.com>
+ *
+ * contents are extracted from arch/arm/include/kgdb.h
+ *
+ */
+
+#ifndef __ARM_KGDB_H__
+#define __ARM_KGDB_H__
+
+#include <linux/ptrace.h>
+
+/*
+ * Break Instruction encoding
+ */
+
+#define BREAK_INSTR_SIZE               4
+#define KGDB_BREAKINST_ESR_VAL         0xf2000000
+#define KGDB_COMPILED_BREAK_ESR_VAL    0xf2000001
+#define CACHE_FLUSH_IS_SAFE            1
+
+#ifndef        __ASSEMBLY__
+
+static inline void arch_kgdb_breakpoint(void)
+{
+#ifndef __ARMEB__
+	asm(".word 0xd4200020");
+#else
+	asm(".word 0x200020d4");
+#endif
+}
+
+
+extern void kgdb_handle_bus_error(void);
+extern int kgdb_fault_expected;
+
+#endif /* !__ASSEMBLY__ */
+
+/*
+ * gdb is expecting the following registers layout.
+ *
+ * r0-r31: 64bit each
+ * f0-f31: unused
+ * fps:    unused
+ *
+ */
+
+#define _GP_REGS		34
+#define _FP_REGS		32
+#define _EXTRA_REGS		2
+#define GDB_MAX_REGS		(_GP_REGS + (_FP_REGS * 3) + _EXTRA_REGS)
+#define DBG_MAX_REG_NUM		(_GP_REGS + _FP_REGS + _EXTRA_REGS)
+
+#define KGDB_MAX_NO_CPUS	1
+#define BUFMAX			400
+#define NUMREGBYTES		(DBG_MAX_REG_NUM << 2)
+
+#endif /* __ASM_KGDB_H__ */
+
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index f667a09..b46a177 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -20,6 +20,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-$(CONFIG_ARM64_ILP32)		+= vdsoilp32/
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
new file mode 100644
index 0000000..a3a7712
--- /dev/null
+++ b/arch/arm64/kernel/kgdb.c
@@ -0,0 +1,287 @@
+/*
+ * Aarch64 KGDB support. 
+ *
+ * most part of code copied from arch/arm/kernel/kgdb.c
+ *
+ * Author:  Vijaya Kumar K <vijaya.kumar@caviumnetworks.com>
+ */
+
+#include <linux/irq.h>
+#include <linux/kdebug.h>
+#include <linux/kgdb.h>
+#include <asm/traps.h>
+#include <asm/debug-monitors.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)},
+	{ "cpsr", 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;
+	int regno;
+	int i;
+
+	/* Just making sure... */
+	if (task == NULL)
+		return;
+
+	/* Initialize to zero */
+	for (regno = 0; regno < GDB_MAX_REGS; regno++)
+		gdb_regs[regno] = 0;
+
+	thread_regs = task_pt_regs(task);
+
+	for(i = 0; i < 31; i++)
+		gdb_regs[i] = thread_regs->regs[i];
+
+	gdb_regs[31] = thread_regs->sp;
+	gdb_regs[32] = thread_regs->pc;
+	gdb_regs[33] = thread_regs->pstate;
+}
+
+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':
+	case 'c':
+		/*
+		 * Try to read optional parameter, pc unchanged if no parm.
+		 * If this was a compiled breakpoint, we need to move
+		 * to the next instruction or we will just breakpoint
+		 * over and over again.
+		 */
+		ptr = &remcom_in_buffer[1];
+		if (kgdb_hex2long(&ptr, &addr))
+			linux_regs->pc = addr;
+		else if (compiled_break == 1)
+			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, unsigned long addr)
+{
+	kgdb_handle_exception(1, SIGTRAP, 0, regs);
+	return 0;
+}
+
+static int kgdb_compiled_brk_fn(struct pt_regs *regs, unsigned int esr,
+				unsigned long addr)
+{
+	compiled_break = 1;
+	kgdb_handle_exception(1, SIGTRAP, 0, regs);
+
+	return 0;
+}
+static struct break_hook kgdb_brkpt_hook = {
+	.esr_mask		= 0xffffffff,
+	.esr_magic		= KGDB_BREAKINST_ESR_VAL,
+	.fn			= kgdb_brk_fn
+};
+
+static struct break_hook kgdb_compiled_brkpt_hook = {
+	.esr_mask		= 0xffffffff,
+	.esr_magic		= KGDB_COMPILED_BREAK_ESR_VAL,
+	.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);
+}
+
+/*
+ * Register our undef instruction hooks with ARM undef core.
+ * We regsiter a hook specifically looking for the KGB break inst
+ * and we handle the normal undef case within the do_undefinstr
+ * handler.
+ */
+struct kgdb_arch arch_kgdb_ops = {
+#ifndef __ARMEB__
+	.gdb_bpt_instr          = {0x00, 0x00, 0x20, 0xd4}
+#else /* ! __ARMEB__ */
+	.gdb_bpt_instr          = {0xd4, 0x20, 0x00, 0x00}
+#endif
+};
+
-- 
1.7.9.5

[RFC PATCH 1/2] Aarch64: KGDB: Add Basic KGDB support

[Will Deacon]

Hello,

[Adding Jason for regset question later on]

On Mon, Sep 16, 2013 at 09:55:49AM +0100, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Add KGDB debug support for kernel debugging.
> With these changes, basic KGDB debugging is possible.
> Target waits for GDB tool on hitting compile time inserted
> break point and GDB tool can establish connection with target
> and can set/clear breakpoints and continue.
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  arch/arm64/include/asm/kgdb.h |   61 +++++++++
>  arch/arm64/kernel/Makefile    |    1 +
>  arch/arm64/kernel/kgdb.c      |  287 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 349 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
> new file mode 100644
> index 0000000..5b5f59e
> --- /dev/null
> +++ b/arch/arm64/include/asm/kgdb.h
> @@ -0,0 +1,61 @@
> +/*
> + * Aarch64 KGDB support
> + *
> + * Author: Vijaya Kumar <vijaya.kumar@caviumnetworks.com>
> + *
> + * contents are extracted from arch/arm/include/kgdb.h
> + *
> + */
> +
> +#ifndef __ARM_KGDB_H__
> +#define __ARM_KGDB_H__
> +
> +#include <linux/ptrace.h>
> +
> +/*
> + * Break Instruction encoding
> + */
> +
> +#define BREAK_INSTR_SIZE               4
> +#define KGDB_BREAKINST_ESR_VAL         0xf2000000
> +#define KGDB_COMPILED_BREAK_ESR_VAL    0xf2000001

These ESR values are tied directly to your immediate choices for the BRK
instruction, so I'd rather they were constructed that way (then we can
#define all of the immediates in a common header, like debug-monitors.h).

> +#define CACHE_FLUSH_IS_SAFE            1
> +
> +#ifndef        __ASSEMBLY__
> +
> +static inline void arch_kgdb_breakpoint(void)
> +{
> +#ifndef __ARMEB__
> +	asm(".word 0xd4200020");
> +#else
> +	asm(".word 0x200020d4");
> +#endif

Huh? Instructions are always little endian. Why can't you just do:

  asm ("brk %0" :: "I" (ARM64_KGDB_BRK_IMM));

or something like that?

> +}
> +
> +
> +extern void kgdb_handle_bus_error(void);
> +extern int kgdb_fault_expected;
> +
> +#endif /* !__ASSEMBLY__ */
> +
> +/*
> + * gdb is expecting the following registers layout.
> + *
> + * r0-r31: 64bit each
> + * f0-f31: unused
> + * fps:    unused
> + *
> + */
> +
> +#define _GP_REGS		34
> +#define _FP_REGS		32
> +#define _EXTRA_REGS		2
> +#define GDB_MAX_REGS		(_GP_REGS + (_FP_REGS * 3) + _EXTRA_REGS)
> +#define DBG_MAX_REG_NUM		(_GP_REGS + _FP_REGS + _EXTRA_REGS)

This doesn't appear to match the comment above, and it's not obvious at all
how you've laid things out. Could you improve the comment please?

> +#define KGDB_MAX_NO_CPUS	1

Where is this used?

> +#define BUFMAX			400

Why did you choose this value? Are you sure it's big enough?

> +#define NUMREGBYTES		(DBG_MAX_REG_NUM << 2)

<< 2? Are you sure?

> +
> +#endif /* __ASM_KGDB_H__ */
> +
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index f667a09..b46a177 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -20,6 +20,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-$(CONFIG_ARM64_ILP32)		+= vdsoilp32/

ILP32 doesn't exist in mainline. Please write your patches against an
upstream kernel.

> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> new file mode 100644
> index 0000000..a3a7712
> --- /dev/null
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -0,0 +1,287 @@
> +/*
> + * Aarch64 KGDB support. 
> + *
> + * most part of code copied from arch/arm/kernel/kgdb.c
> + *
> + * Author:  Vijaya Kumar K <vijaya.kumar@caviumnetworks.com>
> + */
> +
> +#include <linux/irq.h>
> +#include <linux/kdebug.h>
> +#include <linux/kgdb.h>
> +#include <asm/traps.h>
> +#include <asm/debug-monitors.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)},
> +	{ "cpsr", 4, offsetof(struct pt_regs, pstate)},

There isn't a cpsr. Please use the correct terminology. Also, isn't there
some dependency on GDB here, so this interface should be fixed already?

> +	{ "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 },

Ard added support for NEON in the kernel, so maybe it's worth reporting the
vector regs after all.

> +};
> +
> +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;
> +}

Has nobody ported this lot to use regsets yet? I don't see why we have to
reinvent all of the "please copy register x from register file y to this
buffer" logic all over again.

> +
> +void
> +sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *task)
> +{
> +	struct pt_regs *thread_regs;
> +	int regno;
> +	int i;
> +
> +	/* Just making sure... */
> +	if (task == NULL)
> +		return;
> +
> +	/* Initialize to zero */
> +	for (regno = 0; regno < GDB_MAX_REGS; regno++)
> +		gdb_regs[regno] = 0;
> +
> +	thread_regs = task_pt_regs(task);
> +
> +	for(i = 0; i < 31; i++)
> +		gdb_regs[i] = thread_regs->regs[i];
> +
> +	gdb_regs[31] = thread_regs->sp;
> +	gdb_regs[32] = thread_regs->pc;
> +	gdb_regs[33] = thread_regs->pstate;
> +}
> +
> +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':

What are the 'D' and 'k' packets? Do they actually get used for AArch64?
Again, comments would help in explaining the protocol.

> +	case 'c':
> +		/*
> +		 * Try to read optional parameter, pc unchanged if no parm.
> +		 * If this was a compiled breakpoint, we need to move
> +		 * to the next instruction or we will just breakpoint
> +		 * over and over again.
> +		 */
> +		ptr = &remcom_in_buffer[1];
> +		if (kgdb_hex2long(&ptr, &addr))
> +			linux_regs->pc = addr;
> +		else if (compiled_break == 1)
> +			linux_regs->pc += 4;
> +
> +		compiled_break = 0;

I'm not familiar with how kdgb works, but why does this not cause problems
with SMP? Does KGDB just park the secondaries and only deal with exceptions
on the primary CPU?

> +void kgdb_arch_exit(void)
> +{
> +	unregister_break_hook(&kgdb_brkpt_hook);
> +	unregister_break_hook(&kgdb_compiled_brkpt_hook);
> +	unregister_die_notifier(&kgdb_notifier);
> +}
> +
> +/*
> + * Register our undef instruction hooks with ARM undef core.
> + * We regsiter a hook specifically looking for the KGB break inst
> + * and we handle the normal undef case within the do_undefinstr
> + * handler.
> + */
> +struct kgdb_arch arch_kgdb_ops = {
> +#ifndef __ARMEB__
> +	.gdb_bpt_instr          = {0x00, 0x00, 0x20, 0xd4}
> +#else /* ! __ARMEB__ */
> +	.gdb_bpt_instr          = {0xd4, 0x20, 0x00, 0x00}
> +#endif

__ARMEB__????

Will

[RFC PATCH 1/2] Aarch64: KGDB: Add Basic KGDB support

[Vijay Kilari]

On Mon, Sep 16, 2013 at 4:59 PM, Will Deacon <will.deacon@arm.com> wrote:
> Hello,
>
> [Adding Jason for regset question later on]
>
> On Mon, Sep 16, 2013 at 09:55:49AM +0100, vijay.kilari at gmail.com wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>
>> Add KGDB debug support for kernel debugging.
>> With these changes, basic KGDB debugging is possible.
>> Target waits for GDB tool on hitting compile time inserted
>> break point and GDB tool can establish connection with target
>> and can set/clear breakpoints and continue.
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>> ---
>>  arch/arm64/include/asm/kgdb.h |   61 +++++++++
>>  arch/arm64/kernel/Makefile    |    1 +
>>  arch/arm64/kernel/kgdb.c      |  287 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 349 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
>> new file mode 100644
>> index 0000000..5b5f59e
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/kgdb.h
>> @@ -0,0 +1,61 @@
>> +/*
>> + * Aarch64 KGDB support
>> + *
>> + * Author: Vijaya Kumar <vijaya.kumar@caviumnetworks.com>
>> + *
>> + * contents are extracted from arch/arm/include/kgdb.h
>> + *
>> + */
>> +
>> +#ifndef __ARM_KGDB_H__
>> +#define __ARM_KGDB_H__
>> +
>> +#include <linux/ptrace.h>
>> +
>> +/*
>> + * Break Instruction encoding
>> + */
>> +
>> +#define BREAK_INSTR_SIZE               4
>> +#define KGDB_BREAKINST_ESR_VAL         0xf2000000
>> +#define KGDB_COMPILED_BREAK_ESR_VAL    0xf2000001
>
> These ESR values are tied directly to your immediate choices for the BRK
> instruction, so I'd rather they were constructed that way (then we can
> #define all of the immediates in a common header, like debug-monitors.h).
>

Yes, these values are constructed. However to distinguish between
normal break point and compile time break point, I have chosen value
0x1 as immediate
value.

>> +#define CACHE_FLUSH_IS_SAFE            1
>> +
>> +#ifndef        __ASSEMBLY__
>> +
>> +static inline void arch_kgdb_breakpoint(void)
>> +{
>> +#ifndef __ARMEB__
>> +     asm(".word 0xd4200020");
>> +#else
>> +     asm(".word 0x200020d4");
>> +#endif
>
> Huh? Instructions are always little endian. Why can't you just do:
>
>   asm ("brk %0" :: "I" (ARM64_KGDB_BRK_IMM));
>
> or something like that?
>
OK. I will check with this

>> +}
>> +
>> +
>> +extern void kgdb_handle_bus_error(void);
>> +extern int kgdb_fault_expected;
>> +
>> +#endif /* !__ASSEMBLY__ */
>> +
>> +/*
>> + * gdb is expecting the following registers layout.
>> + *
>> + * r0-r31: 64bit each
>> + * f0-f31: unused
>> + * fps:    unused
>> + *
>> + */
>> +
>> +#define _GP_REGS             34
>> +#define _FP_REGS             32
>> +#define _EXTRA_REGS          2
>> +#define GDB_MAX_REGS         (_GP_REGS + (_FP_REGS * 3) + _EXTRA_REGS)
>> +#define DBG_MAX_REG_NUM              (_GP_REGS + _FP_REGS + _EXTRA_REGS)
>
> This doesn't appear to match the comment above, and it's not obvious at all
> how you've laid things out. Could you improve the comment please?
>
>> +#define KGDB_MAX_NO_CPUS     1
>
> Where is this used?
Not used. I will correct it
>
>> +#define BUFMAX                       400
>
> Why did you choose this value? Are you sure it's big enough?
>
>> +#define NUMREGBYTES          (DBG_MAX_REG_NUM << 2)
>
> << 2? Are you sure?
>
 These values are used for defining size of buffer to be used.
This value is legacy and should be enough as there is no major
changes in GDB host side in terms of buffer requirement.

>> +
>> +#endif /* __ASM_KGDB_H__ */
>> +
>> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
>> index f667a09..b46a177 100644
>> --- a/arch/arm64/kernel/Makefile
>> +++ b/arch/arm64/kernel/Makefile
>> @@ -20,6 +20,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-$(CONFIG_ARM64_ILP32)            += vdsoilp32/
>
> ILP32 doesn't exist in mainline. Please write your patches against an
> upstream kernel.
>
OK

>> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
>> new file mode 100644
>> index 0000000..a3a7712
>> --- /dev/null
>> +++ b/arch/arm64/kernel/kgdb.c
>> @@ -0,0 +1,287 @@
>> +/*
>> + * Aarch64 KGDB support.
>> + *
>> + * most part of code copied from arch/arm/kernel/kgdb.c
>> + *
>> + * Author:  Vijaya Kumar K <vijaya.kumar@caviumnetworks.com>
>> + */
>> +
>> +#include <linux/irq.h>
>> +#include <linux/kdebug.h>
>> +#include <linux/kgdb.h>
>> +#include <asm/traps.h>
>> +#include <asm/debug-monitors.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)},
>> +     { "cpsr", 4, offsetof(struct pt_regs, pstate)},
>
> There isn't a cpsr. Please use the correct terminology. Also, isn't there
> some dependency on GDB here, so this interface should be fixed already?
>
OK. I will correct the terminology
Yes, GDB dependency is in terms of offset and size.

>> +     { "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 },
>
> Ard added support for NEON in the kernel, so maybe it's worth reporting the
> vector regs after all.
>
>> +};
>> +
>> +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;
>> +}
>
> Has nobody ported this lot to use regsets yet? I don't see why we have to
> reinvent all of the "please copy register x from register file y to this
> buffer" logic all over again.
>
Nothing I am aware of this.

>> +
>> +void
>> +sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *task)
>> +{
>> +     struct pt_regs *thread_regs;
>> +     int regno;
>> +     int i;
>> +
>> +     /* Just making sure... */
>> +     if (task == NULL)
>> +             return;
>> +
>> +     /* Initialize to zero */
>> +     for (regno = 0; regno < GDB_MAX_REGS; regno++)
>> +             gdb_regs[regno] = 0;
>> +
>> +     thread_regs = task_pt_regs(task);
>> +
>> +     for(i = 0; i < 31; i++)
>> +             gdb_regs[i] = thread_regs->regs[i];
>> +
>> +     gdb_regs[31] = thread_regs->sp;
>> +     gdb_regs[32] = thread_regs->pc;
>> +     gdb_regs[33] = thread_regs->pstate;
>> +}
>> +
>> +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':
>
> What are the 'D' and 'k' packets? Do they actually get used for AArch64?
> Again, comments would help in explaining the protocol.
>
D & k packets are used for GDB detach & Kill packets. This is as good
as 'c' packet where in GDB is disconnected and allows target to continue
to run. So architecture handling of 'D','k' & 'c' are same

>> +     case 'c':
>> +             /*
>> +              * Try to read optional parameter, pc unchanged if no parm.
>> +              * If this was a compiled breakpoint, we need to move
>> +              * to the next instruction or we will just breakpoint
>> +              * over and over again.
>> +              */
>> +             ptr = &remcom_in_buffer[1];
>> +             if (kgdb_hex2long(&ptr, &addr))
>> +                     linux_regs->pc = addr;
>> +             else if (compiled_break == 1)
>> +                     linux_regs->pc += 4;
>> +
>> +             compiled_break = 0;
>
> I'm not familiar with how kdgb works, but why does this not cause problems
> with SMP? Does KGDB just park the secondaries and only deal with exceptions
> on the primary CPU?
>
Yes, KGDB parks other CPU's and only once CPU (primary / Secondary) will
be responding to GDB requests

>> +void kgdb_arch_exit(void)
>> +{
>> +     unregister_break_hook(&kgdb_brkpt_hook);
>> +     unregister_break_hook(&kgdb_compiled_brkpt_hook);
>> +     unregister_die_notifier(&kgdb_notifier);
>> +}
>> +
>> +/*
>> + * Register our undef instruction hooks with ARM undef core.
>> + * We regsiter a hook specifically looking for the KGB break inst
>> + * and we handle the normal undef case within the do_undefinstr
>> + * handler.
>> + */
>> +struct kgdb_arch arch_kgdb_ops = {
>> +#ifndef __ARMEB__
>> +     .gdb_bpt_instr          = {0x00, 0x00, 0x20, 0xd4}
>> +#else /* ! __ARMEB__ */
>> +     .gdb_bpt_instr          = {0xd4, 0x20, 0x00, 0x00}
>> +#endif
>
> __ARMEB__????
 ARMEB is for Big endian encoding.
>
> Will

[RFC PATCH 1/2] Aarch64: KGDB: Add Basic KGDB support

[Will Deacon]

On Mon, Sep 23, 2013 at 08:04:48AM +0100, Vijay Kilari wrote:
> On Mon, Sep 16, 2013 at 4:59 PM, Will Deacon <will.deacon@arm.com> wrote:
> > On Mon, Sep 16, 2013 at 09:55:49AM +0100, vijay.kilari at gmail.com wrote:
> >> diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
> >> new file mode 100644
> >> index 0000000..5b5f59e
> >> --- /dev/null
> >> +++ b/arch/arm64/include/asm/kgdb.h
> >> @@ -0,0 +1,61 @@
> >> +/*
> >> + * Aarch64 KGDB support
> >> + *
> >> + * Author: Vijaya Kumar <vijaya.kumar@caviumnetworks.com>
> >> + *
> >> + * contents are extracted from arch/arm/include/kgdb.h
> >> + *
> >> + */
> >> +
> >> +#ifndef __ARM_KGDB_H__
> >> +#define __ARM_KGDB_H__
> >> +
> >> +#include <linux/ptrace.h>
> >> +
> >> +/*
> >> + * Break Instruction encoding
> >> + */
> >> +
> >> +#define BREAK_INSTR_SIZE               4
> >> +#define KGDB_BREAKINST_ESR_VAL         0xf2000000
> >> +#define KGDB_COMPILED_BREAK_ESR_VAL    0xf2000001
> >
> > These ESR values are tied directly to your immediate choices for the BRK
> > instruction, so I'd rather they were constructed that way (then we can
> > #define all of the immediates in a common header, like debug-monitors.h).
> >
> 
> Yes, these values are constructed. However to distinguish between
> normal break point and compile time break point, I have chosen value
> 0x1 as immediate
> value.

I can see that. What I dislike is the way you've structured the code. Please
can you define the immediates (i.e. 0, 1) in debug-monitors.h?

> >> +#define CACHE_FLUSH_IS_SAFE            1
> >> +
> >> +#ifndef        __ASSEMBLY__
> >> +
> >> +static inline void arch_kgdb_breakpoint(void)
> >> +{
> >> +#ifndef __ARMEB__
> >> +     asm(".word 0xd4200020");
> >> +#else
> >> +     asm(".word 0x200020d4");
> >> +#endif
> >
> > Huh? Instructions are always little endian. Why can't you just do:
> >
> >   asm ("brk %0" :: "I" (ARM64_KGDB_BRK_IMM));
> >
> > or something like that?
> >
> OK. I will check with this

Yes, then you don't need to build the instruction encoding from scratch.
Instead, you just take the relevant definition for the immediate operand.

> >> +#define NUMREGBYTES          (DBG_MAX_REG_NUM << 2)
> >
> > << 2? Are you sure?
> >
>  These values are used for defining size of buffer to be used.
> This value is legacy and should be enough as there is no major
> changes in GDB host side in terms of buffer requirement.

There's no legacy here. If you're referring to arch/arm/, then the register
file is significantly different, as well as the registers being half the
size, so your NUMREGBYTES definition is certainly incorrect.

> >> +     case 'c':
> >> +             /*
> >> +              * Try to read optional parameter, pc unchanged if no parm.
> >> +              * If this was a compiled breakpoint, we need to move
> >> +              * to the next instruction or we will just breakpoint
> >> +              * over and over again.
> >> +              */
> >> +             ptr = &remcom_in_buffer[1];
> >> +             if (kgdb_hex2long(&ptr, &addr))
> >> +                     linux_regs->pc = addr;
> >> +             else if (compiled_break == 1)
> >> +                     linux_regs->pc += 4;
> >> +
> >> +             compiled_break = 0;
> >
> > I'm not familiar with how kdgb works, but why does this not cause problems
> > with SMP? Does KGDB just park the secondaries and only deal with exceptions
> > on the primary CPU?
> >
> Yes, KGDB parks other CPU's and only once CPU (primary / Secondary) will
> be responding to GDB requests

Ok. Is it guaranteed to be the same CPU responding to the requests each
time?

> >> +struct kgdb_arch arch_kgdb_ops = {
> >> +#ifndef __ARMEB__
> >> +     .gdb_bpt_instr          = {0x00, 0x00, 0x20, 0xd4}
> >> +#else /* ! __ARMEB__ */
> >> +     .gdb_bpt_instr          = {0xd4, 0x20, 0x00, 0x00}
> >> +#endif
> >
> > __ARMEB__????
>  ARMEB is for Big endian encoding.

(1) We don't currently support big-endian in mainline Linux

(2) __ARMEB__ isn't be emitted by the AArch64 toolchain

Have you tested this code?

Will

[RFC PATCH 1/2] Aarch64: KGDB: Add Basic KGDB support

[Vijay Kilari]

On Tue, Sep 24, 2013 at 6:58 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Mon, Sep 23, 2013 at 08:04:48AM +0100, Vijay Kilari wrote:
>> On Mon, Sep 16, 2013 at 4:59 PM, Will Deacon <will.deacon@arm.com> wrote:
>> > On Mon, Sep 16, 2013 at 09:55:49AM +0100, vijay.kilari at gmail.com wrote:
>> >> diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
>> >> new file mode 100644
>> >> index 0000000..5b5f59e
>> >> --- /dev/null
>> >> +++ b/arch/arm64/include/asm/kgdb.h
>> >> @@ -0,0 +1,61 @@
>> >> +/*
>> >> + * Aarch64 KGDB support
>> >> + *
>> >> + * Author: Vijaya Kumar <vijaya.kumar@caviumnetworks.com>
>> >> + *
>> >> + * contents are extracted from arch/arm/include/kgdb.h
>> >> + *
>> >> + */
>> >> +
>> >> +#ifndef __ARM_KGDB_H__
>> >> +#define __ARM_KGDB_H__
>> >> +
>> >> +#include <linux/ptrace.h>
>> >> +
>> >> +/*
>> >> + * Break Instruction encoding
>> >> + */
>> >> +
>> >> +#define BREAK_INSTR_SIZE               4
>> >> +#define KGDB_BREAKINST_ESR_VAL         0xf2000000
>> >> +#define KGDB_COMPILED_BREAK_ESR_VAL    0xf2000001
>> >
>> > These ESR values are tied directly to your immediate choices for the BRK
>> > instruction, so I'd rather they were constructed that way (then we can
>> > #define all of the immediates in a common header, like debug-monitors.h).
>> >
>>
>> Yes, these values are constructed. However to distinguish between
>> normal break point and compile time break point, I have chosen value
>> 0x1 as immediate
>> value.
>
> I can see that. What I dislike is the way you've structured the code. Please
> can you define the immediates (i.e. 0, 1) in debug-monitors.h?
>
Yes, I will move this code to debug-monitors.h

>> >> +#define CACHE_FLUSH_IS_SAFE            1
>> >> +
>> >> +#ifndef        __ASSEMBLY__
>> >> +
>> >> +static inline void arch_kgdb_breakpoint(void)
>> >> +{
>> >> +#ifndef __ARMEB__
>> >> +     asm(".word 0xd4200020");
>> >> +#else
>> >> +     asm(".word 0x200020d4");
>> >> +#endif
>> >
>> > Huh? Instructions are always little endian. Why can't you just do:
>> >
>> >   asm ("brk %0" :: "I" (ARM64_KGDB_BRK_IMM));
>> >
>> > or something like that?
>> >
>> OK. I will check with this
>
> Yes, then you don't need to build the instruction encoding from scratch.
> Instead, you just take the relevant definition for the immediate operand.
>
OK. I will include in next version

>> >> +#define NUMREGBYTES          (DBG_MAX_REG_NUM << 2)
>> >
>> > << 2? Are you sure?
>> >
>>  These values are used for defining size of buffer to be used.
>> This value is legacy and should be enough as there is no major
>> changes in GDB host side in terms of buffer requirement.
>
> There's no legacy here. If you're referring to arch/arm/, then the register
> file is significantly different, as well as the registers being half the
> size, so your NUMREGBYTES definition is certainly incorrect.
>
I have reworked this, the right value is.

#define NUMREGBYTES     (((_GP_REGS * 8) - 4) + (_FP_REGS * 16) + \
                        (_EXTRA_REGS * 4))


_GP_REGS is 8 bytes, _FP_REGS is 16 bytes and _EXTRA_REGS is 4 bytes each
 and pstate reg is 4 bytes. So subtract 4 from size contributed
 by _GP_REGS.
 GDB fails to connect for size beyond this with error
 "'g' packet reply is too long"

>> >> +     case 'c':
>> >> +             /*
>> >> +              * Try to read optional parameter, pc unchanged if no parm.
>> >> +              * If this was a compiled breakpoint, we need to move
>> >> +              * to the next instruction or we will just breakpoint
>> >> +              * over and over again.
>> >> +              */
>> >> +             ptr = &remcom_in_buffer[1];
>> >> +             if (kgdb_hex2long(&ptr, &addr))
>> >> +                     linux_regs->pc = addr;
>> >> +             else if (compiled_break == 1)
>> >> +                     linux_regs->pc += 4;
>> >> +
>> >> +             compiled_break = 0;
>> >
>> > I'm not familiar with how kdgb works, but why does this not cause problems
>> > with SMP? Does KGDB just park the secondaries and only deal with exceptions
>> > on the primary CPU?
>> >
>> Yes, KGDB parks other CPU's and only once CPU (primary / Secondary) will
>> be responding to GDB requests
>
> Ok. Is it guaranteed to be the same CPU responding to the requests each
> time?
>
Yes, the same CPU will be responding to the requests.
it is taken care in kernel/debug/debug_core.c

>> >> +struct kgdb_arch arch_kgdb_ops = {
>> >> +#ifndef __ARMEB__
>> >> +     .gdb_bpt_instr          = {0x00, 0x00, 0x20, 0xd4}
>> >> +#else /* ! __ARMEB__ */
>> >> +     .gdb_bpt_instr          = {0xd4, 0x20, 0x00, 0x00}
>> >> +#endif
>> >
>> > __ARMEB__????
>>  ARMEB is for Big endian encoding.
>
> (1) We don't currently support big-endian in mainline Linux
>
> (2) __ARMEB__ isn't be emitted by the AArch64 toolchain
>
> Have you tested this code?
>
This is redundant, Only LE encoding is enough as ARM instructions are
always in LE mode as below
struct kgdb_arch arch_kgdb_ops = {
      .gdb_bpt_instr          = {0x00, 0x00, 0x20, 0xd4}
}

> Will

[RFC PATCH 2/2] 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 written to ELR
register. If not step address is received from GDB tool,
target assumes next step address is PC and ERET is
executed as part of exception return.

ELR register content is protected against context restore in
exception return by checking against Software Step debug
bit MDSCR.SS

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

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
---
 arch/arm64/include/asm/debug-monitors.h |    3 +++
 arch/arm64/kernel/debug-monitors.c      |   15 +++++++++++++
 arch/arm64/kernel/entry.S               |    9 +++++++-
 arch/arm64/kernel/kgdb.c                |   36 +++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index aff3a76..3e4ac0d 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -94,6 +94,9 @@ void kernel_enable_single_step(struct pt_regs *regs);
 void kernel_disable_single_step(void);
 int kernel_active_single_step(void);
 
+void elr_write(unsigned long elr);
+unsigned long elr_read(void);
+
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 int reinstall_suspended_bps(struct pt_regs *regs);
 #else
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index f8b90c0..0408490 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -64,6 +64,21 @@ static u32 mdscr_read(void)
 	return mdscr;
 }
 
+void elr_write(unsigned long elr)
+{
+	unsigned long flags;
+	local_dbg_save(flags);
+	asm volatile("msr elr_el1, %0" :: "r" (elr));
+	local_dbg_restore(flags);
+}
+
+unsigned long elr_read(void)
+{
+	unsigned long elr;
+	asm volatile("mrs %0, elr_el1" : "=r" (elr));
+	return elr;
+}
+
 /*
  * Allow root to disable self-hosted debug from userspace.
  * This is useful if you want to connect an external JTAG debugger.
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 226be77..23d91f1 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -100,7 +100,14 @@
 	pop	x4, x5
 	pop	x6, x7
 	pop	x8, x9
-	msr	elr_el1, x21			// set up the return data
+	.if     \el == 1
+	mrs     x10, mdscr_el1			// check if step debug is enabled
+	tbnz    x10, #0, 1f
+	msr     elr_el1, x21
+	.else
+	msr     elr_el1, x21			// set up the return data
+	.endif
+1:
 	msr	spsr_el1, x22
 	.if	\el == 0
 	msr	sp_el0, x23
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index a3a7712..d5e5884 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -164,9 +164,31 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, int err_code,
 			linux_regs->pc += 4;
 
 		compiled_break = 0;
+
+		/* Disable single step if enabled */
+		if (kernel_active_single_step())
+			kernel_disable_single_step();
+
 		err = 0;
 		break;
+	case 's':
+		/* 
+		 * Update ESR value with step address passed
+		 */
+		ptr = &remcom_in_buffer[1];
+		if (kgdb_hex2long(&ptr, &addr))
+			elr_write(addr);
+		else
+			elr_write(linux_regs->pc);
+
+		if (compiled_break == 1)
+			compiled_break = 0;
 
+		/* Enable step handling if not enabled */
+		if (!kernel_active_single_step())
+			kernel_enable_single_step(linux_regs);
+		err = 0;
+		break;
 	default:
 		err = -1;
 	}
@@ -188,6 +210,14 @@ 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,
+			    unsigned long addr)
+{
+	kgdb_handle_exception(1, SIGTRAP, 0, regs);
+	return 0;
+}
+
 static struct break_hook kgdb_brkpt_hook = {
 	.esr_mask		= 0xffffffff,
 	.esr_magic		= KGDB_BREAKINST_ESR_VAL,
@@ -200,6 +230,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());
@@ -254,6 +288,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;
 }
@@ -268,6 +303,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

[RFC PATCH 2/2] Aarch64: KGDB: Add Step debugging support

[Will Deacon]

On Mon, Sep 16, 2013 at 09:55:50AM +0100, 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 written to ELR
> register. If not step address is received from GDB tool,
> target assumes next step address is PC and ERET is
> executed as part of exception return.
> 
> ELR register content is protected against context restore in
> exception return by checking against Software Step debug
> bit MDSCR.SS
> 
> Software Step debugging is disabled when 'continue' command
> is received
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  arch/arm64/include/asm/debug-monitors.h |    3 +++
>  arch/arm64/kernel/debug-monitors.c      |   15 +++++++++++++
>  arch/arm64/kernel/entry.S               |    9 +++++++-
>  arch/arm64/kernel/kgdb.c                |   36 +++++++++++++++++++++++++++++++
>  4 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index aff3a76..3e4ac0d 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -94,6 +94,9 @@ void kernel_enable_single_step(struct pt_regs *regs);
>  void kernel_disable_single_step(void);
>  int kernel_active_single_step(void);
>  
> +void elr_write(unsigned long elr);
> +unsigned long elr_read(void);
> +
>  #ifdef CONFIG_HAVE_HW_BREAKPOINT
>  int reinstall_suspended_bps(struct pt_regs *regs);
>  #else
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index f8b90c0..0408490 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -64,6 +64,21 @@ static u32 mdscr_read(void)
>  	return mdscr;
>  }
>  
> +void elr_write(unsigned long elr)
> +{
> +	unsigned long flags;
> +	local_dbg_save(flags);
> +	asm volatile("msr elr_el1, %0" :: "r" (elr));
> +	local_dbg_restore(flags);
> +}
> +
> +unsigned long elr_read(void)
> +{
> +	unsigned long elr;
> +	asm volatile("mrs %0, elr_el1" : "=r" (elr));
> +	return elr;
> +}

What?! Why are you writing the ELR here, instead of setting up the pc in
current saved regs?

> +
>  /*
>   * Allow root to disable self-hosted debug from userspace.
>   * This is useful if you want to connect an external JTAG debugger.
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 226be77..23d91f1 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -100,7 +100,14 @@
>  	pop	x4, x5
>  	pop	x6, x7
>  	pop	x8, x9
> -	msr	elr_el1, x21			// set up the return data
> +	.if     \el == 1
> +	mrs     x10, mdscr_el1			// check if step debug is enabled
> +	tbnz    x10, #0, 1f
> +	msr     elr_el1, x21
> +	.else
> +	msr     elr_el1, x21			// set up the return data
> +	.endif
> +1:

This is horrible -- if the pt_regs being restored was set up correctly,
you wouldn't need the conditional code.

>  	msr	spsr_el1, x22
>  	.if	\el == 0
>  	msr	sp_el0, x23
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index a3a7712..d5e5884 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -164,9 +164,31 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, int err_code,
>  			linux_regs->pc += 4;
>  
>  		compiled_break = 0;
> +
> +		/* Disable single step if enabled */
> +		if (kernel_active_single_step())
> +			kernel_disable_single_step();
> +
>  		err = 0;
>  		break;
> +	case 's':
> +		/* 
> +		 * Update ESR value with step address passed
> +		 */

Well, the comment is wrong, but I don't think you should be doing this anyway :)

Will

[RFC PATCH 2/2] Aarch64: KGDB: Add Step debugging support

[Sandeepa Prabhu]

On 16 September 2013 17:00, Will Deacon <will.deacon@arm.com> wrote:
> On Mon, Sep 16, 2013 at 09:55:50AM +0100, 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 written to ELR
>> register. If not step address is received from GDB tool,
>> target assumes next step address is PC and ERET is
>> executed as part of exception return.
>>
>> ELR register content is protected against context restore in
>> exception return by checking against Software Step debug
>> bit MDSCR.SS
>>
>> Software Step debugging is disabled when 'continue' command
>> is received
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>> ---
>>  arch/arm64/include/asm/debug-monitors.h |    3 +++
>>  arch/arm64/kernel/debug-monitors.c      |   15 +++++++++++++
>>  arch/arm64/kernel/entry.S               |    9 +++++++-
>>  arch/arm64/kernel/kgdb.c                |   36 +++++++++++++++++++++++++++++++
>>  4 files changed, 62 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
>> index aff3a76..3e4ac0d 100644
>> --- a/arch/arm64/include/asm/debug-monitors.h
>> +++ b/arch/arm64/include/asm/debug-monitors.h
>> @@ -94,6 +94,9 @@ void kernel_enable_single_step(struct pt_regs *regs);
>>  void kernel_disable_single_step(void);
>>  int kernel_active_single_step(void);
>>
>> +void elr_write(unsigned long elr);
>> +unsigned long elr_read(void);
>> +
>>  #ifdef CONFIG_HAVE_HW_BREAKPOINT
>>  int reinstall_suspended_bps(struct pt_regs *regs);
>>  #else
>> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
>> index f8b90c0..0408490 100644
>> --- a/arch/arm64/kernel/debug-monitors.c
>> +++ b/arch/arm64/kernel/debug-monitors.c
>> @@ -64,6 +64,21 @@ static u32 mdscr_read(void)
>>       return mdscr;
>>  }
>>
>> +void elr_write(unsigned long elr)
>> +{
>> +     unsigned long flags;
>> +     local_dbg_save(flags);
>> +     asm volatile("msr elr_el1, %0" :: "r" (elr));
I think right way to update step address would be update
instruction_pointer(regs) from your kgdb handler and "kernel_exit"
should take care of rest. With this, you don't need to change the low
level handlers in entry.S/debug-monitors.c) at all.
>> +     local_dbg_restore(flags);
>> +}
>> +
>> +unsigned long elr_read(void)
>> +{
>> +     unsigned long elr;
>> +     asm volatile("mrs %0, elr_el1" : "=r" (elr));
>> +     return elr;
>> +}
>
> What?! Why are you writing the ELR here, instead of setting up the pc in
> current saved regs?
>
>> +
>>  /*
>>   * Allow root to disable self-hosted debug from userspace.
>>   * This is useful if you want to connect an external JTAG debugger.
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index 226be77..23d91f1 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -100,7 +100,14 @@
>>       pop     x4, x5
>>       pop     x6, x7
>>       pop     x8, x9
>> -     msr     elr_el1, x21                    // set up the return data
>> +     .if     \el == 1
>> +     mrs     x10, mdscr_el1                  // check if step debug is enabled
>> +     tbnz    x10, #0, 1f
>> +     msr     elr_el1, x21
>> +     .else
>> +     msr     elr_el1, x21                    // set up the return data
>> +     .endif
>> +1:
>
> This is horrible -- if the pt_regs being restored was set up correctly,
> you wouldn't need the conditional code.
>
>>       msr     spsr_el1, x22
>>       .if     \el == 0
>>       msr     sp_el0, x23
>> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
>> index a3a7712..d5e5884 100644
>> --- a/arch/arm64/kernel/kgdb.c
>> +++ b/arch/arm64/kernel/kgdb.c
>> @@ -164,9 +164,31 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, int err_code,
>>                       linux_regs->pc += 4;
>>
>>               compiled_break = 0;
>> +
>> +             /* Disable single step if enabled */
>> +             if (kernel_active_single_step())
>> +                     kernel_disable_single_step();
>> +
>>               err = 0;
>>               break;
>> +     case 's':
>> +             /*
>> +              * Update ESR value with step address passed
>> +              */
>
> Well, the comment is wrong, but I don't think you should be doing this anyway :)
>
> Will

[RFC PATCH 2/2] Aarch64: KGDB: Add Step debugging support

[Vijay Kilari]

Hi Will & Sandeepa

On Mon, Sep 16, 2013 at 6:36 PM, Sandeepa Prabhu
<sandeepa.prabhu@linaro.org> wrote:
> On 16 September 2013 17:00, Will Deacon <will.deacon@arm.com> wrote:
>> On Mon, Sep 16, 2013 at 09:55:50AM +0100, 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 written to ELR
>>> register. If not step address is received from GDB tool,
>>> target assumes next step address is PC and ERET is
>>> executed as part of exception return.
>>>
>>> ELR register content is protected against context restore in
>>> exception return by checking against Software Step debug
>>> bit MDSCR.SS
>>>
>>> Software Step debugging is disabled when 'continue' command
>>> is received
>>>
>>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>> ---
>>>  arch/arm64/include/asm/debug-monitors.h |    3 +++
>>>  arch/arm64/kernel/debug-monitors.c      |   15 +++++++++++++
>>>  arch/arm64/kernel/entry.S               |    9 +++++++-
>>>  arch/arm64/kernel/kgdb.c                |   36 +++++++++++++++++++++++++++++++
>>>  4 files changed, 62 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
>>> index aff3a76..3e4ac0d 100644
>>> --- a/arch/arm64/include/asm/debug-monitors.h
>>> +++ b/arch/arm64/include/asm/debug-monitors.h
>>> @@ -94,6 +94,9 @@ void kernel_enable_single_step(struct pt_regs *regs);
>>>  void kernel_disable_single_step(void);
>>>  int kernel_active_single_step(void);
>>>
>>> +void elr_write(unsigned long elr);
>>> +unsigned long elr_read(void);
>>> +
>>>  #ifdef CONFIG_HAVE_HW_BREAKPOINT
>>>  int reinstall_suspended_bps(struct pt_regs *regs);
>>>  #else
>>> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
>>> index f8b90c0..0408490 100644
>>> --- a/arch/arm64/kernel/debug-monitors.c
>>> +++ b/arch/arm64/kernel/debug-monitors.c
>>> @@ -64,6 +64,21 @@ static u32 mdscr_read(void)
>>>       return mdscr;
>>>  }
>>>
>>> +void elr_write(unsigned long elr)
>>> +{
>>> +     unsigned long flags;
>>> +     local_dbg_save(flags);
>>> +     asm volatile("msr elr_el1, %0" :: "r" (elr));
> I think right way to update step address would be update
> instruction_pointer(regs) from your kgdb handler and "kernel_exit"
> should take care of rest. With this, you don't need to change the low
> level handlers in entry.S/debug-monitors.c) at all.

As per Spec, the step address should be updated in ELR.
where as instruction_pointer(regs) updates pc.
Does PC of pt_regs will be copied to ELR in kernel_exit?

>>> +     local_dbg_restore(flags);
>>> +}
>>> +
>>> +unsigned long elr_read(void)
>>> +{
>>> +     unsigned long elr;
>>> +     asm volatile("mrs %0, elr_el1" : "=r" (elr));
>>> +     return elr;
>>> +}
>>
>> What?! Why are you writing the ELR here, instead of setting up the pc in
>> current saved regs?
>>
>>> +
>>>  /*
>>>   * Allow root to disable self-hosted debug from userspace.
>>>   * This is useful if you want to connect an external JTAG debugger.
>>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>>> index 226be77..23d91f1 100644
>>> --- a/arch/arm64/kernel/entry.S
>>> +++ b/arch/arm64/kernel/entry.S
>>> @@ -100,7 +100,14 @@
>>>       pop     x4, x5
>>>       pop     x6, x7
>>>       pop     x8, x9
>>> -     msr     elr_el1, x21                    // set up the return data
>>> +     .if     \el == 1
>>> +     mrs     x10, mdscr_el1                  // check if step debug is enabled
>>> +     tbnz    x10, #0, 1f
>>> +     msr     elr_el1, x21
>>> +     .else
>>> +     msr     elr_el1, x21                    // set up the return data
>>> +     .endif
>>> +1:
>>
>> This is horrible -- if the pt_regs being restored was set up correctly,
>> you wouldn't need the conditional code.
>>

As per spec (D2.8.4), the target is responding to GDB tool in
exception mode (debug exception).
On receiving GDB command in polling mode, target updates ELR with step
address and
waits for ERET of debug exception exit to jump to the instruction to be stepped.

Does updating pc of pt_regs will work?

Here is the excerpt from ARMv8 document.
To initiate a Software Step debug event, the debugger:
a. Sets SPSR_ELx.SS to 1.
b. Programs the ELR_ELx to point to the instruction to be stepped.
c. Executes an ERET instruction to jump to the instruction to be
stepped. If all of the
conditions defined in Setting PSTATE.SS to 1 to enable software step
to step an item
are met, the ERET instruction copies SPSR_ELx.SS to PSTATE.SS, and the processor
advances to the active-not-pending state

>>>       msr     spsr_el1, x22
>>>       .if     \el == 0
>>>       msr     sp_el0, x23
>>> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
>>> index a3a7712..d5e5884 100644
>>> --- a/arch/arm64/kernel/kgdb.c
>>> +++ b/arch/arm64/kernel/kgdb.c
>>> @@ -164,9 +164,31 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, int err_code,
>>>                       linux_regs->pc += 4;
>>>
>>>               compiled_break = 0;
>>> +
>>> +             /* Disable single step if enabled */
>>> +             if (kernel_active_single_step())
>>> +                     kernel_disable_single_step();
>>> +
>>>               err = 0;
>>>               break;
>>> +     case 's':
>>> +             /*
>>> +              * Update ESR value with step address passed
>>> +              */
>>
>> Well, the comment is wrong, but I don't think you should be doing this anyway :)
>>
 I will correct it


>> Will

[RFC PATCH 2/2] Aarch64: KGDB: Add Step debugging support

[Sandeepa Prabhu]

On 23 September 2013 12:07, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> Hi Will & Sandeepa
>
> On Mon, Sep 16, 2013 at 6:36 PM, Sandeepa Prabhu
> <sandeepa.prabhu@linaro.org> wrote:
>> On 16 September 2013 17:00, Will Deacon <will.deacon@arm.com> wrote:
>>> On Mon, Sep 16, 2013 at 09:55:50AM +0100, 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 written to ELR
>>>> register. If not step address is received from GDB tool,
>>>> target assumes next step address is PC and ERET is
>>>> executed as part of exception return.
>>>>
>>>> ELR register content is protected against context restore in
>>>> exception return by checking against Software Step debug
>>>> bit MDSCR.SS
>>>>
>>>> Software Step debugging is disabled when 'continue' command
>>>> is received
>>>>
>>>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>>> ---
>>>>  arch/arm64/include/asm/debug-monitors.h |    3 +++
>>>>  arch/arm64/kernel/debug-monitors.c      |   15 +++++++++++++
>>>>  arch/arm64/kernel/entry.S               |    9 +++++++-
>>>>  arch/arm64/kernel/kgdb.c                |   36 +++++++++++++++++++++++++++++++
>>>>  4 files changed, 62 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
>>>> index aff3a76..3e4ac0d 100644
>>>> --- a/arch/arm64/include/asm/debug-monitors.h
>>>> +++ b/arch/arm64/include/asm/debug-monitors.h
>>>> @@ -94,6 +94,9 @@ void kernel_enable_single_step(struct pt_regs *regs);
>>>>  void kernel_disable_single_step(void);
>>>>  int kernel_active_single_step(void);
>>>>
>>>> +void elr_write(unsigned long elr);
>>>> +unsigned long elr_read(void);
>>>> +
>>>>  #ifdef CONFIG_HAVE_HW_BREAKPOINT
>>>>  int reinstall_suspended_bps(struct pt_regs *regs);
>>>>  #else
>>>> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
>>>> index f8b90c0..0408490 100644
>>>> --- a/arch/arm64/kernel/debug-monitors.c
>>>> +++ b/arch/arm64/kernel/debug-monitors.c
>>>> @@ -64,6 +64,21 @@ static u32 mdscr_read(void)
>>>>       return mdscr;
>>>>  }
>>>>
>>>> +void elr_write(unsigned long elr)
>>>> +{
>>>> +     unsigned long flags;
>>>> +     local_dbg_save(flags);
>>>> +     asm volatile("msr elr_el1, %0" :: "r" (elr));
>> I think right way to update step address would be update
>> instruction_pointer(regs) from your kgdb handler and "kernel_exit"
>> should take care of rest. With this, you don't need to change the low
>> level handlers in entry.S/debug-monitors.c) at all.
>
> As per Spec, the step address should be updated in ELR.
> where as instruction_pointer(regs) updates pc.
> Does PC of pt_regs will be copied to ELR in kernel_exit?

Yes, ERET in case of single step is no different from normal exception
return, where save copy of ELR_EL1 is restored back.
You can look into entry.S, how pt_regs store/restore happen.

kernel_entry:
...
mrs     x22, elr_el1
mrs     x23, spsr_el1
...
stp     x22, x23, [sp, #S_PC]
....
kernel_exit:
....
 ldp     x21, x22, [sp, #S_PC]
.....
msr     elr_el1, x21                    // set up the return data
...
So you really don't need any conditional code inside entry.S, single
stepping is taken care.

>
>>>> +     local_dbg_restore(flags);
>>>> +}
>>>> +
>>>> +unsigned long elr_read(void)
>>>> +{
>>>> +     unsigned long elr;
>>>> +     asm volatile("mrs %0, elr_el1" : "=r" (elr));
>>>> +     return elr;
>>>> +}
>>>
>>> What?! Why are you writing the ELR here, instead of setting up the pc in
>>> current saved regs?
>>>
>>>> +
>>>>  /*
>>>>   * Allow root to disable self-hosted debug from userspace.
>>>>   * This is useful if you want to connect an external JTAG debugger.
>>>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>>>> index 226be77..23d91f1 100644
>>>> --- a/arch/arm64/kernel/entry.S
>>>> +++ b/arch/arm64/kernel/entry.S
>>>> @@ -100,7 +100,14 @@
>>>>       pop     x4, x5
>>>>       pop     x6, x7
>>>>       pop     x8, x9
>>>> -     msr     elr_el1, x21                    // set up the return data
>>>> +     .if     \el == 1
>>>> +     mrs     x10, mdscr_el1                  // check if step debug is enabled
>>>> +     tbnz    x10, #0, 1f
>>>> +     msr     elr_el1, x21
>>>> +     .else
>>>> +     msr     elr_el1, x21                    // set up the return data
>>>> +     .endif
>>>> +1:
>>>
>>> This is horrible -- if the pt_regs being restored was set up correctly,
>>> you wouldn't need the conditional code.
>>>
>
> As per spec (D2.8.4), the target is responding to GDB tool in
> exception mode (debug exception).
> On receiving GDB command in polling mode, target updates ELR with step
> address and
> waits for ERET of debug exception exit to jump to the instruction to be stepped.
>
> Does updating pc of pt_regs will work?
>
> Here is the excerpt from ARMv8 document.
> To initiate a Software Step debug event, the debugger:
> a. Sets SPSR_ELx.SS to 1.
> b. Programs the ELR_ELx to point to the instruction to be stepped.
> c. Executes an ERET instruction to jump to the instruction to be
> stepped. If all of the
> conditions defined in Setting PSTATE.SS to 1 to enable software step
> to step an item
> are met, the ERET instruction copies SPSR_ELx.SS to PSTATE.SS, and the processor
> advances to the active-not-pending state
>
>>>>       msr     spsr_el1, x22
>>>>       .if     \el == 0
>>>>       msr     sp_el0, x23
>>>> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
>>>> index a3a7712..d5e5884 100644
>>>> --- a/arch/arm64/kernel/kgdb.c
>>>> +++ b/arch/arm64/kernel/kgdb.c
>>>> @@ -164,9 +164,31 @@ int kgdb_arch_handle_exception(int exception_vector, int signo, int err_code,
>>>>                       linux_regs->pc += 4;
>>>>
>>>>               compiled_break = 0;
>>>> +
>>>> +             /* Disable single step if enabled */
>>>> +             if (kernel_active_single_step())
>>>> +                     kernel_disable_single_step();
>>>> +
>>>>               err = 0;
>>>>               break;
>>>> +     case 's':
>>>> +             /*
>>>> +              * Update ESR value with step address passed
>>>> +              */
>>>
>>> Well, the comment is wrong, but I don't think you should be doing this anyway :)
>>>
>  I will correct it
>
>
>>> Will

[RFC PATCH 0/2] Aarch64: KGDB: kernel debugging support

[Will Deacon]

On Mon, Sep 16, 2013 at 09:55:48AM +0100, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Based on the step-handler and break-handler hooks patch
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/189510.html
> KGDB debugging support is added for EL1 debug in Aarch64 mode

Please include that patch in your patch series, since it's not currently
queued anywhere.

Will

[RFC PATCH 0/2] Aarch64: KGDB: kernel debugging support

[Sandeepa Prabhu]

On 16 September 2013 16:53, Will Deacon <will.deacon@arm.com> wrote:
> On Mon, Sep 16, 2013 at 09:55:48AM +0100, vijay.kilari at gmail.com wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>
>> Based on the step-handler and break-handler hooks patch
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/189510.html
>> KGDB debugging support is added for EL1 debug in Aarch64 mode
>
> Please include that patch in your patch series, since it's not currently
> queued anywhere.
Vijay,

My earlier patch
(http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/189510.html)
had review comments and some issues, which I have fixed in Version-2
of it. You may want to rebase your code with V2.
~Sandeepa
>
> Will