[PATCH v5 29/39] arm64: Add helpers to probe local CPU for PAC and BTI support

[Marc Zyngier]

On Fri, 24 Nov 2023 13:08:33 +0000,
Ard Biesheuvel <ardb at kernel.org> wrote:
> 
> On Fri, 24 Nov 2023 at 13:37, Marc Zyngier <maz at kernel.org> wrote:
> >
> > On Fri, 24 Nov 2023 10:19:09 +0000,
> > Ard Biesheuvel <ardb at google.com> wrote:
> > >
> > > From: Ard Biesheuvel <ardb at kernel.org>
> > >
> > > Add some helpers that will be used by the early kernel mapping code to
> > > check feature support on the local CPU. This permits the early kernel
> > > mapping to be created with the right attributes, removing the need for
> > > tearing it down and recreating it.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
> > > ---
> > >  arch/arm64/include/asm/cpufeature.h | 44 ++++++++++++++++++++
> > >  1 file changed, 44 insertions(+)
> > >
> > > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> > > index 99c01417e544..301d4d2211d5 100644
> > > --- a/arch/arm64/include/asm/cpufeature.h
> > > +++ b/arch/arm64/include/asm/cpufeature.h
> > > @@ -921,6 +921,50 @@ static inline bool kaslr_disabled_cmdline(void)
> > >  u32 get_kvm_ipa_limit(void);
> > >  void dump_cpu_features(void);
> > >
> > > +static inline bool cpu_has_bti(void)
> > > +{
> > > +     u64 pfr1;
> > > +
> > > +     if (!IS_ENABLED(CONFIG_ARM64_BTI))
> > > +             return false;
> > > +
> > > +     pfr1 = read_cpuid(ID_AA64PFR1_EL1);
> > > +     pfr1 &= ~id_aa64pfr1_override.mask;
> > > +     pfr1 |= id_aa64pfr1_override.val;
> >
> > There is a potential gotcha here. If the override failed because a
> > filter rejected it, val will be set to full ones for the size of the
> > failed override field, and the corresponding mask will be set to 0
> > (see match_options()).
> >
> > A safer pattern would be:
> >
> >         pfr1 |= id_aa64pfr1_override.val & id_aa64pfr1_override.mask;
> >
> > although we currently don't have such a filter for BTI nor PAC, so the
> > code is OK for now.
> >
> 
> This confuses me.
> 
> What is the point of a value/mask pair if the mask is applied to the
> value, and not to the quantity that is being overridden? Surely, not
> setting those bits in 'value' to begin with makes the whole mask
> redundant, no?
> 
> If the filter is supposed to prevent the override from taking effect,
> wouldn't it be better to use 0/0 for the mask/value pair?
> 
> (/me likely misses some context here)

0/0 would be fine for the purpose of not applying the override.
However, this hack is used to report the failed override at boot time
instead of silently ignoring it (see [1]).

Is it crap? Absolutely. But I didn't have a better idea at the time
(and still don't). Alternatively, we can drop the reporting and be
done with it.

	M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/cpufeature.c#n923


-- 
Without deviation from the norm, progress is not possible.