[PATCH v2 0/5] arm64: IRQ priority masking and Pseudo-NMI fixes

[PATCH v2 0/5] arm64: IRQ priority masking and Pseudo-NMI fixes

[Julien Thierry]

Hi,

[Changing the title to make it reflex more the status of the series.]

Version one[1] of this series attempted to fix the issue reported by
Zenghui[2] when using the function_graph tracer with IRQ priority
masking.

Since then, I realized that priority masking and the use of Pseudo-NMIs
was more broken than I thought.

* Patch 1 is just some rework
* Patch 2 replaces the previous irqflags tracing "simplification" with
  a proper fix when tracing Pseudo-NMI
* Patch 3 fixes the function_graph issue when using priority masking
* Patch 4 introduces some debug to hopefully avoid breaking things in
  the future
* Patch 5 is a rebased version of the patch sent by Wei Li[3] fixing
  an error that can happen during on some platform using the priority
  masking feature

Changes since V1 [1]:
- Fix possible race condition between NMI and trace irqflags
- Simplify the representation of PSR.I in the PMR value
- Include Wei Li's fix
- Rebase on v5.1-rc7

[1] https://marc.info/?l=linux-arm-kernel&m=155542458004480&w=2
[2] https://www.spinics.net/lists/arm-kernel/msg716956.html
[3] https://www.spinics.net/lists/arm-kernel/msg722171.html

Cheers,

Julien

-->

Julien Thierry (4):
  arm64: Do not enable IRQs for ct_user_exit
  arm64: Fix interrupt tracing in the presence of NMIs
  arm64: Fix incorrect irqflag restore for priority masking
  arm64: irqflags: Introduce explicit debugging for IRQ priorities

Wei Li (1):
  arm64: fix kernel stack overflow in kdump capture kernel

 arch/arm64/Kconfig                  | 11 +++++
 arch/arm64/include/asm/arch_gicv3.h |  4 +-
 arch/arm64/include/asm/assembler.h  |  9 ++++
 arch/arm64/include/asm/daifflags.h  | 31 +++++++++++---
 arch/arm64/include/asm/irqflags.h   | 83 +++++++++++++++++++------------------
 arch/arm64/include/asm/kvm_host.h   |  4 +-
 arch/arm64/include/asm/ptrace.h     | 10 ++++-
 arch/arm64/kernel/entry.S           | 76 +++++++++++++++++++++++++--------
 arch/arm64/kernel/irq.c             | 26 ++++++++++++
 arch/arm64/kernel/process.c         |  2 +-
 arch/arm64/kernel/smp.c             |  6 +--
 arch/arm64/kvm/hyp/switch.c         |  2 +-
 drivers/irqchip/irq-gic-v3.c        |  6 +++
 kernel/irq/irqdesc.c                |  8 +++-
 14 files changed, 202 insertions(+), 76 deletions(-)

--
1.9.1

[PATCH v2 0/5] arm64: IRQ priority masking and Pseudo-NMI fixes

[Julien Thierry]

Hi,

[Changing the title to make it reflex more the status of the series.]

Version one[1] of this series attempted to fix the issue reported by
Zenghui[2] when using the function_graph tracer with IRQ priority
masking.

Since then, I realized that priority masking and the use of Pseudo-NMIs
was more broken than I thought.

* Patch 1 is just some rework
* Patch 2 replaces the previous irqflags tracing "simplification" with
  a proper fix when tracing Pseudo-NMI
* Patch 3 fixes the function_graph issue when using priority masking
* Patch 4 introduces some debug to hopefully avoid breaking things in
  the future
* Patch 5 is a rebased version of the patch sent by Wei Li[3] fixing
  an error that can happen during on some platform using the priority
  masking feature

Changes since V1 [1]:
- Fix possible race condition between NMI and trace irqflags
- Simplify the representation of PSR.I in the PMR value
- Include Wei Li's fix
- Rebase on v5.1-rc7

[1] https://marc.info/?l=linux-arm-kernel&m=155542458004480&w=2
[2] https://www.spinics.net/lists/arm-kernel/msg716956.html
[3] https://www.spinics.net/lists/arm-kernel/msg722171.html

Cheers,

Julien

-->

Julien Thierry (4):
  arm64: Do not enable IRQs for ct_user_exit
  arm64: Fix interrupt tracing in the presence of NMIs
  arm64: Fix incorrect irqflag restore for priority masking
  arm64: irqflags: Introduce explicit debugging for IRQ priorities

Wei Li (1):
  arm64: fix kernel stack overflow in kdump capture kernel

 arch/arm64/Kconfig                  | 11 +++++
 arch/arm64/include/asm/arch_gicv3.h |  4 +-
 arch/arm64/include/asm/assembler.h  |  9 ++++
 arch/arm64/include/asm/daifflags.h  | 31 +++++++++++---
 arch/arm64/include/asm/irqflags.h   | 83 +++++++++++++++++++------------------
 arch/arm64/include/asm/kvm_host.h   |  4 +-
 arch/arm64/include/asm/ptrace.h     | 10 ++++-
 arch/arm64/kernel/entry.S           | 76 +++++++++++++++++++++++++--------
 arch/arm64/kernel/irq.c             | 26 ++++++++++++
 arch/arm64/kernel/process.c         |  2 +-
 arch/arm64/kernel/smp.c             |  6 +--
 arch/arm64/kvm/hyp/switch.c         |  2 +-
 drivers/irqchip/irq-gic-v3.c        |  6 +++
 kernel/irq/irqdesc.c                |  8 +++-
 14 files changed, 202 insertions(+), 76 deletions(-)

--
1.9.1

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

[PATCH v2 1/5] arm64: Do not enable IRQs for ct_user_exit

[Julien Thierry]

For el0_dbg and el0_error, DAIF bits get explicitly cleared before
calling ct_user_exit.

When context tracking is disabled, DAIF gets set (almost) immediately
after. When context tracking is enabled, among the first things done
is disabling IRQs.

What is actually needed is:
- PSR.D = 0 so the system can be debugged (should be already the case)
- PSR.A = 0 so async error can be handled during context tracking

Do not clear PSR.I in those two locations.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Cc:Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kernel/entry.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index c50a7a7..6a38903 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -855,7 +855,7 @@ el0_dbg:
 	mov	x1, x25
 	mov	x2, sp
 	bl	do_debug_exception
-	enable_daif
+	enable_da_f
 	ct_user_exit
 	b	ret_to_user
 el0_inv:
@@ -907,7 +907,7 @@ el0_error_naked:
 	enable_dbg
 	mov	x0, sp
 	bl	do_serror
-	enable_daif
+	enable_da_f
 	ct_user_exit
 	b	ret_to_user
 ENDPROC(el0_error)
--
1.9.1

[PATCH v2 1/5] arm64: Do not enable IRQs for ct_user_exit

[Julien Thierry]

For el0_dbg and el0_error, DAIF bits get explicitly cleared before
calling ct_user_exit.

When context tracking is disabled, DAIF gets set (almost) immediately
after. When context tracking is enabled, among the first things done
is disabling IRQs.

What is actually needed is:
- PSR.D = 0 so the system can be debugged (should be already the case)
- PSR.A = 0 so async error can be handled during context tracking

Do not clear PSR.I in those two locations.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Cc:Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kernel/entry.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index c50a7a7..6a38903 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -855,7 +855,7 @@ el0_dbg:
 	mov	x1, x25
 	mov	x2, sp
 	bl	do_debug_exception
-	enable_daif
+	enable_da_f
 	ct_user_exit
 	b	ret_to_user
 el0_inv:
@@ -907,7 +907,7 @@ el0_error_naked:
 	enable_dbg
 	mov	x0, sp
 	bl	do_serror
-	enable_daif
+	enable_da_f
 	ct_user_exit
 	b	ret_to_user
 ENDPROC(el0_error)
--
1.9.1

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

[PATCH v2 2/5] arm64: Fix interrupt tracing in the presence of NMIs

[Julien Thierry]

In the presence of any form of instrumentation, nmi_enter() should be
done before calling any traceable code and any instrumentation code.

Currently, nmi_enter() is done in handle_domain_nmi(), which is much
too late as instrumentation code might get called before. Move the
nmi_enter/exit() calls to the arch IRQ vector handler.

On arm64, it is not possible to know if the IRQ vector handler was
called because of an NMI before acknowledging the interrupt. However, It
is possible to know whether normal interrupts could be taken in the
interrupted context (i.e. if taking an NMI in that context could
introduce a potential race condition).

When interrupting a context with IRQs disabled, call nmi_enter() as soon
as possible. In contexts with IRQs enabled, defer this to the interrupt
controller, which is in a better position to know if an interrupt taken
is an NMI.

Fixes: bc3c03ccb ("arm64: Enable the support of pseudo-NMIs")
Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/entry.S    | 44 +++++++++++++++++++++++++++++++++-----------
 arch/arm64/kernel/irq.c      | 17 +++++++++++++++++
 drivers/irqchip/irq-gic-v3.c |  6 ++++++
 kernel/irq/irqdesc.c         |  8 ++++++--
 4 files changed, 62 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 6a38903..00c1f21 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -420,6 +420,20 @@ tsk	.req	x28		// current thread_info
 	irq_stack_exit
 	.endm

+#ifdef CONFIG_ARM64_PSEUDO_NMI
+	/*
+	 * Set res to 0 if irqs were masked in interrupted context.
+	 * Otherwise set res to non-0 value.
+	 */
+	.macro test_irqs_unmasked res:req, pmr:req
+alternative_if ARM64_HAS_IRQ_PRIO_MASKING
+	sub	\res, \pmr, #GIC_PRIO_IRQON
+alternative_else
+	mov	\res, xzr
+alternative_endif
+	.endm
+#endif
+
 	.text

 /*
@@ -616,19 +630,19 @@ ENDPROC(el1_sync)
 el1_irq:
 	kernel_entry 1
 	enable_da_f
-#ifdef CONFIG_TRACE_IRQFLAGS
+
 #ifdef CONFIG_ARM64_PSEUDO_NMI
 alternative_if ARM64_HAS_IRQ_PRIO_MASKING
 	ldr	x20, [sp, #S_PMR_SAVE]
-alternative_else
-	mov	x20, #GIC_PRIO_IRQON
-alternative_endif
-	cmp	x20, #GIC_PRIO_IRQOFF
-	/* Irqs were disabled, don't trace */
-	b.ls	1f
+alternative_else_nop_endif
+	test_irqs_unmasked	res=x0, pmr=x20
+	cbz	x0, 1f
+	bl	asm_nmi_enter
+1:
 #endif
+
+#ifdef CONFIG_TRACE_IRQFLAGS
 	bl	trace_hardirqs_off
-1:
 #endif

 	irq_handler
@@ -647,14 +661,22 @@ alternative_else_nop_endif
 	bl	preempt_schedule_irq		// irq en/disable is done inside
 1:
 #endif
-#ifdef CONFIG_TRACE_IRQFLAGS
+
 #ifdef CONFIG_ARM64_PSEUDO_NMI
 	/*
 	 * if IRQs were disabled when we received the interrupt, we have an NMI
 	 * and we are not re-enabling interrupt upon eret. Skip tracing.
 	 */
-	cmp	x20, #GIC_PRIO_IRQOFF
-	b.ls	1f
+	test_irqs_unmasked	res=x0, pmr=x20
+	cbz	x0, 1f
+	bl	asm_nmi_exit
+1:
+#endif
+
+#ifdef CONFIG_TRACE_IRQFLAGS
+#ifdef CONFIG_ARM64_PSEUDO_NMI
+	test_irqs_unmasked	res=x0, pmr=x20
+	cbnz	x0, 1f
 #endif
 	bl	trace_hardirqs_on
 1:
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 92fa817..fdd9cb2 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -27,8 +27,10 @@
 #include <linux/smp.h>
 #include <linux/init.h>
 #include <linux/irqchip.h>
+#include <linux/kprobes.h>
 #include <linux/seq_file.h>
 #include <linux/vmalloc.h>
+#include <asm/daifflags.h>
 #include <asm/vmap_stack.h>

 unsigned long irq_err_count;
@@ -76,3 +78,18 @@ void __init init_IRQ(void)
 	if (!handle_arch_irq)
 		panic("No interrupt controller found.");
 }
+
+/*
+ * Stubs to make nmi_enter/exit() code callable from ASM
+ */
+asmlinkage void notrace asm_nmi_enter(void)
+{
+	nmi_enter();
+}
+NOKPROBE_SYMBOL(asm_nmi_enter);
+
+asmlinkage void notrace asm_nmi_exit(void)
+{
+	nmi_exit();
+}
+NOKPROBE_SYMBOL(asm_nmi_exit);
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 15e55d3..4847673 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -495,7 +495,13 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs

 	if (gic_supports_nmi() &&
 	    unlikely(gic_read_rpr() == GICD_INT_NMI_PRI)) {
+		if (interrupts_enabled(regs))
+			nmi_enter();
+
 		gic_handle_nmi(irqnr, regs);
+
+		if (interrupts_enabled(regs))
+			nmi_exit();
 		return;
 	}

diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 9f8a709..b3fdf48 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -679,6 +679,8 @@ int __handle_domain_irq(struct irq_domain *domain, unsigned int hwirq,
  * @hwirq:	The HW irq number to convert to a logical one
  * @regs:	Register file coming from the low-level handling code
  *
+ *		This function must be called from an NMI context.
+ *
  * Returns:	0 on success, or -EINVAL if conversion has failed
  */
 int handle_domain_nmi(struct irq_domain *domain, unsigned int hwirq,
@@ -688,7 +690,10 @@ int handle_domain_nmi(struct irq_domain *domain, unsigned int hwirq,
 	unsigned int irq;
 	int ret = 0;

-	nmi_enter();
+	/*
+	 * NMI context needs to be setup earlier in order to deal with tracing.
+	 */
+	WARN_ON(!in_nmi());

 	irq = irq_find_mapping(domain, hwirq);

@@ -701,7 +706,6 @@ int handle_domain_nmi(struct irq_domain *domain, unsigned int hwirq,
 	else
 		ret = -EINVAL;

-	nmi_exit();
 	set_irq_regs(old_regs);
 	return ret;
 }
--
1.9.1

[PATCH v2 2/5] arm64: Fix interrupt tracing in the presence of NMIs

[Julien Thierry]

In the presence of any form of instrumentation, nmi_enter() should be
done before calling any traceable code and any instrumentation code.

Currently, nmi_enter() is done in handle_domain_nmi(), which is much
too late as instrumentation code might get called before. Move the
nmi_enter/exit() calls to the arch IRQ vector handler.

On arm64, it is not possible to know if the IRQ vector handler was
called because of an NMI before acknowledging the interrupt. However, It
is possible to know whether normal interrupts could be taken in the
interrupted context (i.e. if taking an NMI in that context could
introduce a potential race condition).

When interrupting a context with IRQs disabled, call nmi_enter() as soon
as possible. In contexts with IRQs enabled, defer this to the interrupt
controller, which is in a better position to know if an interrupt taken
is an NMI.

Fixes: bc3c03ccb ("arm64: Enable the support of pseudo-NMIs")
Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/entry.S    | 44 +++++++++++++++++++++++++++++++++-----------
 arch/arm64/kernel/irq.c      | 17 +++++++++++++++++
 drivers/irqchip/irq-gic-v3.c |  6 ++++++
 kernel/irq/irqdesc.c         |  8 ++++++--
 4 files changed, 62 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 6a38903..00c1f21 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -420,6 +420,20 @@ tsk	.req	x28		// current thread_info
 	irq_stack_exit
 	.endm

+#ifdef CONFIG_ARM64_PSEUDO_NMI
+	/*
+	 * Set res to 0 if irqs were masked in interrupted context.
+	 * Otherwise set res to non-0 value.
+	 */
+	.macro test_irqs_unmasked res:req, pmr:req
+alternative_if ARM64_HAS_IRQ_PRIO_MASKING
+	sub	\res, \pmr, #GIC_PRIO_IRQON
+alternative_else
+	mov	\res, xzr
+alternative_endif
+	.endm
+#endif
+
 	.text

 /*
@@ -616,19 +630,19 @@ ENDPROC(el1_sync)
 el1_irq:
 	kernel_entry 1
 	enable_da_f
-#ifdef CONFIG_TRACE_IRQFLAGS
+
 #ifdef CONFIG_ARM64_PSEUDO_NMI
 alternative_if ARM64_HAS_IRQ_PRIO_MASKING
 	ldr	x20, [sp, #S_PMR_SAVE]
-alternative_else
-	mov	x20, #GIC_PRIO_IRQON
-alternative_endif
-	cmp	x20, #GIC_PRIO_IRQOFF
-	/* Irqs were disabled, don't trace */
-	b.ls	1f
+alternative_else_nop_endif
+	test_irqs_unmasked	res=x0, pmr=x20
+	cbz	x0, 1f
+	bl	asm_nmi_enter
+1:
 #endif
+
+#ifdef CONFIG_TRACE_IRQFLAGS
 	bl	trace_hardirqs_off
-1:
 #endif

 	irq_handler
@@ -647,14 +661,22 @@ alternative_else_nop_endif
 	bl	preempt_schedule_irq		// irq en/disable is done inside
 1:
 #endif
-#ifdef CONFIG_TRACE_IRQFLAGS
+
 #ifdef CONFIG_ARM64_PSEUDO_NMI
 	/*
 	 * if IRQs were disabled when we received the interrupt, we have an NMI
 	 * and we are not re-enabling interrupt upon eret. Skip tracing.
 	 */
-	cmp	x20, #GIC_PRIO_IRQOFF
-	b.ls	1f
+	test_irqs_unmasked	res=x0, pmr=x20
+	cbz	x0, 1f
+	bl	asm_nmi_exit
+1:
+#endif
+
+#ifdef CONFIG_TRACE_IRQFLAGS
+#ifdef CONFIG_ARM64_PSEUDO_NMI
+	test_irqs_unmasked	res=x0, pmr=x20
+	cbnz	x0, 1f
 #endif
 	bl	trace_hardirqs_on
 1:
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 92fa817..fdd9cb2 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -27,8 +27,10 @@
 #include <linux/smp.h>
 #include <linux/init.h>
 #include <linux/irqchip.h>
+#include <linux/kprobes.h>
 #include <linux/seq_file.h>
 #include <linux/vmalloc.h>
+#include <asm/daifflags.h>
 #include <asm/vmap_stack.h>

 unsigned long irq_err_count;
@@ -76,3 +78,18 @@ void __init init_IRQ(void)
 	if (!handle_arch_irq)
 		panic("No interrupt controller found.");
 }
+
+/*
+ * Stubs to make nmi_enter/exit() code callable from ASM
+ */
+asmlinkage void notrace asm_nmi_enter(void)
+{
+	nmi_enter();
+}
+NOKPROBE_SYMBOL(asm_nmi_enter);
+
+asmlinkage void notrace asm_nmi_exit(void)
+{
+	nmi_exit();
+}
+NOKPROBE_SYMBOL(asm_nmi_exit);
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 15e55d3..4847673 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -495,7 +495,13 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs

 	if (gic_supports_nmi() &&
 	    unlikely(gic_read_rpr() == GICD_INT_NMI_PRI)) {
+		if (interrupts_enabled(regs))
+			nmi_enter();
+
 		gic_handle_nmi(irqnr, regs);
+
+		if (interrupts_enabled(regs))
+			nmi_exit();
 		return;
 	}

diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 9f8a709..b3fdf48 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -679,6 +679,8 @@ int __handle_domain_irq(struct irq_domain *domain, unsigned int hwirq,
  * @hwirq:	The HW irq number to convert to a logical one
  * @regs:	Register file coming from the low-level handling code
  *
+ *		This function must be called from an NMI context.
+ *
  * Returns:	0 on success, or -EINVAL if conversion has failed
  */
 int handle_domain_nmi(struct irq_domain *domain, unsigned int hwirq,
@@ -688,7 +690,10 @@ int handle_domain_nmi(struct irq_domain *domain, unsigned int hwirq,
 	unsigned int irq;
 	int ret = 0;

-	nmi_enter();
+	/*
+	 * NMI context needs to be setup earlier in order to deal with tracing.
+	 */
+	WARN_ON(!in_nmi());

 	irq = irq_find_mapping(domain, hwirq);

@@ -701,7 +706,6 @@ int handle_domain_nmi(struct irq_domain *domain, unsigned int hwirq,
 	else
 		ret = -EINVAL;

-	nmi_exit();
 	set_irq_regs(old_regs);
 	return ret;
 }
--
1.9.1

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

[PATCH v2 3/5] arm64: Fix incorrect irqflag restore for priority masking

[Julien Thierry]

When using IRQ priority masking to disable interrupts, in order to deal
with the PSR.I state, local_irq_save() would convert the I bit into a
PMR value (GIC_PRIO_IRQOFF). This resulted in local_irq_restore()
potentially modifying the value of PMR in undesired location due to the
state of PSR.I upon flag saving [1].

In an attempt to solve this issue in a less hackish manner, introduce
a bit (GIC_PRIO_IGNORE_PMR) for the PMR values that can represent
whether PSR.I is being used to disable interrupts, in which case it
takes precedence of the status of interrupt masking via PMR.

GIC_PRIO_IGNORE_PMR is chosen such that (<pmr_value> |
GIC_PRIO_IGNORE_PMR) does not mask more interrupts than <pmr_value> as
some sections (e.g. arch_cpu_idle(), interrupt acknowledge path)
requires PMR not to mask interrupts that could be signaled to the
CPU when using only PSR.I.

[1] https://www.spinics.net/lists/arm-kernel/msg716956.html

Fixes: commit 4a503217ce37 ("arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking")
Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Reported-by: Zenghui Yu <yuzenghui@huawei.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Wei Li <liwei391@huawei.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Christoffer Dall <christoffer.dall@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Suzuki K Pouloze <suzuki.poulose@arm.com>
Cc: Oleg Nesterov <oleg@redhat.com>
---
 arch/arm64/include/asm/arch_gicv3.h |  4 ++-
 arch/arm64/include/asm/assembler.h  |  9 +++++
 arch/arm64/include/asm/daifflags.h  | 22 ++++++++----
 arch/arm64/include/asm/irqflags.h   | 69 ++++++++++++++++---------------------
 arch/arm64/include/asm/kvm_host.h   |  4 ++-
 arch/arm64/include/asm/ptrace.h     | 10 ++++--
 arch/arm64/kernel/entry.S           | 30 +++++++++++++---
 arch/arm64/kernel/process.c         |  2 +-
 arch/arm64/kernel/smp.c             |  8 +++--
 arch/arm64/kvm/hyp/switch.c         |  2 +-
 10 files changed, 100 insertions(+), 60 deletions(-)

diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
index 14b41dd..3102c9a 100644
--- a/arch/arm64/include/asm/arch_gicv3.h
+++ b/arch/arm64/include/asm/arch_gicv3.h
@@ -163,7 +163,9 @@ static inline bool gic_prio_masking_enabled(void)

 static inline void gic_pmr_mask_irqs(void)
 {
-	BUILD_BUG_ON(GICD_INT_DEF_PRI <= GIC_PRIO_IRQOFF);
+	BUILD_BUG_ON(GICD_INT_DEF_PRI < (GIC_PRIO_IRQOFF |
+					 GIC_PRIO_IGNORE_PMR));
+	BUILD_BUG_ON(GICD_INT_DEF_PRI >= GIC_PRIO_IRQON);
 	gic_write_pmr(GIC_PRIO_IRQOFF);
 }

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index c5308d0..601154d 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -62,6 +62,15 @@
 	msr	daifclr, #(8 | 4 | 1)
 	.endm

+	.macro	suspend_irq_prio_masking, tmp:req
+#ifdef CONFIG_ARM64_PSEUDO_NMI
+	alternative_if ARM64_HAS_IRQ_PRIO_MASKING
+	mov	\tmp, #(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR)
+	msr_s	SYS_ICC_PMR_EL1, \tmp
+	alternative_else_nop_endif
+#endif
+	.endm
+
 /*
  * Save/restore interrupts.
  */
diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
index db452aa..a32ece9 100644
--- a/arch/arm64/include/asm/daifflags.h
+++ b/arch/arm64/include/asm/daifflags.h
@@ -18,6 +18,7 @@

 #include <linux/irqflags.h>

+#include <asm/arch_gicv3.h>
 #include <asm/cpufeature.h>

 #define DAIF_PROCCTX		0
@@ -32,6 +33,11 @@ static inline void local_daif_mask(void)
 		:
 		:
 		: "memory");
+
+	/* Don't really care for a dsb here, we don't intend to enable IRQs */
+	if (system_uses_irq_prio_masking())
+		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
+
 	trace_hardirqs_off();
 }

@@ -43,7 +49,7 @@ static inline unsigned long local_daif_save(void)

 	if (system_uses_irq_prio_masking()) {
 		/* If IRQs are masked with PMR, reflect it in the flags */
-		if (read_sysreg_s(SYS_ICC_PMR_EL1) <= GIC_PRIO_IRQOFF)
+		if (read_sysreg_s(SYS_ICC_PMR_EL1) != GIC_PRIO_IRQON)
 			flags |= PSR_I_BIT;
 	}

@@ -59,14 +65,16 @@ static inline void local_daif_restore(unsigned long flags)
 	if (!irq_disabled) {
 		trace_hardirqs_on();

-		if (system_uses_irq_prio_masking())
-			arch_local_irq_enable();
-	} else if (!(flags & PSR_A_BIT)) {
+		if (system_uses_irq_prio_masking()) {
+			gic_write_pmr(GIC_PRIO_IRQON);
+			dsb(sy);
+		}
+	} else if (system_uses_irq_prio_masking()) {
 		/*
 		 * If interrupts are disabled but we can take
 		 * asynchronous errors, we can take NMIs
 		 */
-		if (system_uses_irq_prio_masking()) {
+		if (!(flags & PSR_A_BIT)) {
 			flags &= ~PSR_I_BIT;
 			/*
 			 * There has been concern that the write to daif
@@ -87,7 +95,9 @@ static inline void local_daif_restore(unsigned long flags)
 			 *
 			 * So we don't need additional synchronization here.
 			 */
-			arch_local_irq_disable();
+			gic_write_pmr(GIC_PRIO_IRQOFF);
+		} else {
+			gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
 		}
 	}

diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
index 43d8366..516cdfc 100644
--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h
@@ -67,43 +67,48 @@ static inline void arch_local_irq_disable(void)
  */
 static inline unsigned long arch_local_save_flags(void)
 {
-	unsigned long daif_bits;
 	unsigned long flags;

-	daif_bits = read_sysreg(daif);
-
-	/*
-	 * The asm is logically equivalent to:
-	 *
-	 * if (system_uses_irq_prio_masking())
-	 *	flags = (daif_bits & PSR_I_BIT) ?
-	 *			GIC_PRIO_IRQOFF :
-	 *			read_sysreg_s(SYS_ICC_PMR_EL1);
-	 * else
-	 *	flags = daif_bits;
-	 */
 	asm volatile(ALTERNATIVE(
-			"mov	%0, %1\n"
-			"nop\n"
-			"nop",
-			"mrs_s	%0, " __stringify(SYS_ICC_PMR_EL1) "\n"
-			"ands	%1, %1, " __stringify(PSR_I_BIT) "\n"
-			"csel	%0, %0, %2, eq",
-			ARM64_HAS_IRQ_PRIO_MASKING)
-		: "=&r" (flags), "+r" (daif_bits)
-		: "r" ((unsigned long) GIC_PRIO_IRQOFF)
+		"mrs	%0, daif",
+		"mrs_s	%0, " __stringify(SYS_ICC_PMR_EL1) "\n",
+		ARM64_HAS_IRQ_PRIO_MASKING)
+		: "=&r" (flags)
+		:
 		: "memory");

 	return flags;
 }

+static inline int arch_irqs_disabled_flags(unsigned long flags)
+{
+	int res;
+
+	asm volatile(ALTERNATIVE(
+		"and	%w0, %w1, #" __stringify(PSR_I_BIT) "\n"
+		"nop",
+		"cmp	%w1, #" __stringify(GIC_PRIO_IRQON) "\n"
+		"cset	%w0, ne",
+		ARM64_HAS_IRQ_PRIO_MASKING)
+		: "=&r" (res)
+		: "r" ((int) flags)
+		: "memory");
+
+	return res;
+}
+
 static inline unsigned long arch_local_irq_save(void)
 {
 	unsigned long flags;

 	flags = arch_local_save_flags();

-	arch_local_irq_disable();
+	/*
+	 * There are too many states with IRQs disabled, just keep the current
+	 * state if interrupts are already disabled/masked.
+	 */
+	if (!arch_irqs_disabled_flags(flags))
+		arch_local_irq_disable();

 	return flags;
 }
@@ -119,26 +124,10 @@ static inline void arch_local_irq_restore(unsigned long flags)
 			"msr_s	" __stringify(SYS_ICC_PMR_EL1) ", %0\n"
 			"dsb	sy",
 			ARM64_HAS_IRQ_PRIO_MASKING)
-		: "+r" (flags)
 		:
+		: "r" (flags)
 		: "memory");
 }

