[PATCH 1/3] arm64: debug: unmask PSTATE.D earlier

[PATCH 1/3] arm64: debug: unmask PSTATE.D earlier

[Will Deacon]

Clearing PSTATE.D is one of the requirements for generating a debug
exception. The arm64 booting protocol requires that PSTATE.D is set,
since many of the debug registers (for example, the hw_breakpoint
registers) are UNKNOWN out of reset and could potentially generate
spurious, fatal debug exceptions in early boot code if PSTATE.D was
clear. Once the debug registers have been safely initialised, PSTATE.D
is cleared, however this is currently broken for two reasons:

(1) The boot CPU clears PSTATE.D in a postcore_initcall and secondary
    CPUs clear PSTATE.D in secondary_start_kernel. Since the initcall
    runs after SMP (and the scheduler) have been initialised, there is
    no guarantee that it is actually running on the boot CPU. In this
    case, the boot CPU is left with PSTATE.D set and is not capable of
    generating debug exceptions.

(2) In a preemptible kernel, we may explicitly schedule on the IRQ
    return path to EL1. If an IRQ occurs with PSTATE.D set in the idle
    thread, then we may schedule the kthread_init thread, run the
    postcore_initcall to clear PSTATE.D and then context switch back
    to the idle thread before returning from the IRQ. The exception
    return patch will then restore PSTATE.D from the stack, and set it
    again.

This patch fixes the problem by moving the clearing of PSTATE.D earlier
to proc.S. This has the desirable effect of clearing it in one place for
all CPUs, long before we have to worry about the scheduler or any
exception handling. We ensure that the previous reset of MDSCR_EL1 has
completed before unmasking the exception, so that any spurious
exceptions resulting from UNKNOWN debug registers are not generated.

Without this patch applied, the kprobes selftests have been seen to fail
under KVM, where we end up attempting to step the OOL instruction buffer
with PSTATE.D set and therefore fail to complete the step.

Cc: <stable@vger.kernel.org>
Reported-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/debug-monitors.c | 1 -
 arch/arm64/kernel/smp.c            | 1 -
 arch/arm64/mm/proc.S               | 2 ++
 3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 2fbc1b99e8fb..7a907e401c98 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -156,7 +156,6 @@ static int debug_monitors_init(void)
 	/* Clear the OS lock. */
 	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 62ff3c0622e2..c0a772560ab7 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -267,7 +267,6 @@ asmlinkage void secondary_start_kernel(void)
 	set_cpu_online(cpu, true);
 	complete(&cpu_running);
 
-	local_dbg_enable();
 	local_irq_enable();
 	local_async_enable();
 
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index c4317879b938..7757ebf7b2d7 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -180,6 +180,8 @@ ENTRY(__cpu_setup)
 	msr	cpacr_el1, x0			// Enable FP/ASIMD
 	mov	x0, #1 << 12			// Reset mdscr_el1 and disable
 	msr	mdscr_el1, x0			// access to the DCC from EL0
