AArch64: Add single-step and breakpoint handler hooks

PATCH New Sandeepa Prabhu 2 participants 5 messages Aug 6, 2013 – Aug 20, 2013 arm64

lore

Share & tools

lore

Sandeepa Prabhu

author

2013-08-06 06:12 UTC #0 lore raw

PATCH 1/1 AArch64: Add single-step and breakpoint handler hooks

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

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

Reworked the debug exception handler in entry.S: do_dbg to pass the correct break/step address to the handlers, i.e. FAR_EL1 if exception is watchpoint, ELR_EL1 for all other debug exceptions.

Signed-off-by Sandeepa Prabhu <[email protected]>

Signed-off-by Deepak Saxena <[email protected]>

---
 arch/arm64/include/asm/debug-monitors.h |  20 +++++++
 arch/arm64/kernel/debug-monitors.c      | 102 +++++++++++++++++++++++++++++++-
 arch/arm64/kernel/entry.S               |   6 +-
 3 files changed, 124 insertions(+), 4 deletions(-)

arch/arm64/include/asm/debug-monitors.h +20 -0

index a2232d0..aff3a76 100644
@@ -16,6 +16,8 @@
 #ifndef __ASM_DEBUG_MONITORS_H
 #define __ASM_DEBUG_MONITORS_H
 
+#include <linux/list.h>
+
 #ifdef __KERNEL__
 
 #define	DBG_ESR_EVT(x)		(((x) >> 27) & 0x7)

@@ -62,6 +64,24 @@ struct task_struct;
 
 #define DBG_ARCH_ID_RESERVED	0	/* In case of ptrace ABI updates. */
 
+struct step_hook {
+	struct list_head node;
+	int (*fn)(struct pt_regs *regs, unsigned int insn, unsigned long addr);
+};
+
+void register_step_hook(struct step_hook *hook);
+void unregister_step_hook(struct step_hook *hook);
+
+struct break_hook {
+	struct list_head node;
+	u32 esr_magic;
+	u32 esr_mask;
+	int (*fn)(struct pt_regs *regs, unsigned int insn, unsigned long addr);
+};
+
+void register_break_hook(struct break_hook *hook);
+void unregister_break_hook(struct break_hook *hook);
+
 u8 debug_monitors_arch(void);
 
 void enable_debug_monitors(enum debug_el el);

arch/arm64/kernel/debug-monitors.c +99 -3

131 lines not on this page · raw message

arch/arm64/kernel/entry.S +5 -1

index 6ad781b..e7350bd 100644
@@ -288,8 +288,12 @@ el1_dbg:
 	/*
 	 * Debug exception handling
 	 */
+	mrs	x25, far_el1			//far for watchpt
+	cmp	x24, #ESR_EL1_EC_WATCHPT_EL1
+	csel	x0, x25, x22, eq	//addr: x25->far_el1, x22->elr_el1
+	b.ge	do_dbg
 	tbz	x24, #0, el1_inv		// EL1 only
-	mrs	x0, far_el1
+do_dbg:
 	mov	x2, sp				// struct pt_regs
 	bl	do_debug_exception

Signature · 1 line

1.8.1.2

Will Deacon

2013-08-13 11:32 UTC #1 lore raw

PATCH 1/1 AArch64: Add single-step and breakpoint handler hooks

Quoted 32 lines · Sandeepa Prabhu

AArch64 Single Steping and Breakpoint debug exceptions will be used by multiple debug framworks like kprobes & kgdb.
This patch implements the hooks for those frameworks to register their own handlers for handling breakpoint and single step events.
Reworked the debug exception handler in entry.S: do_dbg to pass the correct break/step address to the handlers, i.e. FAR_EL1 if exception is watchpoint, ELR_EL1 for all other debug exceptions.
Signed-off-by Sandeepa Prabhu <[email protected]>
Signed-off-by Deepak Saxena <[email protected]>
---
 arch/arm64/include/asm/debug-monitors.h |  20 +++++++
 arch/arm64/kernel/debug-monitors.c      | 102 +++++++++++++++++++++++++++++++-
 arch/arm64/kernel/entry.S               |   6 +-
 3 files changed, 124 insertions(+), 4 deletions(-)
