[PATCH] arm64: entry: always set GIC_PRIO_PSR_I_SET during entry

[PATCH] arm64: entry: always set GIC_PRIO_PSR_I_SET during entry

[Mark Rutland]

Zenghui reports that booting a kernel with "irqchip.gicv3_pseudo_nmi=1"
on the command line hits a warning during kernel entry, due to the way
we manipulate the PMR.

Early in the entry sequence, we call lockdep_hardirqs_off() to inform
lockdep that interrupts have been masked (as the HW sets DAIF wqhen
entering an exception). Architecturally PMR_EL1 is not affected by
exception entry, and we don't set GIC_PRIO_PSR_I_SET in the PMR early in
the exception entry sequence, so early in exception entry the PMR can
indicate that interrupts are unmasked even though they are masked by
DAIF.

If DEBUG_LOCKDEP is selected, lockdep_hardirqs_off() will check that
interrupts are masked, before we set GIC_PRIO_PSR_I_SET in any of the
exception entry paths, and hence lockdep_hardirqs_off() will WARN() that
something is amiss.

We can avoid this by consistently setting GIC_PRIO_PSR_I_SET during
exception entry so that kernel code sees a consistent environment. We
must also update local_daif_inherit() to undo this, as currently only
touches DAIF. For other paths, local_daif_restore() will update both
DAIF and the PMR. With this done, we can remove the existing special
cases which set this later in the entry code.

We always use (GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET) for consistency with
local_daif_save(), as this will warn if it ever encounters
(GIC_PRIO_IRQOFF | GIC_PRIO_PSR_I_SET), and never sets this itself. This
matches the gic_prio_kentry_setup that we have to retain for
ret_to_user.

The original splat from Zenghui's report was:

| DEBUG_LOCKS_WARN_ON(!irqs_disabled())
| WARNING: CPU: 3 PID: 125 at kernel/locking/lockdep.c:4258 lockdep_hardirqs_off+0xd4/0xe8
| Modules linked in:
| CPU: 3 PID: 125 Comm: modprobe Tainted: G        W         5.12.0-rc8+ #463
| Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
| pstate: 604003c5 (nZCv DAIF +PAN -UAO -TCO BTYPE=--)
| pc : lockdep_hardirqs_off+0xd4/0xe8
| lr : lockdep_hardirqs_off+0xd4/0xe8
| sp : ffff80002a39bad0
| pmr_save: 000000e0
| x29: ffff80002a39bad0 x28: ffff0000de214bc0
| x27: ffff0000de1c0400 x26: 000000000049b328
| x25: 0000000000406f30 x24: ffff0000de1c00a0
| x23: 0000000020400005 x22: ffff8000105f747c
| x21: 0000000096000044 x20: 0000000000498ef9
| x19: ffff80002a39bc88 x18: ffffffffffffffff
| x17: 0000000000000000 x16: ffff800011c61eb0
| x15: ffff800011700a88 x14: 0720072007200720
| x13: 0720072007200720 x12: 0720072007200720
| x11: 0720072007200720 x10: 0720072007200720
| x9 : ffff80002a39bad0 x8 : ffff80002a39bad0
| x7 : ffff8000119f0800 x6 : c0000000ffff7fff
| x5 : ffff8000119f07a8 x4 : 0000000000000001
| x3 : 9bcdab23f2432800 x2 : ffff800011730538
| x1 : 9bcdab23f2432800 x0 : 0000000000000000
| Call trace:
|  lockdep_hardirqs_off+0xd4/0xe8
|  enter_from_kernel_mode.isra.5+0x7c/0xa8
|  el1_abort+0x24/0x100
|  el1_sync_handler+0x80/0xd0
|  el1_sync+0x6c/0x100
|  __arch_clear_user+0xc/0x90
|  load_elf_binary+0x9fc/0x1450
|  bprm_execve+0x404/0x880
|  kernel_execve+0x180/0x188
|  call_usermodehelper_exec_async+0xdc/0x158
|  ret_from_fork+0x10/0x18

Fixes: 23529049c6842382 ("arm64: entry: fix non-NMI user<->kernel transitions")
Fixes: 7cd1ea1010acbede ("arm64: entry: fix non-NMI kernel<->kernel transitions")
Fixes: f0cd5ac1e4c53cb6 ("arm64: entry: fix NMI {user, kernel}->kernel transitions")
Fixes: 2a9b3e6ac69a8bf1 ("arm64: entry: fix EL1 debug transitions")
Link: https://lore.kernel.org/r/f4012761-026f-4e51-3a0c-7524e434e8b3@huawei.com
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reported-by: Zenghui Yu <yuzenghui@huawei.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/daifflags.h |  3 +++
 arch/arm64/kernel/entry-common.c   | 17 -----------------
 arch/arm64/kernel/entry.S          | 15 ++-------------
 3 files changed, 5 insertions(+), 30 deletions(-)

diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
index 1c26d7baa67f..cfdde3a56805 100644
--- a/arch/arm64/include/asm/daifflags.h
+++ b/arch/arm64/include/asm/daifflags.h
@@ -131,6 +131,9 @@ static inline void local_daif_inherit(struct pt_regs *regs)
 	if (interrupts_enabled(regs))
 		trace_hardirqs_on();
 
