[PATCH 0/3] irqchip/gic-v3: pseudo-NMI fixes

[PATCH 0/3] irqchip/gic-v3: pseudo-NMI fixes

[Mark Rutland]

These patches fix a couple of issues with the way GICv3 pseudo-NMIs are
handled:

* The first patch adds a barrier we missed from NMI handling due to an
  oversight.

* The second patch refactors some logic around reads from ICC_IAR1_EL1
  and adds commentary to explain what's going on.

* The third patch descends into madness, reworking gic_handle_irq() to
  consistently manage ICC_PMR_EL1 + DAIF and avoid cases where these can
  be left in an inconsistent state while softirqs are processed.

I've given these a beating in a QEMU KVM VM on a ThunderX2 host.

Note that arm64 has some orthogonal issues with lockdep which are fixed
by:

  https://lore.kernel.org/linux-arm-kernel/20220511131733.4074499-1-mark.rutland@arm.com/

... so to test with lockdep enabled it is necessary to apply that
series too.

Thanks,
Mark.

Mark Rutland (3):
  irqchip/gic-v3: ensure pseudo-NMIs have an ISB between ack and
    handling
  irqchip/gic-v3: refactor ISB + EOIR at ack time
  irqchip/gic-v3: fix priority mask handling

 arch/arm/include/asm/arch_gicv3.h   |   7 +-
 arch/arm64/include/asm/arch_gicv3.h |   6 -
 drivers/irqchip/irq-gic-v3.c        | 183 ++++++++++++++++++----------
 3 files changed, 121 insertions(+), 75 deletions(-)

-- 
2.30.2


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

[PATCH 1/3] irqchip/gic-v3: ensure pseudo-NMIs have an ISB between ack and handling

[Mark Rutland]

There are cases where a context synchronization event is necessary
between an IRQ being raised and being handled, and there are races such
that we cannot rely upon the exception entry being subsequent to the
interrupt being raised.

We identified and fixes this for regular IRQs in commit:

  39a06b67c2c1256b ("irqchip/gic: Ensure we have an ISB between ack and ->handle_irq")

Unfortunately, we forgot to do the same for psuedo-NMIs when support for
those was added in commit:

  f32c926651dcd168 ("irqchip/gic-v3: Handle pseudo-NMIs")

Which means that when pseudo-NMIs are used for PMU support, we'll hit
the same problem.

Apply the same fix as for regular IRQs. Note that when EOI mode 1 is in
use, the call to gic_write_eoir() will provide an ISB.

Fixes: f32c926651dcd168 ("irqchip/gic-v3: Handle pseudo-NMIs")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
---
 drivers/irqchip/irq-gic-v3.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index b252d5534547c..7305d84f2df5a 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -654,6 +654,9 @@ static inline void gic_handle_nmi(u32 irqnr, struct pt_regs *regs)
 
 	if (static_branch_likely(&supports_deactivate_key))
 		gic_write_eoir(irqnr);
