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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 4B262C4167B for ; Tue, 28 Nov 2023 11:04:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=/eSdOBviqG0FZ+wDKQlpY9C8+pbDmKKKgy4z+vEBJlU=; b=xbGPG/T6Dw9oyR GMmNcNoTrjf81mZXwd9+zlt21YeCI1gyNZ5Oqs3p7PgJqtVFGV8Iuoek89dzP3f3G85fOew0oVvij qSBE1gfpgxzqA0MeffpQeCpWUxJSpmQsWUVZZMI9IqmvSYDTvSyqHFg5pMuUl3Ov+xzvWtUjfIIF4 6JZS2ZY2hja+dlUj1izFlpLlBvh3lceLSrhAS/h2A4Hxi3odaQkKgZWsHGo7T5AMC+PmAiF+zcZCK mFlE2SOmo7JkxUCgNFEzcrSXPFjit1YnbX70VCUc+QueM2koVTDu+UxSjEgn1aUR0zx+9umPxlOeR gCXAF5auJxfVpWQ2NK5w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1r7vsd-0050CA-0c; Tue, 28 Nov 2023 11:03:51 +0000 Received: from sin.source.kernel.org ([145.40.73.55]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1r7vsa-0050BC-0D for linux-arm-kernel@lists.infradead.org; Tue, 28 Nov 2023 11:03:50 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id 91B29CE19B9; Tue, 28 Nov 2023 11:03:45 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 85603C433C7; Tue, 28 Nov 2023 11:03:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1701169424; bh=ktUjAWY0KUInjJUP6o6040bvSJ5RRziO3yBBxRdCkAM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=au96HrUhIk6Ro9Dise+laDe3AwteJlorSG+WSXJm67iFNNqpkKYJtbqu0cyiJvVH3 AGPBD5yH6M4L+tETkbx9VxSAX2+/CEjMaIBpLr5OdZnEBn7LXsiMgcBOjYdR+L3LDQ 2rkOQs6XVeDVD25TwnSLPadbQ0JYmJcGCk7Enttxxj1kt4v53y70lk1TYxNorvIcuW Mja6Feqk5/WSFb9YhuanPzpg4VzBHNflkljblJI8LcNERLM/1mi+7pjQ5+XQmJe44T 0+S3kBYPNpTJlEjNXvPdaxhFUEDyBPEq+r7yxp7HFLVyvIgnsBldml2YQEcXfCowyV uO50xZIQwgw0A== Date: Tue, 28 Nov 2023 11:03:40 +0000 From: Will Deacon To: Mark Rutland Cc: Ard Biesheuvel , Ard Biesheuvel , linux-arm-kernel@lists.infradead.org, Catalin Marinas , Marc Zyngier Subject: Re: [PATCH v2 1/3] arm64: Avoid enabling KPTI unnecessarily Message-ID: <20231128110339.GA9717@willie-the-truck> References: <20231127120049.2258650-5-ardb@google.com> <20231127120049.2258650-6-ardb@google.com> <20231127154818.GA8453@willie-the-truck> <20231127163103.GA8627@willie-the-truck> <20231127164127.GB8627@willie-the-truck> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231128_030348_565384_D486949D X-CRM114-Status: GOOD ( 42.01 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 On Mon, Nov 27, 2023 at 05:49:21PM +0000, Mark Rutland wrote: > On Mon, Nov 27, 2023 at 04:41:27PM +0000, Will Deacon wrote: > > On Mon, Nov 27, 2023 at 04:31:03PM +0000, Will Deacon wrote: > > > On Mon, Nov 27, 2023 at 04:52:11PM +0100, Ard Biesheuvel wrote: > > > > On Mon, 27 Nov 2023 at 16:48, Will Deacon wrote: > > > > > On Mon, Nov 27, 2023 at 01:00:51PM +0100, Ard Biesheuvel wrote: > > > > > > > > > > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > > > > > index 646591c67e7a..91d2d6714969 100644 > > > > > > --- a/arch/arm64/kernel/cpufeature.c > > > > > > +++ b/arch/arm64/kernel/cpufeature.c > > > > > > @@ -1839,6 +1839,10 @@ static int __init __kpti_install_ng_mappings(void *__unused) > > > > > > > > > > > > static void __init kpti_install_ng_mappings(void) > > > > > > { > > > > > > + /* Check whether KPTI is going to be used */ > > > > > > + if (!cpus_have_cap(ARM64_UNMAP_KERNEL_AT_EL0)) > > > > > > + return; > > > > > > > > > > Why can't you use arm64_kernel_unmapped_at_el0() here? > > > > > > > > > > > > > Because it relies on alternatives patching, which hasn't occurred yet > > > > at this point. > > > > > > Hmm. Keeping the determination of the capabilities separate from the > > > alternatives patching feels like it's asking for trouble given how > > > many of the boolean system_*() helpers in asm/cpufeature.h are using > > > the alternative_has_cap*() code. > > > > > > Could we move the call to apply_alternatives_all() into > > > setup_system_features() and then you could do the kpti stuff after that? > > > I think sve_setup() and sme_setup() are ok, but I'd be more comfortable > > > moving those later too, given how things like system_supports_sve() rely > > > on the alternatives as well. > > > > Heh, so looking more closely at the history, I think I'm asking to undo > > some of the recent changes from Mark :) > > > > Mark -- looking at e.g. a76521d16028 ("arm64: Avoid cpus_have_const_cap() > > for ARM64_{SVE,SME,SME2,FA64}") and 53d62e995d9e ("arm64: Avoid > > cpus_have_const_cap() for ARM64_HAS_PAN"), why did you not just hoist > > the alternatives patching slightly earlier instead of using > > cpus_have_cap() directly? > > There are a few reasons here. The overall point of those changes was to make > sure that patching the alternatives transitions the system into the patched > state atomically, and so that we don't have a weird transient state where > things can go wrong. My intent was that these initialization functions > initialize structures and state *before* patching the alternatives such that > once patched the system is immediately in a complete state. There are a few > other places that absolutely have to do this before patching, but for the cases > you mention above this isn't strictly necessary. That all sounds reasonable, but we still have a weird period between the call to update_cpu_capabilities(SCOPE_SYSTEM) in setup_system_features() and the subsequent call to apply_alternatives_all() where code needs to know not to inadvertently call something using alternative_has_cap() otherwise it will silently get the wrong answer. I'm just saying we should reduce that period as much as possible. > In addition to that intent, these functions are executed precisely once during > boot, so using cpus_have_cap() avoids the space for the alt_instr and the cost > of the patching of the site for a single check. I was also hoping to get rid of > most of the feature check helpers now that they're largely trivial wrappers > around alternative_has_cap(), which'd make it clearer which callsites care > about the alterntive-ness and clear up the inconsistent naming. We can always remove the helpers when you get round to it, but ideally many of these things would BUG() if they're called too early, much like e.g. cpus_have_final_cap(). Inlining the alternative is going to make that more difficult. > If you want these cases moves back to alternatives-based feature check helpers, > I can go do that. I was thinking of something like the diff below to start with. Will --->8 diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 646591c67e7a..0a5c23e449fe 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -3342,9 +3342,6 @@ void __init setup_system_features(void) * finalized. Finalize and log the available system capabilities. */ update_cpu_capabilities(SCOPE_SYSTEM); - if (IS_ENABLED(CONFIG_ARM64_SW_TTBR0_PAN) && - !cpus_have_cap(ARM64_HAS_PAN)) - pr_info("emulated: Privileged Access Never (PAN) using TTBR0_EL1 switching\n"); /* * Enable all the available capabilities which have not been enabled @@ -3352,6 +3349,16 @@ void __init setup_system_features(void) */ enable_cpu_capabilities(SCOPE_ALL & ~SCOPE_BOOT_CPU); + /* + * Patch in alternative instruction sequences now that the system-wide + * CPU features have been enabled. + */ + apply_alternatives_all(); + + + if (IS_ENABLED(CONFIG_ARM64_SW_TTBR0_PAN) && !system_uses_hw_pan()) + pr_info("emulated: Privileged Access Never (PAN) using TTBR0_EL1 switching\n"); + kpti_install_ng_mappings(); sve_setup(); diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 1559c706d32d..bc9384517db3 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -1171,7 +1171,7 @@ void __init sve_setup(void) unsigned long b; int max_bit; - if (!cpus_have_cap(ARM64_SVE)) + if (!system_supports_sve()) return; /* @@ -1301,7 +1301,7 @@ void __init sme_setup(void) struct vl_info *info = &vl_info[ARM64_VEC_SME]; int min_bit, max_bit; - if (!cpus_have_cap(ARM64_SME)) + if (!system_supports_sme()) return; /* diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index defbab84e9e5..f59d6cea2187 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -441,7 +441,6 @@ void __init smp_cpus_done(unsigned int max_cpus) pr_info("SMP: Total of %d processors activated.\n", num_online_cpus()); setup_system_features(); hyp_mode_check(); - apply_alternatives_all(); setup_user_features(); mark_linear_text_alias_ro(); } _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel