* [PATCH] arm64/cpufeatures: Enforce inline/const properties of cpus_have_const_cap @ 2017-01-18 11:58 Marc Zyngier 2017-01-19 14:37 ` Will Deacon 0 siblings, 1 reply; 4+ messages in thread From: Marc Zyngier @ 2017-01-18 11:58 UTC (permalink / raw) To: linux-arm-kernel Despite being flagged "inline", cpus_have_const_cap may end-up being placed out of line if the compiler decides so. This would be unfortunate, as we want to be able to use this function in HYP, where we need to be 100% sure of what is mapped there. __always_inline seems to be a better choice given the constraint. Also, be a lot tougher on non-const or out-of-range capability values (a non-const cap value shouldn't be used here, and the semantic of an OOR value is at best ill defined). In those two case, BUILD_BUG_ON is what you get. Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm64/include/asm/cpufeature.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index b4989df..4710469 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -105,10 +105,11 @@ static inline bool cpu_have_feature(unsigned int num) } /* System capability check for constant caps */ -static inline bool cpus_have_const_cap(int num) +static __always_inline bool cpus_have_const_cap(int num) { - if (num >= ARM64_NCAPS) - return false; + BUILD_BUG_ON(!__builtin_constant_p(num)); + BUILD_BUG_ON(num >= ARM64_NCAPS); + return static_branch_unlikely(&cpu_hwcap_keys[num]); } -- 2.1.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] arm64/cpufeatures: Enforce inline/const properties of cpus_have_const_cap 2017-01-18 11:58 [PATCH] arm64/cpufeatures: Enforce inline/const properties of cpus_have_const_cap Marc Zyngier @ 2017-01-19 14:37 ` Will Deacon 2017-01-19 14:42 ` Marc Zyngier 0 siblings, 1 reply; 4+ messages in thread From: Will Deacon @ 2017-01-19 14:37 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jan 18, 2017 at 11:58:45AM +0000, Marc Zyngier wrote: > Despite being flagged "inline", cpus_have_const_cap may end-up being > placed out of line if the compiler decides so. This would be unfortunate, > as we want to be able to use this function in HYP, where we need to > be 100% sure of what is mapped there. __always_inline seems to be a > better choice given the constraint. > > Also, be a lot tougher on non-const or out-of-range capability values > (a non-const cap value shouldn't be used here, and the semantic of an > OOR value is at best ill defined). In those two case, BUILD_BUG_ON is > what you get. > > Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm64/include/asm/cpufeature.h | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > index b4989df..4710469 100644 > --- a/arch/arm64/include/asm/cpufeature.h > +++ b/arch/arm64/include/asm/cpufeature.h > @@ -105,10 +105,11 @@ static inline bool cpu_have_feature(unsigned int num) > } > > /* System capability check for constant caps */ > -static inline bool cpus_have_const_cap(int num) > +static __always_inline bool cpus_have_const_cap(int num) > { > - if (num >= ARM64_NCAPS) > - return false; > + BUILD_BUG_ON(!__builtin_constant_p(num)); > + BUILD_BUG_ON(num >= ARM64_NCAPS); This gives different behaviour to cpus_have_const_cap when compared to cpus_have_cap, which I really don't like. What is the current behaviour if you pass a non-constant num parameter? Does the kernel actually build? Maybe it's best to spin a separate patch that makes cpus_have_cap and cpus_have_const_cap both use __always_inline, then we can debate the merit of the BUILD_BUG_ONs separately. Will ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] arm64/cpufeatures: Enforce inline/const properties of cpus_have_const_cap 2017-01-19 14:37 ` Will Deacon @ 2017-01-19 14:42 ` Marc Zyngier 2017-01-19 14:48 ` Will Deacon 0 siblings, 1 reply; 4+ messages in thread From: Marc Zyngier @ 2017-01-19 14:42 UTC (permalink / raw) To: linux-arm-kernel On 19/01/17 14:37, Will Deacon wrote: > On Wed, Jan 18, 2017 at 11:58:45AM +0000, Marc Zyngier wrote: >> Despite being flagged "inline", cpus_have_const_cap may end-up being >> placed out of line if the compiler decides so. This would be unfortunate, >> as we want to be able to use this function in HYP, where we need to >> be 100% sure of what is mapped there. __always_inline seems to be a >> better choice given the constraint. >> >> Also, be a lot tougher on non-const or out-of-range capability values >> (a non-const cap value shouldn't be used here, and the semantic of an >> OOR value is at best ill defined). In those two case, BUILD_BUG_ON is >> what you get. >> >> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> arch/arm64/include/asm/cpufeature.h | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h >> index b4989df..4710469 100644 >> --- a/arch/arm64/include/asm/cpufeature.h >> +++ b/arch/arm64/include/asm/cpufeature.h >> @@ -105,10 +105,11 @@ static inline bool cpu_have_feature(unsigned int num) >> } >> >> /* System capability check for constant caps */ >> -static inline bool cpus_have_const_cap(int num) >> +static __always_inline bool cpus_have_const_cap(int num) >> { >> - if (num >= ARM64_NCAPS) >> - return false; >> + BUILD_BUG_ON(!__builtin_constant_p(num)); >> + BUILD_BUG_ON(num >= ARM64_NCAPS); > > This gives different behaviour to cpus_have_const_cap when compared to > cpus_have_cap, which I really don't like. What is the current behaviour > if you pass a non-constant num parameter? Does the kernel actually build? If your toolchain doesn't support jump labels (gcc 4.8 for example), it will build. But my point here is that if you're using the _const version, it should to be an actual constant, within the range of existing capabilities. Otherwise, I don't really understand what the semantic of _const means here. > Maybe it's best to spin a separate patch that makes cpus_have_cap and > cpus_have_const_cap both use __always_inline, then we can debate the merit > of the BUILD_BUG_ONs separately. Sure, will do. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] arm64/cpufeatures: Enforce inline/const properties of cpus_have_const_cap 2017-01-19 14:42 ` Marc Zyngier @ 2017-01-19 14:48 ` Will Deacon 0 siblings, 0 replies; 4+ messages in thread From: Will Deacon @ 2017-01-19 14:48 UTC (permalink / raw) To: linux-arm-kernel On Thu, Jan 19, 2017 at 02:42:50PM +0000, Marc Zyngier wrote: > On 19/01/17 14:37, Will Deacon wrote: > > On Wed, Jan 18, 2017 at 11:58:45AM +0000, Marc Zyngier wrote: > >> Despite being flagged "inline", cpus_have_const_cap may end-up being > >> placed out of line if the compiler decides so. This would be unfortunate, > >> as we want to be able to use this function in HYP, where we need to > >> be 100% sure of what is mapped there. __always_inline seems to be a > >> better choice given the constraint. > >> > >> Also, be a lot tougher on non-const or out-of-range capability values > >> (a non-const cap value shouldn't be used here, and the semantic of an > >> OOR value is at best ill defined). In those two case, BUILD_BUG_ON is > >> what you get. > >> > >> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> > >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > >> --- > >> arch/arm64/include/asm/cpufeature.h | 7 ++++--- > >> 1 file changed, 4 insertions(+), 3 deletions(-) > >> > >> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > >> index b4989df..4710469 100644 > >> --- a/arch/arm64/include/asm/cpufeature.h > >> +++ b/arch/arm64/include/asm/cpufeature.h > >> @@ -105,10 +105,11 @@ static inline bool cpu_have_feature(unsigned int num) > >> } > >> > >> /* System capability check for constant caps */ > >> -static inline bool cpus_have_const_cap(int num) > >> +static __always_inline bool cpus_have_const_cap(int num) > >> { > >> - if (num >= ARM64_NCAPS) > >> - return false; > >> + BUILD_BUG_ON(!__builtin_constant_p(num)); > >> + BUILD_BUG_ON(num >= ARM64_NCAPS); > > > > This gives different behaviour to cpus_have_const_cap when compared to > > cpus_have_cap, which I really don't like. What is the current behaviour > > if you pass a non-constant num parameter? Does the kernel actually build? > > If your toolchain doesn't support jump labels (gcc 4.8 for example), it > will build. But my point here is that if you're using the _const > version, it should to be an actual constant, within the range of > existing capabilities. Otherwise, I don't really understand what the > semantic of _const means here. There are two things here: 1. GCC can make non-const values constant using a runtime conditional 2. If we treat out-of-range caps as a BUILD_BUG_ON, then we've got different behaviour with cpus_have_cap, which will return false. So I don't think that the BUILD_BUG_ON(num >= ARM64_NCAPS) makes an awful lot of sense, whilst the other BUILD_BUG_ON seems more like a sanity check on jump labels. That might be justifiable if the build failure is more obvious than what we currently get, so it's mainly the range check that I object to. Will ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-01-19 14:48 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-01-18 11:58 [PATCH] arm64/cpufeatures: Enforce inline/const properties of cpus_have_const_cap Marc Zyngier 2017-01-19 14:37 ` Will Deacon 2017-01-19 14:42 ` Marc Zyngier 2017-01-19 14:48 ` Will Deacon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).