[PATCH v2 0/2] irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0

[PATCH v2 0/2] irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0

[Alexandru Elisei]

Trusted Firmware-A's default interrupt routing model is to clear
SCR_EL3.FIQ, which is the only case that GICv3 doesn't support. This series
tries to fix that by detecting it at runtime and using a different priority
value for ICC_PMR_EL1 when masking regular interrupts. As a result, we will
be able to support pseudo-NMIs on all combinations of GIC security states
and firmware configurations.

The series is based on v5.9-rc1, the same as the PMU NMI patches [1] which
I used for testing.

The first patch was there because when I started working on the PMU NMI
patches I found it confusing that there was no clear message stating that
NMIs were successfully enabled. The second patch is the main patch of the
series, where pseudo-NMIs are enabled even if SCR_EL3.FIQ == 0.

The are still some things I'm not 100% sure about regarding the last patch:

- From my very limited experience of trying pseudo-NMIs on 3 machines
  (rockpro64, espressobin-v5 and v7), all of them had SCR_EL3.FIQ zero.
  I tend to believe that since this is the default on TFA, this will also
  be the common case on hardware. However, when Linux is a KVM guest, the
  original set of priorities is used because GIC security is disabled.
  I erred on the side of caution and chose the original set of priorities
  as the common case.

- Most of the changes to arch_local_irq_enable() might seem pointless, but
  I chose to make them so the function looks similar to
  arch_local_irq_disable(). The generated code is identical if the static
  branch is not taken. I tried changing only arch_local_irq_disable(), but
  the enable function ended up looking strangely asymmetrical. If someone
  suggests a better way of doing things, I'd be happy to implement it.

As an aside, the set of priorities that I've added would work in all cases
if there was no need to use GIC_PRIO_PSR_I_SET, but that is a much more
intrusive change and I'm not comfortable attempting it. I'm pretty sure I
will end up breaking things really badly.

I've tested the series using PMU NMIs on the model and on espressobin-v7.
To make testing as painless as possible, I've pushed a branch [1] with
these patches cherry-picked on top of the latest PMU NMI series:

$ git clone -b pmu-nmi-v6-nmi-fiq-clear-v2 git://linux-arm.org/linux-ae

Tests that I've run:

1. On the model:
- Host with SCR_EL3.FIQ == 1 (so using the original priorities), ran
  perf record -a -- iperf3 -c 127.0.0.1 -t 30.
- On a KVM guest (security disabled, so using the original priorities),
  ran the same command as above.

2. On an espressobin-v7:
- Host with SCR_EL3.FIQ == 0 (using the priority added by the series),
  ran perf record -a -- iperf3 -c 127.0.0.1 -t 60.
- On a KVM guest (security disabled, so using the original priorities),
  ran the same command.
- Stress test for two hours with CONFIG_ARM64_DEBUG_PRIORITY_MASKING set
  for the host and guest. On the host, I ran in parallel
  perf record -a -- iperf3 -c 127.0.0.1 -t 7200  and
  perf record -ae L1-dcache-loads -a -- sleep 7200. On the guest, I ran
  the same iperf3 command as on the host.

Changes since v1:
* Rebased on top of v5.9-rc1
* Changed pmr to u64 in arch_local_irq_{enable,disable} to stop
  clang from complaining and to match local_daif_restore().

[1] http://www.linux-arm.org/git?p=linux-ae.git;a=shortlog;h=refs/heads/pmu-nmi-v6-nmi-fiq-clear-v2

Alexandru Elisei (2):
  irqchip/gicv3: Spell out when pseudo-NMIs are enabled
  irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0

 arch/arm64/include/asm/arch_gicv3.h |  8 ++++-
 arch/arm64/include/asm/daifflags.h  |  4 +--
 arch/arm64/include/asm/irqflags.h   | 18 ++++++----
 arch/arm64/include/asm/ptrace.h     | 12 +++++++
 arch/arm64/kernel/entry.S           |  2 +-
 arch/arm64/kernel/image-vars.h      |  2 ++
 arch/arm64/kvm/hyp/nvhe/switch.c    |  2 +-
 drivers/irqchip/irq-gic-v3.c        | 56 +++++++++++++++++++++--------
 8 files changed, 79 insertions(+), 25 deletions(-)

-- 
2.28.0


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

[PATCH v2 1/2] irqchip/gicv3: Spell out when pseudo-NMIs are enabled

[Alexandru Elisei]

When NMIs cannot be enabled, the driver prints a message stating that
unambiguously. When they are enabled, the only feedback we get is a message
regarding the use of synchronization for ICC_PMR_EL1 writes, which is not
as useful for a user who is not intimately familiar with how NMIs are
implemented.

Let's make it obvious that pseudo-NMIs are enabled. Keep the message about
using a barrier for ICC_PMR_EL1 writes, because it has a non-negligible
impact on performance.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 drivers/irqchip/irq-gic-v3.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 324f280ff606..ce8944ae1b84 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1564,8 +1564,8 @@ static void gic_enable_nmi_support(void)
 	if (gic_read_ctlr() & ICC_CTLR_EL1_PMHE_MASK)
 		static_branch_enable(&gic_pmr_sync);
 
-	pr_info("%s ICC_PMR_EL1 synchronisation\n",
-		static_branch_unlikely(&gic_pmr_sync) ? "Forcing" : "Relaxing");
+	pr_info("Pseudo-NMIs enabled using %s ICC_PMR_EL1 synchronisation\n",
+		static_branch_unlikely(&gic_pmr_sync) ? "forced" : "relaxed");
 
 	static_branch_enable(&supports_pseudo_nmis);
 
-- 
2.28.0


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

[PATCH v2 2/2] irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0

[Alexandru Elisei]

The GIC's internal view of the priority mask register and the assigned
interrupt priorities are based on whether GIC security is enabled and
whether firmware routes Group 0 interrupts to EL3. At the moment, we
support priority masking when ICC_PMR_EL1 and interrupt priorities are
either both modified by the GIC, or both left unchanged.

Trusted Firmware-A's default interrupt routing model allows Group 0
interrupts to be delivered to the non-secure world (SCR_EL3.FIQ == 0).
Unfortunately, this is precisely the case that the GIC driver doesn't
support: ICC_PMR_EL1 remains unchanged, but the GIC's view of interrupt
priorities is different from the software programmed values.

