[RFC PATCH 1/2] arm64: hw_breakpoint: convert CPU hotplug notifier to new infrastructure

[RFC PATCH 1/2] arm64: hw_breakpoint: convert CPU hotplug notifier to new infrastructure

[Will Deacon]

The arm64 hw_breakpoint implementation uses a CPU hotplug notifier to
reset the {break,watch}point registers when CPUs come online.

This patch converts the code to the new hotplug mechanism, whilst moving
the invocation earlier to remove the need to disable IRQs explicitly in
the driver (which could cause havok if we trip a watchpoint in an IRQ
handler whilst restoring the debug register state).

Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/hw_breakpoint.c | 48 +++++++++++++--------------------------
 arch/arm64/kernel/suspend.c       | 10 ++++----
 include/linux/cpuhotplug.h        |  1 +
 3 files changed, 23 insertions(+), 36 deletions(-)

diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index 26a6bf77d272..948b73148d56 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -857,7 +857,7 @@ void hw_breakpoint_thread_switch(struct task_struct *next)
 /*
  * CPU initialisation.
  */
-static void hw_breakpoint_reset(void *unused)
+static int hw_breakpoint_reset(unsigned int cpu)
 {
 	int i;
 	struct perf_event **slots;
@@ -888,28 +888,14 @@ static void hw_breakpoint_reset(void *unused)
 			write_wb_reg(AARCH64_DBG_REG_WVR, i, 0UL);
 		}
 	}
-}
 
-static int hw_breakpoint_reset_notify(struct notifier_block *self,
-						unsigned long action,
-						void *hcpu)
-{
-	if ((action & ~CPU_TASKS_FROZEN) == CPU_ONLINE) {
-		local_irq_disable();
-		hw_breakpoint_reset(NULL);
-		local_irq_enable();
-	}
-	return NOTIFY_OK;
+	return 0;
 }
 
-static struct notifier_block hw_breakpoint_reset_nb = {
-	.notifier_call = hw_breakpoint_reset_notify,
-};
-
 #ifdef CONFIG_CPU_PM
-extern void cpu_suspend_set_dbg_restorer(void (*hw_bp_restore)(void *));
+extern void cpu_suspend_set_dbg_restorer(int (*hw_bp_restore)(unsigned int));
 #else
-static inline void cpu_suspend_set_dbg_restorer(void (*hw_bp_restore)(void *))
+static inline void cpu_suspend_set_dbg_restorer(int (*hw_bp_restore)(unsigned int))
 {
 }
 #endif
@@ -919,36 +905,34 @@ static inline void cpu_suspend_set_dbg_restorer(void (*hw_bp_restore)(void *))
  */
 static int __init arch_hw_breakpoint_init(void)
 {
+	int ret;
+
 	core_num_brps = get_num_brps();
 	core_num_wrps = get_num_wrps();
 
 	pr_info("found %d breakpoint and %d watchpoint registers.\n",
 		core_num_brps, core_num_wrps);
 
-	cpu_notifier_register_begin();
-
-	/*
-	 * Reset the breakpoint resources. We assume that a halting
-	 * debugger will leave the world in a nice state for us.
-	 */
-	smp_call_function(hw_breakpoint_reset, NULL, 1);
-	hw_breakpoint_reset(NULL);
-
 	/* Register debug fault handlers. */
 	hook_debug_fault_code(DBG_ESR_EVT_HWBP, breakpoint_handler, SIGTRAP,
 			      TRAP_HWBKPT, "hw-breakpoint handler");
 	hook_debug_fault_code(DBG_ESR_EVT_HWWP, watchpoint_handler, SIGTRAP,
 			      TRAP_HWBKPT, "hw-watchpoint handler");
 
-	/* Register hotplug notifier. */
-	__register_cpu_notifier(&hw_breakpoint_reset_nb);
-
-	cpu_notifier_register_done();
+	/*
+	 * Reset the breakpoint resources. We assume that a halting
+	 * debugger will leave the world in a nice state for us.
+	 */
+	ret = cpuhp_setup_state(CPUHP_AP_PERF_ARM_HW_BREAKPOINT_STARTING,
+			  "CPUHP_AP_PERF_ARM_HW_BREAKPOINT_STARTING",
+			  hw_breakpoint_reset, NULL);
+	if (ret)
+		pr_err("failed to register CPU hotplug notifier: %d\n", ret);
 
 	/* Register cpu_suspend hw breakpoint restore hook */
 	cpu_suspend_set_dbg_restorer(hw_breakpoint_reset);
 
-	return 0;
+	return ret;
 }
 arch_initcall(arch_hw_breakpoint_init);
 
diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
index b616e365cee3..ad734142070d 100644
--- a/arch/arm64/kernel/suspend.c
+++ b/arch/arm64/kernel/suspend.c
@@ -23,8 +23,8 @@ unsigned long *sleep_save_stash;
  * time the notifier runs debug exceptions might have been enabled already,
  * with HW breakpoints registers content still in an unknown state.
  */
-static void (*hw_breakpoint_restore)(void *);
-void __init cpu_suspend_set_dbg_restorer(void (*hw_bp_restore)(void *))
+static int (*hw_breakpoint_restore)(unsigned int);
+void __init cpu_suspend_set_dbg_restorer(int (*hw_bp_restore)(unsigned int))
 {
 	/* Prevent multiple restore hook initializations */
 	if (WARN_ON(hw_breakpoint_restore))
@@ -34,6 +34,8 @@ void __init cpu_suspend_set_dbg_restorer(void (*hw_bp_restore)(void *))
 
 void notrace __cpu_suspend_exit(void)
 {
+	unsigned int cpu = smp_processor_id();
+
 	/*
 	 * We are resuming from reset with the idmap active in TTBR0_EL1.
 	 * We must uninstall the idmap and restore the expected MMU
@@ -45,7 +47,7 @@ void notrace __cpu_suspend_exit(void)
 	 * Restore per-cpu offset before any kernel
 	 * subsystem relying on it has a chance to run.
 	 */
-	set_my_cpu_offset(per_cpu_offset(smp_processor_id()));
+	set_my_cpu_offset(per_cpu_offset(cpu));
 
 	/*
 	 * Restore HW breakpoint registers to sane values
@@ -53,7 +55,7 @@ void notrace __cpu_suspend_exit(void)
 	 * through local_dbg_restore.
 	 */
 	if (hw_breakpoint_restore)
-		hw_breakpoint_restore(NULL);
+		hw_breakpoint_restore(cpu);
 }
 
 /*
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 242bf530edfc..3758fe6d5968 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -45,6 +45,7 @@ enum cpuhp_state {
 	CPUHP_AP_PERF_METAG_STARTING,
 	CPUHP_AP_MIPS_OP_LOONGSON3_STARTING,
 	CPUHP_AP_ARM_VFP_STARTING,
+	CPUHP_AP_PERF_ARM_HW_BREAKPOINT_STARTING,
 	CPUHP_AP_PERF_ARM_STARTING,
 	CPUHP_AP_ARM_L2X0_STARTING,
 	CPUHP_AP_ARM_ARCH_TIMER_STARTING,
-- 
2.1.4

[RFC PATCH 2/2] arm64: debug: convert OS lock CPU hotplug notifier to new infrastructure

[Will Deacon]

The arm64 debug monitor initialisation code uses a CPU hotplug notifier
to clear the OS lock when CPUs come online.

This patch converts the code to the new hotplug mechanism.

Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/debug-monitors.c | 30 ++++++------------------------
 include/linux/cpuhotplug.h         |  1 +
 2 files changed, 7 insertions(+), 24 deletions(-)

diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 91fff48d0f57..3f9023654dac 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -132,36 +132,18 @@ NOKPROBE_SYMBOL(disable_debug_monitors);
 /*
  * OS lock clearing.
  */
-static void clear_os_lock(void *unused)
+static int clear_os_lock(unsigned int cpu)
 {
 	asm volatile("msr oslar_el1, %0" : : "r" (0));
+	isb();
+	return 0;
 }
 
-static int os_lock_notify(struct notifier_block *self,
-				    unsigned long action, void *data)
-{
-	if ((action & ~CPU_TASKS_FROZEN) == CPU_ONLINE)
-		clear_os_lock(NULL);
-	return NOTIFY_OK;
-}
-
-static struct notifier_block os_lock_nb = {
-	.notifier_call = os_lock_notify,
-};
-
 static int debug_monitors_init(void)
 {
-	cpu_notifier_register_begin();
-
-	/* Clear the OS lock. */
-	on_each_cpu(clear_os_lock, NULL, 1);
-	isb();
-
-	/* Register hotplug handler. */
-	__register_cpu_notifier(&os_lock_nb);
-
-	cpu_notifier_register_done();
-	return 0;
+	return cpuhp_setup_state(CPUHP_AP_ARM64_DEBUG_MONITORS_STARTING,
+				 "CPUHP_AP_ARM64_DEBUG_MONITORS_STARTING",
+				 clear_os_lock, NULL);
 }
 postcore_initcall(debug_monitors_init);
 
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 3758fe6d5968..8c999a202452 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -45,6 +45,7 @@ enum cpuhp_state {
 	CPUHP_AP_PERF_METAG_STARTING,
 	CPUHP_AP_MIPS_OP_LOONGSON3_STARTING,
 	CPUHP_AP_ARM_VFP_STARTING,
+	CPUHP_AP_ARM64_DEBUG_MONITORS_STARTING,
 	CPUHP_AP_PERF_ARM_HW_BREAKPOINT_STARTING,
 	CPUHP_AP_PERF_ARM_STARTING,
 	CPUHP_AP_ARM_L2X0_STARTING,
-- 
2.1.4

[RFC PATCH 1/2] arm64: hw_breakpoint: convert CPU hotplug notifier to new infrastructure

[Sebastian Andrzej Siewior]

On 2016-08-26 11:35:57 [+0100], Will Deacon wrote:
> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> index 26a6bf77d272..948b73148d56 100644
> --- a/arch/arm64/kernel/hw_breakpoint.c
> +++ b/arch/arm64/kernel/hw_breakpoint.c
> @@ -857,7 +857,7 @@ void hw_breakpoint_thread_switch(struct task_struct *next)
?
> -static int hw_breakpoint_reset_notify(struct notifier_block *self,
> -						unsigned long action,
> -						void *hcpu)
> -{
> -	if ((action & ~CPU_TASKS_FROZEN) == CPU_ONLINE) {
> -		local_irq_disable();
> -		hw_breakpoint_reset(NULL);

so the old one was invoked at CPU_ONLINE time.

> -		local_irq_enable();
> -	}
> -	return NOTIFY_OK;
> +	return 0;
>  }
?
> @@ -919,36 +905,34 @@ static inline void cpu_suspend_set_dbg_restorer(void (*hw_bp_restore)(void *))
?
> +	/*
> +	 * Reset the breakpoint resources. We assume that a halting
> +	 * debugger will leave the world in a nice state for us.
> +	 */
> +	ret = cpuhp_setup_state(CPUHP_AP_PERF_ARM_HW_BREAKPOINT_STARTING,
> +			  "CPUHP_AP_PERF_ARM_HW_BREAKPOINT_STARTING",
> +			  hw_breakpoint_reset, NULL);

But this will move it to CPU_STARTING. May I ask why?

Sebastian

[RFC PATCH 1/2] arm64: hw_breakpoint: convert CPU hotplug notifier to new infrastructure

[Will Deacon]

On Fri, Aug 26, 2016 at 01:28:06PM +0200, Sebastian Andrzej Siewior wrote:
> On 2016-08-26 11:35:57 [+0100], Will Deacon wrote:
> > diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> > index 26a6bf77d272..948b73148d56 100644
> > --- a/arch/arm64/kernel/hw_breakpoint.c
> > +++ b/arch/arm64/kernel/hw_breakpoint.c
> > @@ -857,7 +857,7 @@ void hw_breakpoint_thread_switch(struct task_struct *next)
> ?
> > -static int hw_breakpoint_reset_notify(struct notifier_block *self,
> > -						unsigned long action,
> > -						void *hcpu)
> > -{
> > -	if ((action & ~CPU_TASKS_FROZEN) == CPU_ONLINE) {
> > -		local_irq_disable();
> > -		hw_breakpoint_reset(NULL);
> 
> so the old one was invoked at CPU_ONLINE time.
> 
> > -		local_irq_enable();
> > -	}
> > -	return NOTIFY_OK;
> > +	return 0;
> >  }
> ?
> > @@ -919,36 +905,34 @@ static inline void cpu_suspend_set_dbg_restorer(void (*hw_bp_restore)(void *))
> ?
> > +	/*
> > +	 * Reset the breakpoint resources. We assume that a halting
> > +	 * debugger will leave the world in a nice state for us.
> > +	 */
> > +	ret = cpuhp_setup_state(CPUHP_AP_PERF_ARM_HW_BREAKPOINT_STARTING,
> > +			  "CPUHP_AP_PERF_ARM_HW_BREAKPOINT_STARTING",
> > +			  hw_breakpoint_reset, NULL);
> 
> But this will move it to CPU_STARTING. May I ask why?

It probably should be there anyway, since the current code is explicitly
disabling interrupts to mitigate being called with them enabled and debug
is something we want as early as possible.

Will

[RFC PATCH 1/2] arm64: hw_breakpoint: convert CPU hotplug notifier to new infrastructure

[Lorenzo Pieralisi]

On Fri, Aug 26, 2016 at 11:35:57AM +0100, Will Deacon wrote:
> The arm64 hw_breakpoint implementation uses a CPU hotplug notifier to
> reset the {break,watch}point registers when CPUs come online.
> 
> This patch converts the code to the new hotplug mechanism, whilst moving
> the invocation earlier to remove the need to disable IRQs explicitly in
> the driver (which could cause havok if we trip a watchpoint in an IRQ
> handler whilst restoring the debug register state).
> 
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/kernel/hw_breakpoint.c | 48 +++++++++++++--------------------------
>  arch/arm64/kernel/suspend.c       | 10 ++++----
>  include/linux/cpuhotplug.h        |  1 +
>  3 files changed, 23 insertions(+), 36 deletions(-)

For both patches:

Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> index 26a6bf77d272..948b73148d56 100644
> --- a/arch/arm64/kernel/hw_breakpoint.c
> +++ b/arch/arm64/kernel/hw_breakpoint.c
> @@ -857,7 +857,7 @@ void hw_breakpoint_thread_switch(struct task_struct *next)
>  /*
>   * CPU initialisation.
>   */
> -static void hw_breakpoint_reset(void *unused)
> +static int hw_breakpoint_reset(unsigned int cpu)
>  {
>  	int i;
>  	struct perf_event **slots;
> @@ -888,28 +888,14 @@ static void hw_breakpoint_reset(void *unused)
>  			write_wb_reg(AARCH64_DBG_REG_WVR, i, 0UL);
>  		}
>  	}
> -}
>  
> -static int hw_breakpoint_reset_notify(struct notifier_block *self,
> -						unsigned long action,
> -						void *hcpu)
> -{
> -	if ((action & ~CPU_TASKS_FROZEN) == CPU_ONLINE) {
> -		local_irq_disable();
> -		hw_breakpoint_reset(NULL);
> -		local_irq_enable();
> -	}
> -	return NOTIFY_OK;
> +	return 0;
>  }
>  
> -static struct notifier_block hw_breakpoint_reset_nb = {
> -	.notifier_call = hw_breakpoint_reset_notify,
> -};
> -
>  #ifdef CONFIG_CPU_PM
> -extern void cpu_suspend_set_dbg_restorer(void (*hw_bp_restore)(void *));
> +extern void cpu_suspend_set_dbg_restorer(int (*hw_bp_restore)(unsigned int));
>  #else
> -static inline void cpu_suspend_set_dbg_restorer(void (*hw_bp_restore)(void *))
> +static inline void cpu_suspend_set_dbg_restorer(int (*hw_bp_restore)(unsigned int))
>  {
>  }
>  #endif
> @@ -919,36 +905,34 @@ static inline void cpu_suspend_set_dbg_restorer(void (*hw_bp_restore)(void *))
>   */
>  static int __init arch_hw_breakpoint_init(void)
>  {
> +	int ret;
> +
>  	core_num_brps = get_num_brps();
>  	core_num_wrps = get_num_wrps();
>  
>  	pr_info("found %d breakpoint and %d watchpoint registers.\n",
>  		core_num_brps, core_num_wrps);
>  
> -	cpu_notifier_register_begin();
> -
> -	/*
> -	 * Reset the breakpoint resources. We assume that a halting
> -	 * debugger will leave the world in a nice state for us.
> -	 */
> -	smp_call_function(hw_breakpoint_reset, NULL, 1);
> -	hw_breakpoint_reset(NULL);
> -
>  	/* Register debug fault handlers. */
>  	hook_debug_fault_code(DBG_ESR_EVT_HWBP, breakpoint_handler, SIGTRAP,
>  			      TRAP_HWBKPT, "hw-breakpoint handler");
>  	hook_debug_fault_code(DBG_ESR_EVT_HWWP, watchpoint_handler, SIGTRAP,
>  			      TRAP_HWBKPT, "hw-watchpoint handler");
>  
> -	/* Register hotplug notifier. */
> -	__register_cpu_notifier(&hw_breakpoint_reset_nb);
> -
> -	cpu_notifier_register_done();
> +	/*
> +	 * Reset the breakpoint resources. We assume that a halting
> +	 * debugger will leave the world in a nice state for us.
> +	 */
> +	ret = cpuhp_setup_state(CPUHP_AP_PERF_ARM_HW_BREAKPOINT_STARTING,
> +			  "CPUHP_AP_PERF_ARM_HW_BREAKPOINT_STARTING",
> +			  hw_breakpoint_reset, NULL);
> +	if (ret)
> +		pr_err("failed to register CPU hotplug notifier: %d\n", ret);
>  
>  	/* Register cpu_suspend hw breakpoint restore hook */
>  	cpu_suspend_set_dbg_restorer(hw_breakpoint_reset);
>  
> -	return 0;
> +	return ret;
>  }
>  arch_initcall(arch_hw_breakpoint_init);
>  
> diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
> index b616e365cee3..ad734142070d 100644
> --- a/arch/arm64/kernel/suspend.c
> +++ b/arch/arm64/kernel/suspend.c
> @@ -23,8 +23,8 @@ unsigned long *sleep_save_stash;
>   * time the notifier runs debug exceptions might have been enabled already,
>   * with HW breakpoints registers content still in an unknown state.
>   */
> -static void (*hw_breakpoint_restore)(void *);
> -void __init cpu_suspend_set_dbg_restorer(void (*hw_bp_restore)(void *))
> +static int (*hw_breakpoint_restore)(unsigned int);
> +void __init cpu_suspend_set_dbg_restorer(int (*hw_bp_restore)(unsigned int))
>  {
>  	/* Prevent multiple restore hook initializations */
>  	if (WARN_ON(hw_breakpoint_restore))
> @@ -34,6 +34,8 @@ void __init cpu_suspend_set_dbg_restorer(void (*hw_bp_restore)(void *))
>  
>  void notrace __cpu_suspend_exit(void)
>  {
> +	unsigned int cpu = smp_processor_id();
> +
>  	/*
>  	 * We are resuming from reset with the idmap active in TTBR0_EL1.
>  	 * We must uninstall the idmap and restore the expected MMU
> @@ -45,7 +47,7 @@ void notrace __cpu_suspend_exit(void)
>  	 * Restore per-cpu offset before any kernel
>  	 * subsystem relying on it has a chance to run.
>  	 */
> -	set_my_cpu_offset(per_cpu_offset(smp_processor_id()));
> +	set_my_cpu_offset(per_cpu_offset(cpu));
>  
>  	/*
>  	 * Restore HW breakpoint registers to sane values
> @@ -53,7 +55,7 @@ void notrace __cpu_suspend_exit(void)
>  	 * through local_dbg_restore.
>  	 */
>  	if (hw_breakpoint_restore)
> -		hw_breakpoint_restore(NULL);
> +		hw_breakpoint_restore(cpu);
>  }
>  
>  /*
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index 242bf530edfc..3758fe6d5968 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -45,6 +45,7 @@ enum cpuhp_state {
>  	CPUHP_AP_PERF_METAG_STARTING,
>  	CPUHP_AP_MIPS_OP_LOONGSON3_STARTING,
>  	CPUHP_AP_ARM_VFP_STARTING,
> +	CPUHP_AP_PERF_ARM_HW_BREAKPOINT_STARTING,
>  	CPUHP_AP_PERF_ARM_STARTING,
>  	CPUHP_AP_ARM_L2X0_STARTING,
>  	CPUHP_AP_ARM_ARCH_TIMER_STARTING,
> -- 
> 2.1.4
> 

[RFC PATCH 1/2] arm64: hw_breakpoint: convert CPU hotplug notifier to new infrastructure

[Sebastian Andrzej Siewior]

On 2016-08-26 12:41:41 [+0100], Will Deacon wrote:
> 
> It probably should be there anyway, since the current code is explicitly
> disabling interrupts to mitigate being called with them enabled and debug
> is something we want as early as possible.

Good. This will go via the ARM64 tree then?

> Will

Sebastian

[RFC PATCH 1/2] arm64: hw_breakpoint: convert CPU hotplug notifier to new infrastructure

[Will Deacon]

On Wed, Aug 31, 2016 at 06:04:28PM +0200, Sebastian Andrzej Siewior wrote:
> On 2016-08-26 12:41:41 [+0100], Will Deacon wrote:
> > 
> > It probably should be there anyway, since the current code is explicitly
> > disabling interrupts to mitigate being called with them enabled and debug
> > is something we want as early as possible.
> 
> Good. This will go via the ARM64 tree then?

Yup, I can queue these two for 4.9 if you're happy enough with them.

Will