arch/arm64/include/asm/debug-monitors.h +2 -0
index a2232d0..aff3a76 100644
@@ -16,6 +16,8 @@
 #ifndef __ASM_DEBUG_MONITORS_H
 #define __ASM_DEBUG_MONITORS_H
 
+#include <linux/list.h>
+
 #ifdef __KERNEL__
 
 #define	DBG_ESR_EVT(x)		(((x) >> 27) & 0x7)

arch/arm64/include/asm/debug-monitors.h struct task_struct; · line 70

Quoted 7 lines

arch/arm64/include/asm/debug-monitors.h +4 -0

@@ -62,6 +64,24 @@ struct task_struct;
 
 #define DBG_ARCH_ID_RESERVED	0	/* In case of ptrace ABI updates. */
 
+struct step_hook {
+	struct list_head node;
+	int (*fn)(struct pt_regs *regs, unsigned int insn, unsigned long addr);
+};

Shouldn't that be esr instead of insn?

arch/arm64/include/asm/debug-monitors.h struct task_struct; · line 77

Quoted 6 lines

arch/arm64/include/asm/debug-monitors.h +6 -0

+void register_step_hook(struct step_hook *hook);
+void unregister_step_hook(struct step_hook *hook);
+
+struct break_hook {
+	struct list_head node;
+	u32 esr_magic;

There's nothing magic about it. esr_val instead?

Quoted 29 lines

arch/arm64/include/asm/debug-monitors.h +7 -0

+	u32 esr_mask;
+	int (*fn)(struct pt_regs *regs, unsigned int insn, unsigned long addr);
+};
+
+void register_break_hook(struct break_hook *hook);
+void unregister_break_hook(struct break_hook *hook);
+
 u8 debug_monitors_arch(void);
 
 void enable_debug_monitors(enum debug_el el);

arch/arm64/kernel/debug-monitors.c +11 -0

index cbfacf7..2846327 100644
@@ -188,6 +188,54 @@ static void clear_regs_spsr_ss(struct pt_regs *regs)
 	regs->pstate = spsr;
 }
 
+/* EL1 Single Step Handler hooks */
+static LIST_HEAD(step_hook);
+static DEFINE_RAW_SPINLOCK(step_lock);
+
+void register_step_hook(struct step_hook *hook)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&step_lock, flags);
+	list_add(&hook->node, &step_hook);
+	raw_spin_unlock_irqrestore(&step_lock, flags);

arch/arm64/kernel/debug-monitors.c static void clear_regs_spsr_ss(struct pt_regs *regs) · line 211

Quoted 10 lines

arch/arm64/kernel/debug-monitors.c +10 -0

+}
+
+void unregister_step_hook(struct step_hook *hook)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&step_lock, flags);
+	list_del(&hook->node);
+	raw_spin_unlock_irqrestore(&step_lock, flags);
+}

Why do you need to disable IRQs during these operations?

Quoted 5 lines

arch/arm64/kernel/debug-monitors.c +5 -0