-static inline int arch_irqs_disabled_flags(unsigned long flags)
-{
-	int res;
-
-	asm volatile(ALTERNATIVE(
-			"and	%w0, %w1, #" __stringify(PSR_I_BIT) "\n"
-			"nop",
-			"cmp	%w1, #" __stringify(GIC_PRIO_IRQOFF) "\n"
-			"cset	%w0, ls",
-			ARM64_HAS_IRQ_PRIO_MASKING)
-		: "=&r" (res)
-		: "r" ((int) flags)
-		: "memory");
-
-	return res;
-}
 #endif
 #endif
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index a01fe087..0ec398c 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -532,9 +532,11 @@ static inline void kvm_arm_vhe_guest_enter(void)
 	 * will not signal the CPU of interrupts of lower priority, and the
 	 * only way to get out will be via guest exceptions.
 	 * Naturally, we want to avoid this.
+	 *
+	 * local_daif_mask() already sets IGNORE_PMR, we just need a
+	 * dsb to ensure the redistributor is forwards EL2 IRQs to the CPU.
 	 */
 	if (system_uses_irq_prio_masking()) {
-		gic_write_pmr(GIC_PRIO_IRQON);
 		dsb(sy);
 	}
 }
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index ec60174..7861a5d 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -35,9 +35,15 @@
  * means masking more IRQs (or at least that the same IRQs remain masked).
  *
  * To mask interrupts, we clear the most significant bit of PMR.
+ *
+ * Some code sections either automatically switch back to PSR.I or explicitly
+ * require to not use priority masking. If bit GIC_PRIO_IGNORE_PMR is included
+ * in the  the priority mask, it indicates that PSR.I should be set and
+ * interrupt disabling temporarily does not rely on IRQ priorities.
  */
-#define GIC_PRIO_IRQON		0xf0
-#define GIC_PRIO_IRQOFF		(GIC_PRIO_IRQON & ~0x80)
+#define GIC_PRIO_IRQON			0xc0
+#define GIC_PRIO_IRQOFF			(GIC_PRIO_IRQON & ~0x80)
+#define GIC_PRIO_IGNORE_PMR		(1 << 4)

 /* Additional SPSR bits not exposed in the UABI */
 #define PSR_IL_BIT		(1 << 20)
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 00c1f21..e6df781 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -258,6 +258,7 @@ alternative_else_nop_endif
 	/*
 	 * Registers that may be useful after this macro is invoked:
 	 *
+	 * x20 - ICC_PMR_EL1
 	 * x21 - aborted SP
 	 * x22 - aborted PC
 	 * x23 - aborted PSTATE
@@ -434,6 +435,16 @@ alternative_endif
 	.endm
 #endif

+
+	.macro irq_entry_ignore_pmr, pmr:req, tmp:req
+#ifdef CONFIG_ARM64_PSEUDO_NMI
+	alternative_if ARM64_HAS_IRQ_PRIO_MASKING
+	orr	\tmp, \pmr, #GIC_PRIO_IGNORE_PMR
+	msr_s	SYS_ICC_PMR_EL1, \tmp
+	alternative_else_nop_endif
+#endif
+	.endm
+
 	.text

 /*
@@ -612,6 +623,7 @@ el1_dbg:
 	cmp	x24, #ESR_ELx_EC_BRK64		// if BRK64
 	cinc	x24, x24, eq			// set bit '0'
 	tbz	x24, #0, el1_inv		// EL1 only
+	suspend_irq_prio_masking tmp=x21
 	mrs	x0, far_el1
 	mov	x2, sp				// struct pt_regs
 	bl	do_debug_exception
@@ -629,12 +641,10 @@ ENDPROC(el1_sync)
 	.align	6
 el1_irq:
 	kernel_entry 1
+	irq_entry_ignore_pmr pmr=x20, tmp=x0
 	enable_da_f

 #ifdef CONFIG_ARM64_PSEUDO_NMI
-alternative_if ARM64_HAS_IRQ_PRIO_MASKING
-	ldr	x20, [sp, #S_PMR_SAVE]
-alternative_else_nop_endif
 	test_irqs_unmasked	res=x0, pmr=x20
 	cbz	x0, 1f
 	bl	asm_nmi_enter
@@ -664,8 +674,9 @@ alternative_else_nop_endif

 #ifdef CONFIG_ARM64_PSEUDO_NMI
 	/*
-	 * if IRQs were disabled when we received the interrupt, we have an NMI
-	 * and we are not re-enabling interrupt upon eret. Skip tracing.
+	 * When using IRQ priority masking, we can get spurious interrupts while
+	 * PMR is set to GIC_PRIO_IRQOFF. An NMI might also have occurred in a
+	 * section with interrupts disabled. Skip tracing in those cases.
 	 */
 	test_irqs_unmasked	res=x0, pmr=x20
 	cbz	x0, 1f
@@ -794,6 +805,7 @@ el0_ia:
 	 * Instruction abort handling
 	 */
 	mrs	x26, far_el1
+	suspend_irq_prio_masking tmp=x0
 	enable_da_f
 #ifdef CONFIG_TRACE_IRQFLAGS
 	bl	trace_hardirqs_off
@@ -839,6 +851,7 @@ el0_sp_pc:
 	 * Stack or PC alignment exception handling
 	 */
 	mrs	x26, far_el1
+	suspend_irq_prio_masking tmp=x0
 	enable_da_f
 #ifdef CONFIG_TRACE_IRQFLAGS
 	bl	trace_hardirqs_off
@@ -873,6 +886,7 @@ el0_dbg:
 	 * Debug exception handling
 	 */
 	tbnz	x24, #0, el0_inv		// EL0 only
+	suspend_irq_prio_masking tmp=x0
 	mrs	x0, far_el1
 	mov	x1, x25
 	mov	x2, sp
@@ -894,7 +908,9 @@ ENDPROC(el0_sync)
 el0_irq:
 	kernel_entry 0
 el0_irq_naked:
+	irq_entry_ignore_pmr pmr=x20, tmp=x0
 	enable_da_f
+
 #ifdef CONFIG_TRACE_IRQFLAGS
 	bl	trace_hardirqs_off
 #endif
@@ -916,6 +932,7 @@ ENDPROC(el0_irq)
 el1_error:
 	kernel_entry 1
 	mrs	x1, esr_el1
+	suspend_irq_prio_masking tmp=x0
 	enable_dbg
 	mov	x0, sp
 	bl	do_serror
@@ -926,6 +943,7 @@ el0_error:
 	kernel_entry 0
 el0_error_naked:
 	mrs	x1, esr_el1
+	suspend_irq_prio_masking tmp=x0
 	enable_dbg
 	mov	x0, sp
 	bl	do_serror
@@ -950,6 +968,7 @@ work_pending:
  */
 ret_to_user:
 	disable_daif
+	suspend_irq_prio_masking tmp=x0
 	ldr	x1, [tsk, #TSK_TI_FLAGS]
 	and	x2, x1, #_TIF_WORK_MASK
 	cbnz	x2, work_pending
@@ -966,6 +985,7 @@ ENDPROC(ret_to_user)
  */
 	.align	6
 el0_svc:
+	suspend_irq_prio_masking tmp=x1
 	mov	x0, sp
 	bl	el0_svc_handler
 	b	ret_to_user
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 3767fb2..f5bae97 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -94,7 +94,7 @@ static void __cpu_do_idle_irqprio(void)
 	 * be raised.
 	 */
 	pmr = gic_read_pmr();
-	gic_write_pmr(GIC_PRIO_IRQON);
+	gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);

 	__cpu_do_idle();

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 824de70..2a6d0dd1 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -192,11 +192,13 @@ static void init_gic_priority_masking(void)

 	WARN_ON(!(cpuflags & PSR_I_BIT));

-	gic_write_pmr(GIC_PRIO_IRQOFF);
-
 	/* We can only unmask PSR.I if we can take aborts */
-	if (!(cpuflags & PSR_A_BIT))
+	if (!(cpuflags & PSR_A_BIT)) {
+		gic_write_pmr(GIC_PRIO_IRQOFF);
 		write_sysreg(cpuflags & ~PSR_I_BIT, daif);
+	} else {
+		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
+	}
 }

 /*
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 3563fe6..083e319 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -533,7 +533,7 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 	 * Naturally, we want to avoid this.
 	 */
 	if (system_uses_irq_prio_masking()) {
-		gic_write_pmr(GIC_PRIO_IRQON);
+		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
 		dsb(sy);
 	}

--
1.9.1

[PATCH v2 3/5] arm64: Fix incorrect irqflag restore for priority masking

[Julien Thierry]

When using IRQ priority masking to disable interrupts, in order to deal
with the PSR.I state, local_irq_save() would convert the I bit into a
PMR value (GIC_PRIO_IRQOFF). This resulted in local_irq_restore()
potentially modifying the value of PMR in undesired location due to the
state of PSR.I upon flag saving [1].

In an attempt to solve this issue in a less hackish manner, introduce
a bit (GIC_PRIO_IGNORE_PMR) for the PMR values that can represent
whether PSR.I is being used to disable interrupts, in which case it
takes precedence of the status of interrupt masking via PMR.

GIC_PRIO_IGNORE_PMR is chosen such that (<pmr_value> |
GIC_PRIO_IGNORE_PMR) does not mask more interrupts than <pmr_value> as
some sections (e.g. arch_cpu_idle(), interrupt acknowledge path)
requires PMR not to mask interrupts that could be signaled to the
CPU when using only PSR.I.

[1] https://www.spinics.net/lists/arm-kernel/msg716956.html

Fixes: commit 4a503217ce37 ("arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking")
Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Reported-by: Zenghui Yu <yuzenghui@huawei.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Wei Li <liwei391@huawei.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Christoffer Dall <christoffer.dall@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Suzuki K Pouloze <suzuki.poulose@arm.com>
Cc: Oleg Nesterov <oleg@redhat.com>
---
 arch/arm64/include/asm/arch_gicv3.h |  4 ++-
 arch/arm64/include/asm/assembler.h  |  9 +++++
 arch/arm64/include/asm/daifflags.h  | 22 ++++++++----
 arch/arm64/include/asm/irqflags.h   | 69 ++++++++++++++++---------------------
 arch/arm64/include/asm/kvm_host.h   |  4 ++-
 arch/arm64/include/asm/ptrace.h     | 10 ++++--
 arch/arm64/kernel/entry.S           | 30 +++++++++++++---
 arch/arm64/kernel/process.c         |  2 +-
 arch/arm64/kernel/smp.c             |  8 +++--
 arch/arm64/kvm/hyp/switch.c         |  2 +-
 10 files changed, 100 insertions(+), 60 deletions(-)

diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
index 14b41dd..3102c9a 100644
--- a/arch/arm64/include/asm/arch_gicv3.h
+++ b/arch/arm64/include/asm/arch_gicv3.h
@@ -163,7 +163,9 @@ static inline bool gic_prio_masking_enabled(void)

 static inline void gic_pmr_mask_irqs(void)
 {
-	BUILD_BUG_ON(GICD_INT_DEF_PRI <= GIC_PRIO_IRQOFF);
+	BUILD_BUG_ON(GICD_INT_DEF_PRI < (GIC_PRIO_IRQOFF |
+					 GIC_PRIO_IGNORE_PMR));
+	BUILD_BUG_ON(GICD_INT_DEF_PRI >= GIC_PRIO_IRQON);
 	gic_write_pmr(GIC_PRIO_IRQOFF);
 }

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index c5308d0..601154d 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -62,6 +62,15 @@
 	msr	daifclr, #(8 | 4 | 1)
 	.endm

+	.macro	suspend_irq_prio_masking, tmp:req
+#ifdef CONFIG_ARM64_PSEUDO_NMI
+	alternative_if ARM64_HAS_IRQ_PRIO_MASKING
+	mov	\tmp, #(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR)
+	msr_s	SYS_ICC_PMR_EL1, \tmp
+	alternative_else_nop_endif
+#endif
+	.endm
+
 /*
  * Save/restore interrupts.
  */
diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
index db452aa..a32ece9 100644
--- a/arch/arm64/include/asm/daifflags.h
+++ b/arch/arm64/include/asm/daifflags.h
@@ -18,6 +18,7 @@

 #include <linux/irqflags.h>

+#include <asm/arch_gicv3.h>
 #include <asm/cpufeature.h>

 #define DAIF_PROCCTX		0
@@ -32,6 +33,11 @@ static inline void local_daif_mask(void)
 		:
 		:
 		: "memory");
+
+	/* Don't really care for a dsb here, we don't intend to enable IRQs */
+	if (system_uses_irq_prio_masking())
+		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
+
 	trace_hardirqs_off();
 }

@@ -43,7 +49,7 @@ static inline unsigned long local_daif_save(void)

 	if (system_uses_irq_prio_masking()) {
 		/* If IRQs are masked with PMR, reflect it in the flags */
-		if (read_sysreg_s(SYS_ICC_PMR_EL1) <= GIC_PRIO_IRQOFF)
+		if (read_sysreg_s(SYS_ICC_PMR_EL1) != GIC_PRIO_IRQON)
 			flags |= PSR_I_BIT;
 	}

@@ -59,14 +65,16 @@ static inline void local_daif_restore(unsigned long flags)
 	if (!irq_disabled) {
 		trace_hardirqs_on();

-		if (system_uses_irq_prio_masking())
-			arch_local_irq_enable();
-	} else if (!(flags & PSR_A_BIT)) {
+		if (system_uses_irq_prio_masking()) {
+			gic_write_pmr(GIC_PRIO_IRQON);
+			dsb(sy);
+		}
+	} else if (system_uses_irq_prio_masking()) {
 		/*
 		 * If interrupts are disabled but we can take
 		 * asynchronous errors, we can take NMIs
 		 */
-		if (system_uses_irq_prio_masking()) {
+		if (!(flags & PSR_A_BIT)) {
 			flags &= ~PSR_I_BIT;
 			/*
 			 * There has been concern that the write to daif
@@ -87,7 +95,9 @@ static inline void local_daif_restore(unsigned long flags)
 			 *
 			 * So we don't need additional synchronization here.
 			 */
-			arch_local_irq_disable();
+			gic_write_pmr(GIC_PRIO_IRQOFF);
+		} else {
+			gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
 		}
 	}

diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
index 43d8366..516cdfc 100644
--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h
@@ -67,43 +67,48 @@ static inline void arch_local_irq_disable(void)
  */
 static inline unsigned long arch_local_save_flags(void)
 {
-	unsigned long daif_bits;
 	unsigned long flags;

-	daif_bits = read_sysreg(daif);
-
-	/*
-	 * The asm is logically equivalent to:
-	 *
-	 * if (system_uses_irq_prio_masking())
-	 *	flags = (daif_bits & PSR_I_BIT) ?
-	 *			GIC_PRIO_IRQOFF :
-	 *			read_sysreg_s(SYS_ICC_PMR_EL1);
-	 * else
-	 *	flags = daif_bits;
-	 */
 	asm volatile(ALTERNATIVE(
-			"mov	%0, %1\n"
-			"nop\n"
-			"nop",
-			"mrs_s	%0, " __stringify(SYS_ICC_PMR_EL1) "\n"
-			"ands	%1, %1, " __stringify(PSR_I_BIT) "\n"
-			"csel	%0, %0, %2, eq",
-			ARM64_HAS_IRQ_PRIO_MASKING)
-		: "=&r" (flags), "+r" (daif_bits)
-		: "r" ((unsigned long) GIC_PRIO_IRQOFF)
+		"mrs	%0, daif",
+		"mrs_s	%0, " __stringify(SYS_ICC_PMR_EL1) "\n",
+		ARM64_HAS_IRQ_PRIO_MASKING)
+		: "=&r" (flags)
+		:
 		: "memory");

 	return flags;
 }

+static inline int arch_irqs_disabled_flags(unsigned long flags)
+{
+	int res;
+
+	asm volatile(ALTERNATIVE(
+		"and	%w0, %w1, #" __stringify(PSR_I_BIT) "\n"
+		"nop",
+		"cmp	%w1, #" __stringify(GIC_PRIO_IRQON) "\n"
+		"cset	%w0, ne",
+		ARM64_HAS_IRQ_PRIO_MASKING)
+		: "=&r" (res)
+		: "r" ((int) flags)
+		: "memory");
+
+	return res;
+}
+
 static inline unsigned long arch_local_irq_save(void)
 {
 	unsigned long flags;

 	flags = arch_local_save_flags();

-	arch_local_irq_disable();
+	/*
+	 * There are too many states with IRQs disabled, just keep the current
+	 * state if interrupts are already disabled/masked.
+	 */
+	if (!arch_irqs_disabled_flags(flags))
+		arch_local_irq_disable();

 	return flags;
 }
