[PATCH v9 0/6] arm64: KGDB Support

[PATCH v9 0/6] arm64: 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.

In first patch, PSTATE.D is set correctly

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

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

Tested with ARM64 simulator

v9: 
 - minor code movement comments fix

v8:
 - fixed comments on local_dbg_{save,restore} macros
 - instruction_pointer() macro to return unsigned long to fix
   compilation warnings

v7:
 - Changes made to set PSTATE.D properly
 - Performed KGDB boot tests
 - Fixed compilation warnings in driver/misc/kgbdts.c

Results:

kgdb boot test:

  [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 from 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
  ~ #

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 (6):
  arm64: Add macros to manage processor debug state
  arm64: KGDB: Add Basic KGDB support
  arm64: KGDB: Add step debugging support
  KGDB: make kgdb_breakpoint() as noinline
  misc: debug: remove compilation warnings
  arm64: KGDB: Add KGDB config

 arch/arm64/Kconfig                      |    1 +
 arch/arm64/include/asm/debug-monitors.h |   64 ++++--
 arch/arm64/include/asm/irqflags.h       |   23 +++
 arch/arm64/include/asm/kgdb.h           |   84 ++++++++
 arch/arm64/include/asm/ptrace.h         |    2 +-
 arch/arm64/kernel/Makefile              |    1 +
 arch/arm64/kernel/debug-monitors.c      |    1 +
 arch/arm64/kernel/kgdb.c                |  336 +++++++++++++++++++++++++++++++
 kernel/debug/debug_core.c               |    2 +-
 9 files changed, 495 insertions(+), 19 deletions(-)
 create mode 100644 arch/arm64/include/asm/kgdb.h
 create mode 100644 arch/arm64/kernel/kgdb.c

-- 
1.7.9.5

[PATCH v9 1/6] arm64: Add macros to manage processor debug state

[vijay.kilari at gmail.com]

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

Add macros to enable and disable to manage PSTATE.D
for debugging. The macros local_dbg_save and local_dbg_restore
are moved to irqflags.h file

KGDB boot tests fail because of PSTATE.D is masked.
unmask it for debugging support

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
---
 arch/arm64/include/asm/debug-monitors.h |   17 -----------------
 arch/arm64/include/asm/irqflags.h       |   23 +++++++++++++++++++++++
 arch/arm64/kernel/debug-monitors.c      |    1 +
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 6231479..ee9f28e 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -43,23 +43,6 @@ enum debug_el {
 #ifndef __ASSEMBLY__
 struct task_struct;
 
-#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)
-
-#define local_dbg_restore(flags)						\
-	do {									\
-		typecheck(unsigned long, flags);				\
-		asm volatile(							\
-		"msr	daif, %0			// local_dbg_restore\n"	\
-		: : "r" (flags) : "memory");					\
-	} while (0)
-
 #define DBG_ARCH_ID_RESERVED	0	/* In case of ptrace ABI updates. */
 
 #define DBG_HOOK_HANDLED	0
diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
index b2fcfbc..11cc941 100644
--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h
@@ -90,5 +90,28 @@ static inline int arch_irqs_disabled_flags(unsigned long flags)
 	return flags & PSR_I_BIT;
 }
 
+/*
+ * save and restore debug state
+ */
+#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)
+
+#define local_dbg_restore(flags)					\
+	do {								\
+		typecheck(unsigned long, flags);			\
+		asm volatile(						\
+		"msr    daif, %0		// local_dbg_restore\n"	\
+		: : "r" (flags) : "memory");				\
+	} while (0)
+
+#define local_dbg_enable()	asm("msr	daifclr, #8" : : : "memory")
+#define local_dbg_disable()	asm("msr	daifset, #8" : : : "memory")
+
 #endif
 #endif
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 23586bd..a86e5b1 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -138,6 +138,7 @@ static void clear_os_lock(void *unused)
 {
 	asm volatile("msr oslar_el1, %0" : : "r" (0));
 	isb();
+	local_dbg_enable();
 }
 
 static int os_lock_notify(struct notifier_block *self,
-- 
1.7.9.5

[PATCH v9 2/6] arm64: 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           |   84 +++++++++
 arch/arm64/kernel/Makefile              |    1 +
 arch/arm64/kernel/kgdb.c                |  288 +++++++++++++++++++++++++++++++
 4 files changed, 420 insertions(+)

diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index ee9f28e..6e9b5b3 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..3c8aafc
--- /dev/null
+++ b/arch/arm64/include/asm/kgdb.h
@@ -0,0 +1,84 @@
+/*
+ * 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  : 64 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)
+#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
+ */
+
+#define BUFMAX			2048
+
+/*
+ * Number of bytes required for gdb_regs buffer.
+ * _GP_REGS: 8 bytes, _FP_REGS: 16 bytes and _EXTRA_REGS: 4 bytes each
+ * GDB fails to connect for size beyond this with error
+ * "'g' packet reply is too long"
+ */
+
+#define NUMREGBYTES	((_GP_REGS * 8) + (_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..4b7a569
--- /dev/null
+++ b/arch/arm64/kernel/kgdb.c
@@ -0,0 +1,288 @@
+/*
+ * 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. Handle same as c packet.
+		 */
+	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 v9 3/6] arm64: 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 |   64 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 56 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index 4b7a569..75c9cf1 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]) {
@@ -162,13 +175,36 @@ 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;
+
+		/*
+		 * 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;
 
-		compiled_break = 0;
+		/*
+		 * Enable single step handling
+		 */
+		if (!kernel_active_single_step())
+			kernel_enable_single_step(linux_regs);
 		err = 0;
 		break;
 	default:
@@ -191,6 +227,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),
@@ -203,6 +245,10 @@ static struct break_hook kgdb_compiled_brkpt_hook = {
 	.fn		= kgdb_compiled_brk_fn
 };
 
+static struct step_hook kgdb_step_hook = {
+	.fn		= kgdb_step_brk_fn
+};
+
 static void kgdb_call_nmi_hook(void *ignored)
 {
 	kgdb_nmicallback(raw_smp_processor_id(), get_irq_regs());
@@ -259,6 +305,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;
 }
 
@@ -271,6 +318,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 v9 4/6] 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>
Acked-by: Jason Wessel <jason.wessel@windriver.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 v9 5/6] misc: debug: remove compilation warnings

[vijay.kilari at gmail.com]

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

typecast instruction_pointer macro to unsigned long to
resolve following compiler warnings like
warning: format '%lx' expects argument of type 'long unsigned int',
but argument 2 has type 'u64' [-Wformat]

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
---
 arch/arm64/include/asm/ptrace.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 0e7fa49..5233966 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -164,7 +164,7 @@ static inline int valid_user_regs(struct user_pt_regs *regs)
 	return 0;
 }
 
-#define instruction_pointer(regs)	(regs)->pc
+#define instruction_pointer(regs)	((unsigned long)(regs)->pc)
 
 #ifdef CONFIG_SMP
 extern unsigned long profile_pc(struct pt_regs *regs);
-- 
1.7.9.5

[PATCH v9 6/6] arm64: KGDB: Add KGDB config

[vijay.kilari at gmail.com]

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

Add HAVE_ARCH_KGDB for arm64 Kconfig

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
---
 arch/arm64/Kconfig |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 6d4dd22..c7a08e0 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -28,6 +28,7 @@ config ARM64
 	select HAVE_HW_BREAKPOINT if PERF_EVENTS
 	select HAVE_MEMBLOCK
 	select HAVE_PERF_EVENTS
+	select HAVE_ARCH_KGDB
 	select IRQ_DOMAIN
 	select MODULES_USE_ELF_RELA
 	select NO_BOOTMEM
-- 
1.7.9.5

[PATCH v9 1/6] arm64: Add macros to manage processor debug state

[Will Deacon]

On Tue, Jan 28, 2014 at 11:20:17AM +0000, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Add macros to enable and disable to manage PSTATE.D
> for debugging. The macros local_dbg_save and local_dbg_restore
> are moved to irqflags.h file
> 
> KGDB boot tests fail because of PSTATE.D is masked.
> unmask it for debugging support
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

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

Cheers,

Will

[PATCH v9 1/6] arm64: Add macros to manage processor debug state

[Vijay Kilari]

Hi Catalin,

   If it is ok, can you please merge these patches

Regards
Vijay

On Wed, Jan 29, 2014 at 4:25 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jan 28, 2014 at 11:20:17AM +0000, vijay.kilari at gmail.com wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>
>> Add macros to enable and disable to manage PSTATE.D
>> for debugging. The macros local_dbg_save and local_dbg_restore
>> are moved to irqflags.h file
>>
>> KGDB boot tests fail because of PSTATE.D is masked.
>> unmask it for debugging support
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> Acked-by: Will Deacon <will.deacon@arm.com>
>
> Cheers,
>
> Will

[PATCH v9 1/6] arm64: Add macros to manage processor debug state

[Catalin Marinas]

On Mon, Feb 17, 2014 at 12:21:45PM +0000, Vijay Kilari wrote:
>    If it is ok, can you please merge these patches

I was about to merge them but I wanted to try first. Enabling kgd tests
at boot time, I get plenty of failures as below. Have you successfully
run the kgdb tests?

kgdbts:RUN singlestep [800/1000]
kgdbts: ERROR PUT: end of test buffer on 'singlestep_breakpoint_test' line 0 expected S0* got $T05thread:01;#07
------------[ cut here ]------------
WARNING: CPU: 1 PID: 1 at /work/Linux/linux-2.6-aarch64/drivers/misc/kgdbts.c:815 run_simple_test+0x28c/0x2d8()
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Tainted: G        W    3.14.0-rc3+ #300
Call trace:
[<ffffffc000087dbc>] dump_backtrace+0x0/0x12c
[<ffffffc000087efc>] show_stack+0x14/0x1c
[<ffffffc00044582c>] dump_stack+0x78/0xc4
[<ffffffc00009552c>] warn_slowpath_common+0x88/0xac
[<ffffffc000095618>] warn_slowpath_null+0x18/0x20
[<ffffffc0002f5cb0>] run_simple_test+0x28c/0x2d8
[<ffffffc0002f5268>] kgdbts_put_char+0x24/0x2c
[<ffffffc0000fcdf8>] put_packet+0xa0/0x118
[<ffffffc0000fe0e0>] gdb_serial_stub+0xb4c/0xd20
[<ffffffc0000fc434>] kgdb_cpu_enter+0x3bc/0x5f4
[<ffffffc0000fc934>] kgdb_handle_exception+0x18c/0x1f0
[<ffffffc000090d50>] kgdb_compiled_brk_fn+0x2c/0x38
[<ffffffc000082158>] brk_handler+0x8c/0xc4
[<ffffffc0000811ec>] do_debug_exception+0x40/0xac
Exception stack(0xffffffc876cbbb80 to 0xffffffc876cbbca0)
bb80: 006a0000 ffffffc0 000003e8 00000000 76cbbd40 ffffffc8 000fb9b8 ffffffc0
bba0: 000002c0 00000000 000d18cc ffffffc0 76cbbc10 ffffffc8 000d1c40 ffffffc0
bbc0: 00641000 ffffffc0 00000001 00000000 00610d40 ffffffc0 00000006 00000000
bbe0: 00000240 00000000 00000020 00000000 00000000 00000000 00000000 00000000
bc00: 76cbbc10 ffffffc8 00000021 00000000 76cbbcb0 ffffffc8 0044337c ffffffc0
bc20: 00648e58 ffffffc0 00000001 00000000 00000000 00000000 00648e60 ffffffc0
bc40: 000070e1 00000000 00000006 00000000 00000030 00000000 006431b0 ffffffc0
bc60: 000070e0 00000000 00000006 00000000 0000006c 00000000 00007005 00000000
bc80: 00000040 00000000 00003648 00000000 00000000 00000000 00000006 00000000
[<ffffffc000083cb4>] el1_dbg+0x18/0x68
[<ffffffc0005d010c>] init_kgdbts+0x24/0x2c
[<ffffffc000081430>] do_one_initcall+0xdc/0x124
[<ffffffc0005b8930>] kernel_init_freeable+0x138/0x1d8
[<ffffffc000440850>] kernel_init+0x10/0xd4

-- 
Catalin

[PATCH v9 1/6] arm64: Add macros to manage processor debug state

[Vijay Kilari]

On Mon, Feb 17, 2014 at 6:31 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Mon, Feb 17, 2014 at 12:21:45PM +0000, Vijay Kilari wrote:
>>    If it is ok, can you please merge these patches
>
> I was about to merge them but I wanted to try first. Enabling kgd tests
> at boot time, I get plenty of failures as below. Have you successfully
> run the kgdb tests?
>

  I re-tested v9 version patches with our internal arm64 simulator and
I don't see these warnings. Also I tested with 3.13 kernel from linaro with v8
foundation model and no errors are seen (logs below).

kgdb: Registered I/O driver kgdbts.
kgdbts:RUN plant and detach test
kgdbts:RUN sw breakpoint test
kgdbts:RUN bad memory access test
kgdbts:RUN singlestep test 1000 iterations
kgdbts:RUN singlestep [0/1000]
kgdbts:RUN singlestep [100/1000]
kgdbts:RUN singlestep [200/1000]
kgdbts:RUN singlestep [300/1000]
kgdbts:RUN singlestep [400/1000]
kgdbts:RUN singlestep [500/1000]
kgdbts:RUN singlestep [600/1000]
kgdbts:RUN singlestep [700/1000]
kgdbts:RUN singlestep [800/1000]
kgdbts:RUN singlestep [900/1000]
kgdbts:RUN do_fork for 100 breakpoints
smc91x 1a000000.ethernet (unregistered net_device): smc91x: IOADDR
ffffff800006a000 doesn't match configuration (300).
.....
EXT4-fs (vda2): re-mounted. Opts: (null)
random: dd urandom read with 13 bits of entropy available
kgdb: Unregistered I/O driver kgdbts, debugger disabled.
smc91x 1a000000.ethernet eth0: link up, 10Mbps, half-duplex, lpa 0x0000
rpcbind: cannot create socket for udp6
rpcbind: cannot create socket for tcp6

Last login: Wed Feb  5 09:32:00 UTC 2014 on tty1
root at genericarmv8:~# uname -a
Linux genericarmv8 3.13.0+ #2 SMP PREEMPT Tue Feb 18 15:53:10 IST 2014
aarch64 GNU/Linux
root at genericarmv8:~#

NOTE: Please check if you have applied all the latest version patches

> kgdbts:RUN singlestep [800/1000]
> kgdbts: ERROR PUT: end of test buffer on 'singlestep_breakpoint_test' line 0 expected S0* got $T05thread:01;#07
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 1 at /work/Linux/linux-2.6-aarch64/drivers/misc/kgdbts.c:815 run_simple_test+0x28c/0x2d8()
> Modules linked in:
> CPU: 1 PID: 1 Comm: swapper/0 Tainted: G        W    3.14.0-rc3+ #300
> Call trace:
> [<ffffffc000087dbc>] dump_backtrace+0x0/0x12c
> [<ffffffc000087efc>] show_stack+0x14/0x1c
> [<ffffffc00044582c>] dump_stack+0x78/0xc4
> [<ffffffc00009552c>] warn_slowpath_common+0x88/0xac
> [<ffffffc000095618>] warn_slowpath_null+0x18/0x20
> [<ffffffc0002f5cb0>] run_simple_test+0x28c/0x2d8
> [<ffffffc0002f5268>] kgdbts_put_char+0x24/0x2c
> [<ffffffc0000fcdf8>] put_packet+0xa0/0x118
> [<ffffffc0000fe0e0>] gdb_serial_stub+0xb4c/0xd20
> [<ffffffc0000fc434>] kgdb_cpu_enter+0x3bc/0x5f4
> [<ffffffc0000fc934>] kgdb_handle_exception+0x18c/0x1f0
> [<ffffffc000090d50>] kgdb_compiled_brk_fn+0x2c/0x38
> [<ffffffc000082158>] brk_handler+0x8c/0xc4
> [<ffffffc0000811ec>] do_debug_exception+0x40/0xac
> Exception stack(0xffffffc876cbbb80 to 0xffffffc876cbbca0)
> bb80: 006a0000 ffffffc0 000003e8 00000000 76cbbd40 ffffffc8 000fb9b8 ffffffc0
> bba0: 000002c0 00000000 000d18cc ffffffc0 76cbbc10 ffffffc8 000d1c40 ffffffc0
> bbc0: 00641000 ffffffc0 00000001 00000000 00610d40 ffffffc0 00000006 00000000
> bbe0: 00000240 00000000 00000020 00000000 00000000 00000000 00000000 00000000
> bc00: 76cbbc10 ffffffc8 00000021 00000000 76cbbcb0 ffffffc8 0044337c ffffffc0
> bc20: 00648e58 ffffffc0 00000001 00000000 00000000 00000000 00648e60 ffffffc0
> bc40: 000070e1 00000000 00000006 00000000 00000030 00000000 006431b0 ffffffc0
> bc60: 000070e0 00000000 00000006 00000000 0000006c 00000000 00007005 00000000
> bc80: 00000040 00000000 00003648 00000000 00000000 00000000 00000006 00000000
> [<ffffffc000083cb4>] el1_dbg+0x18/0x68
> [<ffffffc0005d010c>] init_kgdbts+0x24/0x2c
> [<ffffffc000081430>] do_one_initcall+0xdc/0x124
> [<ffffffc0005b8930>] kernel_init_freeable+0x138/0x1d8
> [<ffffffc000440850>] kernel_init+0x10/0xd4
>
> --
> Catalin

[PATCH v9 1/6] arm64: Add macros to manage processor debug state

[Catalin Marinas]

On Tue, Feb 18, 2014 at 11:29:40AM +0000, Vijay Kilari wrote:
> On Mon, Feb 17, 2014 at 6:31 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Mon, Feb 17, 2014 at 12:21:45PM +0000, Vijay Kilari wrote:
> >>    If it is ok, can you please merge these patches
> >
> > I was about to merge them but I wanted to try first. Enabling kgd tests
> > at boot time, I get plenty of failures as below. Have you successfully
> > run the kgdb tests?
> 
>   I re-tested v9 version patches with our internal arm64 simulator and
> I don't see these warnings. Also I tested with 3.13 kernel from linaro with v8
> foundation model and no errors are seen (logs below).

It's good to know. Could you please rebase against 3.14-rc3 and rerun
the tests? If they work, please send me your .config file that you used
with the foundation model.

(and since you rebase on 3.14-rc3, you could repost a v10 with all the
acks in place so that it's easier for me to merge)

Thanks.

-- 
Catalin

[PATCH v9 1/6] arm64: Add macros to manage processor debug state

[Vijay Kilari]

On Tue, Feb 18, 2014 at 5:32 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Tue, Feb 18, 2014 at 11:29:40AM +0000, Vijay Kilari wrote:
>> On Mon, Feb 17, 2014 at 6:31 PM, Catalin Marinas
>> <catalin.marinas@arm.com> wrote:
>> > On Mon, Feb 17, 2014 at 12:21:45PM +0000, Vijay Kilari wrote:
>> >>    If it is ok, can you please merge these patches
>> >
>> > I was about to merge them but I wanted to try first. Enabling kgd tests
>> > at boot time, I get plenty of failures as below. Have you successfully
>> > run the kgdb tests?
>>
>>   I re-tested v9 version patches with our internal arm64 simulator and
>> I don't see these warnings. Also I tested with 3.13 kernel from linaro with v8
>> foundation model and no errors are seen (logs below).
>
> It's good to know. Could you please rebase against 3.14-rc3 and rerun
> the tests? If they work, please send me your .config file that you used
> with the foundation model.
>
> (and since you rebase on 3.14-rc3, you could repost a v10 with all the
> acks in place so that it's easier for me to merge)
>

 Reposted v10 version of patches which is rebased on 3.14-rc3 kernel.
Test results are as follows on v8 foundation model.

 vda: vda1 vda2
kgdb: Registered I/O driver kgdbts.
kgdbts:RUN plant and detach test
kgdbts:RUN sw breakpoint test
kgdbts:RUN bad memory access test
kgdbts:RUN singlestep test 1000 iterations
kgdbts:RUN singlestep [0/1000]
kgdbts:RUN singlestep [100/1000]
kgdbts:RUN singlestep [200/1000]
kgdbts:RUN singlestep [300/1000]
kgdbts:RUN singlestep [400/1000]
kgdbts:RUN singlestep [500/1000]
kgdbts:RUN singlestep [600/1000]
kgdbts:RUN singlestep [700/1000]
kgdbts:RUN singlestep [800/1000]
kgdbts:RUN singlestep [900/1000]
kgdbts:RUN do_fork for 100 breakpoints
smc91x: not found (-19).
mousedev: PS/2 mouse device common for all mice
TCP: cubic registered
NET: Registered protocol family 17
drivers/rtc/hctosys.c: unable to open rtc device (rtc0)
EXT4-fs (vda2): mounted filesystem with ordered data mode. Opts: (null)
VFS: Mounted root (ext4 filesystem) on device 254:2.
Freeing unused kernel memory: 256K (ffffffc0005d1000 - ffffffc000611000)
udevd[470]: starting version 182
EXT4-fs (vda2): re-mounted. Opts: data=ordered
random: dd urandom read with 11 bits of entropy available
rpcbind: cannot create socket for udp6
rpcbind: cannot create socket for tcp6
rpcbind: cannot get uid of '': Success
kgdb: Unregistered I/O driver kgdbts, debugger disabled.

Last login: Mon Jan 27 08:00:01 UTC 2014 on tty1
root at genericarmv8:~# uname -a
Linux genericarmv8 3.14.0-rc3+ #3 SMP PREEMPT Wed Feb 19 09:06:22 IST
2014 aarch64 GNU/Linux
root at genericarmv8:~#

.config is attached (renamed as config)

> Thanks.
>
> --
> Catalin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: config
Type: application/octet-stream
Size: 37841 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140219/a0dfd53f/attachment-0001.obj>

[PATCH v9 1/6] arm64: Add macros to manage processor debug state

[Catalin Marinas]

On Wed, Feb 19, 2014 at 04:44:05AM +0000, Vijay Kilari wrote:
> On Tue, Feb 18, 2014 at 5:32 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Tue, Feb 18, 2014 at 11:29:40AM +0000, Vijay Kilari wrote:
> >> On Mon, Feb 17, 2014 at 6:31 PM, Catalin Marinas
> >> <catalin.marinas@arm.com> wrote:
> >> > On Mon, Feb 17, 2014 at 12:21:45PM +0000, Vijay Kilari wrote:
> >> >>    If it is ok, can you please merge these patches
> >> >
> >> > I was about to merge them but I wanted to try first. Enabling kgd tests
> >> > at boot time, I get plenty of failures as below. Have you successfully
> >> > run the kgdb tests?
> >>
> >>   I re-tested v9 version patches with our internal arm64 simulator and
> >> I don't see these warnings. Also I tested with 3.13 kernel from linaro with v8
> >> foundation model and no errors are seen (logs below).
> >
> > It's good to know. Could you please rebase against 3.14-rc3 and rerun
> > the tests? If they work, please send me your .config file that you used
> > with the foundation model.
> >
> > (and since you rebase on 3.14-rc3, you could repost a v10 with all the
> > acks in place so that it's easier for me to merge)
> 
>  Reposted v10 version of patches which is rebased on 3.14-rc3 kernel.
> Test results are as follows on v8 foundation model.
> 
>  vda: vda1 vda2
> kgdb: Registered I/O driver kgdbts.
> kgdbts:RUN plant and detach test
> kgdbts:RUN sw breakpoint test
> kgdbts:RUN bad memory access test
> kgdbts:RUN singlestep test 1000 iterations
> kgdbts:RUN singlestep [0/1000]
> kgdbts:RUN singlestep [100/1000]
> kgdbts:RUN singlestep [200/1000]
> kgdbts:RUN singlestep [300/1000]
> kgdbts:RUN singlestep [400/1000]
> kgdbts:RUN singlestep [500/1000]
> kgdbts:RUN singlestep [600/1000]
> kgdbts:RUN singlestep [700/1000]
> kgdbts:RUN singlestep [800/1000]
> kgdbts:RUN singlestep [900/1000]
> kgdbts:RUN do_fork for 100 breakpoints

OK, I eventually managed to reproduce this. But by running with two
CPUs, I get (during the do_fork() tests):

BUG: scheduling while atomic: kworker/u8:0/6/0x00000002
Modules linked in:
CPU: 1 PID: 6 Comm: kworker/u8:0 Not tainted 3.14.0-rc3+ #306
Workqueue: khelper __call_usermodehelper
Call trace:
[<ffffffc000087dbc>] dump_backtrace+0x0/0x12c
[<ffffffc000087efc>] show_stack+0x14/0x1c
[<ffffffc00043c224>] dump_stack+0x78/0xc4
[<ffffffc000439b48>] __schedule_bug+0x40/0x54
[<ffffffc00043d67c>] __schedule+0x514/0x604
[<ffffffc00043d794>] schedule+0x28/0x78
[<ffffffc00043cc90>] schedule_timeout+0x170/0x1bc
[<ffffffc00043e16c>] wait_for_common+0xc0/0x14c
[<ffffffc00043e280>] wait_for_completion_killable+0x14/0x28
[<ffffffc0000942f8>] do_fork+0x158/0x2a8
[<ffffffc000094478>] kernel_thread+0x30/0x38
[<ffffffc0000a842c>] __call_usermodehelper+0x34/0xa8
[<ffffffc0000ab300>] process_one_work+0x118/0x354
[<ffffffc0000abfcc>] worker_thread+0x13c/0x3c0
[<ffffffc0000b1e84>] kthread+0xd4/0xe8


It gets much worse if I run with two CPUs and CONFIG_KGDB_KDB enabled
(but fine with a single CPU).

So no need to post another series for now but please check the multi-CPU
case as well and send a separate fix. I'll dig a bit on my side as well.

-- 
Catalin

[PATCH v9 1/6] arm64: Add macros to manage processor debug state

[Catalin Marinas]

On Wed, Feb 19, 2014 at 11:31:57AM +0000, Catalin Marinas wrote:
> On Wed, Feb 19, 2014 at 04:44:05AM +0000, Vijay Kilari wrote:
> >  Reposted v10 version of patches which is rebased on 3.14-rc3 kernel.
> > Test results are as follows on v8 foundation model.
> > 
> >  vda: vda1 vda2
> > kgdb: Registered I/O driver kgdbts.
> > kgdbts:RUN plant and detach test
> > kgdbts:RUN sw breakpoint test
> > kgdbts:RUN bad memory access test
> > kgdbts:RUN singlestep test 1000 iterations
> > kgdbts:RUN singlestep [0/1000]
> > kgdbts:RUN singlestep [100/1000]
> > kgdbts:RUN singlestep [200/1000]
> > kgdbts:RUN singlestep [300/1000]
> > kgdbts:RUN singlestep [400/1000]
> > kgdbts:RUN singlestep [500/1000]
> > kgdbts:RUN singlestep [600/1000]
> > kgdbts:RUN singlestep [700/1000]
> > kgdbts:RUN singlestep [800/1000]
> > kgdbts:RUN singlestep [900/1000]
> > kgdbts:RUN do_fork for 100 breakpoints
> 
> OK, I eventually managed to reproduce this. But by running with two
> CPUs, I get (during the do_fork() tests):
> 
> BUG: scheduling while atomic: kworker/u8:0/6/0x00000002
> Modules linked in:
> CPU: 1 PID: 6 Comm: kworker/u8:0 Not tainted 3.14.0-rc3+ #306
> Workqueue: khelper __call_usermodehelper
> Call trace:
> [<ffffffc000087dbc>] dump_backtrace+0x0/0x12c
> [<ffffffc000087efc>] show_stack+0x14/0x1c
> [<ffffffc00043c224>] dump_stack+0x78/0xc4
> [<ffffffc000439b48>] __schedule_bug+0x40/0x54
> [<ffffffc00043d67c>] __schedule+0x514/0x604
> [<ffffffc00043d794>] schedule+0x28/0x78
> [<ffffffc00043cc90>] schedule_timeout+0x170/0x1bc
> [<ffffffc00043e16c>] wait_for_common+0xc0/0x14c
> [<ffffffc00043e280>] wait_for_completion_killable+0x14/0x28
> [<ffffffc0000942f8>] do_fork+0x158/0x2a8
> [<ffffffc000094478>] kernel_thread+0x30/0x38
> [<ffffffc0000a842c>] __call_usermodehelper+0x34/0xa8
> [<ffffffc0000ab300>] process_one_work+0x118/0x354
> [<ffffffc0000abfcc>] worker_thread+0x13c/0x3c0
> [<ffffffc0000b1e84>] kthread+0xd4/0xe8
> 
> 
> It gets much worse if I run with two CPUs and CONFIG_KGDB_KDB enabled
> (but fine with a single CPU).
> 
> So no need to post another series for now but please check the multi-CPU
> case as well and send a separate fix. I'll dig a bit on my side as well.

So far I'm done with the investigation. It looks to me like one of the
kgdb tests, kgdb core or the arm64 back-end (or maybe more than one) is
not SMP safe. The errors either appear or disappear based on the printks
I put through the kgdb test or other config options which I enable.

Could you please look into making the kgdb back-end SMP-safe?

Thanks.

-- 
Catalin

[PATCH v9 1/6] arm64: Add macros to manage processor debug state

[Will Deacon]

On Wed, Feb 19, 2014 at 04:03:59PM +0000, Catalin Marinas wrote:
> On Wed, Feb 19, 2014 at 11:31:57AM +0000, Catalin Marinas wrote:
> > BUG: scheduling while atomic: kworker/u8:0/6/0x00000002
> > Modules linked in:
> > CPU: 1 PID: 6 Comm: kworker/u8:0 Not tainted 3.14.0-rc3+ #306
> > Workqueue: khelper __call_usermodehelper
> > Call trace:
> > [<ffffffc000087dbc>] dump_backtrace+0x0/0x12c
> > [<ffffffc000087efc>] show_stack+0x14/0x1c
> > [<ffffffc00043c224>] dump_stack+0x78/0xc4
> > [<ffffffc000439b48>] __schedule_bug+0x40/0x54
> > [<ffffffc00043d67c>] __schedule+0x514/0x604
> > [<ffffffc00043d794>] schedule+0x28/0x78
> > [<ffffffc00043cc90>] schedule_timeout+0x170/0x1bc
> > [<ffffffc00043e16c>] wait_for_common+0xc0/0x14c
> > [<ffffffc00043e280>] wait_for_completion_killable+0x14/0x28
> > [<ffffffc0000942f8>] do_fork+0x158/0x2a8
> > [<ffffffc000094478>] kernel_thread+0x30/0x38
> > [<ffffffc0000a842c>] __call_usermodehelper+0x34/0xa8
> > [<ffffffc0000ab300>] process_one_work+0x118/0x354
> > [<ffffffc0000abfcc>] worker_thread+0x13c/0x3c0
> > [<ffffffc0000b1e84>] kthread+0xd4/0xe8
> > 
> > 
> > It gets much worse if I run with two CPUs and CONFIG_KGDB_KDB enabled
> > (but fine with a single CPU).
> > 
> > So no need to post another series for now but please check the multi-CPU
> > case as well and send a separate fix. I'll dig a bit on my side as well.
> 
> So far I'm done with the investigation. It looks to me like one of the
> kgdb tests, kgdb core or the arm64 back-end (or maybe more than one) is
> not SMP safe. The errors either appear or disappear based on the printks
> I put through the kgdb test or other config options which I enable.
> 
> Could you please look into making the kgdb back-end SMP-safe?

There are certainly potential SMP problems in the back-end, which I asked
about in the initial series:

  http://lkml.kernel.org/r/CALicx6v1eGHRwWPrjzihzBZxCu8t1vpMoq-YfutSm4mRmP6gEQ at mail.gmail.com

The reply from Vijay suggested that everything is confined to a single CPU.

Will

[PATCH v9 1/6] arm64: Add macros to manage processor debug state

[Vijay Kilari]

Hi Will,

On Wed, Feb 19, 2014 at 9:42 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, Feb 19, 2014 at 04:03:59PM +0000, Catalin Marinas wrote:
>> On Wed, Feb 19, 2014 at 11:31:57AM +0000, Catalin Marinas wrote:
>> > BUG: scheduling while atomic: kworker/u8:0/6/0x00000002
>> > Modules linked in:
>> > CPU: 1 PID: 6 Comm: kworker/u8:0 Not tainted 3.14.0-rc3+ #306
>> > Workqueue: khelper __call_usermodehelper
>> > Call trace:
>> > [<ffffffc000087dbc>] dump_backtrace+0x0/0x12c
>> > [<ffffffc000087efc>] show_stack+0x14/0x1c
>> > [<ffffffc00043c224>] dump_stack+0x78/0xc4
>> > [<ffffffc000439b48>] __schedule_bug+0x40/0x54
>> > [<ffffffc00043d67c>] __schedule+0x514/0x604
>> > [<ffffffc00043d794>] schedule+0x28/0x78
>> > [<ffffffc00043cc90>] schedule_timeout+0x170/0x1bc
>> > [<ffffffc00043e16c>] wait_for_common+0xc0/0x14c
>> > [<ffffffc00043e280>] wait_for_completion_killable+0x14/0x28
>> > [<ffffffc0000942f8>] do_fork+0x158/0x2a8
>> > [<ffffffc000094478>] kernel_thread+0x30/0x38
>> > [<ffffffc0000a842c>] __call_usermodehelper+0x34/0xa8
>> > [<ffffffc0000ab300>] process_one_work+0x118/0x354
>> > [<ffffffc0000abfcc>] worker_thread+0x13c/0x3c0
>> > [<ffffffc0000b1e84>] kthread+0xd4/0xe8
>> >
>> >
>> > It gets much worse if I run with two CPUs and CONFIG_KGDB_KDB enabled
>> > (but fine with a single CPU).
>> >
>> > So no need to post another series for now but please check the multi-CPU
>> > case as well and send a separate fix. I'll dig a bit on my side as well.
>>
>> So far I'm done with the investigation. It looks to me like one of the
>> kgdb tests, kgdb core or the arm64 back-end (or maybe more than one) is
>> not SMP safe. The errors either appear or disappear based on the printks
>> I put through the kgdb test or other config options which I enable.
>>
>> Could you please look into making the kgdb back-end SMP-safe?
>
> There are certainly potential SMP problems in the back-end, which I asked
> about in the initial series:
>
>   http://lkml.kernel.org/r/CALicx6v1eGHRwWPrjzihzBZxCu8t1vpMoq-YfutSm4mRmP6gEQ at mail.gmail.com
>
> The reply from Vijay suggested that everything is confined to a single CPU.
>

  This issue is seen only when kgdb test is triggered on secondary cpu's.
when executed on cpu0 issue is not seen. Though cpu0 triggers kgdb tests
the do_fork test might trigger debug exception on other cpu's as well,
which fails.

For the patch 1 you suggested me to call local_dbg_enable() from
clear_os_mask() function
as below

static void clear_os_lock(void *unused)
{
        asm volatile("msr oslar_el1, %0" : : "r" (0));
        isb();
        local_dbg_enable();
}

This is an SMP call. So when more than one core is enabled, this
local_dbg_enable() is called
from SMP call context, which is el1_irq context for secondary cpu's.
So PSTATE.D flag is unmasked
only in irq context but not in normal context.

For CPU0 this clear_os_lock() is not called from smp call. it is
directly called in debug_monitors_init()
call. So PSTATE.D is unmasked for CPU0 and hence kgdb tests passed
when triggered only on
cpu0.

static int debug_monitors_init(void)
{
        /* Clear the OS lock. */
        smp_call_function(clear_os_lock, NULL, 1);
        clear_os_lock(NULL);

        /* Register hotplug handler. */
        register_cpu_notifier(&os_lock_nb);
        return 0;
}

When I made below patch it works. I have tested with 4 cores on foundation model

--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -160,6 +160,8 @@ asmlinkage void secondary_start_kernel(void)
        set_cpu_online(cpu, true);
        complete(&cpu_running);

+       local_dbg_enable();
        local_irq_enable();
        local_async_enable();

kgdb: Registered I/O driver kgdbts.
kgdbts:RUN plant and detach test
kgdbts:RUN sw breakpoint test
kgdbts:RUN bad memory access test
kgdbts:RUN singlestep test 1000 iterations
kgdbts:RUN singlestep [0/1000]
kgdbts:RUN singlestep [100/1000]
kgdbts:RUN singlestep [200/1000]
kgdbts:RUN singlestep [300/1000]
kgdbts:RUN singlestep [400/1000]
kgdbts:RUN singlestep [500/1000]
kgdbts:RUN singlestep [600/1000]
kgdbts:RUN singlestep [700/1000]
kgdbts:RUN singlestep [800/1000]
kgdbts:RUN singlestep [900/1000]
kgdbts:RUN do_fork for 100 breakpoints
smc91x: not found (-19).
mousedev: PS/2 mouse device common for all mice
TCP: cubic registered
NET: Registered protocol family 17
drivers/rtc/hctosys.c: unable to open rtc device (rtc0)
EXT4-fs (vda2): recovery complete
EXT4-fs (vda2): mounted filesystem with ordered data mode. Opts: (null)
VFS: Mounted root (ext4 filesystem) on device 254:2.
Freeing unused kernel memory: 256K (ffffffc0005d1000 - ffffffc000611000)
udevd[476]: starting version 182
EXT4-fs (vda2): re-mounted. Opts: data=ordered
random: dd urandom read with 11 bits of entropy available
rpcbind: cannot create socket for udp6
rpcbind: cannot create socket for tcp6
rpcbind: cannot get uid of '': Success

Last login: Mon Jan 27 08:00:01 UTC 2014 on tty1
root at genericarmv8:~# kgdb: Unregistered I/O driver kgdbts, debugger disabled.

root at genericarmv8:~# uname -a
Linux genericarmv8 3.14.0-rc3+ #18 SMP PREEMPT Thu Feb 20 11:53:17 IST
2014 aarch64 GNU/Linux
root at genericarmv8:~# cat /proc/cpuinfo
Processor       : AArch64 Processor rev 0 (aarch64)
processor       : 0
processor       : 1
processor       : 2
processor       : 3
Features        : fp asimd evtstrm
CPU implementer : 0x41
CPU architecture: AArch64
CPU variant     : 0x0
CPU part        : 0xd00
CPU revision    : 0

Hardware        : Foundation-v8A
root at genericarmv8:~#

- Vijay

> Will

[PATCH v9 1/6] arm64: Add macros to manage processor debug state

[Will Deacon]

On Thu, Feb 20, 2014 at 06:58:25AM +0000, Vijay Kilari wrote:
> Hi Will,

Hi Vijay,

> On Wed, Feb 19, 2014 at 9:42 PM, Will Deacon <will.deacon@arm.com> wrote:
> > The reply from Vijay suggested that everything is confined to a single CPU.
> 
>   This issue is seen only when kgdb test is triggered on secondary cpu's.
> when executed on cpu0 issue is not seen. Though cpu0 triggers kgdb tests
> the do_fork test might trigger debug exception on other cpu's as well,
> which fails.
> 
> For the patch 1 you suggested me to call local_dbg_enable() from
> clear_os_mask() function
> as below
> 
> static void clear_os_lock(void *unused)
> {
>         asm volatile("msr oslar_el1, %0" : : "r" (0));
>         isb();
>         local_dbg_enable();
> }
> 
> This is an SMP call. So when more than one core is enabled, this
> local_dbg_enable() is called
> from SMP call context, which is el1_irq context for secondary cpu's.
> So PSTATE.D flag is unmasked
> only in irq context but not in normal context.

Aha, well spotted!

> For CPU0 this clear_os_lock() is not called from smp call. it is
> directly called in debug_monitors_init()
> call. So PSTATE.D is unmasked for CPU0 and hence kgdb tests passed
> when triggered only on
> cpu0.
> 
> static int debug_monitors_init(void)
> {
>         /* Clear the OS lock. */
>         smp_call_function(clear_os_lock, NULL, 1);
>         clear_os_lock(NULL);
> 
>         /* Register hotplug handler. */
>         register_cpu_notifier(&os_lock_nb);
>         return 0;
> }
> 
> When I made below patch it works. I have tested with 4 cores on foundation model

The main change here is that we enable debug exceptions before we unlock the
os lock. That should be fine, since everything apart from software
breakpoint exceptions are masked when the lock is locked.

> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -160,6 +160,8 @@ asmlinkage void secondary_start_kernel(void)
>         set_cpu_online(cpu, true);
>         complete(&cpu_running);
> 
> +       local_dbg_enable();
>         local_irq_enable();
>         local_async_enable();

The only thing to add then moving the isb(); local_dbg_enable() out of
clear_os_lock and into debug_monitors_init. You can probably make the
smp_call_function an on_each_cpu too.

Does that make sense?

Will

[PATCH v9 1/6] arm64: Add macros to manage processor debug state

[Catalin Marinas]

On Thu, Feb 20, 2014 at 06:58:25AM +0000, Vijay Kilari wrote:
> When I made below patch it works. I have tested with 4 cores on foundation model
> 
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -160,6 +160,8 @@ asmlinkage void secondary_start_kernel(void)
>         set_cpu_online(cpu, true);
>         complete(&cpu_running);
> 
> +       local_dbg_enable();
>         local_irq_enable();
>         local_async_enable();

I tested this as well and seems to work fine. I'll let Will comment on
whether this fix is enough and if yes, I'll fold it into one of your
patches (or push it on top of them).

-- 
Catalin

[PATCH v9 1/6] arm64: Add macros to manage processor debug state

[Vijay Kilari]

On Thu, Feb 20, 2014 at 5:12 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Feb 20, 2014 at 06:58:25AM +0000, Vijay Kilari wrote:
>> Hi Will,
>
> Hi Vijay,
>
>> On Wed, Feb 19, 2014 at 9:42 PM, Will Deacon <will.deacon@arm.com> wrote:
>> > The reply from Vijay suggested that everything is confined to a single CPU.
>>
>>   This issue is seen only when kgdb test is triggered on secondary cpu's.
>> when executed on cpu0 issue is not seen. Though cpu0 triggers kgdb tests
>> the do_fork test might trigger debug exception on other cpu's as well,
>> which fails.
>>
>> For the patch 1 you suggested me to call local_dbg_enable() from
>> clear_os_mask() function
>> as below
>>
>> static void clear_os_lock(void *unused)
>> {
>>         asm volatile("msr oslar_el1, %0" : : "r" (0));
>>         isb();
>>         local_dbg_enable();
>> }
>>
>> This is an SMP call. So when more than one core is enabled, this
>> local_dbg_enable() is called
>> from SMP call context, which is el1_irq context for secondary cpu's.
>> So PSTATE.D flag is unmasked
>> only in irq context but not in normal context.
>
> Aha, well spotted!
>
>> For CPU0 this clear_os_lock() is not called from smp call. it is
>> directly called in debug_monitors_init()
>> call. So PSTATE.D is unmasked for CPU0 and hence kgdb tests passed
>> when triggered only on
>> cpu0.
>>
>> static int debug_monitors_init(void)
>> {
>>         /* Clear the OS lock. */
>>         smp_call_function(clear_os_lock, NULL, 1);
>>         clear_os_lock(NULL);
>>
>>         /* Register hotplug handler. */
>>         register_cpu_notifier(&os_lock_nb);
>>         return 0;
>> }
>>
>> When I made below patch it works. I have tested with 4 cores on foundation model
>
> The main change here is that we enable debug exceptions before we unlock the
> os lock. That should be fine, since everything apart from software
> breakpoint exceptions are masked when the lock is locked.
>
>> --- a/arch/arm64/kernel/smp.c
>> +++ b/arch/arm64/kernel/smp.c
>> @@ -160,6 +160,8 @@ asmlinkage void secondary_start_kernel(void)
>>         set_cpu_online(cpu, true);
>>         complete(&cpu_running);
>>
>> +       local_dbg_enable();
>>         local_irq_enable();
>>         local_async_enable();
>
> The only thing to add then moving the isb(); local_dbg_enable() out of
> clear_os_lock and into debug_monitors_init. You can probably make the
> smp_call_function an on_each_cpu too.
>
> Does that make sense?

Its ok for me.  Isn't require to have isb() after clearing os lock for
every cpu?
Just having isb on cpu0 after clearing os lock is enough?

--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -137,8 +137,6 @@ void disable_debug_monitors(enum debug_el el)
 static void clear_os_lock(void *unused)
 {
        asm volatile("msr oslar_el1, %0" : : "r" (0));
-       isb();
-       local_dbg_enable();
 }

 static int os_lock_notify(struct notifier_block *self,
@@ -157,8 +155,9 @@ static struct notifier_block os_lock_nb = {
 static int debug_monitors_init(void)
 {
        /* Clear the OS lock. */
-       smp_call_function(clear_os_lock, NULL, 1);
-       clear_os_lock(NULL);
+       on_each_cpu(clear_os_lock, NULL, 1);
+       isb();
+       local_dbg_enable();

        /* Register hotplug handler. */
        register_cpu_notifier(&os_lock_nb);
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 7cfb92a..5070dc3 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -160,6 +160,7 @@ asmlinkage void secondary_start_kernel(void)
        set_cpu_online(cpu, true);
        complete(&cpu_running);

+       local_dbg_enable();
        local_irq_enable();
        local_async_enable();

Is this ok?

>
> Will

[PATCH v9 1/6] arm64: Add macros to manage processor debug state

[Will Deacon]

On Thu, Feb 20, 2014 at 12:22:14PM +0000, Vijay Kilari wrote:
> On Thu, Feb 20, 2014 at 5:12 PM, Will Deacon <will.deacon@arm.com> wrote:
> > On Thu, Feb 20, 2014 at 06:58:25AM +0000, Vijay Kilari wrote:
> >> --- a/arch/arm64/kernel/smp.c
> >> +++ b/arch/arm64/kernel/smp.c
> >> @@ -160,6 +160,8 @@ asmlinkage void secondary_start_kernel(void)
> >>         set_cpu_online(cpu, true);
> >>         complete(&cpu_running);
> >>
> >> +       local_dbg_enable();
> >>         local_irq_enable();
> >>         local_async_enable();
> >
> > The only thing to add then moving the isb(); local_dbg_enable() out of
> > clear_os_lock and into debug_monitors_init. You can probably make the
> > smp_call_function an on_each_cpu too.
> >
> > Does that make sense?
> 
> Its ok for me.  Isn't require to have isb() after clearing os lock for
> every cpu?
> Just having isb on cpu0 after clearing os lock is enough?

They'll synchronise on return from the IPI.

> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -137,8 +137,6 @@ void disable_debug_monitors(enum debug_el el)
>  static void clear_os_lock(void *unused)
>  {
>         asm volatile("msr oslar_el1, %0" : : "r" (0));
> -       isb();
> -       local_dbg_enable();
>  }
> 
>  static int os_lock_notify(struct notifier_block *self,
> @@ -157,8 +155,9 @@ static struct notifier_block os_lock_nb = {
>  static int debug_monitors_init(void)
>  {
>         /* Clear the OS lock. */
> -       smp_call_function(clear_os_lock, NULL, 1);
> -       clear_os_lock(NULL);
> +       on_each_cpu(clear_os_lock, NULL, 1);
> +       isb();
> +       local_dbg_enable();
> 
>         /* Register hotplug handler. */
>         register_cpu_notifier(&os_lock_nb);
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 7cfb92a..5070dc3 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -160,6 +160,7 @@ asmlinkage void secondary_start_kernel(void)
>         set_cpu_online(cpu, true);
>         complete(&cpu_running);
> 
> +       local_dbg_enable();
>         local_irq_enable();
>         local_async_enable();
> 
> Is this ok?

Looks good to me:

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

Thanks,

Will

[PATCH v9 1/6] arm64: Add macros to manage processor debug state

[Catalin Marinas]

On Thu, Feb 20, 2014 at 03:56:00PM +0000, Will Deacon wrote:
> On Thu, Feb 20, 2014 at 12:22:14PM +0000, Vijay Kilari wrote:
> > On Thu, Feb 20, 2014 at 5:12 PM, Will Deacon <will.deacon@arm.com> wrote:
> > > On Thu, Feb 20, 2014 at 06:58:25AM +0000, Vijay Kilari wrote:
> > >> --- a/arch/arm64/kernel/smp.c
> > >> +++ b/arch/arm64/kernel/smp.c
> > >> @@ -160,6 +160,8 @@ asmlinkage void secondary_start_kernel(void)
> > >>         set_cpu_online(cpu, true);
> > >>         complete(&cpu_running);
> > >>
> > >> +       local_dbg_enable();
> > >>         local_irq_enable();
> > >>         local_async_enable();
> > >
> > > The only thing to add then moving the isb(); local_dbg_enable() out of
> > > clear_os_lock and into debug_monitors_init. You can probably make the
> > > smp_call_function an on_each_cpu too.
> > >
> > > Does that make sense?
> > 
> > Its ok for me.  Isn't require to have isb() after clearing os lock for
> > every cpu?
> > Just having isb on cpu0 after clearing os lock is enough?
> 
> They'll synchronise on return from the IPI.
> 
> > --- a/arch/arm64/kernel/debug-monitors.c
> > +++ b/arch/arm64/kernel/debug-monitors.c
> > @@ -137,8 +137,6 @@ void disable_debug_monitors(enum debug_el el)
> >  static void clear_os_lock(void *unused)
> >  {
> >         asm volatile("msr oslar_el1, %0" : : "r" (0));
> > -       isb();
> > -       local_dbg_enable();
> >  }
> > 
> >  static int os_lock_notify(struct notifier_block *self,
> > @@ -157,8 +155,9 @@ static struct notifier_block os_lock_nb = {
> >  static int debug_monitors_init(void)
> >  {
> >         /* Clear the OS lock. */
> > -       smp_call_function(clear_os_lock, NULL, 1);
> > -       clear_os_lock(NULL);
> > +       on_each_cpu(clear_os_lock, NULL, 1);
> > +       isb();
> > +       local_dbg_enable();
> > 
> >         /* Register hotplug handler. */
> >         register_cpu_notifier(&os_lock_nb);
> > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> > index 7cfb92a..5070dc3 100644
> > --- a/arch/arm64/kernel/smp.c
> > +++ b/arch/arm64/kernel/smp.c
> > @@ -160,6 +160,7 @@ asmlinkage void secondary_start_kernel(void)
> >         set_cpu_online(cpu, true);
> >         complete(&cpu_running);
> > 
> > +       local_dbg_enable();
> >         local_irq_enable();
> >         local_async_enable();
> > 
> > Is this ok?
> 
> Looks good to me:
> 
>   Acked-by: Will Deacon <will.deacon@arm.com>

Vijay, could you please post a commit log together with the above patch
(and Will's ack) so that I can merge it on top of your other patches?

Thanks.

-- 
Catalin

[PATCH v9 1/6] arm64: Add macros to manage processor debug state

[Vijay Kilari]

Hi Catalin,

On Thu, Feb 20, 2014 at 9:57 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Thu, Feb 20, 2014 at 03:56:00PM +0000, Will Deacon wrote:
>> On Thu, Feb 20, 2014 at 12:22:14PM +0000, Vijay Kilari wrote:
>> > On Thu, Feb 20, 2014 at 5:12 PM, Will Deacon <will.deacon@arm.com> wrote:
>> > > On Thu, Feb 20, 2014 at 06:58:25AM +0000, Vijay Kilari wrote:
>> > >> --- a/arch/arm64/kernel/smp.c
>> > >> +++ b/arch/arm64/kernel/smp.c
>> > >> @@ -160,6 +160,8 @@ asmlinkage void secondary_start_kernel(void)
>> > >>         set_cpu_online(cpu, true);
>> > >>         complete(&cpu_running);
>> > >>
>> > >> +       local_dbg_enable();
>> > >>         local_irq_enable();
>> > >>         local_async_enable();
>> > >
>> > > The only thing to add then moving the isb(); local_dbg_enable() out of
>> > > clear_os_lock and into debug_monitors_init. You can probably make the
>> > > smp_call_function an on_each_cpu too.
>> > >
>> > > Does that make sense?
>> >
>> > Its ok for me.  Isn't require to have isb() after clearing os lock for
>> > every cpu?
>> > Just having isb on cpu0 after clearing os lock is enough?
>>
>> They'll synchronise on return from the IPI.
>>
>> > --- a/arch/arm64/kernel/debug-monitors.c
>> > +++ b/arch/arm64/kernel/debug-monitors.c
>> > @@ -137,8 +137,6 @@ void disable_debug_monitors(enum debug_el el)
>> >  static void clear_os_lock(void *unused)
>> >  {
>> >         asm volatile("msr oslar_el1, %0" : : "r" (0));
>> > -       isb();
>> > -       local_dbg_enable();
>> >  }
>> >
>> >  static int os_lock_notify(struct notifier_block *self,
>> > @@ -157,8 +155,9 @@ static struct notifier_block os_lock_nb = {
>> >  static int debug_monitors_init(void)
>> >  {
>> >         /* Clear the OS lock. */
>> > -       smp_call_function(clear_os_lock, NULL, 1);
>> > -       clear_os_lock(NULL);
>> > +       on_each_cpu(clear_os_lock, NULL, 1);
>> > +       isb();
>> > +       local_dbg_enable();
>> >
>> >         /* Register hotplug handler. */
>> >         register_cpu_notifier(&os_lock_nb);
>> > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
>> > index 7cfb92a..5070dc3 100644
>> > --- a/arch/arm64/kernel/smp.c
>> > +++ b/arch/arm64/kernel/smp.c
>> > @@ -160,6 +160,7 @@ asmlinkage void secondary_start_kernel(void)
>> >         set_cpu_online(cpu, true);
>> >         complete(&cpu_running);
>> >
>> > +       local_dbg_enable();
>> >         local_irq_enable();
>> >         local_async_enable();
>> >
>> > Is this ok?
>>
>> Looks good to me:
>>
>>   Acked-by: Will Deacon <will.deacon@arm.com>
>
> Vijay, could you please post a commit log together with the above patch
> (and Will's ack) so that I can merge it on top of your other patches?
>

  I have sent a patch for this fix named, " [PATCH 1/1] arm64: enable processor
debug state for secondary cpus".
Please apply this on top of other patches.

Thanks
- Vijay
> Thanks.
>
> --
> Catalin