* Re: [PATCH 1/3] arm64: debug: unmask PSTATE.D earlier
2016-07-19 11:19 [PATCH 1/3] arm64: debug: unmask PSTATE.D earlier Will Deacon
@ 2016-07-19 12:30 ` Mark Rutland
2016-07-19 13:00 ` Marc Zyngier
2016-07-19 13:05 ` Robin Murphy
2 siblings, 0 replies; 4+ messages in thread
From: Mark Rutland @ 2016-07-19 12:30 UTC (permalink / raw)
To: Will Deacon; +Cc: linux-eng, catalin.marinas, marc.zyngier, stable
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/3] arm64: debug: unmask PSTATE.D earlier
2016-07-19 11:19 [PATCH 1/3] arm64: debug: unmask PSTATE.D earlier Will Deacon
2016-07-19 12:30 ` Mark Rutland
@ 2016-07-19 13:00 ` Marc Zyngier
2016-07-19 13:05 ` Robin Murphy
2 siblings, 0 replies; 4+ messages in thread
From: Marc Zyngier @ 2016-07-19 13:00 UTC (permalink / raw)
To: Will Deacon, linux-eng; +Cc: catalin.marinas, stable
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...
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/3] arm64: debug: unmask PSTATE.D earlier
2016-07-19 11:19 [PATCH 1/3] arm64: debug: unmask PSTATE.D earlier Will Deacon
2016-07-19 12:30 ` Mark Rutland
2016-07-19 13:00 ` Marc Zyngier
@ 2016-07-19 13:05 ` Robin Murphy
2 siblings, 0 replies; 4+ messages in thread
From: Robin Murphy @ 2016-07-19 13:05 UTC (permalink / raw)
To: Will Deacon, linux-eng; +Cc: catalin.marinas, marc.zyngier, stable
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:
>
^ permalink raw reply [flat|nested] 4+ messages in thread