+/*
+ * Call registered single step handers
+ * There is no Syndrome info to check for determining the handler.
+ * So we call all the registered handlers, until the right handler is
+ * found which returns zero.

arch/arm64/kernel/debug-monitors.c static void clear_regs_spsr_ss(struct pt_regs *regs) · line 227

Quoted 10 lines

arch/arm64/kernel/debug-monitors.c +10 -0

+ */
+static int call_step_hook(struct pt_regs *regs,
+		unsigned int esr, unsigned long addr)
+{
+	struct step_hook *hook;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&step_lock, flags);
+	list_for_each_entry(hook, &step_hook, node)	{
+		raw_spin_unlock_irqrestore(&step_lock, flags);

Is this safe? Can't you stask the function pointer and break out of the loop when you find it instead?

Quoted 56 lines

arch/arm64/kernel/debug-monitors.c +40 -2

+		if (hook->fn(regs, esr, addr) == 0)
+			return 0;
+
+		raw_spin_lock_irqsave(&step_lock, flags);
+	}
+	raw_spin_unlock_irqrestore(&step_lock, flags);
+
+	return 1;
+}
+
 static int single_step_handler(unsigned long addr, unsigned int esr,
 			       struct pt_regs *regs)
 {

@@ -215,8 +263,11 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
 		 */
 		user_rewind_single_step(current);
 	} else {
-		/* TODO: route to KGDB */
-		pr_warning("Unexpected kernel single-step exception at EL1\n");
+		/* Call single step handlers for kgdb/kprobes */
+		if (call_step_hook(regs, addr, esr) == 0)
+			return 0;
+
+		pr_warn("unexpected single step exception at %lx!\n", addr);
 		/*
 		 * Re-enable stepping since we know that we will be
 		 * returning to regs.

@@ -227,11 +278,56 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
 	return 0;
 }
 
+
+static LIST_HEAD(break_hook);
+static DEFINE_RAW_SPINLOCK(break_lock);
+
+void register_break_hook(struct break_hook *hook)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&break_lock, flags);
+	list_add(&hook->node, &break_hook);
+	raw_spin_unlock_irqrestore(&break_lock, flags);
+}
+
+void unregister_break_hook(struct break_hook *hook)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&break_lock, flags);
+	list_del(&hook->node);
+	raw_spin_unlock_irqrestore(&break_lock, flags);
+}
+
+static int call_break_hook(struct pt_regs *regs,
+		unsigned int esr, unsigned long addr)
+{

arch/arm64/kernel/debug-monitors.c static int single_step_handler(unsigned long addr, unsigned int esr, · line 315

Quoted 10 lines

arch/arm64/kernel/debug-monitors.c +10 -0

+	struct break_hook *hook;
+	unsigned long flags;
+	int (*fn)(struct pt_regs *regs,
+		unsigned int esr, unsigned long addr) = NULL;
+
+	raw_spin_lock_irqsave(&break_lock, flags);
+	list_for_each_entry(hook, &break_hook, node)
+		if ((esr & hook->esr_mask) == hook->esr_magic)
+			fn = hook->fn;
+	raw_spin_unlock_irqrestore(&break_lock, flags);

That's better! (although I'm still unsure about the interrupts).

Quoted 18 lines

arch/arm64/kernel/debug-monitors.c +9 -0

+	return fn ? fn(regs, esr, addr) : 1;
+}
+
 static int brk_handler(unsigned long addr, unsigned int esr,
 		       struct pt_regs *regs)
 {
 	siginfo_t info;
 
+	/* Call single step handlers for kgdb/kprobes */
+	if (call_break_hook(regs, esr, addr) == 0)
+		return 0;
+
+	pr_warn("unexpected brk exception at %lx, esr=0x%x\n", addr, esr);
+
 	if (!user_mode(regs))
 		return -EFAULT;

arch/arm64/kernel/debug-monitors.c static int __init debug_traps_init(void) · line 390

Quoted 5 lines

arch/arm64/kernel/debug-monitors.c +1 -1

@@ -291,7 +387,7 @@ static int __init debug_traps_init(void)
 	hook_debug_fault_code(DBG_ESR_EVT_HWSS, single_step_handler, SIGTRAP,
 			      TRAP_HWBKPT, "single-step handler");
 	hook_debug_fault_code(DBG_ESR_EVT_BRK, brk_handler, SIGTRAP,
-			      TRAP_BRKPT, "ptrace BRK handler");
+			      TRAP_BRKPT, "AArch64 BRK handler");

Why change this name?

arch/arm64/kernel/entry.S el1_dbg: · line 291

Quoted 12 lines

