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

[Mark Rutland]

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:
> > > > > From: Ard Biesheuvel <ardb at kernel.org>
> > > > >
> > > > > Commit 42c5a3b04bf6 refactored the KPTI init code in a way that results
> > > > > in the use of non-global kernel mappings even on systems that have no
> > > > > need for it, and even when KPTI has been disabled explicitly via the
> > > > > command line.
> > > > >
> > > > > Ensure that this only happens when we have decided (based on the
> > > > > detected system-wide CPU features) that KPTI should be enabled.
> > > > >
> > > > > Fixes: 42c5a3b04bf6 ("arm64: Split kpti_install_ng_mappings()")
> > > > > Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
> > > > > ---
> > > > >  arch/arm64/kernel/cpufeature.c | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > >
> > > > > 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.

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.

If you want these cases moves back to alternatives-based feature check helpers,
I can go do that.

Thanks,
Mark.