+	else
+		isb()
+
 	/*
 	 * Leave the PSR.I bit set to prevent other NMIs to be
 	 * received while handling this one.
-- 
2.30.2


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

[PATCH 2/3] irqchip/gic-v3: refactor ISB + EOIR at ack time

[Mark Rutland]

There are cases where a context synchronization event is necessary
between an IRQ being raised and being handled, and there are races such
that we cannot rely upon the exception entry being subsequent to the
interrupt being raised. To fix this, we place an ISB between a read of
IAR and the subsequent invocation of an IRQ handler.

When EOI mode 1 is in use, we need to EOI an interrupt prior to invoking
its handler, and we have a write to EOIR for this. As this write to EOIR
requires an ISB, and this is provided by the gic_write_eoir() helper, we
omit the usual ISB in this case, with the logic being:

|	if (static_branch_likely(&supports_deactivate_key))
|		gic_write_eoir(irqnr);
|	else
|		isb();

This is somewhat opaque, and it would be a little clearer if there were
an unconditional ISB, with only the write to EOIR being conditional,
e.g.

|	if (static_branch_likely(&supports_deactivate_key))
|		write_gicreg(irqnr, ICC_EOIR1_EL1);
|
|	isb();

This patch rewrites the code that way, with this logic factored into a
new helper function with comments explaining what the ISB is for, as
were originally laid out in commit:

  39a06b67c2c1256b ("irqchip/gic: Ensure we have an ISB between ack and ->handle_irq")

Note that since then, we removed the IAR polling in commit:

  342677d70ab92142 ("irqchip/gic-v3: Remove acknowledge loop")

... which removed one of the two race conditions.

For consistency, other portions of the driver are made to manipulate
EOIR using write_gicreg() and explcit ISBs, and the gic_write_eoir()
helper function is removed.

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/arch_gicv3.h   |  7 +----
 arch/arm64/include/asm/arch_gicv3.h |  6 ----
 drivers/irqchip/irq-gic-v3.c        | 43 ++++++++++++++++++++++-------
 3 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/arch/arm/include/asm/arch_gicv3.h b/arch/arm/include/asm/arch_gicv3.h
index 413abfb42989e..f82a819eb0dbb 100644
--- a/arch/arm/include/asm/arch_gicv3.h
+++ b/arch/arm/include/asm/arch_gicv3.h
@@ -48,6 +48,7 @@ static inline u32 read_ ## a64(void)		\
 	return read_sysreg(a32); 		\
 }						\
 
+CPUIF_MAP(ICC_EOIR1, ICC_EOIR1_EL1)
 CPUIF_MAP(ICC_PMR, ICC_PMR_EL1)
 CPUIF_MAP(ICC_AP0R0, ICC_AP0R0_EL1)
 CPUIF_MAP(ICC_AP0R1, ICC_AP0R1_EL1)
@@ -63,12 +64,6 @@ CPUIF_MAP(ICC_AP1R3, ICC_AP1R3_EL1)
 
 /* Low-level accessors */
 
-static inline void gic_write_eoir(u32 irq)
-{
-	write_sysreg(irq, ICC_EOIR1);
-	isb();
-}
-
 static inline void gic_write_dir(u32 val)
 {
 	write_sysreg(val, ICC_DIR);
diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
index 8bd5afc7b692e..48d4473e8eee2 100644
--- a/arch/arm64/include/asm/arch_gicv3.h
+++ b/arch/arm64/include/asm/arch_gicv3.h
@@ -26,12 +26,6 @@
  * sets the GP register's most significant bits to 0 with an explicit cast.
  */
 
-static inline void gic_write_eoir(u32 irq)
-{
-	write_sysreg_s(irq, SYS_ICC_EOIR1_EL1);
-	isb();
-}
-
 static __always_inline void gic_write_dir(u32 irq)
 {
 	write_sysreg_s(irq, SYS_ICC_DIR_EL1);
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 7305d84f2df5a..0cbc4e25c48de 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -556,7 +556,8 @@ static void gic_irq_nmi_teardown(struct irq_data *d)
 
 static void gic_eoi_irq(struct irq_data *d)
 {
-	gic_write_eoir(gic_irq(d));
+	write_gicreg(gic_irq(d), ICC_EOIR1_EL1);
+	isb();
 }
 
 static void gic_eoimode1_eoi_irq(struct irq_data *d)
@@ -640,10 +641,38 @@ static void gic_deactivate_unhandled(u32 irqnr)
 		if (irqnr < 8192)
 			gic_write_dir(irqnr);
 	} else {
-		gic_write_eoir(irqnr);
+		write_gicreg(irqnr, ICC_EOIR1_EL1);
+		isb();
 	}
 }
 
+/*
+ * Follow a read of the IAR with any HW maintenance that needs to happen prior
+ * to invoking the relevant IRQ handler. We must do two things:
+ *
+ * (1) Ensure instruction ordering between a read of IAR and subsequent
+ *     instructions in the IRQ handler using an ISB.
+ *
+ *     It is possible for the IAR to report an IRQ which was signalled *after*
+ *     the CPU took an IRQ exception as multiple interrupts can race to be
+ *     recognized by the GIC, earlier interrupts could be withdrawn, and/or
+ *     later interrupts could be prioritized by the GIC.
+ *
+ *     For devices which are tightly coupled to the CPU, such as PMUs, a
+ *     context synchronization event is necessary to ensure that system
+ *     register state is not stale, as these may have been indirectly written
+ *     *after* exception entry.
+ *
+ * (2) Deactivate the interrupt when EOI mode 1 is in use.
+ */
+static inline void gic_complete_ack(u32 irqnr)
+{
+	if (static_branch_likely(&supports_deactivate_key))
+		write_gicreg(irqnr, ICC_EOIR1_EL1);
+
+	isb();
+}
+
 static inline void gic_handle_nmi(u32 irqnr, struct pt_regs *regs)
 {
 	bool irqs_enabled = interrupts_enabled(regs);
@@ -652,10 +681,7 @@ static inline void gic_handle_nmi(u32 irqnr, struct pt_regs *regs)
 	if (irqs_enabled)
 		nmi_enter();
 
-	if (static_branch_likely(&supports_deactivate_key))
-		gic_write_eoir(irqnr);
-	else
-		isb()
+	gic_complete_ack(irqnr);
 
 	/*
 	 * Leave the PSR.I bit set to prevent other NMIs to be
@@ -726,10 +752,7 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
 		gic_arch_enable_irqs();
 	}
 
-	if (static_branch_likely(&supports_deactivate_key))
-		gic_write_eoir(irqnr);
-	else
-		isb();
+	gic_complete_ack(irqnr);
 
 	if (generic_handle_domain_irq(gic_data.domain, irqnr)) {
 		WARN_ONCE(true, "Unexpected interrupt received!\n");
-- 
2.30.2


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

[PATCH 3/3] irqchip/gic-v3: fix priority mask handling

[Mark Rutland]

When a kernel is built with CONFIG_ARM64_PSEUDO_NMI=y and pseudo-NMIs
are enabled at runtime, GICv3's gic_handle_irq() can leave DAIF and
ICC_PMR_EL1 in an unexpected state in some cases, breaking subsequent
usage of local_irq_enable() and resulting in softirqs being run with
IRQs erroneously masked (possibly resulting in deadlocks).

This can happen when an IRQ exception is taken from a context where
regular IRQs were unmasked, and either:

(1) ICC_IAR1_EL1 indicates a special INTID (e.g. as a result of an IRQ
    being withdrawn since the IRQ exception was taken).

(2) ICC_IAR1_EL1 and ICC_RPR_EL1 indicate an NMI was acknowledged.

When an NMI is taken from a context where regular IRQs were masked,
there is no problem.

When CONFIG_ARM64_DEBUG_PRIORITY_MASKING=y, this can be detected with
perf, e.g.

| # ./perf record -a -g -e cycles:k ls -alR / > /dev/null 2>&1
| ------------[ cut here ]------------
| WARNING: CPU: 0 PID: 14 at arch/arm64/include/asm/irqflags.h:32 arch_local_irq_enable+0x4c/0x6c
| Modules linked in:
| CPU: 0 PID: 14 Comm: ksoftirqd/0 Not tainted 5.18.0-rc5-00004-g876c38e3d20b #12
| Hardware name: linux,dummy-virt (DT)
| pstate: 204000c5 (nzCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
| pc : arch_local_irq_enable+0x4c/0x6c
| lr : __do_softirq+0x110/0x5d8
| sp : ffff8000080bbbc0
| pmr_save: 000000f0
| x29: ffff8000080bbbc0 x28: ffff316ac3a6ca40 x27: 0000000000000000
| x26: 0000000000000000 x25: ffffa04611c06008 x24: ffffa04611c06008
| x23: 0000000040400005 x22: 0000000000000200 x21: ffff8000080bbe20
| x20: ffffa0460fe10320 x19: 0000000000000009 x18: 0000000000000000
| x17: ffff91252dfa9000 x16: ffff800008004000 x15: 0000000000004000
| x14: 0000000000000028 x13: ffffa0460fe17578 x12: ffffa0460fed4294
| x11: ffffa0460fedc168 x10: ffffffffffffff80 x9 : ffffa0460fe10a70
| x8 : ffffa0460fedc168 x7 : 000000000000b762 x6 : 00000000057c3bdf
| x5 : ffff8000080bbb18 x4 : 0000000000000000 x3 : 0000000000000001
| x2 : ffff91252dfa9000 x1 : 0000000000000060 x0 : 00000000000000f0
| Call trace:
|  arch_local_irq_enable+0x4c/0x6c
|  __irq_exit_rcu+0x180/0x1ac
|  irq_exit_rcu+0x1c/0x44
|  el1_interrupt+0x4c/0xe4
|  el1h_64_irq_handler+0x18/0x24
|  el1h_64_irq+0x74/0x78
|  smpboot_thread_fn+0x68/0x2c0
|  kthread+0x124/0x130
|  ret_from_fork+0x10/0x20
| irq event stamp: 193241
| hardirqs last  enabled at (193240): [<ffffa0460fe10a9c>] __do_softirq+0x10c/0x5d8
| hardirqs last disabled at (193241): [<ffffa0461102ffe4>] el1_dbg+0x24/0x90
| softirqs last  enabled at (193234): [<ffffa0460fe10e00>] __do_softirq+0x470/0x5d8
| softirqs last disabled at (193239): [<ffffa0460fea9944>] __irq_exit_rcu+0x180/0x1ac
| ---[ end trace 0000000000000000 ]---

The necessary manipulation of DAIF and ICC_PMR_EL1 depends on the
interrupted context, but the structure of gic_handle_irq() makes this
also depend on whether the GIC reports an IRQ, NMI, or special INTID:

*  When the interrupted context had regular IRQs masked (and hence the
   interrupt must be an NMI), the entry code performs the NMI
   entry/exit and gic_handle_irq() should return with DAIF and
   ICC_PMR_EL1 unchanged.

   This is handled correctly today.

* When the interrupted context had regular IRQs unmasked, the entry code
  performs IRQ entry/exit, but expects gic_handle_irq() to always update
  ICC_PMR_EL1 and DAIF.IF to unmask NMIs (but not regular IRQs) prior to
  returning (which it must do prior to invoking any regular IRQ
  handler).

  This unbalanced calling convention is necessary becasue we don't know
  whether an NMI has been taken until acknowledged by a read from
  ICC_IAR1_EL1, and so we need to perform the read with NMI masked in
  case an NMI has been taken (and needs to be handled with NMIs masked).

  Unfortunately, this is not handled consistently:

  - When ICC_IAR1_EL1 reports a special INTID, gic_handle_irq() returns
    immediately without manipulating ICC_PMR_EL1 and DAIF.

  - When RPR_EL1 indicates an NMI, gic_handle_irq() calls
    gic_handle_nmi() to invoke the NMI handler, then returns without
    manipulating ICC_PMR_EL1 and DAIF.

  - For regular IRQs, gic_handle_irq() manipulates ICC_PMR_EL1 and DAIF
    prior to invoking the IRQ handler.

There were related problems with special INTID handling in the past,
where if an exception was taken from a context with regular IRQs masked
and ICC_IAR_EL1 reported a special INTID, gic_handle_irq() would
erroneously unmask NMIs in NMI context permitted an unexpected nested
NMI. That case specifically was fixed by commit:

  a97709f563a078e2 ("irqchip/gic-v3: Do not enable irqs when handling spurious interrups")

... but unfortunately that commit added an inverse problem, where if an
exception was taken from a context with regular IRQs *unmasked* and
ICC_IAR_EL1 reported a special INTID, gic_handle_irq() would erroneously
fail to  unmask NMIs (and consequently regular IRQs could not be
unmasked during softirq processing). Before and after that commit, if an
NMI was taken from a context with regular IRQs unmasked gic_handle_irq()
would not unmask NMIs prior to returning, leading to the same problem
with softirq handling.

This patch fixes this by restructuring gic_handle_irq(), splitting it
into separate irqson/irqsoff helper functions which consistently perform
the DAIF + ICC_PMR1_EL1 manipulation based upon the interrupted context,
regardless of the event indicated by ICC_IAR1_EL1.

The special INTID handling is moved into the low-level IRQ/NMI handler
invocation helper functions, so that early returns don't prevent the
required manipulation of DAIF + ICC_PMR_EL1.

Fixes: f32c926651dcd168 ("irqchip/gic-v3: Handle pseudo-NMIs")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/irq-gic-v3.c | 147 +++++++++++++++++++++--------------
 1 file changed, 89 insertions(+), 58 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 0cbc4e25c48de..82628bc31a9c6 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -673,78 +673,69 @@ static inline void gic_complete_ack(u32 irqnr)
 	isb();
 }
 
-static inline void gic_handle_nmi(u32 irqnr, struct pt_regs *regs)
+static bool gic_rpr_is_nmi_prio(void)
 {
-	bool irqs_enabled = interrupts_enabled(regs);
-	int err;
+	if (!gic_supports_nmi())
+		return false;
 
-	if (irqs_enabled)
-		nmi_enter();
+	return unlikely(gic_read_rpr() == GICD_INT_RPR_PRI(GICD_INT_NMI_PRI));
+}
+
+static bool gic_irqnr_is_special(u32 irqnr)
+{
+	return irqnr >= 1020 && irqnr <= 1023;
+}
+
+static void __gic_handle_irq(u32 irqnr, struct pt_regs *regs)
+{
+	if (gic_irqnr_is_special(irqnr))
+		return;
 
 	gic_complete_ack(irqnr);
 
-	/*
-	 * Leave the PSR.I bit set to prevent other NMIs to be
-	 * received while handling this one.
-	 * PSR.I will be restored when we ERET to the
-	 * interrupted context.
-	 */
-	err = generic_handle_domain_nmi(gic_data.domain, irqnr);
-	if (err)
+	if (generic_handle_domain_irq(gic_data.domain, irqnr)) {
+		WARN_ONCE(true, "Unexpected interrupt (irqnr %u)\n", irqnr);
 		gic_deactivate_unhandled(irqnr);
-
-	if (irqs_enabled)
-		nmi_exit();
+	}
 }
 
-static u32 do_read_iar(struct pt_regs *regs)
+static void __gic_handle_nmi(u32 irqnr, struct pt_regs *regs)
 {
-	u32 iar;
-
-	if (gic_supports_nmi() && unlikely(!interrupts_enabled(regs))) {
-		u64 pmr;
-
-		/*
-		 * We were in a context with IRQs disabled. However, the
-		 * entry code has set PMR to a value that allows any
-		 * interrupt to be acknowledged, and not just NMIs. This can
-		 * lead to surprising effects if the NMI has been retired in
-		 * the meantime, and that there is an IRQ pending. The IRQ
-		 * would then be taken in NMI context, something that nobody
-		 * wants to debug twice.
-		 *
-		 * Until we sort this, drop PMR again to a level that will
-		 * actually only allow NMIs before reading IAR, and then
-		 * restore it to what it was.
-		 */
-		pmr = gic_read_pmr();
-		gic_pmr_mask_irqs();
-		isb();
+	if (gic_irqnr_is_special(irqnr))
+		return;
 
-		iar = gic_read_iar();
+	gic_complete_ack(irqnr);
 
-		gic_write_pmr(pmr);
-	} else {
-		iar = gic_read_iar();
+	if (generic_handle_domain_nmi(gic_data.domain, irqnr)) {
+		WARN_ONCE(true, "Unexpected pseudo-NMI (irqnr %u)\n", irqnr);
+		gic_deactivate_unhandled(irqnr);
 	}
-
-	return iar;
 }
 
-static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
+/*
+ * An exception has been taken from a context with IRQs enabled, and this could
+ * be an IRQ or an NMI.
+ *
+ * The entry code called us with DAIF.IF set to keep NMIs masked. We must clear
+ * DAIF.IF (and update ICC_PMR_EL1 to mask regular IRQs) prior to returning,
+ * after handling any NMI but before handling any IRQ.
+ *
+ * The entry code has performed IRQ entry, and if an NMI is detected we must
+ * perform NMI entry/exit around invoking the handler.
+ */
+static void __gic_handle_irq_from_irqson(struct pt_regs *regs)
 {
+	bool is_nmi;
 	u32 irqnr;
 
-	irqnr = do_read_iar(regs);
+	irqnr = gic_read_iar();
 
-	/* Check for special IDs first */
-	if ((irqnr >= 1020 && irqnr <= 1023))
-		return;
+	is_nmi = gic_rpr_is_nmi_prio();
 
-	if (gic_supports_nmi() &&
-	    unlikely(gic_read_rpr() == GICD_INT_RPR_PRI(GICD_INT_NMI_PRI))) {
-		gic_handle_nmi(irqnr, regs);
-		return;
+	if (is_nmi) {
+		nmi_enter();
+		__gic_handle_nmi(irqnr, regs);
+		nmi_exit();
 	}
 
 	if (gic_prio_masking_enabled()) {
@@ -752,12 +743,52 @@ static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs
 		gic_arch_enable_irqs();
 	}
 
-	gic_complete_ack(irqnr);
+	if (!is_nmi)
+		__gic_handle_irq(irqnr, regs);
+}
 
-	if (generic_handle_domain_irq(gic_data.domain, irqnr)) {
-		WARN_ONCE(true, "Unexpected interrupt received!\n");
-		gic_deactivate_unhandled(irqnr);
-	}
+/*
+ * An exception has been taken from a context with IRQs enabled, which can only
+ * be an NMI.
+ *
+ * The entry code called us with DAIF.IF set to keep NMIs masked. We must leave
+ * DAIF.IF (and ICC_PMR_EL1) unchanged.
+ *
+ * The entry code has performed NMI entry.
+ */
+static void __gic_handle_irq_from_irqsoff(struct pt_regs *regs)
+{
+	u64 pmr;
+	u32 irqnr;
+
+	/*
+	 * We were in a context with IRQs disabled. However, the
+	 * entry code has set PMR to a value that allows any
+	 * interrupt to be acknowledged, and not just NMIs. This can
+	 * lead to surprising effects if the NMI has been retired in
+	 * the meantime, and that there is an IRQ pending. The IRQ
+	 * would then be taken in NMI context, something that nobody
+	 * wants to debug twice.
+	 *
+	 * Until we sort this, drop PMR again to a level that will
+	 * actually only allow NMIs before reading IAR, and then
+	 * restore it to what it was.
+	 */
+	pmr = gic_read_pmr();
+	gic_pmr_mask_irqs();
+	isb();
+	irqnr = gic_read_iar();
+	gic_write_pmr(pmr);
+
+	__gic_handle_nmi(irqnr, regs);
+}
+
+static asmlinkage void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
+{
+	if (unlikely(gic_supports_nmi() && !interrupts_enabled(regs)))
+		__gic_handle_irq_from_irqsoff(regs);
+	else
+		__gic_handle_irq_from_irqson(regs);
 }
 
 static u32 gic_get_pribits(void)
-- 
2.30.2


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

Re: [PATCH 3/3] irqchip/gic-v3: fix priority mask handling

[Joey Gouly]

Hi Mark,

On Fri, May 13, 2022 at 02:30:38PM +0100, Mark Rutland wrote:
> When a kernel is built with CONFIG_ARM64_PSEUDO_NMI=y and pseudo-NMIs
> are enabled at runtime, GICv3's gic_handle_irq() can leave DAIF and
> ICC_PMR_EL1 in an unexpected state in some cases, breaking subsequent
> usage of local_irq_enable() and resulting in softirqs being run with
> IRQs erroneously masked (possibly resulting in deadlocks).
> 
> This can happen when an IRQ exception is taken from a context where
> regular IRQs were unmasked, and either:
> 
> (1) ICC_IAR1_EL1 indicates a special INTID (e.g. as a result of an IRQ
>     being withdrawn since the IRQ exception was taken).
> 
> (2) ICC_IAR1_EL1 and ICC_RPR_EL1 indicate an NMI was acknowledged.
> 
> When an NMI is taken from a context where regular IRQs were masked,
> there is no problem.
> 

[..]

> +/*
> + * An exception has been taken from a context with IRQs enabled, which can only
I think this should be: IRQs *disabled*?

> + * be an NMI.
> + *
> + * The entry code called us with DAIF.IF set to keep NMIs masked. We must leave
> + * DAIF.IF (and ICC_PMR_EL1) unchanged.
> + *
> + * The entry code has performed NMI entry.
> + */
> +static void __gic_handle_irq_from_irqsoff(struct pt_regs *regs)
> +{
> +	u64 pmr;
> +	u32 irqnr;
> +
> +	/*
> +	 * We were in a context with IRQs disabled. However, the
> +	 * entry code has set PMR to a value that allows any
> +	 * interrupt to be acknowledged, and not just NMIs. This can
> +	 * lead to surprising effects if the NMI has been retired in
> +	 * the meantime, and that there is an IRQ pending. The IRQ
> +	 * would then be taken in NMI context, something that nobody
> +	 * wants to debug twice.
> +	 *
> +	 * Until we sort this, drop PMR again to a level that will
> +	 * actually only allow NMIs before reading IAR, and then
> +	 * restore it to what it was.
> +	 */
> +	pmr = gic_read_pmr();
> +	gic_pmr_mask_irqs();
> +	isb();
> +	irqnr = gic_read_iar();
> +	gic_write_pmr(pmr);
> +
> +	__gic_handle_nmi(irqnr, regs);
> +}

Thanks,
Joey

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

Re: [PATCH 3/3] irqchip/gic-v3: fix priority mask handling

[Mark Rutland]

On Fri, May 13, 2022 at 02:45:52PM +0100, Joey Gouly wrote:
> Hi Mark,
> 
> On Fri, May 13, 2022 at 02:30:38PM +0100, Mark Rutland wrote:
> > When a kernel is built with CONFIG_ARM64_PSEUDO_NMI=y and pseudo-NMIs
> > are enabled at runtime, GICv3's gic_handle_irq() can leave DAIF and
> > ICC_PMR_EL1 in an unexpected state in some cases, breaking subsequent
> > usage of local_irq_enable() and resulting in softirqs being run with
> > IRQs erroneously masked (possibly resulting in deadlocks).
> > 
> > This can happen when an IRQ exception is taken from a context where
> > regular IRQs were unmasked, and either:
> > 
> > (1) ICC_IAR1_EL1 indicates a special INTID (e.g. as a result of an IRQ
> >     being withdrawn since the IRQ exception was taken).
> > 
> > (2) ICC_IAR1_EL1 and ICC_RPR_EL1 indicate an NMI was acknowledged.
> > 
> > When an NMI is taken from a context where regular IRQs were masked,
> > there is no problem.
> > 
> 
> [..]
> 
> > +/*
> > + * An exception has been taken from a context with IRQs enabled, which can only
> I think this should be: IRQs *disabled*?

Yes; whoops. I've fixed that up locally.

> > + * be an NMI.
> > + *
> > + * The entry code called us with DAIF.IF set to keep NMIs masked. We must leave
> > + * DAIF.IF (and ICC_PMR_EL1) unchanged.
> > + *
> > + * The entry code has performed NMI entry.
> > + */
> > +static void __gic_handle_irq_from_irqsoff(struct pt_regs *regs)

Thanks,
Mark.

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