Support pseudo-NMIs when SCR_EL3.FIQ == 0 by using a different value to
mask regular interrupts. All the other values remain the same.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arch/arm64/include/asm/arch_gicv3.h |  8 ++++-
 arch/arm64/include/asm/daifflags.h  |  4 +--
 arch/arm64/include/asm/irqflags.h   | 18 ++++++----
 arch/arm64/include/asm/ptrace.h     | 12 +++++++
 arch/arm64/kernel/entry.S           |  2 +-
 arch/arm64/kernel/image-vars.h      |  2 ++
 arch/arm64/kvm/hyp/nvhe/switch.c    |  2 +-
 drivers/irqchip/irq-gic-v3.c        | 52 ++++++++++++++++++++++-------
 8 files changed, 77 insertions(+), 23 deletions(-)

diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
index 6647ae4f0231..908152e8659b 100644
--- a/arch/arm64/include/asm/arch_gicv3.h
+++ b/arch/arm64/include/asm/arch_gicv3.h
@@ -162,7 +162,13 @@ static inline void gic_pmr_mask_irqs(void)
 	 * are applied to IRQ priorities
 	 */
 	BUILD_BUG_ON((0x80 | (GICD_INT_DEF_PRI >> 1)) >= GIC_PRIO_IRQON);
-	gic_write_pmr(GIC_PRIO_IRQOFF);
+	/*
+	 * Same situation as above, but now we make sure that we can mask
+	 * regular interrupts.
+	 */
+	BUILD_BUG_ON((0x80 | (GICD_INT_DEF_PRI >> 1)) < (GIC_PRIO_IRQOFF_NS |
+							 GIC_PRIO_PSR_I_SET));
+	gic_write_pmr(gic_prio_irqoff());
 }
 
 static inline void gic_arch_enable_irqs(void)
diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
index ec213b4a1650..3efa240a6c48 100644
--- a/arch/arm64/include/asm/daifflags.h
+++ b/arch/arm64/include/asm/daifflags.h
@@ -22,7 +22,7 @@
 static inline void local_daif_mask(void)
 {
 	WARN_ON(system_has_prio_mask_debugging() &&
-		(read_sysreg_s(SYS_ICC_PMR_EL1) == (GIC_PRIO_IRQOFF |
+		(read_sysreg_s(SYS_ICC_PMR_EL1) == (gic_prio_irqoff() |
 						    GIC_PRIO_PSR_I_SET)));
 
 	asm volatile(
@@ -87,7 +87,7 @@ static inline void local_daif_restore(unsigned long flags)
 			 * asynchronous errors, we can take NMIs
 			 */
 			flags &= ~PSR_I_BIT;
-			pmr = GIC_PRIO_IRQOFF;
+			pmr = gic_prio_irqoff();
 		} else {
 			pmr = GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET;
 		}
diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
index aa4b6521ef14..af353c78d5f8 100644
--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h
@@ -28,10 +28,13 @@
  */
 static inline void arch_local_irq_enable(void)
 {
+	u64 pmr_irqon = GIC_PRIO_IRQON;
+
 	if (system_has_prio_mask_debugging()) {
-		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
+		u64 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
+		u64 pmr_irqoff = gic_prio_irqoff();
 
-		WARN_ON_ONCE(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
+		WARN_ON_ONCE(pmr != pmr_irqon && pmr != pmr_irqoff);
 	}
 
 	asm volatile(ALTERNATIVE(
@@ -39,7 +42,7 @@ static inline void arch_local_irq_enable(void)
 		__msr_s(SYS_ICC_PMR_EL1, "%0"),
 		ARM64_HAS_IRQ_PRIO_MASKING)
 		:
-		: "r" ((unsigned long) GIC_PRIO_IRQON)
+		: "r" (pmr_irqon)
 		: "memory");
 
 	pmr_sync();
@@ -47,10 +50,13 @@ static inline void arch_local_irq_enable(void)
 
 static inline void arch_local_irq_disable(void)
 {
+	u64 pmr_irqoff = gic_prio_irqoff();
+
 	if (system_has_prio_mask_debugging()) {
-		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
+		u64 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
+		u64 pmr_irqon = GIC_PRIO_IRQON;
 
-		WARN_ON_ONCE(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
+		WARN_ON_ONCE(pmr != pmr_irqon && pmr != pmr_irqoff);
 	}
 
 	asm volatile(ALTERNATIVE(
@@ -58,7 +64,7 @@ static inline void arch_local_irq_disable(void)
 		__msr_s(SYS_ICC_PMR_EL1, "%0"),
 		ARM64_HAS_IRQ_PRIO_MASKING)
 		:
-		: "r" ((unsigned long) GIC_PRIO_IRQOFF)
+		: "r" (pmr_irqoff)
 		: "memory");
 }
 
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 966ed30ed5f7..a19cd6ff4d1b 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -32,6 +32,7 @@
  */
 #define GIC_PRIO_IRQON			0xe0
 #define GIC_PRIO_IRQOFF			(GIC_PRIO_IRQON & ~0x80)
+#define GIC_PRIO_IRQOFF_NS		0xa0
 #define GIC_PRIO_PSR_I_SET		(1 << 4)
 
 /* Additional SPSR bits not exposed in the UABI */
@@ -129,6 +130,17 @@
 #define compat_sp_fiq	regs[29]
 #define compat_lr_fiq	regs[30]
 
+#define gic_prio_irqoff()						\
+	({								\
+		extern struct static_key_false gic_nonsecure_priorities;\
+		u8 __prio = GIC_PRIO_IRQOFF;				\
+									\
+		if (static_branch_unlikely(&gic_nonsecure_priorities))	\
+			__prio = GIC_PRIO_IRQOFF_NS;			\
+									\
+		__prio;							\
+	})
+
 static inline unsigned long compat_psr_to_pstate(const unsigned long psr)
 {
 	unsigned long pstate;
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 2646178c8329..e4fa944dbf1d 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -653,7 +653,7 @@ alternative_else_nop_endif
 #ifdef CONFIG_ARM64_PSEUDO_NMI
 	/*
 	 * 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
+	 * PMR is set to mask interrupts. An NMI might also have occurred in a
 	 * section with interrupts disabled. Skip tracing in those cases.
 	 */
 	test_irqs_unmasked	res=x0, pmr=x20
diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-vars.h
index 9e897c500237..c4476a99dee8 100644
--- a/arch/arm64/kernel/image-vars.h
+++ b/arch/arm64/kernel/image-vars.h
@@ -101,6 +101,8 @@ KVM_NVHE_ALIAS(vgic_v3_cpuif_trap);
 /* Static key checked in pmr_sync(). */
 #ifdef CONFIG_ARM64_PSEUDO_NMI
 KVM_NVHE_ALIAS(gic_pmr_sync);
+/* Static key checked in gic_prio_irqoff(). */
+KVM_NVHE_ALIAS(gic_nonsecure_priorities);
 #endif
 
 #endif /* CONFIG_KVM */
diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
index 341be2f2f312..729a3a59ad6a 100644
--- a/arch/arm64/kvm/hyp/nvhe/switch.c
+++ b/arch/arm64/kvm/hyp/nvhe/switch.c
@@ -237,7 +237,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 
 	/* Returning to host will clear PSR.I, remask PMR if needed */
 	if (system_uses_irq_prio_masking())
-		gic_write_pmr(GIC_PRIO_IRQOFF);
+		gic_write_pmr(gic_prio_irqoff());
 
 	return exit_code;
 }
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index ce8944ae1b84..19e52c025c59 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -75,16 +75,14 @@ static DEFINE_STATIC_KEY_TRUE(supports_deactivate_key);
  *
  * If SCR_EL3.FIQ == 1, the values writen to/read from PMR and RPR at non-secure
  * EL1 are subject to a similar operation thus matching the priorities presented
- * from the (re)distributor when security is enabled.
+ * from the (re)distributor when security is enabled. When SCR_EL3.FIQ == 0,
+ * these values are unchanched by the GIC.
  *
  * see GICv3/GICv4 Architecture Specification (IHI0069D):
  * - section 4.8.1 Non-secure accesses to register fields for Secure interrupt
  *   priorities.
  * - Figure 4-7 Secure read of the priority field for a Non-secure Group 1
  *   interrupt.
- *
- * For now, we only support pseudo-NMIs if we have non-secure view of
- * priorities.
  */
 static DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis);
 
@@ -97,6 +95,9 @@ static DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis);
 DEFINE_STATIC_KEY_FALSE(gic_pmr_sync);
 EXPORT_SYMBOL(gic_pmr_sync);
 
+DEFINE_STATIC_KEY_FALSE(gic_nonsecure_priorities);
+EXPORT_SYMBOL(gic_nonsecure_priorities);
+
 /* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */
 static refcount_t *ppi_nmi_refs;
 
@@ -932,14 +933,16 @@ static void gic_cpu_sys_reg_init(void)
 	/* Set priority mask register */
 	if (!gic_prio_masking_enabled()) {
 		write_gicreg(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
-	} else {
+	} else if (gic_supports_nmi()) {
 		/*
 		 * Mismatch configuration with boot CPU, the system is likely
 		 * to die as interrupt masking will not work properly on all
 		 * CPUs
 		 */
-		WARN_ON(gic_supports_nmi() && group0 &&
-			!gic_dist_security_disabled());
+		if (static_branch_unlikely(&gic_nonsecure_priorities))
+			WARN_ON(!group0 || gic_dist_security_disabled());
+		else
+			WARN_ON(group0 && !gic_dist_security_disabled());
 	}
 
 	/*
@@ -1544,11 +1547,6 @@ static void gic_enable_nmi_support(void)
 	if (!gic_prio_masking_enabled())
 		return;
 
-	if (gic_has_group0() && !gic_dist_security_disabled()) {
-		pr_warn("SCR_EL3.FIQ is cleared, cannot enable use of pseudo-NMIs\n");
-		return;
-	}
-
 	ppi_nmi_refs = kcalloc(gic_data.ppi_nr, sizeof(*ppi_nmi_refs), GFP_KERNEL);
 	if (!ppi_nmi_refs)
 		return;
@@ -1567,6 +1565,36 @@ static void gic_enable_nmi_support(void)
 	pr_info("Pseudo-NMIs enabled using %s ICC_PMR_EL1 synchronisation\n",
 		static_branch_unlikely(&gic_pmr_sync) ? "forced" : "relaxed");
 
+	/*
+	 * How priority values are used by the GIC depends on two things:
+	 * the security state of the GIC (controlled by the GICD_CTRL.DS bit)
+	 * and if Group 0 interrupts can be delivered to Linux in the non-secure
+	 * world as FIQs (controlled by the SCR_EL3.FIQ bit). These affect the
+	 * the ICC_PMR_EL1 register and the priority that software assigns to
+	 * interrupts:
+	 *
+	 * GICD_CTRL.DS | SCR_EL3.FIQ | ICC_PMR_EL1 | Group 1 priority
+	 * -----------------------------------------------------------
+	 *      1       |      -      |  unchanged  |    unchanged
+	 * -----------------------------------------------------------
+	 *      0       |      1      |  non-secure |    non-secure
+	 * -----------------------------------------------------------
+	 *      0       |      0      |  unchanged  |    non-secure
+	 *
+	 * where non-secure means that the value is right-shifted by one and the
+	 * MSB bit set, to make it fit in the non-secure priority range.
+	 *
+	 * In the first two cases, where ICC_PMR_EL1 and the interrupt priority
+	 * are both either modified, or unchanged, we can use the same set of
+	 * priorities.
+	 *
+	 * In the last case, where only the interrupt priorities are modified to
+	 * be in the non-secure range, we use a different PMR value to mask IRQs
+	 * and the rest of the values that we use remain unchanged.
+	 */
+	if (gic_has_group0() && !gic_dist_security_disabled())
+		static_branch_enable(&gic_nonsecure_priorities);
+
 	static_branch_enable(&supports_pseudo_nmis);
 
 	if (static_branch_likely(&supports_deactivate_key))
-- 
2.28.0


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

Re: [PATCH v2 2/2] irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0

[Marc Zyngier]

Hi Alex,

Thanks for taking the time for putting this together, as it has been a
long standing issue that needed fixing.

On Wed, 19 Aug 2020 14:36:30 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> The GIC's internal view of the priority mask register and the assigned
> interrupt priorities are based on whether GIC security is enabled and
> whether firmware routes Group 0 interrupts to EL3. At the moment, we
> support priority masking when ICC_PMR_EL1 and interrupt priorities are
> either both modified by the GIC, or both left unchanged.
> 
> Trusted Firmware-A's default interrupt routing model allows Group 0
> interrupts to be delivered to the non-secure world (SCR_EL3.FIQ == 0).
> Unfortunately, this is precisely the case that the GIC driver doesn't
> support: ICC_PMR_EL1 remains unchanged, but the GIC's view of interrupt
> priorities is different from the software programmed values.
> 
> Support pseudo-NMIs when SCR_EL3.FIQ == 0 by using a different value to
> mask regular interrupts. All the other values remain the same.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arch/arm64/include/asm/arch_gicv3.h |  8 ++++-
>  arch/arm64/include/asm/daifflags.h  |  4 +--
>  arch/arm64/include/asm/irqflags.h   | 18 ++++++----
>  arch/arm64/include/asm/ptrace.h     | 12 +++++++
>  arch/arm64/kernel/entry.S           |  2 +-
>  arch/arm64/kernel/image-vars.h      |  2 ++
>  arch/arm64/kvm/hyp/nvhe/switch.c    |  2 +-
>  drivers/irqchip/irq-gic-v3.c        | 52 ++++++++++++++++++++++-------
>  8 files changed, 77 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
> index 6647ae4f0231..908152e8659b 100644
> --- a/arch/arm64/include/asm/arch_gicv3.h
> +++ b/arch/arm64/include/asm/arch_gicv3.h
> @@ -162,7 +162,13 @@ static inline void gic_pmr_mask_irqs(void)
>  	 * are applied to IRQ priorities
>  	 */
>  	BUILD_BUG_ON((0x80 | (GICD_INT_DEF_PRI >> 1)) >= GIC_PRIO_IRQON);
> -	gic_write_pmr(GIC_PRIO_IRQOFF);
> +	/*
> +	 * Same situation as above, but now we make sure that we can mask
> +	 * regular interrupts.
> +	 */
> +	BUILD_BUG_ON((0x80 | (GICD_INT_DEF_PRI >> 1)) < (GIC_PRIO_IRQOFF_NS |
> +							 GIC_PRIO_PSR_I_SET));
> +	gic_write_pmr(gic_prio_irqoff());
>  }
>  
>  static inline void gic_arch_enable_irqs(void)
> diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
> index ec213b4a1650..3efa240a6c48 100644
> --- a/arch/arm64/include/asm/daifflags.h
> +++ b/arch/arm64/include/asm/daifflags.h
> @@ -22,7 +22,7 @@
>  static inline void local_daif_mask(void)
>  {
>  	WARN_ON(system_has_prio_mask_debugging() &&
> -		(read_sysreg_s(SYS_ICC_PMR_EL1) == (GIC_PRIO_IRQOFF |
> +		(read_sysreg_s(SYS_ICC_PMR_EL1) == (gic_prio_irqoff() |
>  						    GIC_PRIO_PSR_I_SET)));
>  
>  	asm volatile(
> @@ -87,7 +87,7 @@ static inline void local_daif_restore(unsigned long flags)
>  			 * asynchronous errors, we can take NMIs
>  			 */
>  			flags &= ~PSR_I_BIT;
> -			pmr = GIC_PRIO_IRQOFF;
> +			pmr = gic_prio_irqoff();
>  		} else {
>  			pmr = GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET;
>  		}
> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
> index aa4b6521ef14..af353c78d5f8 100644
> --- a/arch/arm64/include/asm/irqflags.h
> +++ b/arch/arm64/include/asm/irqflags.h
> @@ -28,10 +28,13 @@
>   */
>  static inline void arch_local_irq_enable(void)
>  {
> +	u64 pmr_irqon = GIC_PRIO_IRQON;
> +
>  	if (system_has_prio_mask_debugging()) {
> -		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
> +		u64 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
> +		u64 pmr_irqoff = gic_prio_irqoff();
>  
> -		WARN_ON_ONCE(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
> +		WARN_ON_ONCE(pmr != pmr_irqon && pmr != pmr_irqoff);
>  	}
>  
>  	asm volatile(ALTERNATIVE(
> @@ -39,7 +42,7 @@ static inline void arch_local_irq_enable(void)
>  		__msr_s(SYS_ICC_PMR_EL1, "%0"),
>  		ARM64_HAS_IRQ_PRIO_MASKING)
>  		:
> -		: "r" ((unsigned long) GIC_PRIO_IRQON)
> +		: "r" (pmr_irqon)
>  		: "memory");
>  
>  	pmr_sync();
> @@ -47,10 +50,13 @@ static inline void arch_local_irq_enable(void)
>  
>  static inline void arch_local_irq_disable(void)
>  {
> +	u64 pmr_irqoff = gic_prio_irqoff();
> +
>  	if (system_has_prio_mask_debugging()) {
> -		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
> +		u64 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
> +		u64 pmr_irqon = GIC_PRIO_IRQON;
>  
> -		WARN_ON_ONCE(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
> +		WARN_ON_ONCE(pmr != pmr_irqon && pmr != pmr_irqoff);
>  	}
>  
>  	asm volatile(ALTERNATIVE(
> @@ -58,7 +64,7 @@ static inline void arch_local_irq_disable(void)
>  		__msr_s(SYS_ICC_PMR_EL1, "%0"),
>  		ARM64_HAS_IRQ_PRIO_MASKING)
>  		:
> -		: "r" ((unsigned long) GIC_PRIO_IRQOFF)
> +		: "r" (pmr_irqoff)
>  		: "memory");
>  }

I believe all the changes in this file can be avoided, see below.

>  
> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> index 966ed30ed5f7..a19cd6ff4d1b 100644
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -32,6 +32,7 @@
>   */
>  #define GIC_PRIO_IRQON			0xe0
>  #define GIC_PRIO_IRQOFF			(GIC_PRIO_IRQON & ~0x80)
> +#define GIC_PRIO_IRQOFF_NS		0xa0
>  #define GIC_PRIO_PSR_I_SET		(1 << 4)
>  
>  /* Additional SPSR bits not exposed in the UABI */
> @@ -129,6 +130,17 @@
>  #define compat_sp_fiq	regs[29]
>  #define compat_lr_fiq	regs[30]
>  
> +#define gic_prio_irqoff()						\
> +	({								\
> +		extern struct static_key_false gic_nonsecure_priorities;\
> +		u8 __prio = GIC_PRIO_IRQOFF;				\
> +									\
> +		if (static_branch_unlikely(&gic_nonsecure_priorities))	\
> +			__prio = GIC_PRIO_IRQOFF_NS;			\
> +									\
> +		__prio;							\
> +	})
> +

This single change is causing quite a lot of churn, most of which
could be avoided if you actually reused the macro name:

diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 966ed30ed5f7..f85a00817fa5 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -31,9 +31,21 @@
  * interrupt disabling temporarily does not rely on IRQ priorities.
  */
 #define GIC_PRIO_IRQON			0xe0
-#define GIC_PRIO_IRQOFF			(GIC_PRIO_IRQON & ~0x80)
+#define __GIC_PRIO_IRQOFF		(GIC_PRIO_IRQON & ~0x80)
+#define __GIC_PRIO_IRQOFF_NS		0xa0
 #define GIC_PRIO_PSR_I_SET		(1 << 4)
 
+#define GIC_PRIO_IRQOFF						\
+	({								\
+		extern struct static_key_false gic_nonsecure_priorities;\
+		u8 __prio = __GIC_PRIO_IRQOFF;				\
+									\
+		if (static_branch_unlikely(&gic_nonsecure_priorities))	\
+			__prio = __GIC_PRIO_IRQOFF_NS;			\
+									\
+		__prio;							\
+	})
+
 /* Additional SPSR bits not exposed in the UABI */
 #define PSR_MODE_THREAD_BIT	(1 << 0)
 #define PSR_IL_BIT		(1 << 20)

"With this simple trick", a lot of the churn in this patch vanishes:

 arch/arm64/include/asm/arch_gicv3.h |  8 +++++-
 arch/arm64/include/asm/ptrace.h     | 14 +++++++++-
 arch/arm64/kernel/entry.S           |  2 +-
 arch/arm64/kernel/image-vars.h      |  2 ++
 drivers/irqchip/irq-gic-v3.c        | 52 ++++++++++++++++++++++++++++---------
 5 files changed, 63 insertions(+), 15 deletions(-)

>  static inline unsigned long compat_psr_to_pstate(const unsigned long psr)
>  {
>  	unsigned long pstate;
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 2646178c8329..e4fa944dbf1d 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -653,7 +653,7 @@ alternative_else_nop_endif
>  #ifdef CONFIG_ARM64_PSEUDO_NMI
>  	/*
>  	 * 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
> +	 * PMR is set to mask interrupts. An NMI might also have occurred in a
>  	 * section with interrupts disabled. Skip tracing in those cases.
>  	 */
>  	test_irqs_unmasked	res=x0, pmr=x20
> diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-vars.h
> index 9e897c500237..c4476a99dee8 100644
> --- a/arch/arm64/kernel/image-vars.h
> +++ b/arch/arm64/kernel/image-vars.h
> @@ -101,6 +101,8 @@ KVM_NVHE_ALIAS(vgic_v3_cpuif_trap);
>  /* Static key checked in pmr_sync(). */
>  #ifdef CONFIG_ARM64_PSEUDO_NMI
>  KVM_NVHE_ALIAS(gic_pmr_sync);
> +/* Static key checked in gic_prio_irqoff(). */
> +KVM_NVHE_ALIAS(gic_nonsecure_priorities);
>  #endif
>  
>  #endif /* CONFIG_KVM */
> diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
> index 341be2f2f312..729a3a59ad6a 100644
> --- a/arch/arm64/kvm/hyp/nvhe/switch.c
> +++ b/arch/arm64/kvm/hyp/nvhe/switch.c
> @@ -237,7 +237,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
>  
>  	/* Returning to host will clear PSR.I, remask PMR if needed */
>  	if (system_uses_irq_prio_masking())
> -		gic_write_pmr(GIC_PRIO_IRQOFF);
> +		gic_write_pmr(gic_prio_irqoff());
>  
>  	return exit_code;
>  }
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index ce8944ae1b84..19e52c025c59 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -75,16 +75,14 @@ static DEFINE_STATIC_KEY_TRUE(supports_deactivate_key);
>   *
>   * If SCR_EL3.FIQ == 1, the values writen to/read from PMR and RPR at non-secure
>   * EL1 are subject to a similar operation thus matching the priorities presented
> - * from the (re)distributor when security is enabled.
> + * from the (re)distributor when security is enabled. When SCR_EL3.FIQ == 0,
> + * these values are unchanched by the GIC.
>   *
>   * see GICv3/GICv4 Architecture Specification (IHI0069D):
>   * - section 4.8.1 Non-secure accesses to register fields for Secure interrupt
>   *   priorities.
>   * - Figure 4-7 Secure read of the priority field for a Non-secure Group 1
>   *   interrupt.
> - *
> - * For now, we only support pseudo-NMIs if we have non-secure view of
> - * priorities.
>   */
>  static DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis);
>  
> @@ -97,6 +95,9 @@ static DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis);
>  DEFINE_STATIC_KEY_FALSE(gic_pmr_sync);
>  EXPORT_SYMBOL(gic_pmr_sync);
>  
> +DEFINE_STATIC_KEY_FALSE(gic_nonsecure_priorities);
> +EXPORT_SYMBOL(gic_nonsecure_priorities);
> +
>  /* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */
>  static refcount_t *ppi_nmi_refs;
>  
> @@ -932,14 +933,16 @@ static void gic_cpu_sys_reg_init(void)
>  	/* Set priority mask register */
>  	if (!gic_prio_masking_enabled()) {
>  		write_gicreg(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
> -	} else {
> +	} else if (gic_supports_nmi()) {
>  		/*
>  		 * Mismatch configuration with boot CPU, the system is likely
>  		 * to die as interrupt masking will not work properly on all
>  		 * CPUs
>  		 */
> -		WARN_ON(gic_supports_nmi() && group0 &&
> -			!gic_dist_security_disabled());
> +		if (static_branch_unlikely(&gic_nonsecure_priorities))
> +			WARN_ON(!group0 || gic_dist_security_disabled());
> +		else
> +			WARN_ON(group0 && !gic_dist_security_disabled());

It'd be worth adding a comment saying that this never runs on the boot
CPU (I just spent 10 minutes wondering why this worked).

>  	}
>  
>  	/*
> @@ -1544,11 +1547,6 @@ static void gic_enable_nmi_support(void)
>  	if (!gic_prio_masking_enabled())
>  		return;
>  
> -	if (gic_has_group0() && !gic_dist_security_disabled()) {
> -		pr_warn("SCR_EL3.FIQ is cleared, cannot enable use of pseudo-NMIs\n");
> -		return;
> -	}
> -
>  	ppi_nmi_refs = kcalloc(gic_data.ppi_nr, sizeof(*ppi_nmi_refs), GFP_KERNEL);
>  	if (!ppi_nmi_refs)
>  		return;
> @@ -1567,6 +1565,36 @@ static void gic_enable_nmi_support(void)
>  	pr_info("Pseudo-NMIs enabled using %s ICC_PMR_EL1 synchronisation\n",
>  		static_branch_unlikely(&gic_pmr_sync) ? "forced" : "relaxed");
>  
> +	/*
> +	 * How priority values are used by the GIC depends on two things:
> +	 * the security state of the GIC (controlled by the GICD_CTRL.DS bit)
> +	 * and if Group 0 interrupts can be delivered to Linux in the non-secure
> +	 * world as FIQs (controlled by the SCR_EL3.FIQ bit). These affect the
> +	 * the ICC_PMR_EL1 register and the priority that software assigns to
> +	 * interrupts:
> +	 *
> +	 * GICD_CTRL.DS | SCR_EL3.FIQ | ICC_PMR_EL1 | Group 1 priority
> +	 * -----------------------------------------------------------
> +	 *      1       |      -      |  unchanged  |    unchanged
> +	 * -----------------------------------------------------------
> +	 *      0       |      1      |  non-secure |    non-secure
> +	 * -----------------------------------------------------------
> +	 *      0       |      0      |  unchanged  |    non-secure
> +	 *
> +	 * where non-secure means that the value is right-shifted by one and the
> +	 * MSB bit set, to make it fit in the non-secure priority range.
> +	 *
> +	 * In the first two cases, where ICC_PMR_EL1 and the interrupt priority
> +	 * are both either modified, or unchanged, we can use the same set of
> +	 * priorities.
> +	 *
> +	 * In the last case, where only the interrupt priorities are modified to
> +	 * be in the non-secure range, we use a different PMR value to mask IRQs
> +	 * and the rest of the values that we use remain unchanged.
> +	 */
> +	if (gic_has_group0() && !gic_dist_security_disabled())
> +		static_branch_enable(&gic_nonsecure_priorities);
> +
>  	static_branch_enable(&supports_pseudo_nmis);
>  
>  	if (static_branch_likely(&supports_deactivate_key))
> -- 
> 2.28.0
> 
> 

Otherwise, this looks pretty good, and I'd like to take this into 5.10
if you can respin it quickly.

Thanks,

	M.

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

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

Re: [PATCH v2 2/2] irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0

[Alexandru Elisei]

Hi Marc,

On 9/12/20 1:00 PM, Marc Zyngier wrote:
> Hi Alex,
>
> Thanks for taking the time for putting this together, as it has been a
> long standing issue that needed fixing.
>
> On Wed, 19 Aug 2020 14:36:30 +0100,
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>> The GIC's internal view of the priority mask register and the assigned
>> interrupt priorities are based on whether GIC security is enabled and
>> whether firmware routes Group 0 interrupts to EL3. At the moment, we
>> support priority masking when ICC_PMR_EL1 and interrupt priorities are
>> either both modified by the GIC, or both left unchanged.
>>
>> Trusted Firmware-A's default interrupt routing model allows Group 0
>> interrupts to be delivered to the non-secure world (SCR_EL3.FIQ == 0).
>> Unfortunately, this is precisely the case that the GIC driver doesn't
>> support: ICC_PMR_EL1 remains unchanged, but the GIC's view of interrupt
>> priorities is different from the software programmed values.
>>
>> Support pseudo-NMIs when SCR_EL3.FIQ == 0 by using a different value to
>> mask regular interrupts. All the other values remain the same.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  arch/arm64/include/asm/arch_gicv3.h |  8 ++++-
>>  arch/arm64/include/asm/daifflags.h  |  4 +--
>>  arch/arm64/include/asm/irqflags.h   | 18 ++++++----
>>  arch/arm64/include/asm/ptrace.h     | 12 +++++++
>>  arch/arm64/kernel/entry.S           |  2 +-
>>  arch/arm64/kernel/image-vars.h      |  2 ++
>>  arch/arm64/kvm/hyp/nvhe/switch.c    |  2 +-
>>  drivers/irqchip/irq-gic-v3.c        | 52 ++++++++++++++++++++++-------
>>  8 files changed, 77 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
>> index 6647ae4f0231..908152e8659b 100644
>> --- a/arch/arm64/include/asm/arch_gicv3.h
>> +++ b/arch/arm64/include/asm/arch_gicv3.h
>> @@ -162,7 +162,13 @@ static inline void gic_pmr_mask_irqs(void)
>>  	 * are applied to IRQ priorities
>>  	 */
>>  	BUILD_BUG_ON((0x80 | (GICD_INT_DEF_PRI >> 1)) >= GIC_PRIO_IRQON);
>> -	gic_write_pmr(GIC_PRIO_IRQOFF);
>> +	/*
>> +	 * Same situation as above, but now we make sure that we can mask
>> +	 * regular interrupts.
>> +	 */
>> +	BUILD_BUG_ON((0x80 | (GICD_INT_DEF_PRI >> 1)) < (GIC_PRIO_IRQOFF_NS |
>> +							 GIC_PRIO_PSR_I_SET));
>> +	gic_write_pmr(gic_prio_irqoff());
>>  }
>>  
>>  static inline void gic_arch_enable_irqs(void)
>> diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
>> index ec213b4a1650..3efa240a6c48 100644
>> --- a/arch/arm64/include/asm/daifflags.h
>> +++ b/arch/arm64/include/asm/daifflags.h
>> @@ -22,7 +22,7 @@
>>  static inline void local_daif_mask(void)
>>  {
>>  	WARN_ON(system_has_prio_mask_debugging() &&
>> -		(read_sysreg_s(SYS_ICC_PMR_EL1) == (GIC_PRIO_IRQOFF |
>> +		(read_sysreg_s(SYS_ICC_PMR_EL1) == (gic_prio_irqoff() |
>>  						    GIC_PRIO_PSR_I_SET)));
>>  
>>  	asm volatile(
>> @@ -87,7 +87,7 @@ static inline void local_daif_restore(unsigned long flags)
>>  			 * asynchronous errors, we can take NMIs
>>  			 */
>>  			flags &= ~PSR_I_BIT;
>> -			pmr = GIC_PRIO_IRQOFF;
>> +			pmr = gic_prio_irqoff();
>>  		} else {
>>  			pmr = GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET;
>>  		}
>> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
>> index aa4b6521ef14..af353c78d5f8 100644
>> --- a/arch/arm64/include/asm/irqflags.h
>> +++ b/arch/arm64/include/asm/irqflags.h
>> @@ -28,10 +28,13 @@
>>   */
>>  static inline void arch_local_irq_enable(void)
>>  {
>> +	u64 pmr_irqon = GIC_PRIO_IRQON;
>> +
>>  	if (system_has_prio_mask_debugging()) {
>> -		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
>> +		u64 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
>> +		u64 pmr_irqoff = gic_prio_irqoff();
>>  
>> -		WARN_ON_ONCE(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
>> +		WARN_ON_ONCE(pmr != pmr_irqon && pmr != pmr_irqoff);
>>  	}
>>  
>>  	asm volatile(ALTERNATIVE(
>> @@ -39,7 +42,7 @@ static inline void arch_local_irq_enable(void)
>>  		__msr_s(SYS_ICC_PMR_EL1, "%0"),
>>  		ARM64_HAS_IRQ_PRIO_MASKING)
>>  		:
>> -		: "r" ((unsigned long) GIC_PRIO_IRQON)
>> +		: "r" (pmr_irqon)
>>  		: "memory");
>>  
>>  	pmr_sync();
>> @@ -47,10 +50,13 @@ static inline void arch_local_irq_enable(void)
>>  
>>  static inline void arch_local_irq_disable(void)
>>  {
>> +	u64 pmr_irqoff = gic_prio_irqoff();
>> +
>>  	if (system_has_prio_mask_debugging()) {
>> -		u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
>> +		u64 pmr = read_sysreg_s(SYS_ICC_PMR_EL1);
>> +		u64 pmr_irqon = GIC_PRIO_IRQON;
>>  
>> -		WARN_ON_ONCE(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF);
>> +		WARN_ON_ONCE(pmr != pmr_irqon && pmr != pmr_irqoff);
>>  	}
>>  
>>  	asm volatile(ALTERNATIVE(
>> @@ -58,7 +64,7 @@ static inline void arch_local_irq_disable(void)
>>  		__msr_s(SYS_ICC_PMR_EL1, "%0"),
>>  		ARM64_HAS_IRQ_PRIO_MASKING)
>>  		:
>> -		: "r" ((unsigned long) GIC_PRIO_IRQOFF)
>> +		: "r" (pmr_irqoff)
>>  		: "memory");
>>  }
> I believe all the changes in this file can be avoided, see below.
>
>>  
>> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
>> index 966ed30ed5f7..a19cd6ff4d1b 100644
>> --- a/arch/arm64/include/asm/ptrace.h
>> +++ b/arch/arm64/include/asm/ptrace.h
>> @@ -32,6 +32,7 @@
>>   */
>>  #define GIC_PRIO_IRQON			0xe0
>>  #define GIC_PRIO_IRQOFF			(GIC_PRIO_IRQON & ~0x80)
>> +#define GIC_PRIO_IRQOFF_NS		0xa0
>>  #define GIC_PRIO_PSR_I_SET		(1 << 4)
>>  
>>  /* Additional SPSR bits not exposed in the UABI */
>> @@ -129,6 +130,17 @@
>>  #define compat_sp_fiq	regs[29]
>>  #define compat_lr_fiq	regs[30]
>>  
>> +#define gic_prio_irqoff()						\
>> +	({								\
>> +		extern struct static_key_false gic_nonsecure_priorities;\
>> +		u8 __prio = GIC_PRIO_IRQOFF;				\
>> +									\
>> +		if (static_branch_unlikely(&gic_nonsecure_priorities))	\
>> +			__prio = GIC_PRIO_IRQOFF_NS;			\
>> +									\
>> +		__prio;							\
>> +	})
>> +
> This single change is causing quite a lot of churn, most of which
> could be avoided if you actually reused the macro name:
>
> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> index 966ed30ed5f7..f85a00817fa5 100644
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -31,9 +31,21 @@
>   * interrupt disabling temporarily does not rely on IRQ priorities.
>   */
>  #define GIC_PRIO_IRQON			0xe0
> -#define GIC_PRIO_IRQOFF			(GIC_PRIO_IRQON & ~0x80)
> +#define __GIC_PRIO_IRQOFF		(GIC_PRIO_IRQON & ~0x80)
> +#define __GIC_PRIO_IRQOFF_NS		0xa0
>  #define GIC_PRIO_PSR_I_SET		(1 << 4)
>  
> +#define GIC_PRIO_IRQOFF						\
> +	({								\
> +		extern struct static_key_false gic_nonsecure_priorities;\
> +		u8 __prio = __GIC_PRIO_IRQOFF;				\
> +									\
> +		if (static_branch_unlikely(&gic_nonsecure_priorities))	\
> +			__prio = __GIC_PRIO_IRQOFF_NS;			\
> +									\
> +		__prio;							\
> +	})
> +
>  /* Additional SPSR bits not exposed in the UABI */
>  #define PSR_MODE_THREAD_BIT	(1 << 0)
>  #define PSR_IL_BIT		(1 << 20)
>
> "With this simple trick", a lot of the churn in this patch vanishes:

That's a very good idea, and the diffstat looks much better. I'll change it for v3.

>
>  arch/arm64/include/asm/arch_gicv3.h |  8 +++++-
>  arch/arm64/include/asm/ptrace.h     | 14 +++++++++-
>  arch/arm64/kernel/entry.S           |  2 +-
>  arch/arm64/kernel/image-vars.h      |  2 ++
>  drivers/irqchip/irq-gic-v3.c        | 52 ++++++++++++++++++++++++++++---------
>  5 files changed, 63 insertions(+), 15 deletions(-)
>
>>  static inline unsigned long compat_psr_to_pstate(const unsigned long psr)
>>  {
>>  	unsigned long pstate;
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index 2646178c8329..e4fa944dbf1d 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -653,7 +653,7 @@ alternative_else_nop_endif
>>  #ifdef CONFIG_ARM64_PSEUDO_NMI
>>  	/*
>>  	 * 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
>> +	 * PMR is set to mask interrupts. An NMI might also have occurred in a
>>  	 * section with interrupts disabled. Skip tracing in those cases.
>>  	 */
>>  	test_irqs_unmasked	res=x0, pmr=x20
>> diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-vars.h
>> index 9e897c500237..c4476a99dee8 100644
>> --- a/arch/arm64/kernel/image-vars.h
>> +++ b/arch/arm64/kernel/image-vars.h
>> @@ -101,6 +101,8 @@ KVM_NVHE_ALIAS(vgic_v3_cpuif_trap);
>>  /* Static key checked in pmr_sync(). */
>>  #ifdef CONFIG_ARM64_PSEUDO_NMI
>>  KVM_NVHE_ALIAS(gic_pmr_sync);
>> +/* Static key checked in gic_prio_irqoff(). */
>> +KVM_NVHE_ALIAS(gic_nonsecure_priorities);
>>  #endif
>>  
>>  #endif /* CONFIG_KVM */
>> diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
>> index 341be2f2f312..729a3a59ad6a 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/switch.c
>> +++ b/arch/arm64/kvm/hyp/nvhe/switch.c
>> @@ -237,7 +237,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
>>  
>>  	/* Returning to host will clear PSR.I, remask PMR if needed */
>>  	if (system_uses_irq_prio_masking())
>> -		gic_write_pmr(GIC_PRIO_IRQOFF);
>> +		gic_write_pmr(gic_prio_irqoff());
>>  
>>  	return exit_code;
>>  }
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index ce8944ae1b84..19e52c025c59 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -75,16 +75,14 @@ static DEFINE_STATIC_KEY_TRUE(supports_deactivate_key);
>>   *
>>   * If SCR_EL3.FIQ == 1, the values writen to/read from PMR and RPR at non-secure
>>   * EL1 are subject to a similar operation thus matching the priorities presented
>> - * from the (re)distributor when security is enabled.
>> + * from the (re)distributor when security is enabled. When SCR_EL3.FIQ == 0,
>> + * these values are unchanched by the GIC.
>>   *
>>   * see GICv3/GICv4 Architecture Specification (IHI0069D):
>>   * - section 4.8.1 Non-secure accesses to register fields for Secure interrupt
>>   *   priorities.
>>   * - Figure 4-7 Secure read of the priority field for a Non-secure Group 1
>>   *   interrupt.
>> - *
>> - * For now, we only support pseudo-NMIs if we have non-secure view of
>> - * priorities.
>>   */
>>  static DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis);
>>  
>> @@ -97,6 +95,9 @@ static DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis);
>>  DEFINE_STATIC_KEY_FALSE(gic_pmr_sync);
>>  EXPORT_SYMBOL(gic_pmr_sync);
>>  
>> +DEFINE_STATIC_KEY_FALSE(gic_nonsecure_priorities);
>> +EXPORT_SYMBOL(gic_nonsecure_priorities);
>> +
>>  /* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */
>>  static refcount_t *ppi_nmi_refs;
>>  
>> @@ -932,14 +933,16 @@ static void gic_cpu_sys_reg_init(void)
>>  	/* Set priority mask register */
>>  	if (!gic_prio_masking_enabled()) {
>>  		write_gicreg(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
>> -	} else {
>> +	} else if (gic_supports_nmi()) {
>>  		/*
>>  		 * Mismatch configuration with boot CPU, the system is likely
>>  		 * to die as interrupt masking will not work properly on all
>>  		 * CPUs
>>  		 */
>> -		WARN_ON(gic_supports_nmi() && group0 &&
>> -			!gic_dist_security_disabled());
>> +		if (static_branch_unlikely(&gic_nonsecure_priorities))
>> +			WARN_ON(!group0 || gic_dist_security_disabled());
>> +		else
>> +			WARN_ON(group0 && !gic_dist_security_disabled());
> It'd be worth adding a comment saying that this never runs on the boot
> CPU (I just spent 10 minutes wondering why this worked).

Sure, I'll update the comment to state that this branch is only taken before the
boot CPU has enabled pseudo-NMIs, because the call to gic_enable_nmi_support()
comes after gic_cpu_init() in gic_init_bases().

>
>>  	}
>>  
>>  	/*
>> @@ -1544,11 +1547,6 @@ static void gic_enable_nmi_support(void)
>>  	if (!gic_prio_masking_enabled())
>>  		return;
>>  
>> -	if (gic_has_group0() && !gic_dist_security_disabled()) {
>> -		pr_warn("SCR_EL3.FIQ is cleared, cannot enable use of pseudo-NMIs\n");
>> -		return;
>> -	}
>> -
>>  	ppi_nmi_refs = kcalloc(gic_data.ppi_nr, sizeof(*ppi_nmi_refs), GFP_KERNEL);
>>  	if (!ppi_nmi_refs)
>>  		return;
>> @@ -1567,6 +1565,36 @@ static void gic_enable_nmi_support(void)
>>  	pr_info("Pseudo-NMIs enabled using %s ICC_PMR_EL1 synchronisation\n",
>>  		static_branch_unlikely(&gic_pmr_sync) ? "forced" : "relaxed");
>>  
>> +	/*
>> +	 * How priority values are used by the GIC depends on two things:
>> +	 * the security state of the GIC (controlled by the GICD_CTRL.DS bit)
>> +	 * and if Group 0 interrupts can be delivered to Linux in the non-secure
>> +	 * world as FIQs (controlled by the SCR_EL3.FIQ bit). These affect the
>> +	 * the ICC_PMR_EL1 register and the priority that software assigns to
>> +	 * interrupts:
>> +	 *
>> +	 * GICD_CTRL.DS | SCR_EL3.FIQ | ICC_PMR_EL1 | Group 1 priority
>> +	 * -----------------------------------------------------------
>> +	 *      1       |      -      |  unchanged  |    unchanged
>> +	 * -----------------------------------------------------------
>> +	 *      0       |      1      |  non-secure |    non-secure
>> +	 * -----------------------------------------------------------
>> +	 *      0       |      0      |  unchanged  |    non-secure
>> +	 *
>> +	 * where non-secure means that the value is right-shifted by one and the
>> +	 * MSB bit set, to make it fit in the non-secure priority range.
>> +	 *
>> +	 * In the first two cases, where ICC_PMR_EL1 and the interrupt priority
>> +	 * are both either modified, or unchanged, we can use the same set of
>> +	 * priorities.
>> +	 *
>> +	 * In the last case, where only the interrupt priorities are modified to
>> +	 * be in the non-secure range, we use a different PMR value to mask IRQs
>> +	 * and the rest of the values that we use remain unchanged.
>> +	 */
>> +	if (gic_has_group0() && !gic_dist_security_disabled())
>> +		static_branch_enable(&gic_nonsecure_priorities);
>> +
>>  	static_branch_enable(&supports_pseudo_nmis);
>>  
>>  	if (static_branch_likely(&supports_deactivate_key))
>> -- 
>> 2.28.0
>>
>>
> Otherwise, this looks pretty good, and I'd like to take this into 5.10
> if you can respin it quickly.

Sure, I'll respin it as soon as possible and send a v3.

Have you tested the series using the PMU NMI branch from the cover letter? If so,
would you mind giving a Tested-by tag for that series [1]?

[1] https://lkml.org/lkml/2020/8/19/671

Thanks,
Alex

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

Re: [PATCH v2 2/2] irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0

[Marc Zyngier]

On 2020-09-12 14:34, Alexandru Elisei wrote:
> Hi Marc,

[...]

> Have you tested the series using the PMU NMI branch from the cover
> letter? If so, would you mind giving a Tested-by tag for that series 
> [1]?

I haven't had a chance, but that's next on my list once I've gone
through some other bits of review that have been lingering in my
inbox for too long...

         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