arch/arm64/kernel/debug-monitors.c +0 -0

 	return 0;
 }
 arch_initcall(debug_traps_init);

arch/arm64/kernel/entry.S +1 -0

index 6ad781b..e7350bd 100644
@@ -288,8 +288,12 @@ el1_dbg:
 	/*
 	 * Debug exception handling
 	 */
+	mrs	x25, far_el1			//far for watchpt

Useless comment. How about: "// Watchpoint location"?

arch/arm64/kernel/entry.S el1_dbg: · line 295

Quoted 4 lines

arch/arm64/kernel/entry.S +3 -0

+	cmp	x24, #ESR_EL1_EC_WATCHPT_EL1
+	csel	x0, x25, x22, eq	//addr: x25->far_el1, x22->elr_el1
+	b.ge	do_dbg
 	tbz	x24, #0, el1_inv		// EL1 only

I'd rather you left the tbz as the first instruction in el1_dbg, then you can also lose the b.ge.

Finally, I don't see the need for this patch until we have something like kprobes or kgdb, so I'd rather hold off merging this additional support code until we have a user for it.

Cheers,

Will

Sandeepa Prabhu

author

2013-08-13 14:45 UTC #2 lore raw

PATCH 1/1 AArch64: Add single-step and breakpoint handler hooks

Quoted 34 lines · Will Deacon

Quoted 33 lines · Sandeepa Prabhu
AArch64 Single Steping and Breakpoint debug exceptions will be used by multiple debug framworks like kprobes & kgdb.
This patch implements the hooks for those frameworks to register their own handlers for handling breakpoint and single step events.
Reworked the debug exception handler in entry.S: do_dbg to pass the correct break/step address to the handlers, i.e. FAR_EL1 if exception is watchpoint, ELR_EL1 for all other debug exceptions.
Signed-off-by Sandeepa Prabhu <[email protected]>
Signed-off-by Deepak Saxena <[email protected]>
---
 arch/arm64/include/asm/debug-monitors.h |  20 +++++++
 arch/arm64/kernel/debug-monitors.c      | 102 +++++++++++++++++++++++++++++++-
 arch/arm64/kernel/entry.S               |   6 +-
 3 files changed, 124 insertions(+), 4 deletions(-)
arch/arm64/include/asm/debug-monitors.h +2 -0
index a2232d0..aff3a76 100644
@@ -16,6 +16,8 @@
 #ifndef __ASM_DEBUG_MONITORS_H
 #define __ASM_DEBUG_MONITORS_H
 
+#include <linux/list.h>
+
 #ifdef __KERNEL__
 
 #define      DBG_ESR_EVT(x)          (((x) >> 27) & 0x7)
@@ -62,6 +64,24 @@ struct task_struct;
 

Quoted 8 lines

arch/arm64/include/asm/debug-monitors.h struct task_struct; · line 70
Quoted 6 lines
arch/arm64/include/asm/debug-monitors.h +4 -0
 #define DBG_ARCH_ID_RESERVED 0       /* In case of ptrace ABI updates. */
 
+struct step_hook {
+     struct list_head node;
+     int (*fn)(struct pt_regs *regs, unsigned int insn, unsigned long addr);
+};
Shouldn't that be esr instead of insn?

Right, I will change it to esr in next version of the patch.

Quoted 9 lines

arch/arm64/include/asm/debug-monitors.h struct task_struct; · line 77
Quoted 7 lines
arch/arm64/include/asm/debug-monitors.h +6 -0
+void register_step_hook(struct step_hook *hook);
+void unregister_step_hook(struct step_hook *hook);
+
+struct break_hook {
+     struct list_head node;
+     u32 esr_magic;
There's nothing magic about it. esr_val instead?

Hmm, I will incorporate this change as well.

Quoted 34 lines

arch/arm64/include/asm/debug-monitors.h +7 -0

+     u32 esr_mask;
+     int (*fn)(struct pt_regs *regs, unsigned int insn, unsigned long addr);
+};
+
+void register_break_hook(struct break_hook *hook);
+void unregister_break_hook(struct break_hook *hook);
+
 u8 debug_monitors_arch(void);
 
 void enable_debug_monitors(enum debug_el el);

