| Message ID | 20230919092850.1940729-6-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Remove cpus_have_const_cap() | expand |
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() > 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 ? Suzuki > + return __cpus_have_const_cap(num); > + else > + BUG(); > +} > + > +/* > + * Test for a capability without a runtime check. > + * > + * Before system capabilities are finalized, this will BUG(). > + * After system capabilities are finalized, this is patched to avoid a runtime > + * check. > * > * @num must be a compile-time constant. > */
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. 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? Thanks, Mark. > > > Suzuki > > > + return __cpus_have_const_cap(num); > > + else > > + BUG(); > > +} > > + > > +/* > > + * Test for a capability without a runtime check. > > + * > > + * Before system capabilities are finalized, this will BUG(). > > + * After system capabilities are finalized, this is patched to avoid a runtime > > + * check. > > * > > * @num must be a compile-time constant. > > */ >
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. > 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. Suzuki > > Thanks, > Mark. > >> >> >> Suzuki >> >>> + return __cpus_have_const_cap(num); >>> + else >>> + BUG(); >>> +} >>> + >>> +/* >>> + * Test for a capability without a runtime check. >>> + * >>> + * Before system capabilities are finalized, this will BUG(). >>> + * After system capabilities are finalized, this is patched to avoid a runtime >>> + * check. >>> * >>> * @num must be a compile-time constant. >>> */ >>
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.
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: > > > On 19/09/2023 10:28, Mark Rutland wrote: > > > > /* > > > > * 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. > > > 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. I had a go at reworking this to automatically do the right thing, and it needs a more substantial rework of the way we handle the cpucap bitmaps and walk over the capability structures. Given that, I'd like to leave this as-is for now, and follow up with the rework for that. Thanks, Mark.
On 05/10/2023 10:23, Mark Rutland wrote: > 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: >>>> On 19/09/2023 10:28, Mark Rutland wrote: > >>>>> /* >>>>> * 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. >> >>> 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. > > I had a go at reworking this to automatically do the right thing, and it needs > a more substantial rework of the way we handle the cpucap bitmaps and walk over > the capability structures. > > Given that, I'd like to leave this as-is for now, and follow up with the rework > for that. Sure, I understand and this is fine by me. Suzuki
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()) + return __cpus_have_const_cap(num); + else + BUG(); +} + +/* + * Test for a capability without a runtime check. + * + * Before system capabilities are finalized, this will BUG(). + * After system capabilities are finalized, this is patched to avoid a runtime + * check. * * @num must be a compile-time constant. */
The cpus_have_final_boot_cap() function can be used to test a cpucap 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(-)