From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C0D63C433E2 for ; Sat, 12 Sep 2020 12:02:47 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 5F358208B3 for ; Sat, 12 Sep 2020 12:02:47 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="o8HinImm"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="DzBEH0LU" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5F358208B3 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Subject:To:From: Message-ID:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=LCKd49hsgE7J1FhPgD6Owl2MJRWV/DxweYyuWU1O/Jg=; b=o8HinImmNPEI72xe9qEcpiPdk joCaWr3knif7115sm6Y1ij9EZ1pjxfag1wLwCA3ogv4mL2zr1YK+VUjfi4Hb2MHHn4Lxyv/qpmwMW P52Eyur5f8y019R6VeBHl4hcIt64hlDbpxieX4hF6DAeDuwSeXjAplFar5Rk51J1bMI1mD+1fNXEX 2bYeVA0lei8AXxNuhTWQbuMtHkFKeOe2vmdv0xok/jKeLlAmM5TCq1Nc3+e0ZvLnTchkKWl5ylwIt 1UJ7eM4Asi45OGebMuNfTRVTvLz1La12bzkO8NuR8iQD6bpEddE2o9SfWu8VNkP5933fmp3haBbNQ Y+QJPJ8Nw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kH4DC-00035m-3b; Sat, 12 Sep 2020 12:00:58 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kH4D6-00033Q-W5 for linux-arm-kernel@lists.infradead.org; Sat, 12 Sep 2020 12:00:54 +0000 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id A6CF7207EA; Sat, 12 Sep 2020 12:00:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1599912051; bh=IpaqtpvBIrmcYR7m/bPjpB9hmFSlPm1waXx3HuuWiEU=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=DzBEH0LUWx9va/ZJ8pRBDeiWgJst8toWIqcGzkpe2KqzncxrMaZyiEiFZzNQSf75/ /SKDUtbzOg5/K05ZrWneP8Ed70fb6V8GGEweUkaxCvCbLdwDYTp9NsO0zccDSmnC7J KO11LwIS1sLf7aArsHXmBUBKgCe8618g3+gha4Ds= Received: from 78.163-31-62.static.virginmediabusiness.co.uk ([62.31.163.78] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kH4D3-00BEOo-Lw; Sat, 12 Sep 2020 13:00:49 +0100 Date: Sat, 12 Sep 2020 13:00:48 +0100 Message-ID: <87k0wzkxvz.wl-maz@kernel.org> From: Marc Zyngier To: Alexandru Elisei Subject: Re: [PATCH v2 2/2] irqchip/gic-v3: Support pseudo-NMIs when SCR_EL3.FIQ == 0 In-Reply-To: <20200819133630.527243-3-alexandru.elisei@arm.com> References: <20200819133630.527243-1-alexandru.elisei@arm.com> <20200819133630.527243-3-alexandru.elisei@arm.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/26.3 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 62.31.163.78 X-SA-Exim-Rcpt-To: alexandru.elisei@arm.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kvmarm@lists.cs.columbia.edu, jason@lakedaemon.net, catalin.marinas@arm.com, tglx@linutronix.de, will@kernel.org, yuzenghui@huawei.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200912_080053_144450_0578BD74 X-CRM114-Status: GOOD ( 50.72 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: jason@lakedaemon.net, catalin.marinas@arm.com, linux-kernel@vger.kernel.org, yuzenghui@huawei.com, tglx@linutronix.de, will@kernel.org, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 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 > --- > 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