arch/arm64/kernel/debug-monitors.c +15 -0

index cbfacf7..2846327 100644
@@ -188,6 +188,54 @@ static void clear_regs_spsr_ss(struct pt_regs *regs)
      regs->pstate = spsr;
 }
 
+/* EL1 Single Step Handler hooks */
+static LIST_HEAD(step_hook);
+static DEFINE_RAW_SPINLOCK(step_lock);
+
+void register_step_hook(struct step_hook *hook)
+{
+     unsigned long flags;
+
+     raw_spin_lock_irqsave(&step_lock, flags);
+     list_add(&hook->node, &step_hook);
+     raw_spin_unlock_irqrestore(&step_lock, flags);
+}
+
+void unregister_step_hook(struct step_hook *hook)
+{

Quoted 8 lines

arch/arm64/kernel/debug-monitors.c static void clear_regs_spsr_ss(struct pt_regs *regs) · line 211
Quoted 6 lines
arch/arm64/kernel/debug-monitors.c +6 -0
+     unsigned long flags;
+
+     raw_spin_lock_irqsave(&step_lock, flags);
+     list_del(&hook->node);
+     raw_spin_unlock_irqrestore(&step_lock, flags);
+}
Why do you need to disable IRQs during these operations?

The linked list needs locking mechanism to protect while getting accessed/updated from multiple CPUs.
These API will 'never' be called from interrupt context, so I can convert to spin_lock() variant.

Quoted 11 lines

arch/arm64/kernel/debug-monitors.c +10 -0

+/*
+ * Call registered single step handers
+ * There is no Syndrome info to check for determining the handler.
+ * So we call all the registered handlers, until the right handler is
+ * found which returns zero.
+ */
+static int call_step_hook(struct pt_regs *regs,
+             unsigned int esr, unsigned long addr)
+{
+     struct step_hook *hook;

Quoted 8 lines

arch/arm64/kernel/debug-monitors.c static void clear_regs_spsr_ss(struct pt_regs *regs) · line 227
Quoted 5 lines
arch/arm64/kernel/debug-monitors.c +5 -0
+     unsigned long flags;
+
+     raw_spin_lock_irqsave(&step_lock, flags);
+     list_for_each_entry(hook, &step_hook, node)     {
+             raw_spin_unlock_irqrestore(&step_lock, flags);
Is this safe? Can't you stask the function pointer and break out of the loop when you find it instead?

Unlike breakpoints, single step does not provide ESR value to be specified, so there is no way to decide which handler to call. So the implementation is to call all the registered handlers, and individual subsystem (kgdb,kprobes) handlers are in turn responsible for validating and handling it (so we do not break out of loop). Any concerns with this scheme?

well, accessing "hook->fn(regs, esr, addr)" without a spin_lock is not safe with multiple CPU. The safe way is to execute the hook function with spin-lock held. I will rework this in next version.

Quoted 87 lines

Quoted 57 lines

arch/arm64/kernel/debug-monitors.c +40 -2

+             if (hook->fn(regs, esr, addr) == 0)
+                     return 0;
+
+             raw_spin_lock_irqsave(&step_lock, flags);
+     }
+     raw_spin_unlock_irqrestore(&step_lock, flags);
+
+     return 1;
+}
+
 static int single_step_handler(unsigned long addr, unsigned int esr,
                             struct pt_regs *regs)
 {

@@ -215,8 +263,11 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
               */
              user_rewind_single_step(current);
      } else {
-             /* TODO: route to KGDB */
-             pr_warning("Unexpected kernel single-step exception at EL1\n");
+             /* Call single step handlers for kgdb/kprobes */
+             if (call_step_hook(regs, addr, esr) == 0)
+                     return 0;
+
+             pr_warn("unexpected single step exception at %lx!\n", addr);
              /*
               * Re-enable stepping since we know that we will be
               * returning to regs.

@@ -227,11 +278,56 @@ static int single_step_handler(unsigned long addr, unsigned int esr,
      return 0;
 }
 
+
+static LIST_HEAD(break_hook);
+static DEFINE_RAW_SPINLOCK(break_lock);
+
+void register_break_hook(struct break_hook *hook)
+{
+     unsigned long flags;
+
+     raw_spin_lock_irqsave(&break_lock, flags);
+     list_add(&hook->node, &break_hook);
+     raw_spin_unlock_irqrestore(&break_lock, flags);
+}
+
+void unregister_break_hook(struct break_hook *hook)
+{
+     unsigned long flags;
+
+     raw_spin_lock_irqsave(&break_lock, flags);
+     list_del(&hook->node);
+     raw_spin_unlock_irqrestore(&break_lock, flags);
+}
+
+static int call_break_hook(struct pt_regs *regs,
+             unsigned int esr, unsigned long addr)
+{

arch/arm64/kernel/debug-monitors.c static int single_step_handler(unsigned long addr, unsigned int esr, · line 315

Quoted 10 lines

arch/arm64/kernel/debug-monitors.c +10 -0

+     struct break_hook *hook;
+     unsigned long flags;
+     int (*fn)(struct pt_regs *regs,
+             unsigned int esr, unsigned long addr) = NULL;
+
+     raw_spin_lock_irqsave(&break_lock, flags);
+     list_for_each_entry(hook, &break_hook, node)
+             if ((esr & hook->esr_mask) == hook->esr_magic)
+                     fn = hook->fn;
+     raw_spin_unlock_irqrestore(&break_lock, flags);

That's better! (although I'm still unsure about the interrupts).

Quoted 17 lines

arch/arm64/kernel/debug-monitors.c +9 -0

+     return fn ? fn(regs, esr, addr) : 1;
+}
+
 static int brk_handler(unsigned long addr, unsigned int esr,
                     struct pt_regs *regs)
 {
      siginfo_t info;
 
+     /* Call single step handlers for kgdb/kprobes */
+     if (call_break_hook(regs, esr, addr) == 0)
+             return 0;
+
+     pr_warn("unexpected brk exception at %lx, esr=0x%x\n", addr, esr);
+
      if (!user_mode(regs))
              return -EFAULT;

Quoted 8 lines

arch/arm64/kernel/debug-monitors.c static int __init debug_traps_init(void) · line 390

Quoted 6 lines

arch/arm64/kernel/debug-monitors.c +1 -1

@@ -291,7 +387,7 @@ static int __init debug_traps_init(void)
      hook_debug_fault_code(DBG_ESR_EVT_HWSS, single_step_handler, SIGTRAP,
                            TRAP_HWBKPT, "single-step handler");
      hook_debug_fault_code(DBG_ESR_EVT_BRK, brk_handler, SIGTRAP,
-                           TRAP_BRKPT, "ptrace BRK handler");
+                           TRAP_BRKPT, "AArch64 BRK handler");

Why change this name?

The brk_handler will be generic handler for multiple mechanisms like kgdb/kprobes, not just ptrace, so the name should reflect that.

Quoted 15 lines

arch/arm64/kernel/entry.S el1_dbg: · line 291
Quoted 13 lines
arch/arm64/kernel/debug-monitors.c +0 -0
      return 0;
 }
 arch_initcall(debug_traps_init);
arch/arm64/kernel/entry.S +1 -0
index 6ad781b..e7350bd 100644
@@ -288,8 +288,12 @@ el1_dbg:
      /*
       * Debug exception handling
       */
+     mrs     x25, far_el1                    //far for watchpt
Useless comment. How about: "// Watchpoint location"?

Sounds good, I will change it to "// Watchpoint location"

Quoted 8 lines

arch/arm64/kernel/entry.S el1_dbg: · line 295
Quoted 5 lines
arch/arm64/kernel/entry.S +3 -0
+     cmp     x24, #ESR_EL1_EC_WATCHPT_EL1
+     csel    x0, x25, x22, eq        //addr: x25->far_el1, x22->elr_el1
+     b.ge    do_dbg
      tbz     x24, #0, el1_inv                // EL1 only
I'd rather you left the tbz as the first instruction in el1_dbg, then you can also lose the b.ge.

well, my understanding is that the tbz check is needed only for Exception Class < 0x35 as per debug spec. If this is true, and if tbz is first instruction, it fails for breakpoint (EC=0x3A) case and call el1_inv to panic instead of routing to do_debug_exception. I am not sure if we can optimize the code further to eliminate this one branching as well.

Quoted 8 lines

Finally, I don't see the need for this patch until we have something like kprobes or kgdb, so I'd rather hold off merging this additional support code until we have a user for it.
Cheers,
Will

Will Deacon

2013-08-20 13:12 UTC #3 lore raw

PATCH 1/1 AArch64: Add single-step and breakpoint handler hooks

Quoted 7 lines · Sandeepa Prabhu

Quoted 6 lines · Will Deacon

Quoted 4 lines · Sandeepa Prabhu

arch/arm64/kernel/entry.S +3 -0

+     cmp     x24, #ESR_EL1_EC_WATCHPT_EL1
+     csel    x0, x25, x22, eq        //addr: x25->far_el1, x22->elr_el1
+     b.ge    do_dbg
      tbz     x24, #0, el1_inv                // EL1 only

Quoted 8 lines

Quoted 2 lines
I'd rather you left the tbz as the first instruction in el1_dbg, then you can also lose the b.ge.
well, my understanding is that the tbz check is needed only for Exception Class < 0x35 as per debug spec. If this is true, and if tbz is first instruction, it fails for breakpoint (EC=0x3A) case and call el1_inv to panic instead of routing to do_debug_exception. I am not sure if we can optimize the code further to eliminate this one branching as well.

Well, you're actually only interested in 0x3c (BRK instruction executed in AArch64 state), so you should check for that explicitly. I guess it doesn't matter where you check bit #0 first or not, provided you have the branch logic correct.

Will

Sandeepa Prabhu

author

2013-08-20 14:00 UTC #4 lore raw

PATCH 1/1 AArch64: Add single-step and breakpoint handler hooks

Quoted 17 lines · Will Deacon

Quoted 7 lines · Sandeepa Prabhu
Quoted 6 lines · Will Deacon
Quoted 4 lines · Sandeepa Prabhu
arch/arm64/kernel/entry.S +3 -0
+     cmp     x24, #ESR_EL1_EC_WATCHPT_EL1
+     csel    x0, x25, x22, eq        //addr: x25->far_el1, x22->elr_el1
+     b.ge    do_dbg
      tbz     x24, #0, el1_inv                // EL1 only
Quoted 8 lines
Quoted 2 lines
I'd rather you left the tbz as the first instruction in el1_dbg, then you can also lose the b.ge.
well, my understanding is that the tbz check is needed only for Exception Class < 0x35 as per debug spec. If this is true, and if tbz is first instruction, it fails for breakpoint (EC=0x3A) case and call el1_inv to panic instead of routing to do_debug_exception. I am not sure if we can optimize the code further to eliminate this one branching as well.

Quoted 4 lines

Well, you're actually only interested in 0x3c (BRK instruction executed in AArch64 state), so you should check for that explicitly. I guess it doesn't matter where you check bit #0 first or not, provided you have the branch logic correct.

Agreed, then 0x3c is the only case where bit #0 check shall be ignored. I will rework this code accordingly, and as you have mentioned, I will add this patch along with the complete kprobes series later that will be using it.

Thanks,
Sandeepa

Quoted 2 lines

Will