| Message ID | 20200819133630.527243-3-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0 | expand |
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.
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
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.
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))
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(-)