+	if (system_uses_irq_prio_masking())
+		gic_write_pmr(regs->pmr_save);
+
 	/*
 	 * We can't use local_daif_restore(regs->pstate) here as
 	 * system_has_prio_mask_debugging() won't restore the I bit if it can
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index 9d3588450473..117412bae915 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -226,14 +226,6 @@ static void noinstr el1_dbg(struct pt_regs *regs, unsigned long esr)
 {
 	unsigned long far = read_sysreg(far_el1);
 
-	/*
-	 * The CPU masked interrupts, and we are leaving them masked during
-	 * do_debug_exception(). Update PMR as if we had called
-	 * local_daif_mask().
-	 */
-	if (system_uses_irq_prio_masking())
-		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
-
 	arm64_enter_el1_dbg(regs);
 	if (!cortex_a76_erratum_1463225_debug_handler(regs))
 		do_debug_exception(far, esr, regs);
@@ -398,9 +390,6 @@ static void noinstr el0_dbg(struct pt_regs *regs, unsigned long esr)
 	/* Only watchpoints write FAR_EL1, otherwise its UNKNOWN */
 	unsigned long far = read_sysreg(far_el1);
 
-	if (system_uses_irq_prio_masking())
-		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
-
 	enter_from_user_mode();
 	do_debug_exception(far, esr, regs);
 	local_daif_restore(DAIF_PROCCTX_NOIRQ);
