On Mon, Sep 23, 2013 at 08:04:48AM +0100, Vijay Kilari wrote:

On Mon, Sep 16, 2013 at 4:59 PM, Will Deacon [off-list ref] wrote:

quoted

On Mon, Sep 16, 2013 at 09:55:49AM +0100, vijay.kilari at gmail.com wrote:

quoted

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?

quoted

quoted

+#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.

quoted

quoted

+#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.

quoted

quoted

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

quoted

quoted

+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