@@ -119,26 +124,10 @@ static inline void arch_local_irq_restore(unsigned long flags)
 			"msr_s	" __stringify(SYS_ICC_PMR_EL1) ", %0\n"
 			"dsb	sy",
 			ARM64_HAS_IRQ_PRIO_MASKING)
-		: "+r" (flags)
 		:
+		: "r" (flags)
 		: "memory");
 }

-static inline int arch_irqs_disabled_flags(unsigned long flags)
-{
-	int res;
-
-	asm volatile(ALTERNATIVE(
-			"and	%w0, %w1, #" __stringify(PSR_I_BIT) "\n"
-			"nop",
-			"cmp	%w1, #" __stringify(GIC_PRIO_IRQOFF) "\n"
-			"cset	%w0, ls",
-			ARM64_HAS_IRQ_PRIO_MASKING)
-		: "=&r" (res)
-		: "r" ((int) flags)
-		: "memory");
-
-	return res;
-}
 #endif
 #endif
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index a01fe087..0ec398c 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -532,9 +532,11 @@ static inline void kvm_arm_vhe_guest_enter(void)
 	 * will not signal the CPU of interrupts of lower priority, and the
 	 * only way to get out will be via guest exceptions.
 	 * Naturally, we want to avoid this.
+	 *
+	 * local_daif_mask() already sets IGNORE_PMR, we just need a
+	 * dsb to ensure the redistributor is forwards EL2 IRQs to the CPU.
 	 */
 	if (system_uses_irq_prio_masking()) {
-		gic_write_pmr(GIC_PRIO_IRQON);
 		dsb(sy);
 	}
 }
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index ec60174..7861a5d 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -35,9 +35,15 @@
  * means masking more IRQs (or at least that the same IRQs remain masked).
  *
  * To mask interrupts, we clear the most significant bit of PMR.
+ *
+ * Some code sections either automatically switch back to PSR.I or explicitly
+ * require to not use priority masking. If bit GIC_PRIO_IGNORE_PMR is included
+ * in the  the priority mask, it indicates that PSR.I should be set and
+ * interrupt disabling temporarily does not rely on IRQ priorities.
  */
-#define GIC_PRIO_IRQON		0xf0
-#define GIC_PRIO_IRQOFF		(GIC_PRIO_IRQON & ~0x80)
+#define GIC_PRIO_IRQON			0xc0
+#define GIC_PRIO_IRQOFF			(GIC_PRIO_IRQON & ~0x80)
+#define GIC_PRIO_IGNORE_PMR		(1 << 4)

 /* Additional SPSR bits not exposed in the UABI */
 #define PSR_IL_BIT		(1 << 20)
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 00c1f21..e6df781 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -258,6 +258,7 @@ alternative_else_nop_endif
 	/*
 	 * Registers that may be useful after this macro is invoked:
 	 *
+	 * x20 - ICC_PMR_EL1
 	 * x21 - aborted SP
 	 * x22 - aborted PC
 	 * x23 - aborted PSTATE
@@ -434,6 +435,16 @@ alternative_endif
 	.endm
 #endif

+
+	.macro irq_entry_ignore_pmr, pmr:req, tmp:req
+#ifdef CONFIG_ARM64_PSEUDO_NMI
+	alternative_if ARM64_HAS_IRQ_PRIO_MASKING
+	orr	\tmp, \pmr, #GIC_PRIO_IGNORE_PMR
+	msr_s	SYS_ICC_PMR_EL1, \tmp
+	alternative_else_nop_endif
+#endif
+	.endm
+
 	.text

 /*
@@ -612,6 +623,7 @@ el1_dbg:
 	cmp	x24, #ESR_ELx_EC_BRK64		// if BRK64
 	cinc	x24, x24, eq			// set bit '0'
 	tbz	x24, #0, el1_inv		// EL1 only
+	suspend_irq_prio_masking tmp=x21
 	mrs	x0, far_el1
 	mov	x2, sp				// struct pt_regs
 	bl	do_debug_exception
@@ -629,12 +641,10 @@ ENDPROC(el1_sync)
 	.align	6
 el1_irq:
 	kernel_entry 1
+	irq_entry_ignore_pmr pmr=x20, tmp=x0
 	enable_da_f

 #ifdef CONFIG_ARM64_PSEUDO_NMI
-alternative_if ARM64_HAS_IRQ_PRIO_MASKING
-	ldr	x20, [sp, #S_PMR_SAVE]
-alternative_else_nop_endif
 	test_irqs_unmasked	res=x0, pmr=x20
 	cbz	x0, 1f
 	bl	asm_nmi_enter
@@ -664,8 +674,9 @@ alternative_else_nop_endif

 #ifdef CONFIG_ARM64_PSEUDO_NMI
 	/*
-	 * if IRQs were disabled when we received the interrupt, we have an NMI
-	 * and we are not re-enabling interrupt upon eret. Skip tracing.
+	 * When using IRQ priority masking, we can get spurious interrupts while
+	 * PMR is set to GIC_PRIO_IRQOFF. An NMI might also have occurred in a
+	 * section with interrupts disabled. Skip tracing in those cases.
 	 */
 	test_irqs_unmasked	res=x0, pmr=x20
 	cbz	x0, 1f
@@ -794,6 +805,7 @@ el0_ia:
 	 * Instruction abort handling
 	 */
 	mrs	x26, far_el1
+	suspend_irq_prio_masking tmp=x0
 	enable_da_f
 #ifdef CONFIG_TRACE_IRQFLAGS
 	bl	trace_hardirqs_off
@@ -839,6 +851,7 @@ el0_sp_pc:
 	 * Stack or PC alignment exception handling
 	 */
 	mrs	x26, far_el1
+	suspend_irq_prio_masking tmp=x0
 	enable_da_f
 #ifdef CONFIG_TRACE_IRQFLAGS
 	bl	trace_hardirqs_off
@@ -873,6 +886,7 @@ el0_dbg:
 	 * Debug exception handling
 	 */
 	tbnz	x24, #0, el0_inv		// EL0 only
+	suspend_irq_prio_masking tmp=x0
 	mrs	x0, far_el1
 	mov	x1, x25
 	mov	x2, sp
@@ -894,7 +908,9 @@ ENDPROC(el0_sync)
 el0_irq:
 	kernel_entry 0
 el0_irq_naked:
+	irq_entry_ignore_pmr pmr=x20, tmp=x0
 	enable_da_f
+
 #ifdef CONFIG_TRACE_IRQFLAGS
 	bl	trace_hardirqs_off
 #endif
@@ -916,6 +932,7 @@ ENDPROC(el0_irq)
 el1_error:
 	kernel_entry 1
 	mrs	x1, esr_el1
+	suspend_irq_prio_masking tmp=x0
 	enable_dbg
 	mov	x0, sp
 	bl	do_serror
@@ -926,6 +943,7 @@ el0_error:
 	kernel_entry 0
 el0_error_naked:
 	mrs	x1, esr_el1
+	suspend_irq_prio_masking tmp=x0
 	enable_dbg
 	mov	x0, sp
 	bl	do_serror
@@ -950,6 +968,7 @@ work_pending:
  */
 ret_to_user:
 	disable_daif
+	suspend_irq_prio_masking tmp=x0
 	ldr	x1, [tsk, #TSK_TI_FLAGS]
 	and	x2, x1, #_TIF_WORK_MASK
 	cbnz	x2, work_pending
@@ -966,6 +985,7 @@ ENDPROC(ret_to_user)
  */
 	.align	6
 el0_svc:
+	suspend_irq_prio_masking tmp=x1
 	mov	x0, sp
 	bl	el0_svc_handler
 	b	ret_to_user
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 3767fb2..f5bae97 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -94,7 +94,7 @@ static void __cpu_do_idle_irqprio(void)
 	 * be raised.
 	 */
 	pmr = gic_read_pmr();
-	gic_write_pmr(GIC_PRIO_IRQON);
+	gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);

 	__cpu_do_idle();

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 824de70..2a6d0dd1 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -192,11 +192,13 @@ static void init_gic_priority_masking(void)

 	WARN_ON(!(cpuflags & PSR_I_BIT));

-	gic_write_pmr(GIC_PRIO_IRQOFF);
-
 	/* We can only unmask PSR.I if we can take aborts */
-	if (!(cpuflags & PSR_A_BIT))
+	if (!(cpuflags & PSR_A_BIT)) {
+		gic_write_pmr(GIC_PRIO_IRQOFF);
 		write_sysreg(cpuflags & ~PSR_I_BIT, daif);
+	} else {
+		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
+	}
 }

 /*
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 3563fe6..083e319 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -533,7 +533,7 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 	 * Naturally, we want to avoid this.
 	 */
 	if (system_uses_irq_prio_masking()) {
-		gic_write_pmr(GIC_PRIO_IRQON);
+		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
 		dsb(sy);
 	}

--
1.9.1

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

Re: [PATCH v2 3/5] arm64: Fix incorrect irqflag restore for priority masking

[Marc Zyngier]

On 29/04/2019 17:00, Julien Thierry wrote:
> When using IRQ priority masking to disable interrupts, in order to deal
> with the PSR.I state, local_irq_save() would convert the I bit into a
> PMR value (GIC_PRIO_IRQOFF). This resulted in local_irq_restore()
> potentially modifying the value of PMR in undesired location due to the
> state of PSR.I upon flag saving [1].
> 
> In an attempt to solve this issue in a less hackish manner, introduce
> a bit (GIC_PRIO_IGNORE_PMR) for the PMR values that can represent
> whether PSR.I is being used to disable interrupts, in which case it
> takes precedence of the status of interrupt masking via PMR.
> 
> GIC_PRIO_IGNORE_PMR is chosen such that (<pmr_value> |
> GIC_PRIO_IGNORE_PMR) does not mask more interrupts than <pmr_value> as
> some sections (e.g. arch_cpu_idle(), interrupt acknowledge path)
> requires PMR not to mask interrupts that could be signaled to the
> CPU when using only PSR.I.
> 
> [1] https://www.spinics.net/lists/arm-kernel/msg716956.html
> 
> Fixes: commit 4a503217ce37 ("arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking")
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> Reported-by: Zenghui Yu <yuzenghui@huawei.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Wei Li <liwei391@huawei.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Christoffer Dall <christoffer.dall@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Suzuki K Pouloze <suzuki.poulose@arm.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> ---
>  arch/arm64/include/asm/arch_gicv3.h |  4 ++-
>  arch/arm64/include/asm/assembler.h  |  9 +++++
>  arch/arm64/include/asm/daifflags.h  | 22 ++++++++----
>  arch/arm64/include/asm/irqflags.h   | 69 ++++++++++++++++---------------------
>  arch/arm64/include/asm/kvm_host.h   |  4 ++-
>  arch/arm64/include/asm/ptrace.h     | 10 ++++--
>  arch/arm64/kernel/entry.S           | 30 +++++++++++++---
>  arch/arm64/kernel/process.c         |  2 +-
>  arch/arm64/kernel/smp.c             |  8 +++--
>  arch/arm64/kvm/hyp/switch.c         |  2 +-
>  10 files changed, 100 insertions(+), 60 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
> index 14b41dd..3102c9a 100644
> --- a/arch/arm64/include/asm/arch_gicv3.h
> +++ b/arch/arm64/include/asm/arch_gicv3.h
> @@ -163,7 +163,9 @@ static inline bool gic_prio_masking_enabled(void)
> 
>  static inline void gic_pmr_mask_irqs(void)
>  {
> -	BUILD_BUG_ON(GICD_INT_DEF_PRI <= GIC_PRIO_IRQOFF);
> +	BUILD_BUG_ON(GICD_INT_DEF_PRI < (GIC_PRIO_IRQOFF |
> +					 GIC_PRIO_IGNORE_PMR));
> +	BUILD_BUG_ON(GICD_INT_DEF_PRI >= GIC_PRIO_IRQON);
>  	gic_write_pmr(GIC_PRIO_IRQOFF);
>  }
> 
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index c5308d0..601154d 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -62,6 +62,15 @@
>  	msr	daifclr, #(8 | 4 | 1)
>  	.endm
> 
> +	.macro	suspend_irq_prio_masking, tmp:req
> +#ifdef CONFIG_ARM64_PSEUDO_NMI
> +	alternative_if ARM64_HAS_IRQ_PRIO_MASKING
> +	mov	\tmp, #(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR)
> +	msr_s	SYS_ICC_PMR_EL1, \tmp
> +	alternative_else_nop_endif
> +#endif
> +	.endm
> +
>  /*
>   * Save/restore interrupts.
>   */
> diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
> index db452aa..a32ece9 100644
> --- a/arch/arm64/include/asm/daifflags.h
> +++ b/arch/arm64/include/asm/daifflags.h
> @@ -18,6 +18,7 @@
> 
>  #include <linux/irqflags.h>
> 
> +#include <asm/arch_gicv3.h>
>  #include <asm/cpufeature.h>
> 
>  #define DAIF_PROCCTX		0
> @@ -32,6 +33,11 @@ static inline void local_daif_mask(void)
>  		:
>  		:
>  		: "memory");
> +
> +	/* Don't really care for a dsb here, we don't intend to enable IRQs */
> +	if (system_uses_irq_prio_masking())
> +		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
> +
>  	trace_hardirqs_off();
>  }
> 
> @@ -43,7 +49,7 @@ static inline unsigned long local_daif_save(void)
> 
>  	if (system_uses_irq_prio_masking()) {
>  		/* If IRQs are masked with PMR, reflect it in the flags */
> -		if (read_sysreg_s(SYS_ICC_PMR_EL1) <= GIC_PRIO_IRQOFF)
> +		if (read_sysreg_s(SYS_ICC_PMR_EL1) != GIC_PRIO_IRQON)
>  			flags |= PSR_I_BIT;
>  	}
> 
> @@ -59,14 +65,16 @@ static inline void local_daif_restore(unsigned long flags)
>  	if (!irq_disabled) {
>  		trace_hardirqs_on();
> 
> -		if (system_uses_irq_prio_masking())
> -			arch_local_irq_enable();
> -	} else if (!(flags & PSR_A_BIT)) {
> +		if (system_uses_irq_prio_masking()) {
> +			gic_write_pmr(GIC_PRIO_IRQON);
> +			dsb(sy);
> +		}
> +	} else if (system_uses_irq_prio_masking()) {
>  		/*
>  		 * If interrupts are disabled but we can take
>  		 * asynchronous errors, we can take NMIs
>  		 */
> -		if (system_uses_irq_prio_masking()) {
> +		if (!(flags & PSR_A_BIT)) {
>  			flags &= ~PSR_I_BIT;
>  			/*
>  			 * There has been concern that the write to daif
> @@ -87,7 +95,9 @@ static inline void local_daif_restore(unsigned long flags)
>  			 *
>  			 * So we don't need additional synchronization here.
>  			 */
> -			arch_local_irq_disable();
> +			gic_write_pmr(GIC_PRIO_IRQOFF);
> +		} else {
> +			gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
>  		}

Nit: move the write to PMR outside of the if/else clause with an
intermediate variable to hold the PMR value. Not a big deal, but I think
it helps with the nesting.

>  	}
> 
> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
> index 43d8366..516cdfc 100644
> --- a/arch/arm64/include/asm/irqflags.h
> +++ b/arch/arm64/include/asm/irqflags.h
> @@ -67,43 +67,48 @@ static inline void arch_local_irq_disable(void)
>   */
>  static inline unsigned long arch_local_save_flags(void)
>  {
> -	unsigned long daif_bits;
>  	unsigned long flags;
> 
> -	daif_bits = read_sysreg(daif);
> -
> -	/*
> -	 * The asm is logically equivalent to:
> -	 *
> -	 * if (system_uses_irq_prio_masking())
> -	 *	flags = (daif_bits & PSR_I_BIT) ?
> -	 *			GIC_PRIO_IRQOFF :
> -	 *			read_sysreg_s(SYS_ICC_PMR_EL1);
> -	 * else
> -	 *	flags = daif_bits;
> -	 */
>  	asm volatile(ALTERNATIVE(
> -			"mov	%0, %1\n"
> -			"nop\n"
> -			"nop",
> -			"mrs_s	%0, " __stringify(SYS_ICC_PMR_EL1) "\n"
> -			"ands	%1, %1, " __stringify(PSR_I_BIT) "\n"
> -			"csel	%0, %0, %2, eq",
> -			ARM64_HAS_IRQ_PRIO_MASKING)
> -		: "=&r" (flags), "+r" (daif_bits)
> -		: "r" ((unsigned long) GIC_PRIO_IRQOFF)
> +		"mrs	%0, daif",
> +		"mrs_s	%0, " __stringify(SYS_ICC_PMR_EL1) "\n",
> +		ARM64_HAS_IRQ_PRIO_MASKING)
> +		: "=&r" (flags)
> +		:
>  		: "memory");
> 
>  	return flags;
>  }
> 
> +static inline int arch_irqs_disabled_flags(unsigned long flags)
> +{
> +	int res;
> +
> +	asm volatile(ALTERNATIVE(
> +		"and	%w0, %w1, #" __stringify(PSR_I_BIT) "\n"
> +		"nop",
> +		"cmp	%w1, #" __stringify(GIC_PRIO_IRQON) "\n"
> +		"cset	%w0, ne",
> +		ARM64_HAS_IRQ_PRIO_MASKING)
> +		: "=&r" (res)
> +		: "r" ((int) flags)
> +		: "memory");

I wonder if this should have "cc" as part of the clobber list.

> +
> +	return res;
> +}
> +
>  static inline unsigned long arch_local_irq_save(void)
>  {
>  	unsigned long flags;
> 
>  	flags = arch_local_save_flags();
> 
> -	arch_local_irq_disable();
> +	/*
> +	 * There are too many states with IRQs disabled, just keep the current
> +	 * state if interrupts are already disabled/masked.
> +	 */
> +	if (!arch_irqs_disabled_flags(flags))
> +		arch_local_irq_disable();
> 
>  	return flags;
>  }
> @@ -119,26 +124,10 @@ static inline void arch_local_irq_restore(unsigned long flags)
>  			"msr_s	" __stringify(SYS_ICC_PMR_EL1) ", %0\n"
>  			"dsb	sy",
>  			ARM64_HAS_IRQ_PRIO_MASKING)
> -		: "+r" (flags)
>  		:
> +		: "r" (flags)
>  		: "memory");
>  }
> 
> -static inline int arch_irqs_disabled_flags(unsigned long flags)
> -{
> -	int res;
> -
> -	asm volatile(ALTERNATIVE(
> -			"and	%w0, %w1, #" __stringify(PSR_I_BIT) "\n"
> -			"nop",
> -			"cmp	%w1, #" __stringify(GIC_PRIO_IRQOFF) "\n"
> -			"cset	%w0, ls",
> -			ARM64_HAS_IRQ_PRIO_MASKING)
> -		: "=&r" (res)
> -		: "r" ((int) flags)
> -		: "memory");
> -
> -	return res;
> -}
>  #endif
>  #endif
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index a01fe087..0ec398c 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -532,9 +532,11 @@ static inline void kvm_arm_vhe_guest_enter(void)
>  	 * will not signal the CPU of interrupts of lower priority, and the
>  	 * only way to get out will be via guest exceptions.
>  	 * Naturally, we want to avoid this.
> +	 *
> +	 * local_daif_mask() already sets IGNORE_PMR, we just need a
> +	 * dsb to ensure the redistributor is forwards EL2 IRQs to the CPU.
>  	 */
>  	if (system_uses_irq_prio_masking()) {
> -		gic_write_pmr(GIC_PRIO_IRQON);
>  		dsb(sy);
>  	}

nit: drop the (now superfluous) brackets.

>  }
> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> index ec60174..7861a5d 100644
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -35,9 +35,15 @@
>   * means masking more IRQs (or at least that the same IRQs remain masked).
>   *
>   * To mask interrupts, we clear the most significant bit of PMR.
> + *
> + * Some code sections either automatically switch back to PSR.I or explicitly
> + * require to not use priority masking. If bit GIC_PRIO_IGNORE_PMR is included
> + * in the  the priority mask, it indicates that PSR.I should be set and
> + * interrupt disabling temporarily does not rely on IRQ priorities.
>   */
> -#define GIC_PRIO_IRQON		0xf0
> -#define GIC_PRIO_IRQOFF		(GIC_PRIO_IRQON & ~0x80)
> +#define GIC_PRIO_IRQON			0xc0
> +#define GIC_PRIO_IRQOFF			(GIC_PRIO_IRQON & ~0x80)
> +#define GIC_PRIO_IGNORE_PMR		(1 << 4)

The naming of this last value is still a bit odd: it says "ignore PMR",
and yet it only works because it is a valid PMR value. So it is not so
much that PMR is not used (it is), but that PSTATE.I is set, thus
overriding the interrupt signaling.

How about something like GIC_PRIO_PSTATE_I_SET, which has the advantage
of exactly matching the above comment?

> 
>  /* Additional SPSR bits not exposed in the UABI */
>  #define PSR_IL_BIT		(1 << 20)
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 00c1f21..e6df781 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -258,6 +258,7 @@ alternative_else_nop_endif
>  	/*
>  	 * Registers that may be useful after this macro is invoked:
>  	 *
> +	 * x20 - ICC_PMR_EL1
>  	 * x21 - aborted SP
>  	 * x22 - aborted PC
>  	 * x23 - aborted PSTATE
> @@ -434,6 +435,16 @@ alternative_endif
>  	.endm
>  #endif
> 
> +
> +	.macro irq_entry_ignore_pmr, pmr:req, tmp:req
> +#ifdef CONFIG_ARM64_PSEUDO_NMI
> +	alternative_if ARM64_HAS_IRQ_PRIO_MASKING
> +	orr	\tmp, \pmr, #GIC_PRIO_IGNORE_PMR
> +	msr_s	SYS_ICC_PMR_EL1, \tmp
> +	alternative_else_nop_endif
> +#endif
> +	.endm
> +
>  	.text
> 
>  /*
> @@ -612,6 +623,7 @@ el1_dbg:
>  	cmp	x24, #ESR_ELx_EC_BRK64		// if BRK64
>  	cinc	x24, x24, eq			// set bit '0'
>  	tbz	x24, #0, el1_inv		// EL1 only
> +	suspend_irq_prio_masking tmp=x21
>  	mrs	x0, far_el1
>  	mov	x2, sp				// struct pt_regs
>  	bl	do_debug_exception
> @@ -629,12 +641,10 @@ ENDPROC(el1_sync)
>  	.align	6
>  el1_irq:
>  	kernel_entry 1
> +	irq_entry_ignore_pmr pmr=x20, tmp=x0

What are the conditions for which we use 'suspend', and those we use
'ignore'? They obviously are very similar, and putting the rational
wouldn't hurt.

Of course, should you decide to adopt my naming suggestion above, some
names need to change too... ;-)

>  	enable_da_f
> 
>  #ifdef CONFIG_ARM64_PSEUDO_NMI
> -alternative_if ARM64_HAS_IRQ_PRIO_MASKING
> -	ldr	x20, [sp, #S_PMR_SAVE]
> -alternative_else_nop_endif
>  	test_irqs_unmasked	res=x0, pmr=x20
>  	cbz	x0, 1f
>  	bl	asm_nmi_enter
> @@ -664,8 +674,9 @@ alternative_else_nop_endif
> 
>  #ifdef CONFIG_ARM64_PSEUDO_NMI
>  	/*
> -	 * if IRQs were disabled when we received the interrupt, we have an NMI
> -	 * and we are not re-enabling interrupt upon eret. Skip tracing.
> +	 * When using IRQ priority masking, we can get spurious interrupts while
> +	 * PMR is set to GIC_PRIO_IRQOFF. An NMI might also have occurred in a
> +	 * section with interrupts disabled. Skip tracing in those cases.
>  	 */
>  	test_irqs_unmasked	res=x0, pmr=x20
>  	cbz	x0, 1f
> @@ -794,6 +805,7 @@ el0_ia:
>  	 * Instruction abort handling
>  	 */
>  	mrs	x26, far_el1
> +	suspend_irq_prio_masking tmp=x0
>  	enable_da_f
>  #ifdef CONFIG_TRACE_IRQFLAGS
>  	bl	trace_hardirqs_off
> @@ -839,6 +851,7 @@ el0_sp_pc:
>  	 * Stack or PC alignment exception handling
>  	 */
>  	mrs	x26, far_el1
> +	suspend_irq_prio_masking tmp=x0
>  	enable_da_f
>  #ifdef CONFIG_TRACE_IRQFLAGS
>  	bl	trace_hardirqs_off
> @@ -873,6 +886,7 @@ el0_dbg:
>  	 * Debug exception handling
>  	 */
>  	tbnz	x24, #0, el0_inv		// EL0 only
> +	suspend_irq_prio_masking tmp=x0
>  	mrs	x0, far_el1
>  	mov	x1, x25
>  	mov	x2, sp
> @@ -894,7 +908,9 @@ ENDPROC(el0_sync)
>  el0_irq:
>  	kernel_entry 0
>  el0_irq_naked:
> +	irq_entry_ignore_pmr pmr=x20, tmp=x0
>  	enable_da_f
> +
>  #ifdef CONFIG_TRACE_IRQFLAGS
>  	bl	trace_hardirqs_off
>  #endif
> @@ -916,6 +932,7 @@ ENDPROC(el0_irq)
>  el1_error:
>  	kernel_entry 1
>  	mrs	x1, esr_el1
> +	suspend_irq_prio_masking tmp=x0
>  	enable_dbg
>  	mov	x0, sp
>  	bl	do_serror
> @@ -926,6 +943,7 @@ el0_error:
>  	kernel_entry 0
>  el0_error_naked:
>  	mrs	x1, esr_el1
> +	suspend_irq_prio_masking tmp=x0
>  	enable_dbg
>  	mov	x0, sp
>  	bl	do_serror
> @@ -950,6 +968,7 @@ work_pending:
>   */
>  ret_to_user:
>  	disable_daif
> +	suspend_irq_prio_masking tmp=x0
>  	ldr	x1, [tsk, #TSK_TI_FLAGS]
>  	and	x2, x1, #_TIF_WORK_MASK
>  	cbnz	x2, work_pending
> @@ -966,6 +985,7 @@ ENDPROC(ret_to_user)
>   */
>  	.align	6
>  el0_svc:
> +	suspend_irq_prio_masking tmp=x1
>  	mov	x0, sp
>  	bl	el0_svc_handler
>  	b	ret_to_user
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 3767fb2..f5bae97 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -94,7 +94,7 @@ static void __cpu_do_idle_irqprio(void)
>  	 * be raised.
>  	 */
>  	pmr = gic_read_pmr();
> -	gic_write_pmr(GIC_PRIO_IRQON);
> +	gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
> 
>  	__cpu_do_idle();
> 
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 824de70..2a6d0dd1 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -192,11 +192,13 @@ static void init_gic_priority_masking(void)
> 
>  	WARN_ON(!(cpuflags & PSR_I_BIT));
> 
> -	gic_write_pmr(GIC_PRIO_IRQOFF);
> -
>  	/* We can only unmask PSR.I if we can take aborts */
> -	if (!(cpuflags & PSR_A_BIT))
> +	if (!(cpuflags & PSR_A_BIT)) {
> +		gic_write_pmr(GIC_PRIO_IRQOFF);
>  		write_sysreg(cpuflags & ~PSR_I_BIT, daif);
> +	} else {
> +		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
> +	}
>  }
> 
>  /*
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 3563fe6..083e319 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -533,7 +533,7 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
>  	 * Naturally, we want to avoid this.
>  	 */
>  	if (system_uses_irq_prio_masking()) {
> -		gic_write_pmr(GIC_PRIO_IRQON);
> +		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
>  		dsb(sy);
>  	}
> 
> --
> 1.9.1
> 

Overall, I like the way it looks, and my comments are more cosmetic than
anything else.

Thanks,

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

Re: [PATCH v2 3/5] arm64: Fix incorrect irqflag restore for priority masking

[Marc Zyngier]

On 29/04/2019 17:00, Julien Thierry wrote:
> When using IRQ priority masking to disable interrupts, in order to deal
> with the PSR.I state, local_irq_save() would convert the I bit into a
> PMR value (GIC_PRIO_IRQOFF). This resulted in local_irq_restore()
> potentially modifying the value of PMR in undesired location due to the
> state of PSR.I upon flag saving [1].
> 
> In an attempt to solve this issue in a less hackish manner, introduce
> a bit (GIC_PRIO_IGNORE_PMR) for the PMR values that can represent
> whether PSR.I is being used to disable interrupts, in which case it
> takes precedence of the status of interrupt masking via PMR.
> 
> GIC_PRIO_IGNORE_PMR is chosen such that (<pmr_value> |
> GIC_PRIO_IGNORE_PMR) does not mask more interrupts than <pmr_value> as
> some sections (e.g. arch_cpu_idle(), interrupt acknowledge path)
> requires PMR not to mask interrupts that could be signaled to the
> CPU when using only PSR.I.
> 
> [1] https://www.spinics.net/lists/arm-kernel/msg716956.html
> 
> Fixes: commit 4a503217ce37 ("arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking")
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> Reported-by: Zenghui Yu <yuzenghui@huawei.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Wei Li <liwei391@huawei.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Christoffer Dall <christoffer.dall@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Suzuki K Pouloze <suzuki.poulose@arm.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> ---
>  arch/arm64/include/asm/arch_gicv3.h |  4 ++-
>  arch/arm64/include/asm/assembler.h  |  9 +++++
>  arch/arm64/include/asm/daifflags.h  | 22 ++++++++----
>  arch/arm64/include/asm/irqflags.h   | 69 ++++++++++++++++---------------------
>  arch/arm64/include/asm/kvm_host.h   |  4 ++-
>  arch/arm64/include/asm/ptrace.h     | 10 ++++--
>  arch/arm64/kernel/entry.S           | 30 +++++++++++++---
>  arch/arm64/kernel/process.c         |  2 +-
>  arch/arm64/kernel/smp.c             |  8 +++--
>  arch/arm64/kvm/hyp/switch.c         |  2 +-
>  10 files changed, 100 insertions(+), 60 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
> index 14b41dd..3102c9a 100644
> --- a/arch/arm64/include/asm/arch_gicv3.h
> +++ b/arch/arm64/include/asm/arch_gicv3.h
> @@ -163,7 +163,9 @@ static inline bool gic_prio_masking_enabled(void)
> 
>  static inline void gic_pmr_mask_irqs(void)
>  {
> -	BUILD_BUG_ON(GICD_INT_DEF_PRI <= GIC_PRIO_IRQOFF);
> +	BUILD_BUG_ON(GICD_INT_DEF_PRI < (GIC_PRIO_IRQOFF |
> +					 GIC_PRIO_IGNORE_PMR));
> +	BUILD_BUG_ON(GICD_INT_DEF_PRI >= GIC_PRIO_IRQON);
>  	gic_write_pmr(GIC_PRIO_IRQOFF);
>  }
> 
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index c5308d0..601154d 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -62,6 +62,15 @@
>  	msr	daifclr, #(8 | 4 | 1)
>  	.endm
> 
> +	.macro	suspend_irq_prio_masking, tmp:req
> +#ifdef CONFIG_ARM64_PSEUDO_NMI
> +	alternative_if ARM64_HAS_IRQ_PRIO_MASKING
> +	mov	\tmp, #(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR)
> +	msr_s	SYS_ICC_PMR_EL1, \tmp
> +	alternative_else_nop_endif
> +#endif
> +	.endm
> +
>  /*
>   * Save/restore interrupts.
>   */
> diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
> index db452aa..a32ece9 100644
> --- a/arch/arm64/include/asm/daifflags.h
> +++ b/arch/arm64/include/asm/daifflags.h
> @@ -18,6 +18,7 @@
> 
>  #include <linux/irqflags.h>
> 
> +#include <asm/arch_gicv3.h>
>  #include <asm/cpufeature.h>
> 
>  #define DAIF_PROCCTX		0
> @@ -32,6 +33,11 @@ static inline void local_daif_mask(void)
>  		:
>  		:
>  		: "memory");
> +
> +	/* Don't really care for a dsb here, we don't intend to enable IRQs */
> +	if (system_uses_irq_prio_masking())
> +		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
> +
>  	trace_hardirqs_off();
>  }
> 
> @@ -43,7 +49,7 @@ static inline unsigned long local_daif_save(void)
> 
>  	if (system_uses_irq_prio_masking()) {
>  		/* If IRQs are masked with PMR, reflect it in the flags */
> -		if (read_sysreg_s(SYS_ICC_PMR_EL1) <= GIC_PRIO_IRQOFF)
> +		if (read_sysreg_s(SYS_ICC_PMR_EL1) != GIC_PRIO_IRQON)
>  			flags |= PSR_I_BIT;
>  	}
> 
> @@ -59,14 +65,16 @@ static inline void local_daif_restore(unsigned long flags)
>  	if (!irq_disabled) {
>  		trace_hardirqs_on();
> 
> -		if (system_uses_irq_prio_masking())
> -			arch_local_irq_enable();
> -	} else if (!(flags & PSR_A_BIT)) {
> +		if (system_uses_irq_prio_masking()) {
> +			gic_write_pmr(GIC_PRIO_IRQON);
> +			dsb(sy);
> +		}
> +	} else if (system_uses_irq_prio_masking()) {
>  		/*
>  		 * If interrupts are disabled but we can take
>  		 * asynchronous errors, we can take NMIs
>  		 */
> -		if (system_uses_irq_prio_masking()) {
> +		if (!(flags & PSR_A_BIT)) {
>  			flags &= ~PSR_I_BIT;
>  			/*
>  			 * There has been concern that the write to daif
> @@ -87,7 +95,9 @@ static inline void local_daif_restore(unsigned long flags)
>  			 *
>  			 * So we don't need additional synchronization here.
>  			 */
> -			arch_local_irq_disable();
> +			gic_write_pmr(GIC_PRIO_IRQOFF);
> +		} else {
> +			gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
>  		}

Nit: move the write to PMR outside of the if/else clause with an
intermediate variable to hold the PMR value. Not a big deal, but I think
it helps with the nesting.

>  	}
> 
> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
> index 43d8366..516cdfc 100644
> --- a/arch/arm64/include/asm/irqflags.h
> +++ b/arch/arm64/include/asm/irqflags.h
> @@ -67,43 +67,48 @@ static inline void arch_local_irq_disable(void)
>   */
>  static inline unsigned long arch_local_save_flags(void)
>  {
> -	unsigned long daif_bits;
>  	unsigned long flags;
> 
> -	daif_bits = read_sysreg(daif);
> -
> -	/*
> -	 * The asm is logically equivalent to:
> -	 *
> -	 * if (system_uses_irq_prio_masking())
> -	 *	flags = (daif_bits & PSR_I_BIT) ?
> -	 *			GIC_PRIO_IRQOFF :
> -	 *			read_sysreg_s(SYS_ICC_PMR_EL1);
> -	 * else
> -	 *	flags = daif_bits;
> -	 */
>  	asm volatile(ALTERNATIVE(
> -			"mov	%0, %1\n"
> -			"nop\n"
> -			"nop",
> -			"mrs_s	%0, " __stringify(SYS_ICC_PMR_EL1) "\n"
> -			"ands	%1, %1, " __stringify(PSR_I_BIT) "\n"
> -			"csel	%0, %0, %2, eq",
> -			ARM64_HAS_IRQ_PRIO_MASKING)
> -		: "=&r" (flags), "+r" (daif_bits)
> -		: "r" ((unsigned long) GIC_PRIO_IRQOFF)
> +		"mrs	%0, daif",
> +		"mrs_s	%0, " __stringify(SYS_ICC_PMR_EL1) "\n",
> +		ARM64_HAS_IRQ_PRIO_MASKING)
> +		: "=&r" (flags)
> +		:
>  		: "memory");
> 
>  	return flags;
>  }
> 
> +static inline int arch_irqs_disabled_flags(unsigned long flags)
> +{
> +	int res;
> +
> +	asm volatile(ALTERNATIVE(
> +		"and	%w0, %w1, #" __stringify(PSR_I_BIT) "\n"
> +		"nop",
> +		"cmp	%w1, #" __stringify(GIC_PRIO_IRQON) "\n"
> +		"cset	%w0, ne",
> +		ARM64_HAS_IRQ_PRIO_MASKING)
> +		: "=&r" (res)
> +		: "r" ((int) flags)
> +		: "memory");

I wonder if this should have "cc" as part of the clobber list.

> +
> +	return res;
> +}
> +
>  static inline unsigned long arch_local_irq_save(void)
>  {
>  	unsigned long flags;
> 
>  	flags = arch_local_save_flags();
> 
> -	arch_local_irq_disable();
> +	/*
> +	 * There are too many states with IRQs disabled, just keep the current
> +	 * state if interrupts are already disabled/masked.
> +	 */
> +	if (!arch_irqs_disabled_flags(flags))
> +		arch_local_irq_disable();
> 
>  	return flags;
>  }
> @@ -119,26 +124,10 @@ static inline void arch_local_irq_restore(unsigned long flags)
>  			"msr_s	" __stringify(SYS_ICC_PMR_EL1) ", %0\n"
>  			"dsb	sy",
>  			ARM64_HAS_IRQ_PRIO_MASKING)
> -		: "+r" (flags)
>  		:
> +		: "r" (flags)
>  		: "memory");
>  }
> 
> -static inline int arch_irqs_disabled_flags(unsigned long flags)
> -{
> -	int res;
> -
> -	asm volatile(ALTERNATIVE(
> -			"and	%w0, %w1, #" __stringify(PSR_I_BIT) "\n"
> -			"nop",
> -			"cmp	%w1, #" __stringify(GIC_PRIO_IRQOFF) "\n"
> -			"cset	%w0, ls",
> -			ARM64_HAS_IRQ_PRIO_MASKING)
> -		: "=&r" (res)
> -		: "r" ((int) flags)
> -		: "memory");
> -
> -	return res;
> -}
>  #endif
>  #endif
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index a01fe087..0ec398c 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -532,9 +532,11 @@ static inline void kvm_arm_vhe_guest_enter(void)
>  	 * will not signal the CPU of interrupts of lower priority, and the
>  	 * only way to get out will be via guest exceptions.
>  	 * Naturally, we want to avoid this.
> +	 *
> +	 * local_daif_mask() already sets IGNORE_PMR, we just need a
> +	 * dsb to ensure the redistributor is forwards EL2 IRQs to the CPU.
>  	 */
>  	if (system_uses_irq_prio_masking()) {
> -		gic_write_pmr(GIC_PRIO_IRQON);
>  		dsb(sy);
>  	}

nit: drop the (now superfluous) brackets.

>  }
> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> index ec60174..7861a5d 100644
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -35,9 +35,15 @@
>   * means masking more IRQs (or at least that the same IRQs remain masked).
>   *
>   * To mask interrupts, we clear the most significant bit of PMR.
> + *
> + * Some code sections either automatically switch back to PSR.I or explicitly
> + * require to not use priority masking. If bit GIC_PRIO_IGNORE_PMR is included
> + * in the  the priority mask, it indicates that PSR.I should be set and
> + * interrupt disabling temporarily does not rely on IRQ priorities.
>   */
> -#define GIC_PRIO_IRQON		0xf0
> -#define GIC_PRIO_IRQOFF		(GIC_PRIO_IRQON & ~0x80)
> +#define GIC_PRIO_IRQON			0xc0
> +#define GIC_PRIO_IRQOFF			(GIC_PRIO_IRQON & ~0x80)
> +#define GIC_PRIO_IGNORE_PMR		(1 << 4)

The naming of this last value is still a bit odd: it says "ignore PMR",
and yet it only works because it is a valid PMR value. So it is not so
much that PMR is not used (it is), but that PSTATE.I is set, thus
overriding the interrupt signaling.

How about something like GIC_PRIO_PSTATE_I_SET, which has the advantage
of exactly matching the above comment?

> 
>  /* Additional SPSR bits not exposed in the UABI */
>  #define PSR_IL_BIT		(1 << 20)
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 00c1f21..e6df781 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -258,6 +258,7 @@ alternative_else_nop_endif
>  	/*
>  	 * Registers that may be useful after this macro is invoked:
>  	 *
> +	 * x20 - ICC_PMR_EL1
>  	 * x21 - aborted SP
>  	 * x22 - aborted PC
>  	 * x23 - aborted PSTATE
> @@ -434,6 +435,16 @@ alternative_endif
>  	.endm
>  #endif
> 
> +
> +	.macro irq_entry_ignore_pmr, pmr:req, tmp:req
> +#ifdef CONFIG_ARM64_PSEUDO_NMI
> +	alternative_if ARM64_HAS_IRQ_PRIO_MASKING
> +	orr	\tmp, \pmr, #GIC_PRIO_IGNORE_PMR
> +	msr_s	SYS_ICC_PMR_EL1, \tmp
> +	alternative_else_nop_endif
> +#endif
> +	.endm
> +
>  	.text
> 
>  /*
> @@ -612,6 +623,7 @@ el1_dbg:
>  	cmp	x24, #ESR_ELx_EC_BRK64		// if BRK64
>  	cinc	x24, x24, eq			// set bit '0'
>  	tbz	x24, #0, el1_inv		// EL1 only
> +	suspend_irq_prio_masking tmp=x21
>  	mrs	x0, far_el1
>  	mov	x2, sp				// struct pt_regs
>  	bl	do_debug_exception
> @@ -629,12 +641,10 @@ ENDPROC(el1_sync)
>  	.align	6
>  el1_irq:
>  	kernel_entry 1
> +	irq_entry_ignore_pmr pmr=x20, tmp=x0

What are the conditions for which we use 'suspend', and those we use
'ignore'? They obviously are very similar, and putting the rational
wouldn't hurt.

Of course, should you decide to adopt my naming suggestion above, some
names need to change too... ;-)

>  	enable_da_f
> 
>  #ifdef CONFIG_ARM64_PSEUDO_NMI
> -alternative_if ARM64_HAS_IRQ_PRIO_MASKING
> -	ldr	x20, [sp, #S_PMR_SAVE]
> -alternative_else_nop_endif
>  	test_irqs_unmasked	res=x0, pmr=x20
>  	cbz	x0, 1f
>  	bl	asm_nmi_enter
> @@ -664,8 +674,9 @@ alternative_else_nop_endif
> 
>  #ifdef CONFIG_ARM64_PSEUDO_NMI
>  	/*
> -	 * if IRQs were disabled when we received the interrupt, we have an NMI
> -	 * and we are not re-enabling interrupt upon eret. Skip tracing.
> +	 * When using IRQ priority masking, we can get spurious interrupts while
> +	 * PMR is set to GIC_PRIO_IRQOFF. An NMI might also have occurred in a
> +	 * section with interrupts disabled. Skip tracing in those cases.
>  	 */
>  	test_irqs_unmasked	res=x0, pmr=x20
>  	cbz	x0, 1f
> @@ -794,6 +805,7 @@ el0_ia:
>  	 * Instruction abort handling
>  	 */
>  	mrs	x26, far_el1
> +	suspend_irq_prio_masking tmp=x0
>  	enable_da_f
>  #ifdef CONFIG_TRACE_IRQFLAGS
>  	bl	trace_hardirqs_off
> @@ -839,6 +851,7 @@ el0_sp_pc:
>  	 * Stack or PC alignment exception handling
>  	 */
>  	mrs	x26, far_el1
> +	suspend_irq_prio_masking tmp=x0
>  	enable_da_f
>  #ifdef CONFIG_TRACE_IRQFLAGS
>  	bl	trace_hardirqs_off
> @@ -873,6 +886,7 @@ el0_dbg:
>  	 * Debug exception handling
>  	 */
>  	tbnz	x24, #0, el0_inv		// EL0 only
> +	suspend_irq_prio_masking tmp=x0
>  	mrs	x0, far_el1
>  	mov	x1, x25
>  	mov	x2, sp
> @@ -894,7 +908,9 @@ ENDPROC(el0_sync)
>  el0_irq:
>  	kernel_entry 0
>  el0_irq_naked:
> +	irq_entry_ignore_pmr pmr=x20, tmp=x0
>  	enable_da_f
> +
>  #ifdef CONFIG_TRACE_IRQFLAGS
>  	bl	trace_hardirqs_off
>  #endif
> @@ -916,6 +932,7 @@ ENDPROC(el0_irq)
>  el1_error:
>  	kernel_entry 1
>  	mrs	x1, esr_el1
> +	suspend_irq_prio_masking tmp=x0
>  	enable_dbg
>  	mov	x0, sp
>  	bl	do_serror
> @@ -926,6 +943,7 @@ el0_error:
>  	kernel_entry 0
>  el0_error_naked:
>  	mrs	x1, esr_el1
> +	suspend_irq_prio_masking tmp=x0
>  	enable_dbg
>  	mov	x0, sp
>  	bl	do_serror
> @@ -950,6 +968,7 @@ work_pending:
>   */
>  ret_to_user:
>  	disable_daif
> +	suspend_irq_prio_masking tmp=x0
>  	ldr	x1, [tsk, #TSK_TI_FLAGS]
>  	and	x2, x1, #_TIF_WORK_MASK
>  	cbnz	x2, work_pending
> @@ -966,6 +985,7 @@ ENDPROC(ret_to_user)
>   */
>  	.align	6
>  el0_svc:
> +	suspend_irq_prio_masking tmp=x1
>  	mov	x0, sp
>  	bl	el0_svc_handler
>  	b	ret_to_user
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 3767fb2..f5bae97 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -94,7 +94,7 @@ static void __cpu_do_idle_irqprio(void)
>  	 * be raised.
>  	 */
>  	pmr = gic_read_pmr();
> -	gic_write_pmr(GIC_PRIO_IRQON);
> +	gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
> 
>  	__cpu_do_idle();
> 
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 824de70..2a6d0dd1 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -192,11 +192,13 @@ static void init_gic_priority_masking(void)
> 
>  	WARN_ON(!(cpuflags & PSR_I_BIT));
> 
> -	gic_write_pmr(GIC_PRIO_IRQOFF);
> -
>  	/* We can only unmask PSR.I if we can take aborts */
> -	if (!(cpuflags & PSR_A_BIT))
> +	if (!(cpuflags & PSR_A_BIT)) {
> +		gic_write_pmr(GIC_PRIO_IRQOFF);
>  		write_sysreg(cpuflags & ~PSR_I_BIT, daif);
> +	} else {
> +		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
> +	}
>  }
> 
>  /*
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 3563fe6..083e319 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -533,7 +533,7 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
>  	 * Naturally, we want to avoid this.
>  	 */
>  	if (system_uses_irq_prio_masking()) {
> -		gic_write_pmr(GIC_PRIO_IRQON);
> +		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
>  		dsb(sy);
>  	}
> 
> --
> 1.9.1
> 

Overall, I like the way it looks, and my comments are more cosmetic than
anything else.

Thanks,

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

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

Re: [PATCH v2 3/5] arm64: Fix incorrect irqflag restore for priority masking

[Julien Thierry]

On 07/05/2019 09:36, Marc Zyngier wrote:
> On 29/04/2019 17:00, Julien Thierry wrote:
>> When using IRQ priority masking to disable interrupts, in order to deal
>> with the PSR.I state, local_irq_save() would convert the I bit into a
>> PMR value (GIC_PRIO_IRQOFF). This resulted in local_irq_restore()
>> potentially modifying the value of PMR in undesired location due to the
>> state of PSR.I upon flag saving [1].
>>
>> In an attempt to solve this issue in a less hackish manner, introduce
>> a bit (GIC_PRIO_IGNORE_PMR) for the PMR values that can represent
>> whether PSR.I is being used to disable interrupts, in which case it
>> takes precedence of the status of interrupt masking via PMR.
>>
>> GIC_PRIO_IGNORE_PMR is chosen such that (<pmr_value> |
>> GIC_PRIO_IGNORE_PMR) does not mask more interrupts than <pmr_value> as
>> some sections (e.g. arch_cpu_idle(), interrupt acknowledge path)
>> requires PMR not to mask interrupts that could be signaled to the
>> CPU when using only PSR.I.
>>
>> [1] https://www.spinics.net/lists/arm-kernel/msg716956.html
>>
>> Fixes: commit 4a503217ce37 ("arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking")
>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>> Reported-by: Zenghui Yu <yuzenghui@huawei.com>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Wei Li <liwei391@huawei.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Christoffer Dall <christoffer.dall@arm.com>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: James Morse <james.morse@arm.com>
>> Cc: Suzuki K Pouloze <suzuki.poulose@arm.com>
>> Cc: Oleg Nesterov <oleg@redhat.com>
>> ---
>>  arch/arm64/include/asm/arch_gicv3.h |  4 ++-
>>  arch/arm64/include/asm/assembler.h  |  9 +++++
>>  arch/arm64/include/asm/daifflags.h  | 22 ++++++++----
>>  arch/arm64/include/asm/irqflags.h   | 69 ++++++++++++++++---------------------
>>  arch/arm64/include/asm/kvm_host.h   |  4 ++-
>>  arch/arm64/include/asm/ptrace.h     | 10 ++++--
>>  arch/arm64/kernel/entry.S           | 30 +++++++++++++---
>>  arch/arm64/kernel/process.c         |  2 +-
>>  arch/arm64/kernel/smp.c             |  8 +++--
>>  arch/arm64/kvm/hyp/switch.c         |  2 +-
>>  10 files changed, 100 insertions(+), 60 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
>> index 14b41dd..3102c9a 100644
>> --- a/arch/arm64/include/asm/arch_gicv3.h
>> +++ b/arch/arm64/include/asm/arch_gicv3.h
>> @@ -163,7 +163,9 @@ static inline bool gic_prio_masking_enabled(void)
>>
>>  static inline void gic_pmr_mask_irqs(void)
>>  {
>> -	BUILD_BUG_ON(GICD_INT_DEF_PRI <= GIC_PRIO_IRQOFF);
>> +	BUILD_BUG_ON(GICD_INT_DEF_PRI < (GIC_PRIO_IRQOFF |
>> +					 GIC_PRIO_IGNORE_PMR));
>> +	BUILD_BUG_ON(GICD_INT_DEF_PRI >= GIC_PRIO_IRQON);
>>  	gic_write_pmr(GIC_PRIO_IRQOFF);
>>  }
>>
>> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
>> index c5308d0..601154d 100644
>> --- a/arch/arm64/include/asm/assembler.h
>> +++ b/arch/arm64/include/asm/assembler.h
>> @@ -62,6 +62,15 @@
>>  	msr	daifclr, #(8 | 4 | 1)
>>  	.endm
>>
>> +	.macro	suspend_irq_prio_masking, tmp:req
>> +#ifdef CONFIG_ARM64_PSEUDO_NMI
>> +	alternative_if ARM64_HAS_IRQ_PRIO_MASKING
>> +	mov	\tmp, #(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR)
>> +	msr_s	SYS_ICC_PMR_EL1, \tmp
>> +	alternative_else_nop_endif
>> +#endif
>> +	.endm
>> +
>>  /*
>>   * Save/restore interrupts.
>>   */
>> diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
>> index db452aa..a32ece9 100644
>> --- a/arch/arm64/include/asm/daifflags.h
>> +++ b/arch/arm64/include/asm/daifflags.h
>> @@ -18,6 +18,7 @@
>>
>>  #include <linux/irqflags.h>
>>
>> +#include <asm/arch_gicv3.h>
>>  #include <asm/cpufeature.h>
>>
>>  #define DAIF_PROCCTX		0
>> @@ -32,6 +33,11 @@ static inline void local_daif_mask(void)
>>  		:
>>  		:
>>  		: "memory");
>> +
>> +	/* Don't really care for a dsb here, we don't intend to enable IRQs */
>> +	if (system_uses_irq_prio_masking())
>> +		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
>> +
>>  	trace_hardirqs_off();
>>  }
>>
>> @@ -43,7 +49,7 @@ static inline unsigned long local_daif_save(void)
>>
>>  	if (system_uses_irq_prio_masking()) {
>>  		/* If IRQs are masked with PMR, reflect it in the flags */
>> -		if (read_sysreg_s(SYS_ICC_PMR_EL1) <= GIC_PRIO_IRQOFF)
>> +		if (read_sysreg_s(SYS_ICC_PMR_EL1) != GIC_PRIO_IRQON)
>>  			flags |= PSR_I_BIT;
>>  	}
>>
>> @@ -59,14 +65,16 @@ static inline void local_daif_restore(unsigned long flags)
>>  	if (!irq_disabled) {
>>  		trace_hardirqs_on();
>>
>> -		if (system_uses_irq_prio_masking())
>> -			arch_local_irq_enable();
>> -	} else if (!(flags & PSR_A_BIT)) {
>> +		if (system_uses_irq_prio_masking()) {
>> +			gic_write_pmr(GIC_PRIO_IRQON);
>> +			dsb(sy);
>> +		}
>> +	} else if (system_uses_irq_prio_masking()) {
>>  		/*
>>  		 * If interrupts are disabled but we can take
>>  		 * asynchronous errors, we can take NMIs
>>  		 */
>> -		if (system_uses_irq_prio_masking()) {
>> +		if (!(flags & PSR_A_BIT)) {
>>  			flags &= ~PSR_I_BIT;
>>  			/*
>>  			 * There has been concern that the write to daif
>> @@ -87,7 +95,9 @@ static inline void local_daif_restore(unsigned long flags)
>>  			 *
>>  			 * So we don't need additional synchronization here.
>>  			 */
>> -			arch_local_irq_disable();
>> +			gic_write_pmr(GIC_PRIO_IRQOFF);
>> +		} else {
>> +			gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
>>  		}
> 
> Nit: move the write to PMR outside of the if/else clause with an
> intermediate variable to hold the PMR value. Not a big deal, but I think
> it helps with the nesting.
> 

Done.

>>  	}
>>
>> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
>> index 43d8366..516cdfc 100644
>> --- a/arch/arm64/include/asm/irqflags.h
>> +++ b/arch/arm64/include/asm/irqflags.h
>> @@ -67,43 +67,48 @@ static inline void arch_local_irq_disable(void)
>>   */
>>  static inline unsigned long arch_local_save_flags(void)
>>  {
>> -	unsigned long daif_bits;
>>  	unsigned long flags;
>>
>> -	daif_bits = read_sysreg(daif);
>> -
>> -	/*
>> -	 * The asm is logically equivalent to:
>> -	 *
>> -	 * if (system_uses_irq_prio_masking())
>> -	 *	flags = (daif_bits & PSR_I_BIT) ?
>> -	 *			GIC_PRIO_IRQOFF :
>> -	 *			read_sysreg_s(SYS_ICC_PMR_EL1);
>> -	 * else
>> -	 *	flags = daif_bits;
>> -	 */
>>  	asm volatile(ALTERNATIVE(
>> -			"mov	%0, %1\n"
>> -			"nop\n"
>> -			"nop",
>> -			"mrs_s	%0, " __stringify(SYS_ICC_PMR_EL1) "\n"
>> -			"ands	%1, %1, " __stringify(PSR_I_BIT) "\n"
>> -			"csel	%0, %0, %2, eq",
>> -			ARM64_HAS_IRQ_PRIO_MASKING)
>> -		: "=&r" (flags), "+r" (daif_bits)
>> -		: "r" ((unsigned long) GIC_PRIO_IRQOFF)
>> +		"mrs	%0, daif",
>> +		"mrs_s	%0, " __stringify(SYS_ICC_PMR_EL1) "\n",
>> +		ARM64_HAS_IRQ_PRIO_MASKING)
>> +		: "=&r" (flags)
>> +		:
>>  		: "memory");
>>
>>  	return flags;
>>  }
>>
>> +static inline int arch_irqs_disabled_flags(unsigned long flags)
>> +{
>> +	int res;
>> +
>> +	asm volatile(ALTERNATIVE(
>> +		"and	%w0, %w1, #" __stringify(PSR_I_BIT) "\n"
>> +		"nop",
>> +		"cmp	%w1, #" __stringify(GIC_PRIO_IRQON) "\n"
>> +		"cset	%w0, ne",
>> +		ARM64_HAS_IRQ_PRIO_MASKING)
>> +		: "=&r" (res)
>> +		: "r" ((int) flags)
>> +		: "memory");
> 
> I wonder if this should have "cc" as part of the clobber list.

Is there any special semantic to "cc" on arm64? All I can find is that
in the general case it indicates that it is modifying the "flags" register.

Is your suggestion only for the PMR case? Or is it something that we
should add regardless of PMR?
The latter makes sense to me, but for the former, I fail to understand
why this should affect only PMR.

> 
>> +
>> +	return res;
>> +}
>> +
>>  static inline unsigned long arch_local_irq_save(void)
>>  {
>>  	unsigned long flags;
>>
>>  	flags = arch_local_save_flags();
>>
>> -	arch_local_irq_disable();
>> +	/*
>> +	 * There are too many states with IRQs disabled, just keep the current
>> +	 * state if interrupts are already disabled/masked.
>> +	 */
>> +	if (!arch_irqs_disabled_flags(flags))
>> +		arch_local_irq_disable();
>>
>>  	return flags;
>>  }
>> @@ -119,26 +124,10 @@ static inline void arch_local_irq_restore(unsigned long flags)
>>  			"msr_s	" __stringify(SYS_ICC_PMR_EL1) ", %0\n"
>>  			"dsb	sy",
>>  			ARM64_HAS_IRQ_PRIO_MASKING)
>> -		: "+r" (flags)
>>  		:
>> +		: "r" (flags)
>>  		: "memory");
>>  }
>>
>> -static inline int arch_irqs_disabled_flags(unsigned long flags)
>> -{
>> -	int res;
>> -
>> -	asm volatile(ALTERNATIVE(
>> -			"and	%w0, %w1, #" __stringify(PSR_I_BIT) "\n"
>> -			"nop",
>> -			"cmp	%w1, #" __stringify(GIC_PRIO_IRQOFF) "\n"
>> -			"cset	%w0, ls",
>> -			ARM64_HAS_IRQ_PRIO_MASKING)
>> -		: "=&r" (res)
>> -		: "r" ((int) flags)
>> -		: "memory");
>> -
>> -	return res;
>> -}
>>  #endif
>>  #endif
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index a01fe087..0ec398c 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -532,9 +532,11 @@ static inline void kvm_arm_vhe_guest_enter(void)
>>  	 * will not signal the CPU of interrupts of lower priority, and the
>>  	 * only way to get out will be via guest exceptions.
>>  	 * Naturally, we want to avoid this.
>> +	 *
>> +	 * local_daif_mask() already sets IGNORE_PMR, we just need a
>> +	 * dsb to ensure the redistributor is forwards EL2 IRQs to the CPU.
>>  	 */
>>  	if (system_uses_irq_prio_masking()) {
>> -		gic_write_pmr(GIC_PRIO_IRQON);
>>  		dsb(sy);
>>  	}
> 
> nit: drop the (now superfluous) brackets.
> 

Done.

>>  }
>> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
>> index ec60174..7861a5d 100644
>> --- a/arch/arm64/include/asm/ptrace.h
>> +++ b/arch/arm64/include/asm/ptrace.h
>> @@ -35,9 +35,15 @@
>>   * means masking more IRQs (or at least that the same IRQs remain masked).
>>   *
>>   * To mask interrupts, we clear the most significant bit of PMR.
>> + *
>> + * Some code sections either automatically switch back to PSR.I or explicitly
>> + * require to not use priority masking. If bit GIC_PRIO_IGNORE_PMR is included
>> + * in the  the priority mask, it indicates that PSR.I should be set and
>> + * interrupt disabling temporarily does not rely on IRQ priorities.
>>   */
>> -#define GIC_PRIO_IRQON		0xf0
>> -#define GIC_PRIO_IRQOFF		(GIC_PRIO_IRQON & ~0x80)
>> +#define GIC_PRIO_IRQON			0xc0
>> +#define GIC_PRIO_IRQOFF			(GIC_PRIO_IRQON & ~0x80)
>> +#define GIC_PRIO_IGNORE_PMR		(1 << 4)
> 
> The naming of this last value is still a bit odd: it says "ignore PMR",
> and yet it only works because it is a valid PMR value. So it is not so
> much that PMR is not used (it is), but that PSTATE.I is set, thus
> overriding the interrupt signaling.
> 
> How about something like GIC_PRIO_PSTATE_I_SET, which has the advantage
> of exactly matching the above comment?
> 

Sounds good to me, also depends on how I go with the next comment.

>>
>>  /* Additional SPSR bits not exposed in the UABI */
>>  #define PSR_IL_BIT		(1 << 20)
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index 00c1f21..e6df781 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -258,6 +258,7 @@ alternative_else_nop_endif
>>  	/*
>>  	 * Registers that may be useful after this macro is invoked:
>>  	 *
>> +	 * x20 - ICC_PMR_EL1
>>  	 * x21 - aborted SP
>>  	 * x22 - aborted PC
>>  	 * x23 - aborted PSTATE
>> @@ -434,6 +435,16 @@ alternative_endif
>>  	.endm
>>  #endif
>>
>> +
>> +	.macro irq_entry_ignore_pmr, pmr:req, tmp:req
>> +#ifdef CONFIG_ARM64_PSEUDO_NMI
>> +	alternative_if ARM64_HAS_IRQ_PRIO_MASKING
>> +	orr	\tmp, \pmr, #GIC_PRIO_IGNORE_PMR
>> +	msr_s	SYS_ICC_PMR_EL1, \tmp
>> +	alternative_else_nop_endif
>> +#endif
>> +	.endm
>> +
>>  	.text
>>
>>  /*
>> @@ -612,6 +623,7 @@ el1_dbg:
>>  	cmp	x24, #ESR_ELx_EC_BRK64		// if BRK64
>>  	cinc	x24, x24, eq			// set bit '0'
>>  	tbz	x24, #0, el1_inv		// EL1 only
>> +	suspend_irq_prio_masking tmp=x21
>>  	mrs	x0, far_el1
>>  	mov	x2, sp				// struct pt_regs
>>  	bl	do_debug_exception
>> @@ -629,12 +641,10 @@ ENDPROC(el1_sync)
>>  	.align	6
>>  el1_irq:
>>  	kernel_entry 1
>> +	irq_entry_ignore_pmr pmr=x20, tmp=x0
> 
> What are the conditions for which we use 'suspend', and those we use
> 'ignore'? They obviously are very similar, and putting the rational
> wouldn't hurt.
> 

The conditions are probably the wrong ones. Or at least they don't
really warrant slapping names on them like I did.

I wanted to factor the assembly code by using helpers while avoiding to
add unnecessary instructions in said helpers. And since there are at
least two cases (one where we have to modify the current value of PMR
and others where we can just set an immediate value) I ended up with two
helpers, trying to put semantics onto them.

I'll try to get something cleaner.

> Of course, should you decide to adopt my naming suggestion above, some
> names need to change too... ;-)
> 
>>  	enable_da_f
>>
>>  #ifdef CONFIG_ARM64_PSEUDO_NMI
>> -alternative_if ARM64_HAS_IRQ_PRIO_MASKING
>> -	ldr	x20, [sp, #S_PMR_SAVE]
>> -alternative_else_nop_endif
>>  	test_irqs_unmasked	res=x0, pmr=x20
>>  	cbz	x0, 1f
>>  	bl	asm_nmi_enter
>> @@ -664,8 +674,9 @@ alternative_else_nop_endif
>>
>>  #ifdef CONFIG_ARM64_PSEUDO_NMI
>>  	/*
>> -	 * if IRQs were disabled when we received the interrupt, we have an NMI
>> -	 * and we are not re-enabling interrupt upon eret. Skip tracing.
>> +	 * When using IRQ priority masking, we can get spurious interrupts while
>> +	 * PMR is set to GIC_PRIO_IRQOFF. An NMI might also have occurred in a
>> +	 * section with interrupts disabled. Skip tracing in those cases.
>>  	 */
>>  	test_irqs_unmasked	res=x0, pmr=x20
>>  	cbz	x0, 1f
>> @@ -794,6 +805,7 @@ el0_ia:
>>  	 * Instruction abort handling
>>  	 */
>>  	mrs	x26, far_el1
>> +	suspend_irq_prio_masking tmp=x0
>>  	enable_da_f
>>  #ifdef CONFIG_TRACE_IRQFLAGS
>>  	bl	trace_hardirqs_off
>> @@ -839,6 +851,7 @@ el0_sp_pc:
>>  	 * Stack or PC alignment exception handling
>>  	 */
>>  	mrs	x26, far_el1
>> +	suspend_irq_prio_masking tmp=x0
>>  	enable_da_f
>>  #ifdef CONFIG_TRACE_IRQFLAGS
>>  	bl	trace_hardirqs_off
>> @@ -873,6 +886,7 @@ el0_dbg:
>>  	 * Debug exception handling
>>  	 */
>>  	tbnz	x24, #0, el0_inv		// EL0 only
>> +	suspend_irq_prio_masking tmp=x0
>>  	mrs	x0, far_el1
>>  	mov	x1, x25
>>  	mov	x2, sp
>> @@ -894,7 +908,9 @@ ENDPROC(el0_sync)
>>  el0_irq:
>>  	kernel_entry 0
>>  el0_irq_naked:
>> +	irq_entry_ignore_pmr pmr=x20, tmp=x0
>>  	enable_da_f
>> +
>>  #ifdef CONFIG_TRACE_IRQFLAGS
>>  	bl	trace_hardirqs_off
>>  #endif
>> @@ -916,6 +932,7 @@ ENDPROC(el0_irq)
>>  el1_error:
>>  	kernel_entry 1
>>  	mrs	x1, esr_el1
>> +	suspend_irq_prio_masking tmp=x0
>>  	enable_dbg
>>  	mov	x0, sp
>>  	bl	do_serror
>> @@ -926,6 +943,7 @@ el0_error:
>>  	kernel_entry 0
>>  el0_error_naked:
>>  	mrs	x1, esr_el1
>> +	suspend_irq_prio_masking tmp=x0
>>  	enable_dbg
>>  	mov	x0, sp
>>  	bl	do_serror
>> @@ -950,6 +968,7 @@ work_pending:
>>   */
>>  ret_to_user:
>>  	disable_daif
>> +	suspend_irq_prio_masking tmp=x0
>>  	ldr	x1, [tsk, #TSK_TI_FLAGS]
>>  	and	x2, x1, #_TIF_WORK_MASK
>>  	cbnz	x2, work_pending
>> @@ -966,6 +985,7 @@ ENDPROC(ret_to_user)
>>   */
>>  	.align	6
>>  el0_svc:
>> +	suspend_irq_prio_masking tmp=x1
>>  	mov	x0, sp
>>  	bl	el0_svc_handler
>>  	b	ret_to_user
>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>> index 3767fb2..f5bae97 100644
>> --- a/arch/arm64/kernel/process.c
>> +++ b/arch/arm64/kernel/process.c
>> @@ -94,7 +94,7 @@ static void __cpu_do_idle_irqprio(void)
>>  	 * be raised.
>>  	 */
>>  	pmr = gic_read_pmr();
>> -	gic_write_pmr(GIC_PRIO_IRQON);
>> +	gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
>>
>>  	__cpu_do_idle();
>>
>> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
>> index 824de70..2a6d0dd1 100644
>> --- a/arch/arm64/kernel/smp.c
>> +++ b/arch/arm64/kernel/smp.c
>> @@ -192,11 +192,13 @@ static void init_gic_priority_masking(void)
>>
>>  	WARN_ON(!(cpuflags & PSR_I_BIT));
>>
>> -	gic_write_pmr(GIC_PRIO_IRQOFF);
>> -
>>  	/* We can only unmask PSR.I if we can take aborts */
>> -	if (!(cpuflags & PSR_A_BIT))
>> +	if (!(cpuflags & PSR_A_BIT)) {
>> +		gic_write_pmr(GIC_PRIO_IRQOFF);
>>  		write_sysreg(cpuflags & ~PSR_I_BIT, daif);
>> +	} else {
>> +		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
>> +	}
>>  }
>>
>>  /*
>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>> index 3563fe6..083e319 100644
>> --- a/arch/arm64/kvm/hyp/switch.c
>> +++ b/arch/arm64/kvm/hyp/switch.c
>> @@ -533,7 +533,7 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
>>  	 * Naturally, we want to avoid this.
>>  	 */
>>  	if (system_uses_irq_prio_masking()) {
>> -		gic_write_pmr(GIC_PRIO_IRQON);
>> +		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
>>  		dsb(sy);
>>  	}
>>
>> --
>> 1.9.1
>>
> 
> Overall, I like the way it looks, and my comments are more cosmetic than
> anything else.
> 

Thanks,

-- 
Julien Thierry

Re: [PATCH v2 3/5] arm64: Fix incorrect irqflag restore for priority masking

[Julien Thierry]

On 07/05/2019 09:36, Marc Zyngier wrote:
> On 29/04/2019 17:00, Julien Thierry wrote:
>> When using IRQ priority masking to disable interrupts, in order to deal
>> with the PSR.I state, local_irq_save() would convert the I bit into a
>> PMR value (GIC_PRIO_IRQOFF). This resulted in local_irq_restore()
>> potentially modifying the value of PMR in undesired location due to the
>> state of PSR.I upon flag saving [1].
>>
>> In an attempt to solve this issue in a less hackish manner, introduce
>> a bit (GIC_PRIO_IGNORE_PMR) for the PMR values that can represent
>> whether PSR.I is being used to disable interrupts, in which case it
>> takes precedence of the status of interrupt masking via PMR.
>>
>> GIC_PRIO_IGNORE_PMR is chosen such that (<pmr_value> |
>> GIC_PRIO_IGNORE_PMR) does not mask more interrupts than <pmr_value> as
>> some sections (e.g. arch_cpu_idle(), interrupt acknowledge path)
>> requires PMR not to mask interrupts that could be signaled to the
>> CPU when using only PSR.I.
>>
>> [1] https://www.spinics.net/lists/arm-kernel/msg716956.html
>>
>> Fixes: commit 4a503217ce37 ("arm64: irqflags: Use ICC_PMR_EL1 for interrupt masking")
>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>> Reported-by: Zenghui Yu <yuzenghui@huawei.com>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Wei Li <liwei391@huawei.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Christoffer Dall <christoffer.dall@arm.com>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: James Morse <james.morse@arm.com>
>> Cc: Suzuki K Pouloze <suzuki.poulose@arm.com>
>> Cc: Oleg Nesterov <oleg@redhat.com>
>> ---
>>  arch/arm64/include/asm/arch_gicv3.h |  4 ++-
>>  arch/arm64/include/asm/assembler.h  |  9 +++++
>>  arch/arm64/include/asm/daifflags.h  | 22 ++++++++----
>>  arch/arm64/include/asm/irqflags.h   | 69 ++++++++++++++++---------------------
>>  arch/arm64/include/asm/kvm_host.h   |  4 ++-
>>  arch/arm64/include/asm/ptrace.h     | 10 ++++--
>>  arch/arm64/kernel/entry.S           | 30 +++++++++++++---
>>  arch/arm64/kernel/process.c         |  2 +-
>>  arch/arm64/kernel/smp.c             |  8 +++--
>>  arch/arm64/kvm/hyp/switch.c         |  2 +-
>>  10 files changed, 100 insertions(+), 60 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
>> index 14b41dd..3102c9a 100644
>> --- a/arch/arm64/include/asm/arch_gicv3.h
>> +++ b/arch/arm64/include/asm/arch_gicv3.h
>> @@ -163,7 +163,9 @@ static inline bool gic_prio_masking_enabled(void)
>>
>>  static inline void gic_pmr_mask_irqs(void)
>>  {
>> -	BUILD_BUG_ON(GICD_INT_DEF_PRI <= GIC_PRIO_IRQOFF);
>> +	BUILD_BUG_ON(GICD_INT_DEF_PRI < (GIC_PRIO_IRQOFF |
>> +					 GIC_PRIO_IGNORE_PMR));
>> +	BUILD_BUG_ON(GICD_INT_DEF_PRI >= GIC_PRIO_IRQON);
>>  	gic_write_pmr(GIC_PRIO_IRQOFF);
>>  }
>>
>> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
>> index c5308d0..601154d 100644
>> --- a/arch/arm64/include/asm/assembler.h
>> +++ b/arch/arm64/include/asm/assembler.h
>> @@ -62,6 +62,15 @@
>>  	msr	daifclr, #(8 | 4 | 1)
>>  	.endm
>>
>> +	.macro	suspend_irq_prio_masking, tmp:req
>> +#ifdef CONFIG_ARM64_PSEUDO_NMI
>> +	alternative_if ARM64_HAS_IRQ_PRIO_MASKING
>> +	mov	\tmp, #(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR)
>> +	msr_s	SYS_ICC_PMR_EL1, \tmp
>> +	alternative_else_nop_endif
>> +#endif
>> +	.endm
>> +
>>  /*
>>   * Save/restore interrupts.
>>   */
>> diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
>> index db452aa..a32ece9 100644
>> --- a/arch/arm64/include/asm/daifflags.h
>> +++ b/arch/arm64/include/asm/daifflags.h
>> @@ -18,6 +18,7 @@
>>
>>  #include <linux/irqflags.h>
>>
>> +#include <asm/arch_gicv3.h>
>>  #include <asm/cpufeature.h>
>>
>>  #define DAIF_PROCCTX		0
>> @@ -32,6 +33,11 @@ static inline void local_daif_mask(void)
>>  		:
>>  		:
>>  		: "memory");
>> +
>> +	/* Don't really care for a dsb here, we don't intend to enable IRQs */
>> +	if (system_uses_irq_prio_masking())
>> +		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
>> +
>>  	trace_hardirqs_off();
>>  }
>>
>> @@ -43,7 +49,7 @@ static inline unsigned long local_daif_save(void)
>>
>>  	if (system_uses_irq_prio_masking()) {
>>  		/* If IRQs are masked with PMR, reflect it in the flags */
>> -		if (read_sysreg_s(SYS_ICC_PMR_EL1) <= GIC_PRIO_IRQOFF)
>> +		if (read_sysreg_s(SYS_ICC_PMR_EL1) != GIC_PRIO_IRQON)
>>  			flags |= PSR_I_BIT;
>>  	}
>>
>> @@ -59,14 +65,16 @@ static inline void local_daif_restore(unsigned long flags)
>>  	if (!irq_disabled) {
>>  		trace_hardirqs_on();
>>
>> -		if (system_uses_irq_prio_masking())
>> -			arch_local_irq_enable();
>> -	} else if (!(flags & PSR_A_BIT)) {
>> +		if (system_uses_irq_prio_masking()) {
>> +			gic_write_pmr(GIC_PRIO_IRQON);
>> +			dsb(sy);
>> +		}
>> +	} else if (system_uses_irq_prio_masking()) {
>>  		/*
>>  		 * If interrupts are disabled but we can take
>>  		 * asynchronous errors, we can take NMIs
>>  		 */
>> -		if (system_uses_irq_prio_masking()) {
>> +		if (!(flags & PSR_A_BIT)) {
>>  			flags &= ~PSR_I_BIT;
>>  			/*
>>  			 * There has been concern that the write to daif
>> @@ -87,7 +95,9 @@ static inline void local_daif_restore(unsigned long flags)
>>  			 *
>>  			 * So we don't need additional synchronization here.
>>  			 */
>> -			arch_local_irq_disable();
>> +			gic_write_pmr(GIC_PRIO_IRQOFF);
>> +		} else {
>> +			gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
>>  		}
> 
> Nit: move the write to PMR outside of the if/else clause with an
> intermediate variable to hold the PMR value. Not a big deal, but I think
> it helps with the nesting.
> 

Done.

>>  	}
>>
>> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
>> index 43d8366..516cdfc 100644
>> --- a/arch/arm64/include/asm/irqflags.h
>> +++ b/arch/arm64/include/asm/irqflags.h
>> @@ -67,43 +67,48 @@ static inline void arch_local_irq_disable(void)
>>   */
>>  static inline unsigned long arch_local_save_flags(void)
>>  {
>> -	unsigned long daif_bits;
>>  	unsigned long flags;
>>
>> -	daif_bits = read_sysreg(daif);
>> -
>> -	/*
>> -	 * The asm is logically equivalent to:
>> -	 *
>> -	 * if (system_uses_irq_prio_masking())
>> -	 *	flags = (daif_bits & PSR_I_BIT) ?
>> -	 *			GIC_PRIO_IRQOFF :
>> -	 *			read_sysreg_s(SYS_ICC_PMR_EL1);
>> -	 * else
>> -	 *	flags = daif_bits;
>> -	 */
>>  	asm volatile(ALTERNATIVE(
>> -			"mov	%0, %1\n"
>> -			"nop\n"
>> -			"nop",
>> -			"mrs_s	%0, " __stringify(SYS_ICC_PMR_EL1) "\n"
>> -			"ands	%1, %1, " __stringify(PSR_I_BIT) "\n"
>> -			"csel	%0, %0, %2, eq",
>> -			ARM64_HAS_IRQ_PRIO_MASKING)
>> -		: "=&r" (flags), "+r" (daif_bits)
>> -		: "r" ((unsigned long) GIC_PRIO_IRQOFF)
>> +		"mrs	%0, daif",
>> +		"mrs_s	%0, " __stringify(SYS_ICC_PMR_EL1) "\n",
>> +		ARM64_HAS_IRQ_PRIO_MASKING)
>> +		: "=&r" (flags)
>> +		:
>>  		: "memory");
>>
>>  	return flags;
>>  }
>>
>> +static inline int arch_irqs_disabled_flags(unsigned long flags)
>> +{
>> +	int res;
>> +
>> +	asm volatile(ALTERNATIVE(
>> +		"and	%w0, %w1, #" __stringify(PSR_I_BIT) "\n"
>> +		"nop",
>> +		"cmp	%w1, #" __stringify(GIC_PRIO_IRQON) "\n"
>> +		"cset	%w0, ne",
>> +		ARM64_HAS_IRQ_PRIO_MASKING)
>> +		: "=&r" (res)
>> +		: "r" ((int) flags)
>> +		: "memory");
> 
> I wonder if this should have "cc" as part of the clobber list.

Is there any special semantic to "cc" on arm64? All I can find is that
in the general case it indicates that it is modifying the "flags" register.

Is your suggestion only for the PMR case? Or is it something that we
should add regardless of PMR?
The latter makes sense to me, but for the former, I fail to understand
why this should affect only PMR.

> 
>> +
>> +	return res;
>> +}
>> +
>>  static inline unsigned long arch_local_irq_save(void)
>>  {
>>  	unsigned long flags;
>>
>>  	flags = arch_local_save_flags();
>>
>> -	arch_local_irq_disable();
>> +	/*
>> +	 * There are too many states with IRQs disabled, just keep the current
>> +	 * state if interrupts are already disabled/masked.
>> +	 */
>> +	if (!arch_irqs_disabled_flags(flags))
>> +		arch_local_irq_disable();
>>
>>  	return flags;
>>  }
>> @@ -119,26 +124,10 @@ static inline void arch_local_irq_restore(unsigned long flags)
>>  			"msr_s	" __stringify(SYS_ICC_PMR_EL1) ", %0\n"
>>  			"dsb	sy",
>>  			ARM64_HAS_IRQ_PRIO_MASKING)
>> -		: "+r" (flags)
>>  		:
>> +		: "r" (flags)
>>  		: "memory");
>>  }
>>
>> -static inline int arch_irqs_disabled_flags(unsigned long flags)
>> -{
>> -	int res;
>> -
>> -	asm volatile(ALTERNATIVE(
>> -			"and	%w0, %w1, #" __stringify(PSR_I_BIT) "\n"
>> -			"nop",
>> -			"cmp	%w1, #" __stringify(GIC_PRIO_IRQOFF) "\n"
>> -			"cset	%w0, ls",
>> -			ARM64_HAS_IRQ_PRIO_MASKING)
>> -		: "=&r" (res)
>> -		: "r" ((int) flags)
>> -		: "memory");
>> -
>> -	return res;
>> -}
>>  #endif
>>  #endif
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index a01fe087..0ec398c 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -532,9 +532,11 @@ static inline void kvm_arm_vhe_guest_enter(void)
>>  	 * will not signal the CPU of interrupts of lower priority, and the
>>  	 * only way to get out will be via guest exceptions.
>>  	 * Naturally, we want to avoid this.
>> +	 *
>> +	 * local_daif_mask() already sets IGNORE_PMR, we just need a
>> +	 * dsb to ensure the redistributor is forwards EL2 IRQs to the CPU.
>>  	 */
>>  	if (system_uses_irq_prio_masking()) {
>> -		gic_write_pmr(GIC_PRIO_IRQON);
>>  		dsb(sy);
>>  	}
> 
> nit: drop the (now superfluous) brackets.
> 

Done.

>>  }
>> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
>> index ec60174..7861a5d 100644
>> --- a/arch/arm64/include/asm/ptrace.h
>> +++ b/arch/arm64/include/asm/ptrace.h
>> @@ -35,9 +35,15 @@
>>   * means masking more IRQs (or at least that the same IRQs remain masked).
>>   *
>>   * To mask interrupts, we clear the most significant bit of PMR.
>> + *
>> + * Some code sections either automatically switch back to PSR.I or explicitly
>> + * require to not use priority masking. If bit GIC_PRIO_IGNORE_PMR is included
>> + * in the  the priority mask, it indicates that PSR.I should be set and
>> + * interrupt disabling temporarily does not rely on IRQ priorities.
>>   */
>> -#define GIC_PRIO_IRQON		0xf0
>> -#define GIC_PRIO_IRQOFF		(GIC_PRIO_IRQON & ~0x80)
>> +#define GIC_PRIO_IRQON			0xc0
>> +#define GIC_PRIO_IRQOFF			(GIC_PRIO_IRQON & ~0x80)
>> +#define GIC_PRIO_IGNORE_PMR		(1 << 4)
> 
> The naming of this last value is still a bit odd: it says "ignore PMR",
> and yet it only works because it is a valid PMR value. So it is not so
> much that PMR is not used (it is), but that PSTATE.I is set, thus
> overriding the interrupt signaling.
> 
> How about something like GIC_PRIO_PSTATE_I_SET, which has the advantage
> of exactly matching the above comment?
> 

Sounds good to me, also depends on how I go with the next comment.

>>
>>  /* Additional SPSR bits not exposed in the UABI */
>>  #define PSR_IL_BIT		(1 << 20)
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index 00c1f21..e6df781 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -258,6 +258,7 @@ alternative_else_nop_endif
>>  	/*
>>  	 * Registers that may be useful after this macro is invoked:
>>  	 *
>> +	 * x20 - ICC_PMR_EL1
>>  	 * x21 - aborted SP
>>  	 * x22 - aborted PC
>>  	 * x23 - aborted PSTATE
>> @@ -434,6 +435,16 @@ alternative_endif
>>  	.endm
>>  #endif
>>
>> +
>> +	.macro irq_entry_ignore_pmr, pmr:req, tmp:req
>> +#ifdef CONFIG_ARM64_PSEUDO_NMI
>> +	alternative_if ARM64_HAS_IRQ_PRIO_MASKING
>> +	orr	\tmp, \pmr, #GIC_PRIO_IGNORE_PMR
>> +	msr_s	SYS_ICC_PMR_EL1, \tmp
>> +	alternative_else_nop_endif
>> +#endif
>> +	.endm
>> +
>>  	.text
>>
>>  /*
>> @@ -612,6 +623,7 @@ el1_dbg:
>>  	cmp	x24, #ESR_ELx_EC_BRK64		// if BRK64
>>  	cinc	x24, x24, eq			// set bit '0'
>>  	tbz	x24, #0, el1_inv		// EL1 only
>> +	suspend_irq_prio_masking tmp=x21
>>  	mrs	x0, far_el1
>>  	mov	x2, sp				// struct pt_regs
>>  	bl	do_debug_exception
>> @@ -629,12 +641,10 @@ ENDPROC(el1_sync)
>>  	.align	6
>>  el1_irq:
>>  	kernel_entry 1
>> +	irq_entry_ignore_pmr pmr=x20, tmp=x0
> 
> What are the conditions for which we use 'suspend', and those we use
> 'ignore'? They obviously are very similar, and putting the rational
> wouldn't hurt.
> 

The conditions are probably the wrong ones. Or at least they don't
really warrant slapping names on them like I did.

I wanted to factor the assembly code by using helpers while avoiding to
add unnecessary instructions in said helpers. And since there are at
least two cases (one where we have to modify the current value of PMR
and others where we can just set an immediate value) I ended up with two
helpers, trying to put semantics onto them.

I'll try to get something cleaner.

> Of course, should you decide to adopt my naming suggestion above, some
> names need to change too... ;-)
> 
>>  	enable_da_f
>>
>>  #ifdef CONFIG_ARM64_PSEUDO_NMI
>> -alternative_if ARM64_HAS_IRQ_PRIO_MASKING
>> -	ldr	x20, [sp, #S_PMR_SAVE]
>> -alternative_else_nop_endif
>>  	test_irqs_unmasked	res=x0, pmr=x20
>>  	cbz	x0, 1f
>>  	bl	asm_nmi_enter
>> @@ -664,8 +674,9 @@ alternative_else_nop_endif
>>
>>  #ifdef CONFIG_ARM64_PSEUDO_NMI
>>  	/*
>> -	 * if IRQs were disabled when we received the interrupt, we have an NMI
>> -	 * and we are not re-enabling interrupt upon eret. Skip tracing.
>> +	 * When using IRQ priority masking, we can get spurious interrupts while
>> +	 * PMR is set to GIC_PRIO_IRQOFF. An NMI might also have occurred in a
>> +	 * section with interrupts disabled. Skip tracing in those cases.
>>  	 */
>>  	test_irqs_unmasked	res=x0, pmr=x20
>>  	cbz	x0, 1f
>> @@ -794,6 +805,7 @@ el0_ia:
>>  	 * Instruction abort handling
>>  	 */
>>  	mrs	x26, far_el1
>> +	suspend_irq_prio_masking tmp=x0
>>  	enable_da_f
>>  #ifdef CONFIG_TRACE_IRQFLAGS
>>  	bl	trace_hardirqs_off
>> @@ -839,6 +851,7 @@ el0_sp_pc:
>>  	 * Stack or PC alignment exception handling
>>  	 */
>>  	mrs	x26, far_el1
>> +	suspend_irq_prio_masking tmp=x0
>>  	enable_da_f
>>  #ifdef CONFIG_TRACE_IRQFLAGS
>>  	bl	trace_hardirqs_off
>> @@ -873,6 +886,7 @@ el0_dbg:
>>  	 * Debug exception handling
>>  	 */
>>  	tbnz	x24, #0, el0_inv		// EL0 only
>> +	suspend_irq_prio_masking tmp=x0
>>  	mrs	x0, far_el1
>>  	mov	x1, x25
>>  	mov	x2, sp
>> @@ -894,7 +908,9 @@ ENDPROC(el0_sync)
>>  el0_irq:
>>  	kernel_entry 0
>>  el0_irq_naked:
>> +	irq_entry_ignore_pmr pmr=x20, tmp=x0
>>  	enable_da_f
>> +
>>  #ifdef CONFIG_TRACE_IRQFLAGS
>>  	bl	trace_hardirqs_off
>>  #endif
>> @@ -916,6 +932,7 @@ ENDPROC(el0_irq)
>>  el1_error:
>>  	kernel_entry 1
>>  	mrs	x1, esr_el1
>> +	suspend_irq_prio_masking tmp=x0
>>  	enable_dbg
>>  	mov	x0, sp
>>  	bl	do_serror
>> @@ -926,6 +943,7 @@ el0_error:
>>  	kernel_entry 0
>>  el0_error_naked:
>>  	mrs	x1, esr_el1
>> +	suspend_irq_prio_masking tmp=x0
>>  	enable_dbg
>>  	mov	x0, sp
>>  	bl	do_serror
>> @@ -950,6 +968,7 @@ work_pending:
>>   */
>>  ret_to_user:
>>  	disable_daif
>> +	suspend_irq_prio_masking tmp=x0
>>  	ldr	x1, [tsk, #TSK_TI_FLAGS]
>>  	and	x2, x1, #_TIF_WORK_MASK
>>  	cbnz	x2, work_pending
>> @@ -966,6 +985,7 @@ ENDPROC(ret_to_user)
>>   */
>>  	.align	6
>>  el0_svc:
>> +	suspend_irq_prio_masking tmp=x1
>>  	mov	x0, sp
>>  	bl	el0_svc_handler
>>  	b	ret_to_user
>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>> index 3767fb2..f5bae97 100644
>> --- a/arch/arm64/kernel/process.c
>> +++ b/arch/arm64/kernel/process.c
>> @@ -94,7 +94,7 @@ static void __cpu_do_idle_irqprio(void)
>>  	 * be raised.
>>  	 */
>>  	pmr = gic_read_pmr();
>> -	gic_write_pmr(GIC_PRIO_IRQON);
>> +	gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
>>
>>  	__cpu_do_idle();
>>
>> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
>> index 824de70..2a6d0dd1 100644
>> --- a/arch/arm64/kernel/smp.c
>> +++ b/arch/arm64/kernel/smp.c
>> @@ -192,11 +192,13 @@ static void init_gic_priority_masking(void)
>>
>>  	WARN_ON(!(cpuflags & PSR_I_BIT));
>>
>> -	gic_write_pmr(GIC_PRIO_IRQOFF);
>> -
>>  	/* We can only unmask PSR.I if we can take aborts */
>> -	if (!(cpuflags & PSR_A_BIT))
>> +	if (!(cpuflags & PSR_A_BIT)) {
>> +		gic_write_pmr(GIC_PRIO_IRQOFF);
>>  		write_sysreg(cpuflags & ~PSR_I_BIT, daif);
>> +	} else {
>> +		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
>> +	}
>>  }
>>
>>  /*
>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>> index 3563fe6..083e319 100644
>> --- a/arch/arm64/kvm/hyp/switch.c
>> +++ b/arch/arm64/kvm/hyp/switch.c
>> @@ -533,7 +533,7 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
>>  	 * Naturally, we want to avoid this.
>>  	 */
>>  	if (system_uses_irq_prio_masking()) {
>> -		gic_write_pmr(GIC_PRIO_IRQON);
>> +		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
>>  		dsb(sy);
>>  	}
>>
>> --
>> 1.9.1
>>
> 
> Overall, I like the way it looks, and my comments are more cosmetic than
> anything else.
> 

Thanks,

-- 
Julien Thierry

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

Re: [PATCH v2 3/5] arm64: Fix incorrect irqflag restore for priority masking

[Robin Murphy]

On 14/05/2019 10:25, Julien Thierry wrote:
[...]
>>> +static inline int arch_irqs_disabled_flags(unsigned long flags)
>>> +{
>>> +	int res;
>>> +
>>> +	asm volatile(ALTERNATIVE(
>>> +		"and	%w0, %w1, #" __stringify(PSR_I_BIT) "\n"
>>> +		"nop",
>>> +		"cmp	%w1, #" __stringify(GIC_PRIO_IRQON) "\n"
>>> +		"cset	%w0, ne",
>>> +		ARM64_HAS_IRQ_PRIO_MASKING)
>>> +		: "=&r" (res)
>>> +		: "r" ((int) flags)
>>> +		: "memory");
>>
>> I wonder if this should have "cc" as part of the clobber list.
> 
> Is there any special semantic to "cc" on arm64? All I can find is that
> in the general case it indicates that it is modifying the "flags" register.
> 
> Is your suggestion only for the PMR case? Or is it something that we
> should add regardless of PMR?
> The latter makes sense to me, but for the former, I fail to understand
> why this should affect only PMR.

The PMR case really ought to have have a cc clobber, because who knows 
what this may end up inlined into, and compilers can get pretty 
aggressive with instruction scheduling in ways which leave a live value 
in CPSR across sizeable chunks of other code. It's true that the non-PMR 
case doesn't need it, but the surrounding code still needs to be 
generated to accommodate both possible versions of the alternative. From 
the look of the rest of the patch, the existing pseudo-NMI code has this 
bug in a few places.

Technically you could omit it when ARM64_PSEUDO_NMI is configured out 
entirely, but at that point you may as well omit the whole alternative 
as well. It's probably not worth the bother unless it proves to have a 
significant impact on codegen overall. On which note the memory clobber 
also seems superfluous either way :/

That said, now that I've been looking at it for this long, if the aim is 
just to create a zero/nonzero value then couldn't the PMR case just be 
"eor %w0, %w1, #GIC_PRIO_IRQON" and avoid the need for clobbers at all?

Robin.

Re: [PATCH v2 3/5] arm64: Fix incorrect irqflag restore for priority masking

[Robin Murphy]

On 14/05/2019 10:25, Julien Thierry wrote:
[...]
>>> +static inline int arch_irqs_disabled_flags(unsigned long flags)
>>> +{
>>> +	int res;
>>> +
>>> +	asm volatile(ALTERNATIVE(
>>> +		"and	%w0, %w1, #" __stringify(PSR_I_BIT) "\n"
>>> +		"nop",
>>> +		"cmp	%w1, #" __stringify(GIC_PRIO_IRQON) "\n"
>>> +		"cset	%w0, ne",
>>> +		ARM64_HAS_IRQ_PRIO_MASKING)
>>> +		: "=&r" (res)
>>> +		: "r" ((int) flags)
>>> +		: "memory");
>>
>> I wonder if this should have "cc" as part of the clobber list.
> 
> Is there any special semantic to "cc" on arm64? All I can find is that
> in the general case it indicates that it is modifying the "flags" register.
> 
> Is your suggestion only for the PMR case? Or is it something that we
> should add regardless of PMR?
> The latter makes sense to me, but for the former, I fail to understand
> why this should affect only PMR.

The PMR case really ought to have have a cc clobber, because who knows 
what this may end up inlined into, and compilers can get pretty 
aggressive with instruction scheduling in ways which leave a live value 
in CPSR across sizeable chunks of other code. It's true that the non-PMR 
case doesn't need it, but the surrounding code still needs to be 
generated to accommodate both possible versions of the alternative. From 
the look of the rest of the patch, the existing pseudo-NMI code has this 
bug in a few places.

Technically you could omit it when ARM64_PSEUDO_NMI is configured out 
entirely, but at that point you may as well omit the whole alternative 
as well. It's probably not worth the bother unless it proves to have a 
significant impact on codegen overall. On which note the memory clobber 
also seems superfluous either way :/

That said, now that I've been looking at it for this long, if the aim is 
just to create a zero/nonzero value then couldn't the PMR case just be 
"eor %w0, %w1, #GIC_PRIO_IRQON" and avoid the need for clobbers at all?

Robin.

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

Re: [PATCH v2 3/5] arm64: Fix incorrect irqflag restore for priority masking

[Julien Thierry]

On 14/05/2019 13:01, Robin Murphy wrote:
> On 14/05/2019 10:25, Julien Thierry wrote:
> [...]
>>>> +static inline int arch_irqs_disabled_flags(unsigned long flags)
>>>> +{
>>>> +    int res;
>>>> +
>>>> +    asm volatile(ALTERNATIVE(
>>>> +        "and    %w0, %w1, #" __stringify(PSR_I_BIT) "\n"
>>>> +        "nop",
>>>> +        "cmp    %w1, #" __stringify(GIC_PRIO_IRQON) "\n"
>>>> +        "cset    %w0, ne",
>>>> +        ARM64_HAS_IRQ_PRIO_MASKING)
>>>> +        : "=&r" (res)
>>>> +        : "r" ((int) flags)
>>>> +        : "memory");
>>>
>>> I wonder if this should have "cc" as part of the clobber list.
>>
>> Is there any special semantic to "cc" on arm64? All I can find is that
>> in the general case it indicates that it is modifying the "flags"
>> register.
>>
>> Is your suggestion only for the PMR case? Or is it something that we
>> should add regardless of PMR?
>> The latter makes sense to me, but for the former, I fail to understand
>> why this should affect only PMR.
> 
> The PMR case really ought to have have a cc clobber, because who knows
> what this may end up inlined into, and compilers can get pretty
> aggressive with instruction scheduling in ways which leave a live value
> in CPSR across sizeable chunks of other code. It's true that the non-PMR
> case doesn't need it, but the surrounding code still needs to be
> generated to accommodate both possible versions of the alternative. From
> the look of the rest of the patch, the existing pseudo-NMI code has this
> bug in a few places.
> 
> Technically you could omit it when ARM64_PSEUDO_NMI is configured out
> entirely, but at that point you may as well omit the whole alternative
> as well. It's probably not worth the bother unless it proves to have a
> significant impact on codegen overall. On which note the memory clobber
> also seems superfluous either way :/
> 

Right, I see. I misunderstood what was meant by "cc" indicating that the
assembly modified the flags.

Due to the context I interpreted it as irqflags whereas it concerns the
condition flags (hence the 'c' I presume...). It all makes more sense now.

> That said, now that I've been looking at it for this long, if the aim is
> just to create a zero/nonzero value then couldn't the PMR case just be
> "eor %w0, %w1, #GIC_PRIO_IRQON" and avoid the need for clobbers at all?
> 

Yes, definitely seems like it would be better! I'll take that
suggestion, thanks.

Cheers,

-- 
Julien Thierry

Re: [PATCH v2 3/5] arm64: Fix incorrect irqflag restore for priority masking

[Julien Thierry]

On 14/05/2019 13:01, Robin Murphy wrote:
> On 14/05/2019 10:25, Julien Thierry wrote:
> [...]
>>>> +static inline int arch_irqs_disabled_flags(unsigned long flags)
>>>> +{
>>>> +    int res;
>>>> +
>>>> +    asm volatile(ALTERNATIVE(
>>>> +        "and    %w0, %w1, #" __stringify(PSR_I_BIT) "\n"
>>>> +        "nop",
>>>> +        "cmp    %w1, #" __stringify(GIC_PRIO_IRQON) "\n"
>>>> +        "cset    %w0, ne",
>>>> +        ARM64_HAS_IRQ_PRIO_MASKING)
>>>> +        : "=&r" (res)
>>>> +        : "r" ((int) flags)
>>>> +        : "memory");
>>>
>>> I wonder if this should have "cc" as part of the clobber list.
>>
>> Is there any special semantic to "cc" on arm64? All I can find is that
>> in the general case it indicates that it is modifying the "flags"
>> register.
>>
>> Is your suggestion only for the PMR case? Or is it something that we
>> should add regardless of PMR?
>> The latter makes sense to me, but for the former, I fail to understand
>> why this should affect only PMR.
> 
> The PMR case really ought to have have a cc clobber, because who knows
> what this may end up inlined into, and compilers can get pretty
> aggressive with instruction scheduling in ways which leave a live value
> in CPSR across sizeable chunks of other code. It's true that the non-PMR
> case doesn't need it, but the surrounding code still needs to be
> generated to accommodate both possible versions of the alternative. From
> the look of the rest of the patch, the existing pseudo-NMI code has this
> bug in a few places.
> 
> Technically you could omit it when ARM64_PSEUDO_NMI is configured out
> entirely, but at that point you may as well omit the whole alternative
> as well. It's probably not worth the bother unless it proves to have a
> significant impact on codegen overall. On which note the memory clobber
> also seems superfluous either way :/
> 

Right, I see. I misunderstood what was meant by "cc" indicating that the
assembly modified the flags.

Due to the context I interpreted it as irqflags whereas it concerns the
condition flags (hence the 'c' I presume...). It all makes more sense now.

> That said, now that I've been looking at it for this long, if the aim is
> just to create a zero/nonzero value then couldn't the PMR case just be
> "eor %w0, %w1, #GIC_PRIO_IRQON" and avoid the need for clobbers at all?
> 

Yes, definitely seems like it would be better! I'll take that
suggestion, thanks.

Cheers,

-- 
Julien Thierry

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

[PATCH v2 4/5] arm64: irqflags: Introduce explicit debugging for IRQ priorities

[Julien Thierry]

Using IRQ priority masking to enable/disable interrupts is a bit
sensitive as it requires to deal with both ICC_PMR_EL1 and PSR.I.

Introduce some validity checks to both highlight the states in which
functions dealing with IRQ enabling/disabling can (not) be called, and
bark a warning when called in an unexpected state.

Since these checks are done on hotpaths, introduce a build option to
choose whether to do the checking.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/Kconfig                 | 11 +++++++++++
 arch/arm64/include/asm/daifflags.h |  9 +++++++++
 arch/arm64/include/asm/irqflags.h  | 14 ++++++++++++++
 3 files changed, 34 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 7e34b9e..3fb38f3 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1359,6 +1359,17 @@ config ARM64_PSEUDO_NMI

 	  If unsure, say N

+if ARM64_PSEUDO_NMI
+config ARM64_DEBUG_PRIORITY_MASKING
+	bool "Debug interrupt priority masking"
+	help
+	  This adds runtime checks to functions enabling/disabling
+	  interrupts when using priority masking. The additional checks verify
+	  the validity of ICC_PMR_EL1 when calling concerned functions.
+
+	  If unsure, say N
+endif
+
 config RELOCATABLE
 	bool
 	help
diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
index a32ece9..9512968 100644
--- a/arch/arm64/include/asm/daifflags.h
+++ b/arch/arm64/include/asm/daifflags.h
@@ -28,6 +28,11 @@
 /* mask/save/unmask/restore all exceptions, including interrupts. */
 static inline void local_daif_mask(void)
 {
+	WARN_ON(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
+		system_uses_irq_prio_masking() &&
+		(read_sysreg_s(SYS_ICC_PMR_EL1) == (GIC_PRIO_IRQOFF |
+						    GIC_PRIO_IGNORE_PMR)));
+
 	asm volatile(
 		"msr	daifset, #0xf		// local_daif_mask\n"
 		:
@@ -62,6 +67,10 @@ static inline void local_daif_restore(unsigned long flags)
 {
 	bool irq_disabled = flags & PSR_I_BIT;

+	WARN_ON(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
+		system_uses_irq_prio_masking() &&
+		!(read_sysreg(daif) & PSR_I_BIT));
+
 	if (!irq_disabled) {
 		trace_hardirqs_on();

diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
index 516cdfc..a40abc2 100644
--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h
@@ -40,6 +40,13 @@
  */
 static inline void arch_local_irq_enable(void)
 {
+	if (IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
+	    system_uses_irq_prio_masking()) {
+		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
+
+		WARN_ON(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
+	}
+
 	asm volatile(ALTERNATIVE(
 		"msr	daifclr, #2		// arch_local_irq_enable\n"
 		"nop",
@@ -53,6 +60,13 @@ static inline void arch_local_irq_enable(void)

 static inline void arch_local_irq_disable(void)
 {
+	if (IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
+	    system_uses_irq_prio_masking()) {
+		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
+
+		WARN_ON(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
+	}
+
 	asm volatile(ALTERNATIVE(
 		"msr	daifset, #2		// arch_local_irq_disable",
 		"msr_s  " __stringify(SYS_ICC_PMR_EL1) ", %0",
--
1.9.1

[PATCH v2 4/5] arm64: irqflags: Introduce explicit debugging for IRQ priorities

[Julien Thierry]

Using IRQ priority masking to enable/disable interrupts is a bit
sensitive as it requires to deal with both ICC_PMR_EL1 and PSR.I.

Introduce some validity checks to both highlight the states in which
functions dealing with IRQ enabling/disabling can (not) be called, and
bark a warning when called in an unexpected state.

Since these checks are done on hotpaths, introduce a build option to
choose whether to do the checking.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/Kconfig                 | 11 +++++++++++
 arch/arm64/include/asm/daifflags.h |  9 +++++++++
 arch/arm64/include/asm/irqflags.h  | 14 ++++++++++++++
 3 files changed, 34 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 7e34b9e..3fb38f3 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1359,6 +1359,17 @@ config ARM64_PSEUDO_NMI

 	  If unsure, say N

+if ARM64_PSEUDO_NMI
+config ARM64_DEBUG_PRIORITY_MASKING
+	bool "Debug interrupt priority masking"
+	help
+	  This adds runtime checks to functions enabling/disabling
+	  interrupts when using priority masking. The additional checks verify
+	  the validity of ICC_PMR_EL1 when calling concerned functions.
+
+	  If unsure, say N
+endif
+
 config RELOCATABLE
 	bool
 	help
diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
index a32ece9..9512968 100644
--- a/arch/arm64/include/asm/daifflags.h
+++ b/arch/arm64/include/asm/daifflags.h
@@ -28,6 +28,11 @@
 /* mask/save/unmask/restore all exceptions, including interrupts. */
 static inline void local_daif_mask(void)
 {
+	WARN_ON(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
+		system_uses_irq_prio_masking() &&
+		(read_sysreg_s(SYS_ICC_PMR_EL1) == (GIC_PRIO_IRQOFF |
+						    GIC_PRIO_IGNORE_PMR)));
+
 	asm volatile(
 		"msr	daifset, #0xf		// local_daif_mask\n"
 		:
@@ -62,6 +67,10 @@ static inline void local_daif_restore(unsigned long flags)
 {
 	bool irq_disabled = flags & PSR_I_BIT;

+	WARN_ON(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
+		system_uses_irq_prio_masking() &&
+		!(read_sysreg(daif) & PSR_I_BIT));
+
 	if (!irq_disabled) {
 		trace_hardirqs_on();

diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
index 516cdfc..a40abc2 100644
--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h
@@ -40,6 +40,13 @@
  */
 static inline void arch_local_irq_enable(void)
 {
+	if (IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
+	    system_uses_irq_prio_masking()) {
+		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
+
+		WARN_ON(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
+	}
+
 	asm volatile(ALTERNATIVE(
 		"msr	daifclr, #2		// arch_local_irq_enable\n"
 		"nop",
@@ -53,6 +60,13 @@ static inline void arch_local_irq_enable(void)

 static inline void arch_local_irq_disable(void)
 {
+	if (IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
+	    system_uses_irq_prio_masking()) {
+		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
+
+		WARN_ON(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
+	}
+
 	asm volatile(ALTERNATIVE(
 		"msr	daifset, #2		// arch_local_irq_disable",
 		"msr_s  " __stringify(SYS_ICC_PMR_EL1) ", %0",
--
1.9.1

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

Re: [PATCH v2 4/5] arm64: irqflags: Introduce explicit debugging for IRQ priorities

[Marc Zyngier]

On 29/04/2019 17:00, Julien Thierry wrote:
> Using IRQ priority masking to enable/disable interrupts is a bit
> sensitive as it requires to deal with both ICC_PMR_EL1 and PSR.I.
> 
> Introduce some validity checks to both highlight the states in which
> functions dealing with IRQ enabling/disabling can (not) be called, and
> bark a warning when called in an unexpected state.
> 
> Since these checks are done on hotpaths, introduce a build option to
> choose whether to do the checking.
> 
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/Kconfig                 | 11 +++++++++++
>  arch/arm64/include/asm/daifflags.h |  9 +++++++++
>  arch/arm64/include/asm/irqflags.h  | 14 ++++++++++++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 7e34b9e..3fb38f3 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1359,6 +1359,17 @@ config ARM64_PSEUDO_NMI
> 
>  	  If unsure, say N
> 
> +if ARM64_PSEUDO_NMI
> +config ARM64_DEBUG_PRIORITY_MASKING
> +	bool "Debug interrupt priority masking"
> +	help
> +	  This adds runtime checks to functions enabling/disabling
> +	  interrupts when using priority masking. The additional checks verify
> +	  the validity of ICC_PMR_EL1 when calling concerned functions.
> +
> +	  If unsure, say N
> +endif
> +
>  config RELOCATABLE
>  	bool
>  	help
> diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
> index a32ece9..9512968 100644
> --- a/arch/arm64/include/asm/daifflags.h
> +++ b/arch/arm64/include/asm/daifflags.h
> @@ -28,6 +28,11 @@
>  /* mask/save/unmask/restore all exceptions, including interrupts. */
>  static inline void local_daif_mask(void)
>  {
> +	WARN_ON(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
> +		system_uses_irq_prio_masking() &&

Given that you repeat these conditions all over the place, how about a
helper:

#define DEBUG_PRIORITY_MASKING_CHECK(x)			\
	(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) && \
	 system_uses_irq_prio_masking() && (x))

or some variant thereof.

> +		(read_sysreg_s(SYS_ICC_PMR_EL1) == (GIC_PRIO_IRQOFF |
> +						    GIC_PRIO_IGNORE_PMR)));
> +
>  	asm volatile(
>  		"msr	daifset, #0xf		// local_daif_mask\n"
>  		:
> @@ -62,6 +67,10 @@ static inline void local_daif_restore(unsigned long flags)
>  {
>  	bool irq_disabled = flags & PSR_I_BIT;
> 
> +	WARN_ON(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
> +		system_uses_irq_prio_masking() &&
> +		!(read_sysreg(daif) & PSR_I_BIT));
> +
>  	if (!irq_disabled) {
>  		trace_hardirqs_on();
> 
> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
> index 516cdfc..a40abc2 100644
> --- a/arch/arm64/include/asm/irqflags.h
> +++ b/arch/arm64/include/asm/irqflags.h
> @@ -40,6 +40,13 @@
>   */
>  static inline void arch_local_irq_enable(void)
>  {
> +	if (IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
> +	    system_uses_irq_prio_masking()) {
> +		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
> +
> +		WARN_ON(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
> +	}
> +
>  	asm volatile(ALTERNATIVE(
>  		"msr	daifclr, #2		// arch_local_irq_enable\n"
>  		"nop",
> @@ -53,6 +60,13 @@ static inline void arch_local_irq_enable(void)
> 
>  static inline void arch_local_irq_disable(void)
>  {
> +	if (IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
> +	    system_uses_irq_prio_masking()) {
> +		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
> +
> +		WARN_ON(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
> +	}
> +
>  	asm volatile(ALTERNATIVE(
>  		"msr	daifset, #2		// arch_local_irq_disable",
>  		"msr_s  " __stringify(SYS_ICC_PMR_EL1) ", %0",
> --
> 1.9.1
> 

nit: There is also the question of using WARN_ON in a context that will
be extremely noisy if we're in a condition where this fires.
WARN_ON_ONCE, maybe? This is a debugging thing, so maybe we just don't care.

Thanks,

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

Re: [PATCH v2 4/5] arm64: irqflags: Introduce explicit debugging for IRQ priorities

[Marc Zyngier]

On 29/04/2019 17:00, Julien Thierry wrote:
> Using IRQ priority masking to enable/disable interrupts is a bit
> sensitive as it requires to deal with both ICC_PMR_EL1 and PSR.I.
> 
> Introduce some validity checks to both highlight the states in which
> functions dealing with IRQ enabling/disabling can (not) be called, and
> bark a warning when called in an unexpected state.
> 
> Since these checks are done on hotpaths, introduce a build option to
> choose whether to do the checking.
> 
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/Kconfig                 | 11 +++++++++++
>  arch/arm64/include/asm/daifflags.h |  9 +++++++++
>  arch/arm64/include/asm/irqflags.h  | 14 ++++++++++++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 7e34b9e..3fb38f3 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1359,6 +1359,17 @@ config ARM64_PSEUDO_NMI
> 
>  	  If unsure, say N
> 
> +if ARM64_PSEUDO_NMI
> +config ARM64_DEBUG_PRIORITY_MASKING
> +	bool "Debug interrupt priority masking"
> +	help
> +	  This adds runtime checks to functions enabling/disabling
> +	  interrupts when using priority masking. The additional checks verify
> +	  the validity of ICC_PMR_EL1 when calling concerned functions.
> +
> +	  If unsure, say N
> +endif
> +
>  config RELOCATABLE
>  	bool
>  	help
> diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
> index a32ece9..9512968 100644
> --- a/arch/arm64/include/asm/daifflags.h
> +++ b/arch/arm64/include/asm/daifflags.h
> @@ -28,6 +28,11 @@
>  /* mask/save/unmask/restore all exceptions, including interrupts. */
>  static inline void local_daif_mask(void)
>  {
> +	WARN_ON(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
> +		system_uses_irq_prio_masking() &&

Given that you repeat these conditions all over the place, how about a
helper:

#define DEBUG_PRIORITY_MASKING_CHECK(x)			\
	(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) && \
	 system_uses_irq_prio_masking() && (x))

or some variant thereof.

> +		(read_sysreg_s(SYS_ICC_PMR_EL1) == (GIC_PRIO_IRQOFF |
> +						    GIC_PRIO_IGNORE_PMR)));
> +
>  	asm volatile(
>  		"msr	daifset, #0xf		// local_daif_mask\n"
>  		:
> @@ -62,6 +67,10 @@ static inline void local_daif_restore(unsigned long flags)
>  {
>  	bool irq_disabled = flags & PSR_I_BIT;
> 
> +	WARN_ON(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
> +		system_uses_irq_prio_masking() &&
> +		!(read_sysreg(daif) & PSR_I_BIT));
> +
>  	if (!irq_disabled) {
>  		trace_hardirqs_on();
> 
> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
> index 516cdfc..a40abc2 100644
> --- a/arch/arm64/include/asm/irqflags.h
> +++ b/arch/arm64/include/asm/irqflags.h
> @@ -40,6 +40,13 @@
>   */
>  static inline void arch_local_irq_enable(void)
>  {
> +	if (IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
> +	    system_uses_irq_prio_masking()) {
> +		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
> +
> +		WARN_ON(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
> +	}
> +
>  	asm volatile(ALTERNATIVE(
>  		"msr	daifclr, #2		// arch_local_irq_enable\n"
>  		"nop",
> @@ -53,6 +60,13 @@ static inline void arch_local_irq_enable(void)
> 
>  static inline void arch_local_irq_disable(void)
>  {
> +	if (IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
> +	    system_uses_irq_prio_masking()) {
> +		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
> +
> +		WARN_ON(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
> +	}
> +
>  	asm volatile(ALTERNATIVE(
>  		"msr	daifset, #2		// arch_local_irq_disable",
>  		"msr_s  " __stringify(SYS_ICC_PMR_EL1) ", %0",
> --
> 1.9.1
> 

nit: There is also the question of using WARN_ON in a context that will
be extremely noisy if we're in a condition where this fires.
WARN_ON_ONCE, maybe? This is a debugging thing, so maybe we just don't care.

Thanks,

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

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

Re: [PATCH v2 4/5] arm64: irqflags: Introduce explicit debugging for IRQ priorities

[Julien Thierry]

On 07/05/2019 09:44, Marc Zyngier wrote:
> On 29/04/2019 17:00, Julien Thierry wrote:
>> Using IRQ priority masking to enable/disable interrupts is a bit
>> sensitive as it requires to deal with both ICC_PMR_EL1 and PSR.I.
>>
>> Introduce some validity checks to both highlight the states in which
>> functions dealing with IRQ enabling/disabling can (not) be called, and
>> bark a warning when called in an unexpected state.
>>
>> Since these checks are done on hotpaths, introduce a build option to
>> choose whether to do the checking.
>>
>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> ---
>>  arch/arm64/Kconfig                 | 11 +++++++++++
>>  arch/arm64/include/asm/daifflags.h |  9 +++++++++
>>  arch/arm64/include/asm/irqflags.h  | 14 ++++++++++++++
>>  3 files changed, 34 insertions(+)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 7e34b9e..3fb38f3 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -1359,6 +1359,17 @@ config ARM64_PSEUDO_NMI
>>
>>  	  If unsure, say N
>>
>> +if ARM64_PSEUDO_NMI
>> +config ARM64_DEBUG_PRIORITY_MASKING
>> +	bool "Debug interrupt priority masking"
>> +	help
>> +	  This adds runtime checks to functions enabling/disabling
>> +	  interrupts when using priority masking. The additional checks verify
>> +	  the validity of ICC_PMR_EL1 when calling concerned functions.
>> +
>> +	  If unsure, say N
>> +endif
>> +
>>  config RELOCATABLE
>>  	bool
>>  	help
>> diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
>> index a32ece9..9512968 100644
>> --- a/arch/arm64/include/asm/daifflags.h
>> +++ b/arch/arm64/include/asm/daifflags.h
>> @@ -28,6 +28,11 @@
>>  /* mask/save/unmask/restore all exceptions, including interrupts. */
>>  static inline void local_daif_mask(void)
>>  {
>> +	WARN_ON(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
>> +		system_uses_irq_prio_masking() &&
> 
> Given that you repeat these conditions all over the place, how about a
> helper:
> 
> #define DEBUG_PRIORITY_MASKING_CHECK(x)			\
> 	(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) && \
> 	 system_uses_irq_prio_masking() && (x))
> 
> or some variant thereof.
> 

Yes, good point, I'll do that.

>> +		(read_sysreg_s(SYS_ICC_PMR_EL1) == (GIC_PRIO_IRQOFF |
>> +						    GIC_PRIO_IGNORE_PMR)));
>> +
>>  	asm volatile(
>>  		"msr	daifset, #0xf		// local_daif_mask\n"
>>  		:
>> @@ -62,6 +67,10 @@ static inline void local_daif_restore(unsigned long flags)
>>  {
>>  	bool irq_disabled = flags & PSR_I_BIT;
>>
>> +	WARN_ON(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
>> +		system_uses_irq_prio_masking() &&
>> +		!(read_sysreg(daif) & PSR_I_BIT));
>> +
>>  	if (!irq_disabled) {
>>  		trace_hardirqs_on();
>>
>> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
>> index 516cdfc..a40abc2 100644
>> --- a/arch/arm64/include/asm/irqflags.h
>> +++ b/arch/arm64/include/asm/irqflags.h
>> @@ -40,6 +40,13 @@
>>   */
>>  static inline void arch_local_irq_enable(void)
>>  {
>> +	if (IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
>> +	    system_uses_irq_prio_masking()) {
>> +		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
>> +
>> +		WARN_ON(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
>> +	}
>> +
>>  	asm volatile(ALTERNATIVE(
>>  		"msr	daifclr, #2		// arch_local_irq_enable\n"
>>  		"nop",
>> @@ -53,6 +60,13 @@ static inline void arch_local_irq_enable(void)
>>
>>  static inline void arch_local_irq_disable(void)
>>  {
>> +	if (IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
>> +	    system_uses_irq_prio_masking()) {
>> +		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
>> +
>> +		WARN_ON(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
>> +	}
>> +
>>  	asm volatile(ALTERNATIVE(
>>  		"msr	daifset, #2		// arch_local_irq_disable",
>>  		"msr_s  " __stringify(SYS_ICC_PMR_EL1) ", %0",
>> --
>> 1.9.1
>>
> 
> nit: There is also the question of using WARN_ON in a context that will
> be extremely noisy if we're in a condition where this fires.
> WARN_ON_ONCE, maybe? This is a debugging thing, so maybe we just don't care.
> 

Yes, thinking about it, it did get pretty noisy when I checked this was
working. WARN_ON_ONCE might be a good option.

Thanks,

-- 
Julien Thierry

Re: [PATCH v2 4/5] arm64: irqflags: Introduce explicit debugging for IRQ priorities

[Julien Thierry]

On 07/05/2019 09:44, Marc Zyngier wrote:
> On 29/04/2019 17:00, Julien Thierry wrote:
>> Using IRQ priority masking to enable/disable interrupts is a bit
>> sensitive as it requires to deal with both ICC_PMR_EL1 and PSR.I.
>>
>> Introduce some validity checks to both highlight the states in which
>> functions dealing with IRQ enabling/disabling can (not) be called, and
>> bark a warning when called in an unexpected state.
>>
>> Since these checks are done on hotpaths, introduce a build option to
>> choose whether to do the checking.
>>
>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> ---
>>  arch/arm64/Kconfig                 | 11 +++++++++++
>>  arch/arm64/include/asm/daifflags.h |  9 +++++++++
>>  arch/arm64/include/asm/irqflags.h  | 14 ++++++++++++++
>>  3 files changed, 34 insertions(+)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 7e34b9e..3fb38f3 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -1359,6 +1359,17 @@ config ARM64_PSEUDO_NMI
>>
>>  	  If unsure, say N
>>
>> +if ARM64_PSEUDO_NMI
>> +config ARM64_DEBUG_PRIORITY_MASKING
>> +	bool "Debug interrupt priority masking"
>> +	help
>> +	  This adds runtime checks to functions enabling/disabling
>> +	  interrupts when using priority masking. The additional checks verify
>> +	  the validity of ICC_PMR_EL1 when calling concerned functions.
>> +
>> +	  If unsure, say N
>> +endif
>> +
>>  config RELOCATABLE
>>  	bool
>>  	help
>> diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
>> index a32ece9..9512968 100644
>> --- a/arch/arm64/include/asm/daifflags.h
>> +++ b/arch/arm64/include/asm/daifflags.h
>> @@ -28,6 +28,11 @@
>>  /* mask/save/unmask/restore all exceptions, including interrupts. */
>>  static inline void local_daif_mask(void)
>>  {
>> +	WARN_ON(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
>> +		system_uses_irq_prio_masking() &&
> 
> Given that you repeat these conditions all over the place, how about a
> helper:
> 
> #define DEBUG_PRIORITY_MASKING_CHECK(x)			\
> 	(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) && \
> 	 system_uses_irq_prio_masking() && (x))
> 
> or some variant thereof.
> 

Yes, good point, I'll do that.

>> +		(read_sysreg_s(SYS_ICC_PMR_EL1) == (GIC_PRIO_IRQOFF |
>> +						    GIC_PRIO_IGNORE_PMR)));
>> +
>>  	asm volatile(
>>  		"msr	daifset, #0xf		// local_daif_mask\n"
>>  		:
>> @@ -62,6 +67,10 @@ static inline void local_daif_restore(unsigned long flags)
>>  {
>>  	bool irq_disabled = flags & PSR_I_BIT;
>>
>> +	WARN_ON(IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
>> +		system_uses_irq_prio_masking() &&
>> +		!(read_sysreg(daif) & PSR_I_BIT));
>> +
>>  	if (!irq_disabled) {
>>  		trace_hardirqs_on();
>>
>> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
>> index 516cdfc..a40abc2 100644
>> --- a/arch/arm64/include/asm/irqflags.h
>> +++ b/arch/arm64/include/asm/irqflags.h
>> @@ -40,6 +40,13 @@
>>   */
>>  static inline void arch_local_irq_enable(void)
>>  {
>> +	if (IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
>> +	    system_uses_irq_prio_masking()) {
>> +		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
>> +
>> +		WARN_ON(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
>> +	}
>> +
>>  	asm volatile(ALTERNATIVE(
>>  		"msr	daifclr, #2		// arch_local_irq_enable\n"
>>  		"nop",
>> @@ -53,6 +60,13 @@ static inline void arch_local_irq_enable(void)
>>
>>  static inline void arch_local_irq_disable(void)
>>  {
>> +	if (IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
>> +	    system_uses_irq_prio_masking()) {
>> +		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
>> +
>> +		WARN_ON(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
>> +	}
>> +
>>  	asm volatile(ALTERNATIVE(
>>  		"msr	daifset, #2		// arch_local_irq_disable",
>>  		"msr_s  " __stringify(SYS_ICC_PMR_EL1) ", %0",
>> --
>> 1.9.1
>>
> 
> nit: There is also the question of using WARN_ON in a context that will
> be extremely noisy if we're in a condition where this fires.
> WARN_ON_ONCE, maybe? This is a debugging thing, so maybe we just don't care.
> 

Yes, thinking about it, it did get pretty noisy when I checked this was
working. WARN_ON_ONCE might be a good option.

Thanks,

-- 
Julien Thierry

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

[PATCH v2 5/5] arm64: fix kernel stack overflow in kdump capture kernel

[Julien Thierry]

From: Wei Li <liwei391@huawei.com>

When enabling ARM64_PSEUDO_NMI feature in kdump capture kernel, it will
report a kernel stack overflow exception:

[    0.000000] CPU features: detected: IRQ priority masking
[    0.000000] alternatives: patching kernel code
[    0.000000] Insufficient stack space to handle exception!
[    0.000000] ESR: 0x96000044 -- DABT (current EL)
[    0.000000] FAR: 0x0000000000000040
[    0.000000] Task stack:     [0xffff0000097f0000..0xffff0000097f4000]
[    0.000000] IRQ stack:      [0x0000000000000000..0x0000000000004000]
[    0.000000] Overflow stack: [0xffff80002b7cf290..0xffff80002b7d0290]
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.19.34-lw+ #3
[    0.000000] pstate: 400003c5 (nZcv DAIF -PAN -UAO)
[    0.000000] pc : el1_sync+0x0/0xb8
[    0.000000] lr : el1_irq+0xb8/0x140
[    0.000000] sp : 0000000000000040
[    0.000000] pmr_save: 00000070
[    0.000000] x29: ffff0000097f3f60 x28: ffff000009806240
[    0.000000] x27: 0000000080000000 x26: 0000000000004000
[    0.000000] x25: 0000000000000000 x24: ffff000009329028
[    0.000000] x23: 0000000040000005 x22: ffff000008095c6c
[    0.000000] x21: ffff0000097f3f70 x20: 0000000000000070
[    0.000000] x19: ffff0000097f3e30 x18: ffffffffffffffff
[    0.000000] x17: 0000000000000000 x16: 0000000000000000
[    0.000000] x15: ffff0000097f9708 x14: ffff000089a382ef
[    0.000000] x13: ffff000009a382fd x12: ffff000009824000
[    0.000000] x11: ffff0000097fb7b0 x10: ffff000008730028
[    0.000000] x9 : ffff000009440018 x8 : 000000000000000d
[    0.000000] x7 : 6b20676e69686374 x6 : 000000000000003b
[    0.000000] x5 : 0000000000000000 x4 : ffff000008093600
[    0.000000] x3 : 0000000400000008 x2 : 7db2e689fc2b8e00
[    0.000000] x1 : 0000000000000000 x0 : ffff0000097f3e30
[    0.000000] Kernel panic - not syncing: kernel stack overflow
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.19.34-lw+ #3
[    0.000000] Call trace:
[    0.000000]  dump_backtrace+0x0/0x1b8
[    0.000000]  show_stack+0x24/0x30
[    0.000000]  dump_stack+0xa8/0xcc
[    0.000000]  panic+0x134/0x30c
[    0.000000]  __stack_chk_fail+0x0/0x28
[    0.000000]  handle_bad_stack+0xfc/0x108
[    0.000000]  __bad_stack+0x90/0x94
[    0.000000]  el1_sync+0x0/0xb8
[    0.000000]  init_gic_priority_masking+0x4c/0x70
[    0.000000]  smp_prepare_boot_cpu+0x60/0x68
[    0.000000]  start_kernel+0x1e8/0x53c
[    0.000000] ---[ end Kernel panic - not syncing: kernel stack overflow ]---

The reason is init_gic_priority_masking() may unmask PSR.I while the
irq stacks are not inited yet. Some "NMI" could be raised unfortunately
and it will just go into this exception.

In this patch, we just write the PMR in smp_prepare_boot_cpu(), and delay
unmasking PSR.I after irq stacks inited in init_IRQ().

Fixes: e79321883842 ("arm64: Switch to PMR masking when starting CPUs")
Signed-off-by: Wei Li <liwei391@huawei.com>
[JT: make init_gic_priority_masking() not modify daif, rebase on other
     priority masking fixes]
Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 arch/arm64/kernel/irq.c | 9 +++++++++
 arch/arm64/kernel/smp.c | 8 +-------
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index fdd9cb2..e8daa7a 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -77,6 +77,15 @@ void __init init_IRQ(void)
 	irqchip_init();
 	if (!handle_arch_irq)
 		panic("No interrupt controller found.");
+
+	if (system_uses_irq_prio_masking()) {
+		/*
+		 * Now that we have a stack for our IRQ handler, set
+		 * the PMR/PSR pair to a consistent state.
+		 */
+		WARN_ON(read_sysreg(daif) & PSR_A_BIT);
+		local_daif_restore(DAIF_PROCCTX_NOIRQ);
+	}
 }
 
 /*
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 2a6d0dd1..c08a075 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -192,13 +192,7 @@ static void init_gic_priority_masking(void)
 
 	WARN_ON(!(cpuflags & PSR_I_BIT));
 
-	/* We can only unmask PSR.I if we can take aborts */
-	if (!(cpuflags & PSR_A_BIT)) {
-		gic_write_pmr(GIC_PRIO_IRQOFF);
-		write_sysreg(cpuflags & ~PSR_I_BIT, daif);
-	} else {
-		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
-	}
+	gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
 }
 
 /*
-- 
1.9.1

[PATCH v2 5/5] arm64: fix kernel stack overflow in kdump capture kernel

[Julien Thierry]

From: Wei Li <liwei391@huawei.com>

When enabling ARM64_PSEUDO_NMI feature in kdump capture kernel, it will
report a kernel stack overflow exception:

[    0.000000] CPU features: detected: IRQ priority masking
[    0.000000] alternatives: patching kernel code
[    0.000000] Insufficient stack space to handle exception!
[    0.000000] ESR: 0x96000044 -- DABT (current EL)
[    0.000000] FAR: 0x0000000000000040
[    0.000000] Task stack:     [0xffff0000097f0000..0xffff0000097f4000]
[    0.000000] IRQ stack:      [0x0000000000000000..0x0000000000004000]
[    0.000000] Overflow stack: [0xffff80002b7cf290..0xffff80002b7d0290]
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.19.34-lw+ #3
[    0.000000] pstate: 400003c5 (nZcv DAIF -PAN -UAO)
[    0.000000] pc : el1_sync+0x0/0xb8
[    0.000000] lr : el1_irq+0xb8/0x140
[    0.000000] sp : 0000000000000040
[    0.000000] pmr_save: 00000070
[    0.000000] x29: ffff0000097f3f60 x28: ffff000009806240
[    0.000000] x27: 0000000080000000 x26: 0000000000004000
[    0.000000] x25: 0000000000000000 x24: ffff000009329028
[    0.000000] x23: 0000000040000005 x22: ffff000008095c6c
[    0.000000] x21: ffff0000097f3f70 x20: 0000000000000070
[    0.000000] x19: ffff0000097f3e30 x18: ffffffffffffffff
[    0.000000] x17: 0000000000000000 x16: 0000000000000000
[    0.000000] x15: ffff0000097f9708 x14: ffff000089a382ef
[    0.000000] x13: ffff000009a382fd x12: ffff000009824000
[    0.000000] x11: ffff0000097fb7b0 x10: ffff000008730028
[    0.000000] x9 : ffff000009440018 x8 : 000000000000000d
[    0.000000] x7 : 6b20676e69686374 x6 : 000000000000003b
[    0.000000] x5 : 0000000000000000 x4 : ffff000008093600
[    0.000000] x3 : 0000000400000008 x2 : 7db2e689fc2b8e00
[    0.000000] x1 : 0000000000000000 x0 : ffff0000097f3e30
[    0.000000] Kernel panic - not syncing: kernel stack overflow
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.19.34-lw+ #3
[    0.000000] Call trace:
[    0.000000]  dump_backtrace+0x0/0x1b8
[    0.000000]  show_stack+0x24/0x30
[    0.000000]  dump_stack+0xa8/0xcc
[    0.000000]  panic+0x134/0x30c
[    0.000000]  __stack_chk_fail+0x0/0x28
[    0.000000]  handle_bad_stack+0xfc/0x108
[    0.000000]  __bad_stack+0x90/0x94
[    0.000000]  el1_sync+0x0/0xb8
[    0.000000]  init_gic_priority_masking+0x4c/0x70
[    0.000000]  smp_prepare_boot_cpu+0x60/0x68
[    0.000000]  start_kernel+0x1e8/0x53c
[    0.000000] ---[ end Kernel panic - not syncing: kernel stack overflow ]---

The reason is init_gic_priority_masking() may unmask PSR.I while the
irq stacks are not inited yet. Some "NMI" could be raised unfortunately
and it will just go into this exception.

In this patch, we just write the PMR in smp_prepare_boot_cpu(), and delay
unmasking PSR.I after irq stacks inited in init_IRQ().

Fixes: e79321883842 ("arm64: Switch to PMR masking when starting CPUs")
Signed-off-by: Wei Li <liwei391@huawei.com>
[JT: make init_gic_priority_masking() not modify daif, rebase on other
     priority masking fixes]
Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 arch/arm64/kernel/irq.c | 9 +++++++++
 arch/arm64/kernel/smp.c | 8 +-------
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index fdd9cb2..e8daa7a 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -77,6 +77,15 @@ void __init init_IRQ(void)
 	irqchip_init();
 	if (!handle_arch_irq)
 		panic("No interrupt controller found.");
+
+	if (system_uses_irq_prio_masking()) {
+		/*
+		 * Now that we have a stack for our IRQ handler, set
+		 * the PMR/PSR pair to a consistent state.
+		 */
+		WARN_ON(read_sysreg(daif) & PSR_A_BIT);
+		local_daif_restore(DAIF_PROCCTX_NOIRQ);
+	}
 }
 
 /*
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 2a6d0dd1..c08a075 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -192,13 +192,7 @@ static void init_gic_priority_masking(void)
 
 	WARN_ON(!(cpuflags & PSR_I_BIT));
 
-	/* We can only unmask PSR.I if we can take aborts */
-	if (!(cpuflags & PSR_A_BIT)) {
-		gic_write_pmr(GIC_PRIO_IRQOFF);
-		write_sysreg(cpuflags & ~PSR_I_BIT, daif);
-	} else {
-		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
-	}
+	gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_IGNORE_PMR);
 }
 
 /*
-- 
1.9.1


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

Re: [PATCH v2 0/5] arm64: IRQ priority masking and Pseudo-NMI fixes

[Will Deacon]

Hi Julien,

On Mon, Apr 29, 2019 at 05:00:02PM +0100, Julien Thierry wrote:
> [Changing the title to make it reflex more the status of the series.]
> 
> Version one[1] of this series attempted to fix the issue reported by
> Zenghui[2] when using the function_graph tracer with IRQ priority
> masking.
> 
> Since then, I realized that priority masking and the use of Pseudo-NMIs
> was more broken than I thought.

Do you plan to respin this in light of Marc's comments?

Will

Re: [PATCH v2 0/5] arm64: IRQ priority masking and Pseudo-NMI fixes

[Will Deacon]

Hi Julien,

On Mon, Apr 29, 2019 at 05:00:02PM +0100, Julien Thierry wrote:
> [Changing the title to make it reflex more the status of the series.]
> 
> Version one[1] of this series attempted to fix the issue reported by
> Zenghui[2] when using the function_graph tracer with IRQ priority
> masking.
> 
> Since then, I realized that priority masking and the use of Pseudo-NMIs
> was more broken than I thought.

Do you plan to respin this in light of Marc's comments?

Will

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

Re: [PATCH v2 0/5] arm64: IRQ priority masking and Pseudo-NMI fixes

[Will Deacon]

Hi again, Julien,

On Thu, May 23, 2019 at 05:51:55PM +0100, Will Deacon wrote:
> On Mon, Apr 29, 2019 at 05:00:02PM +0100, Julien Thierry wrote:
> > [Changing the title to make it reflex more the status of the series.]
> > 
> > Version one[1] of this series attempted to fix the issue reported by
> > Zenghui[2] when using the function_graph tracer with IRQ priority
> > masking.
> > 
> > Since then, I realized that priority masking and the use of Pseudo-NMIs
> > was more broken than I thought.
> 
> Do you plan to respin this in light of Marc's comments?

For now, I marked this as depending on BROKEN in mainline, but please can
you look at respinning these fixes so that we can get things fixed properly
for 5.3?

Thanks,

Will

Re: [PATCH v2 0/5] arm64: IRQ priority masking and Pseudo-NMI fixes

[Will Deacon]

Hi again, Julien,

On Thu, May 23, 2019 at 05:51:55PM +0100, Will Deacon wrote:
> On Mon, Apr 29, 2019 at 05:00:02PM +0100, Julien Thierry wrote:
> > [Changing the title to make it reflex more the status of the series.]
> > 
> > Version one[1] of this series attempted to fix the issue reported by
> > Zenghui[2] when using the function_graph tracer with IRQ priority
> > masking.
> > 
> > Since then, I realized that priority masking and the use of Pseudo-NMIs
> > was more broken than I thought.
> 
> Do you plan to respin this in light of Marc's comments?

For now, I marked this as depending on BROKEN in mainline, but please can
you look at respinning these fixes so that we can get things fixed properly
for 5.3?

Thanks,

Will

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