+	isb					// Unmask debug exceptions now,
+	msr	daifclr, #8			// since this is per-cpu
 	reset_pmuserenr_el0 x0			// Disable PMU access from EL0
 	/*
 	 * Memory region attributes for LPAE:
-- 
2.1.4

Re: [PATCH 1/3] arm64: debug: unmask PSTATE.D earlier

[Mark Rutland]

On Tue, Jul 19, 2016 at 12:19:53PM +0100, Will Deacon wrote:
> Clearing PSTATE.D is one of the requirements for generating a debug
> exception. The arm64 booting protocol requires that PSTATE.D is set,
> since many of the debug registers (for example, the hw_breakpoint
> registers) are UNKNOWN out of reset and could potentially generate
> spurious, fatal debug exceptions in early boot code if PSTATE.D was
> clear. Once the debug registers have been safely initialised, PSTATE.D
> is cleared, however this is currently broken for two reasons:
> 
> (1) The boot CPU clears PSTATE.D in a postcore_initcall and secondary
>     CPUs clear PSTATE.D in secondary_start_kernel. Since the initcall
>     runs after SMP (and the scheduler) have been initialised, there is
>     no guarantee that it is actually running on the boot CPU. In this
>     case, the boot CPU is left with PSTATE.D set and is not capable of
>     generating debug exceptions.
> 
> (2) In a preemptible kernel, we may explicitly schedule on the IRQ
>     return path to EL1. If an IRQ occurs with PSTATE.D set in the idle
>     thread, then we may schedule the kthread_init thread, run the
>     postcore_initcall to clear PSTATE.D and then context switch back
>     to the idle thread before returning from the IRQ. The exception
>     return patch will then restore PSTATE.D from the stack, and set it
>     again.

Nit: s/patch/path/

> 
> This patch fixes the problem by moving the clearing of PSTATE.D earlier
> to proc.S. This has the desirable effect of clearing it in one place for
> all CPUs, long before we have to worry about the scheduler or any
> exception handling. We ensure that the previous reset of MDSCR_EL1 has
> completed before unmasking the exception, so that any spurious
> exceptions resulting from UNKNOWN debug registers are not generated.
> 
> Without this patch applied, the kprobes selftests have been seen to fail
> under KVM, where we end up attempting to step the OOL instruction buffer
> with PSTATE.D set and therefore fail to complete the step.
> 
> Cc: <stable@vger.kernel.org>
> Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Otherwise, this looks good to me. Unifying the primary/secondary init is
a nice cleanup regardless.

FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

> ---
>  arch/arm64/kernel/debug-monitors.c | 1 -
>  arch/arm64/kernel/smp.c            | 1 -
>  arch/arm64/mm/proc.S               | 2 ++
>  3 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 2fbc1b99e8fb..7a907e401c98 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -156,7 +156,6 @@ static int debug_monitors_init(void)
>  	/* Clear the OS lock. */
>  	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 62ff3c0622e2..c0a772560ab7 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -267,7 +267,6 @@ asmlinkage void secondary_start_kernel(void)
>  	set_cpu_online(cpu, true);
>  	complete(&cpu_running);
>  
> -	local_dbg_enable();
>  	local_irq_enable();
>  	local_async_enable();
>  
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index c4317879b938..7757ebf7b2d7 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -180,6 +180,8 @@ ENTRY(__cpu_setup)
>  	msr	cpacr_el1, x0			// Enable FP/ASIMD
>  	mov	x0, #1 << 12			// Reset mdscr_el1 and disable
>  	msr	mdscr_el1, x0			// access to the DCC from EL0
> +	isb					// Unmask debug exceptions now,
> +	msr	daifclr, #8			// since this is per-cpu
>  	reset_pmuserenr_el0 x0			// Disable PMU access from EL0
>  	/*
>  	 * Memory region attributes for LPAE:
> -- 
> 2.1.4
> 

Re: [PATCH 1/3] arm64: debug: unmask PSTATE.D earlier

[Marc Zyngier]

On 19/07/16 12:19, Will Deacon wrote:
> Clearing PSTATE.D is one of the requirements for generating a debug
> exception. The arm64 booting protocol requires that PSTATE.D is set,
> since many of the debug registers (for example, the hw_breakpoint
> registers) are UNKNOWN out of reset and could potentially generate
> spurious, fatal debug exceptions in early boot code if PSTATE.D was
> clear. Once the debug registers have been safely initialised, PSTATE.D
> is cleared, however this is currently broken for two reasons:
> 
> (1) The boot CPU clears PSTATE.D in a postcore_initcall and secondary
>     CPUs clear PSTATE.D in secondary_start_kernel. Since the initcall
>     runs after SMP (and the scheduler) have been initialised, there is
>     no guarantee that it is actually running on the boot CPU. In this
>     case, the boot CPU is left with PSTATE.D set and is not capable of
>     generating debug exceptions.
> 
> (2) In a preemptible kernel, we may explicitly schedule on the IRQ
>     return path to EL1. If an IRQ occurs with PSTATE.D set in the idle
>     thread, then we may schedule the kthread_init thread, run the
>     postcore_initcall to clear PSTATE.D and then context switch back
>     to the idle thread before returning from the IRQ. The exception
>     return patch will then restore PSTATE.D from the stack, and set it
>     again.
> 
> This patch fixes the problem by moving the clearing of PSTATE.D earlier
> to proc.S. This has the desirable effect of clearing it in one place for
> all CPUs, long before we have to worry about the scheduler or any
> exception handling. We ensure that the previous reset of MDSCR_EL1 has
> completed before unmasking the exception, so that any spurious
> exceptions resulting from UNKNOWN debug registers are not generated.
> 
> Without this patch applied, the kprobes selftests have been seen to fail
> under KVM, where we end up attempting to step the OOL instruction buffer
> with PSTATE.D set and therefore fail to complete the step.
> 
> Cc: <stable@vger.kernel.org>
> Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Gave it a spin on both Juno and Seattle, both host and KVM guest, and
everything went smoothly, so:

Tested-by: Marc Zyngier <marc.zyngier@arm.com>

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/3] arm64: debug: unmask PSTATE.D earlier

[Robin Murphy]

On 19/07/16 12:19, Will Deacon wrote:
> Clearing PSTATE.D is one of the requirements for generating a debug
> exception. The arm64 booting protocol requires that PSTATE.D is set,
> since many of the debug registers (for example, the hw_breakpoint
> registers) are UNKNOWN out of reset and could potentially generate
> spurious, fatal debug exceptions in early boot code if PSTATE.D was
> clear. Once the debug registers have been safely initialised, PSTATE.D
> is cleared, however this is currently broken for two reasons:
> 
> (1) The boot CPU clears PSTATE.D in a postcore_initcall and secondary
>     CPUs clear PSTATE.D in secondary_start_kernel. Since the initcall
>     runs after SMP (and the scheduler) have been initialised, there is
>     no guarantee that it is actually running on the boot CPU. In this
>     case, the boot CPU is left with PSTATE.D set and is not capable of
>     generating debug exceptions.
> 
> (2) In a preemptible kernel, we may explicitly schedule on the IRQ
>     return path to EL1. If an IRQ occurs with PSTATE.D set in the idle
>     thread, then we may schedule the kthread_init thread, run the
>     postcore_initcall to clear PSTATE.D and then context switch back
>     to the idle thread before returning from the IRQ. The exception
>     return patch will then restore PSTATE.D from the stack, and set it
>     again.
> 
> This patch fixes the problem by moving the clearing of PSTATE.D earlier
> to proc.S. This has the desirable effect of clearing it in one place for
> all CPUs, long before we have to worry about the scheduler or any
> exception handling. We ensure that the previous reset of MDSCR_EL1 has
> completed before unmasking the exception, so that any spurious
> exceptions resulting from UNKNOWN debug registers are not generated.
> 
> Without this patch applied, the kprobes selftests have been seen to fail
> under KVM, where we end up attempting to step the OOL instruction buffer
> with PSTATE.D set and therefore fail to complete the step.
> 
> Cc: <stable@vger.kernel.org>
> Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/kernel/debug-monitors.c | 1 -
>  arch/arm64/kernel/smp.c            | 1 -
>  arch/arm64/mm/proc.S               | 2 ++
>  3 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 2fbc1b99e8fb..7a907e401c98 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -156,7 +156,6 @@ static int debug_monitors_init(void)
>  	/* Clear the OS lock. */
>  	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 62ff3c0622e2..c0a772560ab7 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -267,7 +267,6 @@ asmlinkage void secondary_start_kernel(void)
>  	set_cpu_online(cpu, true);
>  	complete(&cpu_running);
>  
> -	local_dbg_enable();

The above look to be the only uses of local_dbg_enable() - do we still
need to keep the definition?

Robin.

>  	local_irq_enable();
>  	local_async_enable();
>  
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index c4317879b938..7757ebf7b2d7 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -180,6 +180,8 @@ ENTRY(__cpu_setup)
>  	msr	cpacr_el1, x0			// Enable FP/ASIMD
>  	mov	x0, #1 << 12			// Reset mdscr_el1 and disable
>  	msr	mdscr_el1, x0			// access to the DCC from EL0
> +	isb					// Unmask debug exceptions now,
> +	msr	daifclr, #8			// since this is per-cpu
>  	reset_pmuserenr_el0 x0			// Disable PMU access from EL0
>  	/*
>  	 * Memory region attributes for LPAE:
>