@@ -408,9 +397,6 @@ static void noinstr el0_dbg(struct pt_regs *regs, unsigned long esr)
 
 static void noinstr el0_svc(struct pt_regs *regs)
 {
-	if (system_uses_irq_prio_masking())
-		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
-
 	enter_from_user_mode();
 	cortex_a76_erratum_1463225_svc_handler();
 	do_el0_svc(regs);
@@ -486,9 +472,6 @@ static void noinstr el0_cp15(struct pt_regs *regs, unsigned long esr)
 
 static void noinstr el0_svc_compat(struct pt_regs *regs)
 {
-	if (system_uses_irq_prio_masking())
-		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
-
 	enter_from_user_mode();
 	cortex_a76_erratum_1463225_svc_handler();
 	do_el0_svc_compat(regs);
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 6acfc5e6b5e0..941bb52b5e21 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -292,6 +292,8 @@ alternative_else_nop_endif
 alternative_if ARM64_HAS_IRQ_PRIO_MASKING
 	mrs_s	x20, SYS_ICC_PMR_EL1
 	str	x20, [sp, #S_PMR_SAVE]
+	mov	x20, #GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET
+	msr_s	SYS_ICC_PMR_EL1, x20
 alternative_else_nop_endif
 
 	/* Re-enable tag checking (TCO set on exception entry) */
@@ -524,15 +526,6 @@ alternative_endif
 #endif
 	.endm
 
-	.macro	gic_prio_irq_setup, pmr:req, tmp:req
-#ifdef CONFIG_ARM64_PSEUDO_NMI
-	alternative_if ARM64_HAS_IRQ_PRIO_MASKING
-	orr	\tmp, \pmr, #GIC_PRIO_PSR_I_SET
-	msr_s	SYS_ICC_PMR_EL1, \tmp
-	alternative_else_nop_endif
-#endif
-	.endm
-
 	.text
 
 /*
@@ -662,7 +655,6 @@ SYM_CODE_END(el1_sync)
 	.align	6
 SYM_CODE_START_LOCAL_NOALIGN(el1_irq)
 	kernel_entry 1
-	gic_prio_irq_setup pmr=x20, tmp=x1
 	enable_da_f
 
 	mov	x0, sp
@@ -727,7 +719,6 @@ SYM_CODE_END(el0_error_compat)
 SYM_CODE_START_LOCAL_NOALIGN(el0_irq)
 	kernel_entry 0
 el0_irq_naked:
-	gic_prio_irq_setup pmr=x20, tmp=x0
 	user_exit_irqoff
 	enable_da_f
 
@@ -742,7 +733,6 @@ SYM_CODE_END(el0_irq)
 SYM_CODE_START_LOCAL(el1_error)
 	kernel_entry 1
 	mrs	x1, esr_el1
-	gic_prio_kentry_setup tmp=x2
 	enable_dbg
 	mov	x0, sp
 	bl	do_serror
@@ -753,7 +743,6 @@ SYM_CODE_START_LOCAL(el0_error)
 	kernel_entry 0
 el0_error_naked:
 	mrs	x25, esr_el1
-	gic_prio_kentry_setup tmp=x2
 	user_exit_irqoff
 	enable_dbg
 	mov	x0, sp
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: entry: always set GIC_PRIO_PSR_I_SET during entry

[Marc Zyngier]

Hi Mark,

On Wed, 28 Apr 2021 12:15:55 +0100,
Mark Rutland <mark.rutland@arm.com> wrote:
> 
> Zenghui reports that booting a kernel with "irqchip.gicv3_pseudo_nmi=1"
> on the command line hits a warning during kernel entry, due to the way
> we manipulate the PMR.
> 
> Early in the entry sequence, we call lockdep_hardirqs_off() to inform
> lockdep that interrupts have been masked (as the HW sets DAIF wqhen
> entering an exception). Architecturally PMR_EL1 is not affected by
> exception entry, and we don't set GIC_PRIO_PSR_I_SET in the PMR early in
> the exception entry sequence, so early in exception entry the PMR can
> indicate that interrupts are unmasked even though they are masked by
> DAIF.
> 
> If DEBUG_LOCKDEP is selected, lockdep_hardirqs_off() will check that
> interrupts are masked, before we set GIC_PRIO_PSR_I_SET in any of the
> exception entry paths, and hence lockdep_hardirqs_off() will WARN() that
> something is amiss.
> 
> We can avoid this by consistently setting GIC_PRIO_PSR_I_SET during
> exception entry so that kernel code sees a consistent environment. We
> must also update local_daif_inherit() to undo this, as currently only
> touches DAIF. For other paths, local_daif_restore() will update both
> DAIF and the PMR. With this done, we can remove the existing special
> cases which set this later in the entry code.
> 
> We always use (GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET) for consistency with
> local_daif_save(), as this will warn if it ever encounters
> (GIC_PRIO_IRQOFF | GIC_PRIO_PSR_I_SET), and never sets this itself. This
> matches the gic_prio_kentry_setup that we have to retain for
> ret_to_user.
> 
> The original splat from Zenghui's report was:
> 
> | DEBUG_LOCKS_WARN_ON(!irqs_disabled())
> | WARNING: CPU: 3 PID: 125 at kernel/locking/lockdep.c:4258 lockdep_hardirqs_off+0xd4/0xe8
> | Modules linked in:
> | CPU: 3 PID: 125 Comm: modprobe Tainted: G        W         5.12.0-rc8+ #463
> | Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
> | pstate: 604003c5 (nZCv DAIF +PAN -UAO -TCO BTYPE=--)
> | pc : lockdep_hardirqs_off+0xd4/0xe8
> | lr : lockdep_hardirqs_off+0xd4/0xe8
> | sp : ffff80002a39bad0
> | pmr_save: 000000e0
> | x29: ffff80002a39bad0 x28: ffff0000de214bc0
> | x27: ffff0000de1c0400 x26: 000000000049b328
> | x25: 0000000000406f30 x24: ffff0000de1c00a0
> | x23: 0000000020400005 x22: ffff8000105f747c
> | x21: 0000000096000044 x20: 0000000000498ef9
> | x19: ffff80002a39bc88 x18: ffffffffffffffff
> | x17: 0000000000000000 x16: ffff800011c61eb0
> | x15: ffff800011700a88 x14: 0720072007200720
> | x13: 0720072007200720 x12: 0720072007200720
> | x11: 0720072007200720 x10: 0720072007200720
> | x9 : ffff80002a39bad0 x8 : ffff80002a39bad0
> | x7 : ffff8000119f0800 x6 : c0000000ffff7fff
> | x5 : ffff8000119f07a8 x4 : 0000000000000001
> | x3 : 9bcdab23f2432800 x2 : ffff800011730538
> | x1 : 9bcdab23f2432800 x0 : 0000000000000000
> | Call trace:
> |  lockdep_hardirqs_off+0xd4/0xe8
> |  enter_from_kernel_mode.isra.5+0x7c/0xa8
> |  el1_abort+0x24/0x100
> |  el1_sync_handler+0x80/0xd0
> |  el1_sync+0x6c/0x100
> |  __arch_clear_user+0xc/0x90
> |  load_elf_binary+0x9fc/0x1450
> |  bprm_execve+0x404/0x880
> |  kernel_execve+0x180/0x188
> |  call_usermodehelper_exec_async+0xdc/0x158
> |  ret_from_fork+0x10/0x18
> 
> Fixes: 23529049c6842382 ("arm64: entry: fix non-NMI user<->kernel transitions")
> Fixes: 7cd1ea1010acbede ("arm64: entry: fix non-NMI kernel<->kernel transitions")
> Fixes: f0cd5ac1e4c53cb6 ("arm64: entry: fix NMI {user, kernel}->kernel transitions")
> Fixes: 2a9b3e6ac69a8bf1 ("arm64: entry: fix EL1 debug transitions")
> Link: https://lore.kernel.org/r/f4012761-026f-4e51-3a0c-7524e434e8b3@huawei.com
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Reported-by: Zenghui Yu <yuzenghui@huawei.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/daifflags.h |  3 +++
>  arch/arm64/kernel/entry-common.c   | 17 -----------------
>  arch/arm64/kernel/entry.S          | 15 ++-------------
>  3 files changed, 5 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
> index 1c26d7baa67f..cfdde3a56805 100644
> --- a/arch/arm64/include/asm/daifflags.h
> +++ b/arch/arm64/include/asm/daifflags.h
> @@ -131,6 +131,9 @@ static inline void local_daif_inherit(struct pt_regs *regs)
>  	if (interrupts_enabled(regs))
>  		trace_hardirqs_on();
>  
> +	if (system_uses_irq_prio_masking())
> +		gic_write_pmr(regs->pmr_save);
> +
>  	/*
>  	 * We can't use local_daif_restore(regs->pstate) here as
>  	 * system_has_prio_mask_debugging() won't restore the I bit if it can
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 9d3588450473..117412bae915 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -226,14 +226,6 @@ static void noinstr el1_dbg(struct pt_regs *regs, unsigned long esr)
>  {
>  	unsigned long far = read_sysreg(far_el1);
>  
> -	/*
> -	 * The CPU masked interrupts, and we are leaving them masked during
> -	 * do_debug_exception(). Update PMR as if we had called
> -	 * local_daif_mask().
> -	 */
> -	if (system_uses_irq_prio_masking())
> -		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
> -
>  	arm64_enter_el1_dbg(regs);
>  	if (!cortex_a76_erratum_1463225_debug_handler(regs))
>  		do_debug_exception(far, esr, regs);
> @@ -398,9 +390,6 @@ static void noinstr el0_dbg(struct pt_regs *regs, unsigned long esr)
>  	/* Only watchpoints write FAR_EL1, otherwise its UNKNOWN */
>  	unsigned long far = read_sysreg(far_el1);
>  
> -	if (system_uses_irq_prio_masking())
> -		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
> -
>  	enter_from_user_mode();
>  	do_debug_exception(far, esr, regs);
>  	local_daif_restore(DAIF_PROCCTX_NOIRQ);
> @@ -408,9 +397,6 @@ static void noinstr el0_dbg(struct pt_regs *regs, unsigned long esr)
>  
>  static void noinstr el0_svc(struct pt_regs *regs)
>  {
> -	if (system_uses_irq_prio_masking())
> -		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
> -
>  	enter_from_user_mode();
>  	cortex_a76_erratum_1463225_svc_handler();
>  	do_el0_svc(regs);
> @@ -486,9 +472,6 @@ static void noinstr el0_cp15(struct pt_regs *regs, unsigned long esr)
>  
>  static void noinstr el0_svc_compat(struct pt_regs *regs)
>  {
> -	if (system_uses_irq_prio_masking())
> -		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
> -
>  	enter_from_user_mode();
>  	cortex_a76_erratum_1463225_svc_handler();
>  	do_el0_svc_compat(regs);
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 6acfc5e6b5e0..941bb52b5e21 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -292,6 +292,8 @@ alternative_else_nop_endif
>  alternative_if ARM64_HAS_IRQ_PRIO_MASKING
>  	mrs_s	x20, SYS_ICC_PMR_EL1
>  	str	x20, [sp, #S_PMR_SAVE]
> +	mov	x20, #GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET
> +	msr_s	SYS_ICC_PMR_EL1, x20

I'm a bit worried about forcing PMR to IRQON at this stage. We could
have been in IRQOFF state and entering the kernel because of a
NMI. Don't we risk losing track of how we made it here? In the current
code, the ORR makes it plain that only the I bit has changed at this
stage.

Is there some other state tracking that make this complexity
irrelevant?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: entry: always set GIC_PRIO_PSR_I_SET during entry

[Mark Rutland]

On Wed, Apr 28, 2021 at 03:41:17PM +0100, Marc Zyngier wrote:
> Hi Mark,
> 
> On Wed, 28 Apr 2021 12:15:55 +0100,
> Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > Zenghui reports that booting a kernel with "irqchip.gicv3_pseudo_nmi=1"
> > on the command line hits a warning during kernel entry, due to the way
> > we manipulate the PMR.
> > 
> > Early in the entry sequence, we call lockdep_hardirqs_off() to inform
> > lockdep that interrupts have been masked (as the HW sets DAIF wqhen
> > entering an exception). Architecturally PMR_EL1 is not affected by
> > exception entry, and we don't set GIC_PRIO_PSR_I_SET in the PMR early in
> > the exception entry sequence, so early in exception entry the PMR can
> > indicate that interrupts are unmasked even though they are masked by
> > DAIF.
> > 
> > If DEBUG_LOCKDEP is selected, lockdep_hardirqs_off() will check that
> > interrupts are masked, before we set GIC_PRIO_PSR_I_SET in any of the
> > exception entry paths, and hence lockdep_hardirqs_off() will WARN() that
> > something is amiss.
> > 
> > We can avoid this by consistently setting GIC_PRIO_PSR_I_SET during
> > exception entry so that kernel code sees a consistent environment. We
> > must also update local_daif_inherit() to undo this, as currently only
> > touches DAIF. For other paths, local_daif_restore() will update both
> > DAIF and the PMR. With this done, we can remove the existing special
> > cases which set this later in the entry code.
> > 
> > We always use (GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET) for consistency with
> > local_daif_save(), as this will warn if it ever encounters
> > (GIC_PRIO_IRQOFF | GIC_PRIO_PSR_I_SET), and never sets this itself. This
> > matches the gic_prio_kentry_setup that we have to retain for
> > ret_to_user.
> > 
> > The original splat from Zenghui's report was:
> > 
> > | DEBUG_LOCKS_WARN_ON(!irqs_disabled())
> > | WARNING: CPU: 3 PID: 125 at kernel/locking/lockdep.c:4258 lockdep_hardirqs_off+0xd4/0xe8
> > | Modules linked in:
> > | CPU: 3 PID: 125 Comm: modprobe Tainted: G        W         5.12.0-rc8+ #463
> > | Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
> > | pstate: 604003c5 (nZCv DAIF +PAN -UAO -TCO BTYPE=--)
> > | pc : lockdep_hardirqs_off+0xd4/0xe8
> > | lr : lockdep_hardirqs_off+0xd4/0xe8
> > | sp : ffff80002a39bad0
> > | pmr_save: 000000e0
> > | x29: ffff80002a39bad0 x28: ffff0000de214bc0
> > | x27: ffff0000de1c0400 x26: 000000000049b328
> > | x25: 0000000000406f30 x24: ffff0000de1c00a0
> > | x23: 0000000020400005 x22: ffff8000105f747c
> > | x21: 0000000096000044 x20: 0000000000498ef9
> > | x19: ffff80002a39bc88 x18: ffffffffffffffff
> > | x17: 0000000000000000 x16: ffff800011c61eb0
> > | x15: ffff800011700a88 x14: 0720072007200720
> > | x13: 0720072007200720 x12: 0720072007200720
> > | x11: 0720072007200720 x10: 0720072007200720
> > | x9 : ffff80002a39bad0 x8 : ffff80002a39bad0
> > | x7 : ffff8000119f0800 x6 : c0000000ffff7fff
> > | x5 : ffff8000119f07a8 x4 : 0000000000000001
> > | x3 : 9bcdab23f2432800 x2 : ffff800011730538
> > | x1 : 9bcdab23f2432800 x0 : 0000000000000000
> > | Call trace:
> > |  lockdep_hardirqs_off+0xd4/0xe8
> > |  enter_from_kernel_mode.isra.5+0x7c/0xa8
> > |  el1_abort+0x24/0x100
> > |  el1_sync_handler+0x80/0xd0
> > |  el1_sync+0x6c/0x100
> > |  __arch_clear_user+0xc/0x90
> > |  load_elf_binary+0x9fc/0x1450
> > |  bprm_execve+0x404/0x880
> > |  kernel_execve+0x180/0x188
> > |  call_usermodehelper_exec_async+0xdc/0x158
> > |  ret_from_fork+0x10/0x18
> > 
> > Fixes: 23529049c6842382 ("arm64: entry: fix non-NMI user<->kernel transitions")
> > Fixes: 7cd1ea1010acbede ("arm64: entry: fix non-NMI kernel<->kernel transitions")
> > Fixes: f0cd5ac1e4c53cb6 ("arm64: entry: fix NMI {user, kernel}->kernel transitions")
> > Fixes: 2a9b3e6ac69a8bf1 ("arm64: entry: fix EL1 debug transitions")
> > Link: https://lore.kernel.org/r/f4012761-026f-4e51-3a0c-7524e434e8b3@huawei.com
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Reported-by: Zenghui Yu <yuzenghui@huawei.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Will Deacon <will@kernel.org>
> > ---
> >  arch/arm64/include/asm/daifflags.h |  3 +++
> >  arch/arm64/kernel/entry-common.c   | 17 -----------------
> >  arch/arm64/kernel/entry.S          | 15 ++-------------
> >  3 files changed, 5 insertions(+), 30 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
> > index 1c26d7baa67f..cfdde3a56805 100644
> > --- a/arch/arm64/include/asm/daifflags.h
> > +++ b/arch/arm64/include/asm/daifflags.h
> > @@ -131,6 +131,9 @@ static inline void local_daif_inherit(struct pt_regs *regs)
> >  	if (interrupts_enabled(regs))
> >  		trace_hardirqs_on();
> >  
> > +	if (system_uses_irq_prio_masking())
> > +		gic_write_pmr(regs->pmr_save);
> > +
> >  	/*
> >  	 * We can't use local_daif_restore(regs->pstate) here as
> >  	 * system_has_prio_mask_debugging() won't restore the I bit if it can
> > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> > index 9d3588450473..117412bae915 100644
> > --- a/arch/arm64/kernel/entry-common.c
> > +++ b/arch/arm64/kernel/entry-common.c
> > @@ -226,14 +226,6 @@ static void noinstr el1_dbg(struct pt_regs *regs, unsigned long esr)
> >  {
> >  	unsigned long far = read_sysreg(far_el1);
> >  
> > -	/*
> > -	 * The CPU masked interrupts, and we are leaving them masked during
> > -	 * do_debug_exception(). Update PMR as if we had called
> > -	 * local_daif_mask().
> > -	 */
> > -	if (system_uses_irq_prio_masking())
> > -		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
> > -
> >  	arm64_enter_el1_dbg(regs);
> >  	if (!cortex_a76_erratum_1463225_debug_handler(regs))
> >  		do_debug_exception(far, esr, regs);
> > @@ -398,9 +390,6 @@ static void noinstr el0_dbg(struct pt_regs *regs, unsigned long esr)
> >  	/* Only watchpoints write FAR_EL1, otherwise its UNKNOWN */
> >  	unsigned long far = read_sysreg(far_el1);
> >  
> > -	if (system_uses_irq_prio_masking())
> > -		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
> > -
> >  	enter_from_user_mode();
> >  	do_debug_exception(far, esr, regs);
> >  	local_daif_restore(DAIF_PROCCTX_NOIRQ);
> > @@ -408,9 +397,6 @@ static void noinstr el0_dbg(struct pt_regs *regs, unsigned long esr)
> >  
> >  static void noinstr el0_svc(struct pt_regs *regs)
> >  {
> > -	if (system_uses_irq_prio_masking())
> > -		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
> > -
> >  	enter_from_user_mode();
> >  	cortex_a76_erratum_1463225_svc_handler();
> >  	do_el0_svc(regs);
> > @@ -486,9 +472,6 @@ static void noinstr el0_cp15(struct pt_regs *regs, unsigned long esr)
> >  
> >  static void noinstr el0_svc_compat(struct pt_regs *regs)
> >  {
> > -	if (system_uses_irq_prio_masking())
> > -		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
> > -
> >  	enter_from_user_mode();
> >  	cortex_a76_erratum_1463225_svc_handler();
> >  	do_el0_svc_compat(regs);
> > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > index 6acfc5e6b5e0..941bb52b5e21 100644
> > --- a/arch/arm64/kernel/entry.S
> > +++ b/arch/arm64/kernel/entry.S
> > @@ -292,6 +292,8 @@ alternative_else_nop_endif
> >  alternative_if ARM64_HAS_IRQ_PRIO_MASKING
> >  	mrs_s	x20, SYS_ICC_PMR_EL1
> >  	str	x20, [sp, #S_PMR_SAVE]
> > +	mov	x20, #GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET
> > +	msr_s	SYS_ICC_PMR_EL1, x20
> 
> I'm a bit worried about forcing PMR to IRQON at this stage. We could
> have been in IRQOFF state and entering the kernel because of a
> NMI. Don't we risk losing track of how we made it here?

I also worried about this, but after digging for a while I convinced
myself there wasn't a problem, as:

* This is what local_daif_mask() does today. It sets the PMR to
 GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET regardless of its original state.

* For checking how we got here, we use either:
 - regs::pmr_save, which is sampled (immediately) before we alter HW PMR
   in the asm above.
 - DAIF, which is unaffected by this.

* This is transparent to local_irq_save() .. local_irq_restore(), as
  that blindly saves/restores the PMR.

... and from digging through the history, I don't think this was
necessary even when it was introduced in commit:

  bd82d4bd21880b7c ("arm64: Fix incorrect irqflag restore for priority masking")

... for which (AFAICT) all of the above held true too.

Also, as mentioned above, local_daif_save() explicitly WARN()s if it
ever encounters (GIC_PRIO_IRQOFF | GIC_PRIO_PSR_I_SET), and I'd like to
ensure by construction that this case is never exposed to C code such as
the entry-common logic.

FWIW, I also don't *like* this, and still intend to rework local_irq_*()
and local_daif_*() to remove the pseudo-DAIF and PSR_I_SET logic, but
that still needs some preparatory work, and this is at least
self-consistent today.

> In the current code, the ORR makes it plain that only the I bit has
> changed at this stage.

I agree it makes that clear, however, given that would be inconsistent
with how local_daif_mask() programs the PMR (and given it'll WARN()) if
it's ever used with (GIC_PRIO_IRQOFF | GIC_PRIO_PSR_I_SET), I don't
think we can do that for all the entry cases (and would strongly prefer
to remove the existing inconsistency).

> Is there some other state tracking that make this complexity
> irrelevant?

Hopefully I've covered that above?

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: entry: always set GIC_PRIO_PSR_I_SET during entry

[Mark Rutland]

On Wed, Apr 28, 2021 at 04:19:11PM +0100, Mark Rutland wrote:
> On Wed, Apr 28, 2021 at 03:41:17PM +0100, Marc Zyngier wrote:
> > Hi Mark,
> > 
> > On Wed, 28 Apr 2021 12:15:55 +0100,
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > > 
> > > Zenghui reports that booting a kernel with "irqchip.gicv3_pseudo_nmi=1"
> > > on the command line hits a warning during kernel entry, due to the way
> > > we manipulate the PMR.
> > > 
> > > Early in the entry sequence, we call lockdep_hardirqs_off() to inform
> > > lockdep that interrupts have been masked (as the HW sets DAIF wqhen
> > > entering an exception). Architecturally PMR_EL1 is not affected by
> > > exception entry, and we don't set GIC_PRIO_PSR_I_SET in the PMR early in
> > > the exception entry sequence, so early in exception entry the PMR can
> > > indicate that interrupts are unmasked even though they are masked by
> > > DAIF.
> > > 
> > > If DEBUG_LOCKDEP is selected, lockdep_hardirqs_off() will check that
> > > interrupts are masked, before we set GIC_PRIO_PSR_I_SET in any of the
> > > exception entry paths, and hence lockdep_hardirqs_off() will WARN() that
> > > something is amiss.
> > > 
> > > We can avoid this by consistently setting GIC_PRIO_PSR_I_SET during
> > > exception entry so that kernel code sees a consistent environment. We
> > > must also update local_daif_inherit() to undo this, as currently only
> > > touches DAIF. For other paths, local_daif_restore() will update both
> > > DAIF and the PMR. With this done, we can remove the existing special
> > > cases which set this later in the entry code.
> > > 
> > > We always use (GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET) for consistency with
> > > local_daif_save(), as this will warn if it ever encounters
> > > (GIC_PRIO_IRQOFF | GIC_PRIO_PSR_I_SET), and never sets this itself. This
> > > matches the gic_prio_kentry_setup that we have to retain for
> > > ret_to_user.
> > > 
> > > The original splat from Zenghui's report was:
> > > 
> > > | DEBUG_LOCKS_WARN_ON(!irqs_disabled())
> > > | WARNING: CPU: 3 PID: 125 at kernel/locking/lockdep.c:4258 lockdep_hardirqs_off+0xd4/0xe8
> > > | Modules linked in:
> > > | CPU: 3 PID: 125 Comm: modprobe Tainted: G        W         5.12.0-rc8+ #463
> > > | Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
> > > | pstate: 604003c5 (nZCv DAIF +PAN -UAO -TCO BTYPE=--)
> > > | pc : lockdep_hardirqs_off+0xd4/0xe8
> > > | lr : lockdep_hardirqs_off+0xd4/0xe8
> > > | sp : ffff80002a39bad0
> > > | pmr_save: 000000e0
> > > | x29: ffff80002a39bad0 x28: ffff0000de214bc0
> > > | x27: ffff0000de1c0400 x26: 000000000049b328
> > > | x25: 0000000000406f30 x24: ffff0000de1c00a0
> > > | x23: 0000000020400005 x22: ffff8000105f747c
> > > | x21: 0000000096000044 x20: 0000000000498ef9
> > > | x19: ffff80002a39bc88 x18: ffffffffffffffff
> > > | x17: 0000000000000000 x16: ffff800011c61eb0
> > > | x15: ffff800011700a88 x14: 0720072007200720
> > > | x13: 0720072007200720 x12: 0720072007200720
> > > | x11: 0720072007200720 x10: 0720072007200720
> > > | x9 : ffff80002a39bad0 x8 : ffff80002a39bad0
> > > | x7 : ffff8000119f0800 x6 : c0000000ffff7fff
> > > | x5 : ffff8000119f07a8 x4 : 0000000000000001
> > > | x3 : 9bcdab23f2432800 x2 : ffff800011730538
> > > | x1 : 9bcdab23f2432800 x0 : 0000000000000000
> > > | Call trace:
> > > |  lockdep_hardirqs_off+0xd4/0xe8
> > > |  enter_from_kernel_mode.isra.5+0x7c/0xa8
> > > |  el1_abort+0x24/0x100
> > > |  el1_sync_handler+0x80/0xd0
> > > |  el1_sync+0x6c/0x100
> > > |  __arch_clear_user+0xc/0x90
> > > |  load_elf_binary+0x9fc/0x1450
> > > |  bprm_execve+0x404/0x880
> > > |  kernel_execve+0x180/0x188
> > > |  call_usermodehelper_exec_async+0xdc/0x158
> > > |  ret_from_fork+0x10/0x18
> > > 
> > > Fixes: 23529049c6842382 ("arm64: entry: fix non-NMI user<->kernel transitions")
> > > Fixes: 7cd1ea1010acbede ("arm64: entry: fix non-NMI kernel<->kernel transitions")
> > > Fixes: f0cd5ac1e4c53cb6 ("arm64: entry: fix NMI {user, kernel}->kernel transitions")
> > > Fixes: 2a9b3e6ac69a8bf1 ("arm64: entry: fix EL1 debug transitions")
> > > Link: https://lore.kernel.org/r/f4012761-026f-4e51-3a0c-7524e434e8b3@huawei.com
> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > Reported-by: Zenghui Yu <yuzenghui@huawei.com>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Marc Zyngier <maz@kernel.org>
> > > Cc: Will Deacon <will@kernel.org>
> > > ---
> > >  arch/arm64/include/asm/daifflags.h |  3 +++
> > >  arch/arm64/kernel/entry-common.c   | 17 -----------------
> > >  arch/arm64/kernel/entry.S          | 15 ++-------------
> > >  3 files changed, 5 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
> > > index 1c26d7baa67f..cfdde3a56805 100644
> > > --- a/arch/arm64/include/asm/daifflags.h
> > > +++ b/arch/arm64/include/asm/daifflags.h
> > > @@ -131,6 +131,9 @@ static inline void local_daif_inherit(struct pt_regs *regs)
> > >  	if (interrupts_enabled(regs))
> > >  		trace_hardirqs_on();
> > >  
> > > +	if (system_uses_irq_prio_masking())
> > > +		gic_write_pmr(regs->pmr_save);
> > > +
> > >  	/*
> > >  	 * We can't use local_daif_restore(regs->pstate) here as
> > >  	 * system_has_prio_mask_debugging() won't restore the I bit if it can
> > > diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> > > index 9d3588450473..117412bae915 100644
> > > --- a/arch/arm64/kernel/entry-common.c
> > > +++ b/arch/arm64/kernel/entry-common.c
> > > @@ -226,14 +226,6 @@ static void noinstr el1_dbg(struct pt_regs *regs, unsigned long esr)
> > >  {
> > >  	unsigned long far = read_sysreg(far_el1);
> > >  
> > > -	/*
> > > -	 * The CPU masked interrupts, and we are leaving them masked during
> > > -	 * do_debug_exception(). Update PMR as if we had called
> > > -	 * local_daif_mask().
> > > -	 */
> > > -	if (system_uses_irq_prio_masking())
> > > -		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
> > > -
> > >  	arm64_enter_el1_dbg(regs);
> > >  	if (!cortex_a76_erratum_1463225_debug_handler(regs))
> > >  		do_debug_exception(far, esr, regs);
> > > @@ -398,9 +390,6 @@ static void noinstr el0_dbg(struct pt_regs *regs, unsigned long esr)
> > >  	/* Only watchpoints write FAR_EL1, otherwise its UNKNOWN */
> > >  	unsigned long far = read_sysreg(far_el1);
> > >  
> > > -	if (system_uses_irq_prio_masking())
> > > -		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
> > > -
> > >  	enter_from_user_mode();
> > >  	do_debug_exception(far, esr, regs);
> > >  	local_daif_restore(DAIF_PROCCTX_NOIRQ);
> > > @@ -408,9 +397,6 @@ static void noinstr el0_dbg(struct pt_regs *regs, unsigned long esr)
> > >  
> > >  static void noinstr el0_svc(struct pt_regs *regs)
> > >  {
> > > -	if (system_uses_irq_prio_masking())
> > > -		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
> > > -
> > >  	enter_from_user_mode();
> > >  	cortex_a76_erratum_1463225_svc_handler();
> > >  	do_el0_svc(regs);
> > > @@ -486,9 +472,6 @@ static void noinstr el0_cp15(struct pt_regs *regs, unsigned long esr)
> > >  
> > >  static void noinstr el0_svc_compat(struct pt_regs *regs)
> > >  {
> > > -	if (system_uses_irq_prio_masking())
> > > -		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
> > > -
> > >  	enter_from_user_mode();
> > >  	cortex_a76_erratum_1463225_svc_handler();
> > >  	do_el0_svc_compat(regs);
> > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > > index 6acfc5e6b5e0..941bb52b5e21 100644
> > > --- a/arch/arm64/kernel/entry.S
> > > +++ b/arch/arm64/kernel/entry.S
> > > @@ -292,6 +292,8 @@ alternative_else_nop_endif
> > >  alternative_if ARM64_HAS_IRQ_PRIO_MASKING
> > >  	mrs_s	x20, SYS_ICC_PMR_EL1
> > >  	str	x20, [sp, #S_PMR_SAVE]
> > > +	mov	x20, #GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET
> > > +	msr_s	SYS_ICC_PMR_EL1, x20
> > 
> > I'm a bit worried about forcing PMR to IRQON at this stage. We could
> > have been in IRQOFF state and entering the kernel because of a
> > NMI. Don't we risk losing track of how we made it here?
> 
> I also worried about this, but after digging for a while I convinced
> myself there wasn't a problem, as:
> 
> * This is what local_daif_mask() does today. It sets the PMR to
>  GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET regardless of its original state.
> 
> * For checking how we got here, we use either:
>  - regs::pmr_save, which is sampled (immediately) before we alter HW PMR
>    in the asm above.
>  - DAIF, which is unaffected by this.
> 
> * This is transparent to local_irq_save() .. local_irq_restore(), as
>   that blindly saves/restores the PMR.
> 

... oh, and within the GIC driver:

* for a pNMI, we'll leave both PMR and DAIF as-is

* for an IRQ, we'll set the PMU to GIC_PRIO_IRQOFF (regardless of the
  original value), then unmask DAIF.

... so that'll do the right thing even if we enter with PMR set to
(GIC_PRIO_IRQON | CIG_PRIO_PSR_I_SET).

Thanks.
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: entry: always set GIC_PRIO_PSR_I_SET during entry

[Marc Zyngier]

On Wed, 28 Apr 2021 16:29:39 +0100,
Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Wed, Apr 28, 2021 at 04:19:11PM +0100, Mark Rutland wrote:
> > On Wed, Apr 28, 2021 at 03:41:17PM +0100, Marc Zyngier wrote:
> > > Hi Mark,

[...]

> > > I'm a bit worried about forcing PMR to IRQON at this stage. We could
> > > have been in IRQOFF state and entering the kernel because of a
> > > NMI. Don't we risk losing track of how we made it here?
> > 
> > I also worried about this, but after digging for a while I convinced
> > myself there wasn't a problem, as:
> > 
> > * This is what local_daif_mask() does today. It sets the PMR to
> >  GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET regardless of its original state.
> > 
> > * For checking how we got here, we use either:
> >  - regs::pmr_save, which is sampled (immediately) before we alter HW PMR
> >    in the asm above.
> >  - DAIF, which is unaffected by this.
> > 
> > * This is transparent to local_irq_save() .. local_irq_restore(), as
> >   that blindly saves/restores the PMR.
> > 
> 
> ... oh, and within the GIC driver:
> 
> * for a pNMI, we'll leave both PMR and DAIF as-is
> 
> * for an IRQ, we'll set the PMU to GIC_PRIO_IRQOFF (regardless of the
>   original value), then unmask DAIF.
> 
> ... so that'll do the right thing even if we enter with PMR set to
> (GIC_PRIO_IRQON | CIG_PRIO_PSR_I_SET).

Fair enough. FWIW:

Acked-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: entry: always set GIC_PRIO_PSR_I_SET during entry

[Catalin Marinas]

On Wed, 28 Apr 2021 12:15:55 +0100, Mark Rutland wrote:
> Zenghui reports that booting a kernel with "irqchip.gicv3_pseudo_nmi=1"
> on the command line hits a warning during kernel entry, due to the way
> we manipulate the PMR.
> 
> Early in the entry sequence, we call lockdep_hardirqs_off() to inform
> lockdep that interrupts have been masked (as the HW sets DAIF wqhen
> entering an exception). Architecturally PMR_EL1 is not affected by
> exception entry, and we don't set GIC_PRIO_PSR_I_SET in the PMR early in
> the exception entry sequence, so early in exception entry the PMR can
> indicate that interrupts are unmasked even though they are masked by
> DAIF.
> 
> [...]

Applied to arm64 (for-next/core), thanks!

[1/1] arm64: entry: always set GIC_PRIO_PSR_I_SET during entry
      https://git.kernel.org/arm64/c/4d6a38da8e79

-- 
Catalin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel