[PATCH v2 1/3] arm64: Avoid enabling KPTI unnecessarily

[Will Deacon]

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 <will at kernel.org> 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();
 }