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

[Mark Rutland <mark.rutland@arm.com>]

On Tue, Nov 28, 2023 at 11:03:40AM +0000, Will Deacon wrote:
> 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@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.

Sure. My thinking was that the window is small, and we're only using a few
__init functions there, but I agree that there's a risk, and I'm happy to go
rework as you say.

> > 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.

That's fair.

My major goal is that it's clear where we're using an altnernative and where
we're not; if the thinking is the vast majority *should* be alternatives, I can
annotate the non-alternative cases to make that clear.

> > 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.

Sure; as-is that breaks the hyp offset patching, and we'll need ensure we still
call hyp_mode_check() before apply_alternatives_all(). In hyp_mode_check() we
call kvm_compute_layout(), which generates the 'va_mask', 'tag_val', and
'tag_lsb' values which are used by the kvm_update_va_mask() patching callback
(for kern_hyp_va() and __kern_hyp_va()).

I think that can be moved before enable_cpu_capabilities() ratehr than needing
to be between enable_cpu_capabilities() and apply_alternatives_all(), but I'll
need to go digging to make certain.

Do you want this as a cleanup for v6.7-rc*, or as something to queue for
v6.8-rc1?

Thanks,
Mark.

> 
> 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