Re: [PATCH 05/37] arm64: Add cpus_have_final_boot_cap()

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

On Fri, Sep 22, 2023 at 11:26:21AM +0100, Suzuki K Poulose wrote:
> On 21/09/2023 17:36, Mark Rutland wrote:
> > On Thu, Sep 21, 2023 at 10:13:31AM +0100, Suzuki K Poulose wrote:
> > > Hi Mark
> > > 
> > > On 19/09/2023 10:28, Mark Rutland wrote:
> > > > The cpus_have_final_boot_cap() function can be used to test a cpucap
> > > 
> > > nit: cpus_have_final_cap()
> > 
> > Thanks; fixed now.
> > 
> > > > while also verifying that we do not consume the cpucap until system
> > > > capabilities have been finalized. It would be helpful if we could do
> > > > likewise for boot cpucaps.
> > > > 
> > > > This patch adds a new cpus_have_final_boot_cap() helper which can be
> > > > used to test a cpucap while also verifying that boot capabilities have
> > > > been finalized. Users will be added in subsequent patches.
> > > > 
> > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > Cc: Mark Brown <broonie@kernel.org>
> > > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > > Cc: Will Deacon <will@kernel.org>
> > > > ---
> > > >    arch/arm64/include/asm/cpufeature.h | 27 +++++++++++++++++++++++++--
> > > >    1 file changed, 25 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> > > > index 7d5317bc2429f..e832b86c6b57f 100644
> > > > --- a/arch/arm64/include/asm/cpufeature.h
> > > > +++ b/arch/arm64/include/asm/cpufeature.h
> > > > @@ -438,6 +438,11 @@ unsigned long cpu_get_elf_hwcap2(void);
> > > >    #define cpu_set_named_feature(name) cpu_set_feature(cpu_feature(name))
> > > >    #define cpu_have_named_feature(name) cpu_have_feature(cpu_feature(name))
> > > > +static __always_inline bool boot_capabilities_finalized(void)
> > > > +{
> > > > +	return alternative_has_cap_likely(ARM64_ALWAYS_BOOT);
> > > > +}
> > > > +
> > > >    static __always_inline bool system_capabilities_finalized(void)
> > > >    {
> > > >    	return alternative_has_cap_likely(ARM64_ALWAYS_SYSTEM);
> > > > @@ -473,8 +478,26 @@ static __always_inline bool __cpus_have_const_cap(int num)
> > > >    /*
> > > >     * Test for a capability without a runtime check.
> > > >     *
> > > > - * Before capabilities are finalized, this will BUG().
> > > > - * After capabilities are finalized, this is patched to avoid a runtime check.
> > > > + * Before boot capabilities are finalized, this will BUG().
> > > > + * After boot capabilities are finalized, this is patched to avoid a runtime
> > > > + * check.
> > > > + *
> > > > + * @num must be a compile-time constant.
> > > > + */
> > > > +static __always_inline bool cpus_have_final_boot_cap(int num)
> > > > +{
> > > > +	if (boot_capabilities_finalized())
> > > 
> > > Does this need to make sure the cap is really a "BOOT" cap ? It is a bit of
> > > an overkill, but prevents users from incorrectly assuming the cap is
> > > finalised ?
> > 
> > Do you have an idea in mind for how to do that?
> > 
> > I had also wanted that, but we don't have the information available when
> > compiling the callsites today since that's determined by the
> > arm64_cpu_capabilities::type flags.
> > 
> > We could us an alternative callback for boot_capabilities_finalized() that
> > goes and checks the arm64_cpu_capabilities::type flags, but that doesn't seem
> > very nice.
> > 
> Thats what I had initially in mind, and is why I called it an
> overkill.
> 
> But may be another option is to have a different alternative construct
> for all capabilities, which defaults to BUG() and then patched to
> "false" or "true" based on the real status ? This may be more
> complicated.

I think that's possible with something like the ARM64_CB_BIT bit to say "this
alternative is applied when this cap is finalized", and either a bitmap of
finalized alternatives, or another walk over the arm64_cpu_capabilities when
applying alternatives.

I'll have a look and see how painful that is.

> > Otherwise, given this only has a few users, I could have those directly use:
> > 
> > 	BUG_ON(!boot_capabilities_finalized());
> > 
> > ... and remove cpus_have_final_boot_cap() for now?
> 
> I don't think that is necessary. We could keep your patch
> as is, if we can't verify the boot capability easily.

Thanks!

Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel