| Message ID | 20240515-hedging-passage-44fd394ab1be@spud (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Conor Dooley |
| Headers | show |
| Series | [v1] RISC-V: separate Zbb optimisations requiring and not requiring toolchain support | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/vmtest-fixes-PR | fail | merge-conflict |
On Wed, May 15, 2024 at 04:27:40PM +0100, Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > It seems a bit ridiculous to require toolchain support for BPF to > assemble Zbb instructions, so introduce hidden a Kconfig option that > controls the use of any toolchain-requiring optimisations while support > is available. > > Zbb support has always depended on alternatives, so while adjusting the > config options guarding optimisations, remove any checks for > whether or not alternatives are enabled. > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > > This patch stems out of a conversation about Zba optimisations in BPF. > I'm not super sold on the approach in all honesty, even though we > recently had a conversation about respecting the Kconfig options - but > at this point I'd be convinced to just add some wording to the Kconfig > options mentioning that BPF optimisations are excluded. > Having hidden options that mean someone can turn what on what they > think are Zbb optimisations but not actually get any cos their toolchain > doesn't support it seems crap to me. I don't wanna add another > user-visible option for that situation cos I wanna try to minimise the > extent of our extension-related Kconfig options, not blow them up like > Augustus Gloop! > > Cheers, > Conor. > > CC: xiao.w.wang@intel.com > CC: Andrew Jones <ajones@ventanamicro.com> > CC: pulehui@huawei.com > CC: Charlie Jenkins <charlie@rivosinc.com> > CC: Paul Walmsley <paul.walmsley@sifive.com> > CC: Palmer Dabbelt <palmer@dabbelt.com> > CC: Conor Dooley <conor.dooley@microchip.com> > CC: linux-riscv@lists.infradead.org > CC: linux-kernel@vger.kernel.org > --- > arch/riscv/Kconfig | 15 ++++++++++++--- > arch/riscv/include/asm/arch_hweight.h | 4 ++-- > arch/riscv/include/asm/bitops.h | 4 ++-- > arch/riscv/include/asm/checksum.h | 3 +-- > arch/riscv/lib/csum.c | 9 +++------ > arch/riscv/lib/strcmp.S | 4 ++-- > arch/riscv/lib/strlen.S | 4 ++-- > arch/riscv/lib/strncmp.S | 4 ++-- > 8 files changed, 26 insertions(+), 21 deletions(-) > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index e927b52b420c..f216a52ed181 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -605,14 +605,23 @@ config TOOLCHAIN_HAS_VECTOR_CRYPTO > def_bool $(as-instr, .option arch$(comma) +v$(comma) +zvkb) > depends on AS_HAS_OPTION_ARCH > > -config RISCV_ISA_ZBB > - bool "Zbb extension support for bit manipulation instructions" > +config RISCV_ISA_ZBB_ALT > + def_bool RISCV_ISA_ZBB > depends on TOOLCHAIN_HAS_ZBB > depends on RISCV_ALTERNATIVE > + help > + This option controls whether or not we build optimisations that > + depend on toolchain support. It's automatically enabled whenever the > + toolchain in use supports assembling Zbb instructions and > + RISCV_ISA_ZBB is set. > + > +config RISCV_ISA_ZBB > + bool "Zbb extension support for bit manipulation instructions" > default y > help > Add support for enabling optimisations in the kernel when the > - Zbb extension is detected at boot. > + Zbb extension is detected at boot. Some optimisations may > + additionally depend on toolchain support for Zbb. > > The Zbb extension provides instructions to accelerate a number > of bit-specific operations (count bit population, sign extending, > diff --git a/arch/riscv/include/asm/arch_hweight.h b/arch/riscv/include/asm/arch_hweight.h > index 85b2c443823e..a677f6b82228 100644 > --- a/arch/riscv/include/asm/arch_hweight.h > +++ b/arch/riscv/include/asm/arch_hweight.h > @@ -19,7 +19,7 @@ > > static __always_inline unsigned int __arch_hweight32(unsigned int w) > { > -#ifdef CONFIG_RISCV_ISA_ZBB > +#ifdef CONFIG_RISCV_ISA_ZBB_ALT > asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0, > RISCV_ISA_EXT_ZBB, 1) > : : : : legacy); > @@ -50,7 +50,7 @@ static inline unsigned int __arch_hweight8(unsigned int w) > #if BITS_PER_LONG == 64 > static __always_inline unsigned long __arch_hweight64(__u64 w) > { > -# ifdef CONFIG_RISCV_ISA_ZBB > +# ifdef CONFIG_RISCV_ISA_ZBB_ALT > asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0, > RISCV_ISA_EXT_ZBB, 1) > : : : : legacy); > diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h > index 880606b0469a..3ed810a6123d 100644 > --- a/arch/riscv/include/asm/bitops.h > +++ b/arch/riscv/include/asm/bitops.h > @@ -15,7 +15,7 @@ > #include <asm/barrier.h> > #include <asm/bitsperlong.h> > > -#if !defined(CONFIG_RISCV_ISA_ZBB) || defined(NO_ALTERNATIVE) > +#if !defined(CONFIG_RISCV_ISA_ZBB_ALT) || defined(NO_ALTERNATIVE) > #include <asm-generic/bitops/__ffs.h> > #include <asm-generic/bitops/__fls.h> > #include <asm-generic/bitops/ffs.h> > @@ -175,7 +175,7 @@ static __always_inline int variable_fls(unsigned int x) > variable_fls(x_); \ > }) > > -#endif /* !defined(CONFIG_RISCV_ISA_ZBB) || defined(NO_ALTERNATIVE) */ > +#endif /* !defined(CONFIG_RISCV_ISA_ZBB_ALT) || defined(NO_ALTERNATIVE) */ > > #include <asm-generic/bitops/ffz.h> > #include <asm-generic/bitops/fls64.h> > diff --git a/arch/riscv/include/asm/checksum.h b/arch/riscv/include/asm/checksum.h > index 88e6f1499e88..956224ea8199 100644 > --- a/arch/riscv/include/asm/checksum.h > +++ b/arch/riscv/include/asm/checksum.h > @@ -49,8 +49,7 @@ static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl) > * ZBB only saves three instructions on 32-bit and five on 64-bit so not > * worth checking if supported without Alternatives. > */ > - if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && > - IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { > + if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB_ALT)) { > unsigned long fold_temp; > > asm goto(ALTERNATIVE("j %l[no_zbb]", "nop", 0, > diff --git a/arch/riscv/lib/csum.c b/arch/riscv/lib/csum.c > index 7fb12c59e571..7a97394c252b 100644 > --- a/arch/riscv/lib/csum.c > +++ b/arch/riscv/lib/csum.c > @@ -44,8 +44,7 @@ __sum16 csum_ipv6_magic(const struct in6_addr *saddr, > * Zbb support saves 4 instructions, so not worth checking without > * alternatives if supported > */ > - if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && > - IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { > + if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB_ALT)) { > unsigned long fold_temp; > > /* > @@ -161,8 +160,7 @@ do_csum_with_alignment(const unsigned char *buff, int len) > * Zbb support saves 6 instructions, so not worth checking without > * alternatives if supported > */ > - if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && > - IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { > + if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB_ALT)) { > unsigned long fold_temp; > > /* > @@ -248,8 +246,7 @@ do_csum_no_alignment(const unsigned char *buff, int len) > * Zbb support saves 6 instructions, so not worth checking without > * alternatives if supported > */ > - if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && > - IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { > + if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB_ALT)) { > unsigned long fold_temp; > > /* > diff --git a/arch/riscv/lib/strcmp.S b/arch/riscv/lib/strcmp.S > index 687b2bea5c43..a4dd2ac306f1 100644 > --- a/arch/riscv/lib/strcmp.S > +++ b/arch/riscv/lib/strcmp.S > @@ -8,7 +8,7 @@ > /* int strcmp(const char *cs, const char *ct) */ > SYM_FUNC_START(strcmp) > > - ALTERNATIVE("nop", "j strcmp_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB) > + ALTERNATIVE("nop", "j strcmp_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB_ALT) > > /* > * Returns > @@ -43,7 +43,7 @@ SYM_FUNC_START(strcmp) > * The code was published as part of the bitmanip manual > * in Appendix A. > */ > -#ifdef CONFIG_RISCV_ISA_ZBB > +#ifdef CONFIG_RISCV_ISA_ZBB_ALT > strcmp_zbb: > > .option push > diff --git a/arch/riscv/lib/strlen.S b/arch/riscv/lib/strlen.S > index 8ae3064e45ff..3ab1310a7b83 100644 > --- a/arch/riscv/lib/strlen.S > +++ b/arch/riscv/lib/strlen.S > @@ -8,7 +8,7 @@ > /* int strlen(const char *s) */ > SYM_FUNC_START(strlen) > > - ALTERNATIVE("nop", "j strlen_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB) > + ALTERNATIVE("nop", "j strlen_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB_ALT) I appreciate the direction this is going in, getting rid of the CONFIG_RISCV_ALTERNATIVE checks in the code that checks CONFIG_RISCV_ISA_ZBB is a great change. I am missing why these str functions are changed to use CONFIG_RISCV_ISA_ZBB_ALT when the __arch_hweight* functions were left as using CONFIG_RISCV_ISA_ZBB in their alternatives. - Charlie > > /* > * Returns > @@ -33,7 +33,7 @@ SYM_FUNC_START(strlen) > /* > * Variant of strlen using the ZBB extension if available > */ > -#ifdef CONFIG_RISCV_ISA_ZBB > +#ifdef CONFIG_RISCV_ISA_ZBB_ALT > strlen_zbb: > > #ifdef CONFIG_CPU_BIG_ENDIAN > diff --git a/arch/riscv/lib/strncmp.S b/arch/riscv/lib/strncmp.S > index aba5b3148621..aeed830804d7 100644 > --- a/arch/riscv/lib/strncmp.S > +++ b/arch/riscv/lib/strncmp.S > @@ -8,7 +8,7 @@ > /* int strncmp(const char *cs, const char *ct, size_t count) */ > SYM_FUNC_START(strncmp) > > - ALTERNATIVE("nop", "j strncmp_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB) > + ALTERNATIVE("nop", "j strncmp_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB_ALT) > > /* > * Returns > @@ -46,7 +46,7 @@ SYM_FUNC_START(strncmp) > /* > * Variant of strncmp using the ZBB extension if available > */ > -#ifdef CONFIG_RISCV_ISA_ZBB > +#ifdef CONFIG_RISCV_ISA_ZBB_ALT > strncmp_zbb: > > .option push > -- > 2.43.0 >
On Wed, May 15, 2024 at 09:58:50PM -0700, Charlie Jenkins wrote: > On Wed, May 15, 2024 at 04:27:40PM +0100, Conor Dooley wrote: > > diff --git a/arch/riscv/lib/strlen.S b/arch/riscv/lib/strlen.S > > index 8ae3064e45ff..3ab1310a7b83 100644 > > --- a/arch/riscv/lib/strlen.S > > +++ b/arch/riscv/lib/strlen.S > > @@ -8,7 +8,7 @@ > > /* int strlen(const char *s) */ > > SYM_FUNC_START(strlen) > > > > - ALTERNATIVE("nop", "j strlen_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB) > > + ALTERNATIVE("nop", "j strlen_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB_ALT) > I am missing why these str functions are changed to use > CONFIG_RISCV_ISA_ZBB_ALT when the __arch_hweight* functions were left as > using CONFIG_RISCV_ISA_ZBB in their alternatives. I don't think I missed anything in the __arch_hweight*() functions, their final argument is 1 and they are not conditional on a config option as the whole block of code they're in is wrapped in ifdeffery: # ifdef CONFIG_RISCV_ISA_ZBB_ALT asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0, RISCV_ISA_EXT_ZBB, 1) : : : : legacy); Cheers, Conor.
On Wed, May 15, 2024 at 04:27:40PM GMT, Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > It seems a bit ridiculous to require toolchain support for BPF to > assemble Zbb instructions, so introduce hidden a Kconfig option that s/hidden a/a hidden/ > controls the use of any toolchain-requiring optimisations while support > is available. > > Zbb support has always depended on alternatives, so while adjusting the > config options guarding optimisations, remove any checks for > whether or not alternatives are enabled. > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > > This patch stems out of a conversation about Zba optimisations in BPF. > I'm not super sold on the approach in all honesty, even though we > recently had a conversation about respecting the Kconfig options - but > at this point I'd be convinced to just add some wording to the Kconfig > options mentioning that BPF optimisations are excluded. > Having hidden options that mean someone can turn what on what they > think are Zbb optimisations but not actually get any cos their toolchain > doesn't support it seems crap to me. I don't wanna add another > user-visible option for that situation cos I wanna try to minimise the > extent of our extension-related Kconfig options, not blow them up like > Augustus Gloop! > > Cheers, > Conor. > > CC: xiao.w.wang@intel.com > CC: Andrew Jones <ajones@ventanamicro.com> > CC: pulehui@huawei.com > CC: Charlie Jenkins <charlie@rivosinc.com> > CC: Paul Walmsley <paul.walmsley@sifive.com> > CC: Palmer Dabbelt <palmer@dabbelt.com> > CC: Conor Dooley <conor.dooley@microchip.com> > CC: linux-riscv@lists.infradead.org > CC: linux-kernel@vger.kernel.org > --- > arch/riscv/Kconfig | 15 ++++++++++++--- > arch/riscv/include/asm/arch_hweight.h | 4 ++-- > arch/riscv/include/asm/bitops.h | 4 ++-- > arch/riscv/include/asm/checksum.h | 3 +-- > arch/riscv/lib/csum.c | 9 +++------ > arch/riscv/lib/strcmp.S | 4 ++-- > arch/riscv/lib/strlen.S | 4 ++-- > arch/riscv/lib/strncmp.S | 4 ++-- > 8 files changed, 26 insertions(+), 21 deletions(-) > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index e927b52b420c..f216a52ed181 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -605,14 +605,23 @@ config TOOLCHAIN_HAS_VECTOR_CRYPTO > def_bool $(as-instr, .option arch$(comma) +v$(comma) +zvkb) > depends on AS_HAS_OPTION_ARCH > > -config RISCV_ISA_ZBB > - bool "Zbb extension support for bit manipulation instructions" > +config RISCV_ISA_ZBB_ALT > + def_bool RISCV_ISA_ZBB > depends on TOOLCHAIN_HAS_ZBB > depends on RISCV_ALTERNATIVE > + help > + This option controls whether or not we build optimisations that > + depend on toolchain support. It's automatically enabled whenever the > + toolchain in use supports assembling Zbb instructions and > + RISCV_ISA_ZBB is set. > + > +config RISCV_ISA_ZBB > + bool "Zbb extension support for bit manipulation instructions" > default y > help > Add support for enabling optimisations in the kernel when the > - Zbb extension is detected at boot. > + Zbb extension is detected at boot. Some optimisations may > + additionally depend on toolchain support for Zbb. > > The Zbb extension provides instructions to accelerate a number > of bit-specific operations (count bit population, sign extending, > diff --git a/arch/riscv/include/asm/arch_hweight.h b/arch/riscv/include/asm/arch_hweight.h > index 85b2c443823e..a677f6b82228 100644 > --- a/arch/riscv/include/asm/arch_hweight.h > +++ b/arch/riscv/include/asm/arch_hweight.h > @@ -19,7 +19,7 @@ > > static __always_inline unsigned int __arch_hweight32(unsigned int w) > { > -#ifdef CONFIG_RISCV_ISA_ZBB > +#ifdef CONFIG_RISCV_ISA_ZBB_ALT So the new hidden config is a shorthand for #if defined(CONFIG_RISCV_ISA_ZBB) && \ defined(CONFIG_TOOLCHAIN_HAS_ZBB) && \ defined(CONFIG_RISCV_ALTERNATIVE) which is reasonable to add, since that's a mouthful, but I'm not sure the name, RISCV_ISA_ZBB_ALT, does a good job conveying all that. If we instead just dropped the 'depends on TOOLCHAIN_HAS_ZBB' from config RISCV_ISA_ZBB (keeping the 'depends on RISCV_ALTERNATIVE', since nobody is really complaining about that), then we could change this to #if defined(CONFIG_RISCV_ISA_ZBB) && defined(CONFIG_TOOLCHAIN_HAS_ZBB) which is verbose, but easy to understand. > asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0, > RISCV_ISA_EXT_ZBB, 1) > : : : : legacy); > @@ -50,7 +50,7 @@ static inline unsigned int __arch_hweight8(unsigned int w) > #if BITS_PER_LONG == 64 > static __always_inline unsigned long __arch_hweight64(__u64 w) > { > -# ifdef CONFIG_RISCV_ISA_ZBB > +# ifdef CONFIG_RISCV_ISA_ZBB_ALT nit: While we're here we could remove that space after the # (and the corresponding one on the # endif) > asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0, > RISCV_ISA_EXT_ZBB, 1) > : : : : legacy); > diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h > index 880606b0469a..3ed810a6123d 100644 > --- a/arch/riscv/include/asm/bitops.h > +++ b/arch/riscv/include/asm/bitops.h > @@ -15,7 +15,7 @@ > #include <asm/barrier.h> > #include <asm/bitsperlong.h> > > -#if !defined(CONFIG_RISCV_ISA_ZBB) || defined(NO_ALTERNATIVE) > +#if !defined(CONFIG_RISCV_ISA_ZBB_ALT) || defined(NO_ALTERNATIVE) nit: It's sufficient to check !defined(CONFIG_RISCV_ISA_ZBB), so no need for this change or its #endif comment change below. > #include <asm-generic/bitops/__ffs.h> > #include <asm-generic/bitops/__fls.h> > #include <asm-generic/bitops/ffs.h> > @@ -175,7 +175,7 @@ static __always_inline int variable_fls(unsigned int x) > variable_fls(x_); \ > }) > > -#endif /* !defined(CONFIG_RISCV_ISA_ZBB) || defined(NO_ALTERNATIVE) */ > +#endif /* !defined(CONFIG_RISCV_ISA_ZBB_ALT) || defined(NO_ALTERNATIVE) */ > > #include <asm-generic/bitops/ffz.h> > #include <asm-generic/bitops/fls64.h> > diff --git a/arch/riscv/include/asm/checksum.h b/arch/riscv/include/asm/checksum.h > index 88e6f1499e88..956224ea8199 100644 > --- a/arch/riscv/include/asm/checksum.h > +++ b/arch/riscv/include/asm/checksum.h > @@ -49,8 +49,7 @@ static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl) > * ZBB only saves three instructions on 32-bit and five on 64-bit so not > * worth checking if supported without Alternatives. > */ > - if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && > - IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { > + if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB_ALT)) { When we can use C like this, rather than #if, we can create a helper to reduce the verbosity of what I proposed above, e.g. static inline bool csum_zbb(void) { return IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && IS_ENABLED(CONFIG_TOOLCHAIN_HAS_ZBB); } > unsigned long fold_temp; > > asm goto(ALTERNATIVE("j %l[no_zbb]", "nop", 0, > diff --git a/arch/riscv/lib/csum.c b/arch/riscv/lib/csum.c > index 7fb12c59e571..7a97394c252b 100644 > --- a/arch/riscv/lib/csum.c > +++ b/arch/riscv/lib/csum.c > @@ -44,8 +44,7 @@ __sum16 csum_ipv6_magic(const struct in6_addr *saddr, > * Zbb support saves 4 instructions, so not worth checking without > * alternatives if supported > */ > - if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && > - IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { > + if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB_ALT)) { > unsigned long fold_temp; > > /* > @@ -161,8 +160,7 @@ do_csum_with_alignment(const unsigned char *buff, int len) > * Zbb support saves 6 instructions, so not worth checking without > * alternatives if supported > */ > - if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && > - IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { > + if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB_ALT)) { > unsigned long fold_temp; > > /* > @@ -248,8 +246,7 @@ do_csum_no_alignment(const unsigned char *buff, int len) > * Zbb support saves 6 instructions, so not worth checking without > * alternatives if supported > */ > - if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && > - IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { > + if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB_ALT)) { > unsigned long fold_temp; > > /* > diff --git a/arch/riscv/lib/strcmp.S b/arch/riscv/lib/strcmp.S > index 687b2bea5c43..a4dd2ac306f1 100644 > --- a/arch/riscv/lib/strcmp.S > +++ b/arch/riscv/lib/strcmp.S > @@ -8,7 +8,7 @@ > /* int strcmp(const char *cs, const char *ct) */ > SYM_FUNC_START(strcmp) > > - ALTERNATIVE("nop", "j strcmp_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB) > + ALTERNATIVE("nop", "j strcmp_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB_ALT) Here we have a problem with the compound config expression, but we could rework these functions to be like this #if defined(CONFIG_RISCV_ISA_ZBB) && defined(CONFIG_TOOLCHAIN_HAS_ZBB) ALTERNATIVE("j strcmp_nozbb", "nop", 0, RISCV_ISA_EXT_ZBB, 1) ...zbb impl... ret #endif strcmp_nozbb: ...no zbb impl... > > /* > * Returns > @@ -43,7 +43,7 @@ SYM_FUNC_START(strcmp) > * The code was published as part of the bitmanip manual > * in Appendix A. > */ > -#ifdef CONFIG_RISCV_ISA_ZBB > +#ifdef CONFIG_RISCV_ISA_ZBB_ALT > strcmp_zbb: > > .option push > diff --git a/arch/riscv/lib/strlen.S b/arch/riscv/lib/strlen.S > index 8ae3064e45ff..3ab1310a7b83 100644 > --- a/arch/riscv/lib/strlen.S > +++ b/arch/riscv/lib/strlen.S > @@ -8,7 +8,7 @@ > /* int strlen(const char *s) */ > SYM_FUNC_START(strlen) > > - ALTERNATIVE("nop", "j strlen_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB) > + ALTERNATIVE("nop", "j strlen_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB_ALT) > > /* > * Returns > @@ -33,7 +33,7 @@ SYM_FUNC_START(strlen) > /* > * Variant of strlen using the ZBB extension if available > */ > -#ifdef CONFIG_RISCV_ISA_ZBB > +#ifdef CONFIG_RISCV_ISA_ZBB_ALT > strlen_zbb: > > #ifdef CONFIG_CPU_BIG_ENDIAN > diff --git a/arch/riscv/lib/strncmp.S b/arch/riscv/lib/strncmp.S > index aba5b3148621..aeed830804d7 100644 > --- a/arch/riscv/lib/strncmp.S > +++ b/arch/riscv/lib/strncmp.S > @@ -8,7 +8,7 @@ > /* int strncmp(const char *cs, const char *ct, size_t count) */ > SYM_FUNC_START(strncmp) > > - ALTERNATIVE("nop", "j strncmp_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB) > + ALTERNATIVE("nop", "j strncmp_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB_ALT) > > /* > * Returns > @@ -46,7 +46,7 @@ SYM_FUNC_START(strncmp) > /* > * Variant of strncmp using the ZBB extension if available > */ > -#ifdef CONFIG_RISCV_ISA_ZBB > +#ifdef CONFIG_RISCV_ISA_ZBB_ALT > strncmp_zbb: > > .option push > -- > 2.43.0 > Thanks, drew
On Thu, May 16, 2024 at 09:59:44AM +0200, Andrew Jones wrote: > On Wed, May 15, 2024 at 04:27:40PM GMT, Conor Dooley wrote: > > So the new hidden config is a shorthand for > > #if defined(CONFIG_RISCV_ISA_ZBB) && \ > defined(CONFIG_TOOLCHAIN_HAS_ZBB) && \ > defined(CONFIG_RISCV_ALTERNATIVE) > > which is reasonable to add, since that's a mouthful, but I'm not sure the > name, RISCV_ISA_ZBB_ALT, does a good job conveying all that. > > If we instead just dropped the 'depends on TOOLCHAIN_HAS_ZBB' from > config RISCV_ISA_ZBB (keeping the 'depends on RISCV_ALTERNATIVE', > since nobody is really complaining about that), then we could change > this to > > #if defined(CONFIG_RISCV_ISA_ZBB) && defined(CONFIG_TOOLCHAIN_HAS_ZBB) Yeah, I think this is a cleaner solution. > > asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0, > > RISCV_ISA_EXT_ZBB, 1) > > : : : : legacy); > > diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h > > index 880606b0469a..3ed810a6123d 100644 > > --- a/arch/riscv/include/asm/bitops.h > > +++ b/arch/riscv/include/asm/bitops.h > > @@ -15,7 +15,7 @@ > > #include <asm/barrier.h> > > #include <asm/bitsperlong.h> > > > > -#if !defined(CONFIG_RISCV_ISA_ZBB) || defined(NO_ALTERNATIVE) > > +#if !defined(CONFIG_RISCV_ISA_ZBB_ALT) || defined(NO_ALTERNATIVE) > > nit: It's sufficient to check !defined(CONFIG_RISCV_ISA_ZBB), so no need > for this change or its #endif comment change below. Are you sure? I did test leaving this as-was and it broke the build for llvm-14. Cheers, Conor.
On May 16, 2024 10:59:44 AM GMT+02:00, Conor Dooley <conor.dooley@microchip.com> wrote: >On Thu, May 16, 2024 at 09:59:44AM +0200, Andrew Jones wrote: >> On Wed, May 15, 2024 at 04:27:40PM GMT, Conor Dooley wrote: >> >> So the new hidden config is a shorthand for >> >> #if defined(CONFIG_RISCV_ISA_ZBB) && \ >> defined(CONFIG_TOOLCHAIN_HAS_ZBB) && \ >> defined(CONFIG_RISCV_ALTERNATIVE) >> >> which is reasonable to add, since that's a mouthful, but I'm not sure the >> name, RISCV_ISA_ZBB_ALT, does a good job conveying all that. >> >> If we instead just dropped the 'depends on TOOLCHAIN_HAS_ZBB' from >> config RISCV_ISA_ZBB (keeping the 'depends on RISCV_ALTERNATIVE', >> since nobody is really complaining about that), then we could change >> this to >> >> #if defined(CONFIG_RISCV_ISA_ZBB) && defined(CONFIG_TOOLCHAIN_HAS_ZBB) > >Yeah, I think this is a cleaner solution. > >> > asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0, >> > RISCV_ISA_EXT_ZBB, 1) >> > : : : : legacy); >> > diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h >> > index 880606b0469a..3ed810a6123d 100644 >> > --- a/arch/riscv/include/asm/bitops.h >> > +++ b/arch/riscv/include/asm/bitops.h >> > @@ -15,7 +15,7 @@ >> > #include <asm/barrier.h> >> > #include <asm/bitsperlong.h> >> > >> > -#if !defined(CONFIG_RISCV_ISA_ZBB) || defined(NO_ALTERNATIVE) >> > +#if !defined(CONFIG_RISCV_ISA_ZBB_ALT) || defined(NO_ALTERNATIVE) >> >> nit: It's sufficient to check !defined(CONFIG_RISCV_ISA_ZBB), so no need >> for this change or its #endif comment change below. > >Are you sure? I did test leaving this as-was and it broke the build for >llvm-14. Oops, sorry. I didn't look at the full context. You were right. Thanks, drew > >Cheers, >Conor.
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index e927b52b420c..f216a52ed181 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -605,14 +605,23 @@ config TOOLCHAIN_HAS_VECTOR_CRYPTO def_bool $(as-instr, .option arch$(comma) +v$(comma) +zvkb) depends on AS_HAS_OPTION_ARCH -config RISCV_ISA_ZBB - bool "Zbb extension support for bit manipulation instructions" +config RISCV_ISA_ZBB_ALT + def_bool RISCV_ISA_ZBB depends on TOOLCHAIN_HAS_ZBB depends on RISCV_ALTERNATIVE + help + This option controls whether or not we build optimisations that + depend on toolchain support. It's automatically enabled whenever the + toolchain in use supports assembling Zbb instructions and + RISCV_ISA_ZBB is set. + +config RISCV_ISA_ZBB + bool "Zbb extension support for bit manipulation instructions" default y help Add support for enabling optimisations in the kernel when the - Zbb extension is detected at boot. + Zbb extension is detected at boot. Some optimisations may + additionally depend on toolchain support for Zbb. The Zbb extension provides instructions to accelerate a number of bit-specific operations (count bit population, sign extending, diff --git a/arch/riscv/include/asm/arch_hweight.h b/arch/riscv/include/asm/arch_hweight.h index 85b2c443823e..a677f6b82228 100644 --- a/arch/riscv/include/asm/arch_hweight.h +++ b/arch/riscv/include/asm/arch_hweight.h @@ -19,7 +19,7 @@ static __always_inline unsigned int __arch_hweight32(unsigned int w) { -#ifdef CONFIG_RISCV_ISA_ZBB +#ifdef CONFIG_RISCV_ISA_ZBB_ALT asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0, RISCV_ISA_EXT_ZBB, 1) : : : : legacy); @@ -50,7 +50,7 @@ static inline unsigned int __arch_hweight8(unsigned int w) #if BITS_PER_LONG == 64 static __always_inline unsigned long __arch_hweight64(__u64 w) { -# ifdef CONFIG_RISCV_ISA_ZBB +# ifdef CONFIG_RISCV_ISA_ZBB_ALT asm goto(ALTERNATIVE("j %l[legacy]", "nop", 0, RISCV_ISA_EXT_ZBB, 1) : : : : legacy); diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h index 880606b0469a..3ed810a6123d 100644 --- a/arch/riscv/include/asm/bitops.h +++ b/arch/riscv/include/asm/bitops.h @@ -15,7 +15,7 @@ #include <asm/barrier.h> #include <asm/bitsperlong.h> -#if !defined(CONFIG_RISCV_ISA_ZBB) || defined(NO_ALTERNATIVE) +#if !defined(CONFIG_RISCV_ISA_ZBB_ALT) || defined(NO_ALTERNATIVE) #include <asm-generic/bitops/__ffs.h> #include <asm-generic/bitops/__fls.h> #include <asm-generic/bitops/ffs.h> @@ -175,7 +175,7 @@ static __always_inline int variable_fls(unsigned int x) variable_fls(x_); \ }) -#endif /* !defined(CONFIG_RISCV_ISA_ZBB) || defined(NO_ALTERNATIVE) */ +#endif /* !defined(CONFIG_RISCV_ISA_ZBB_ALT) || defined(NO_ALTERNATIVE) */ #include <asm-generic/bitops/ffz.h> #include <asm-generic/bitops/fls64.h> diff --git a/arch/riscv/include/asm/checksum.h b/arch/riscv/include/asm/checksum.h index 88e6f1499e88..956224ea8199 100644 --- a/arch/riscv/include/asm/checksum.h +++ b/arch/riscv/include/asm/checksum.h @@ -49,8 +49,7 @@ static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl) * ZBB only saves three instructions on 32-bit and five on 64-bit so not * worth checking if supported without Alternatives. */ - if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && - IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { + if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB_ALT)) { unsigned long fold_temp; asm goto(ALTERNATIVE("j %l[no_zbb]", "nop", 0, diff --git a/arch/riscv/lib/csum.c b/arch/riscv/lib/csum.c index 7fb12c59e571..7a97394c252b 100644 --- a/arch/riscv/lib/csum.c +++ b/arch/riscv/lib/csum.c @@ -44,8 +44,7 @@ __sum16 csum_ipv6_magic(const struct in6_addr *saddr, * Zbb support saves 4 instructions, so not worth checking without * alternatives if supported */ - if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && - IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { + if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB_ALT)) { unsigned long fold_temp; /* @@ -161,8 +160,7 @@ do_csum_with_alignment(const unsigned char *buff, int len) * Zbb support saves 6 instructions, so not worth checking without * alternatives if supported */ - if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && - IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { + if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB_ALT)) { unsigned long fold_temp; /* @@ -248,8 +246,7 @@ do_csum_no_alignment(const unsigned char *buff, int len) * Zbb support saves 6 instructions, so not worth checking without * alternatives if supported */ - if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && - IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { + if (IS_ENABLED(CONFIG_RISCV_ISA_ZBB_ALT)) { unsigned long fold_temp; /* diff --git a/arch/riscv/lib/strcmp.S b/arch/riscv/lib/strcmp.S index 687b2bea5c43..a4dd2ac306f1 100644 --- a/arch/riscv/lib/strcmp.S +++ b/arch/riscv/lib/strcmp.S @@ -8,7 +8,7 @@ /* int strcmp(const char *cs, const char *ct) */ SYM_FUNC_START(strcmp) - ALTERNATIVE("nop", "j strcmp_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB) + ALTERNATIVE("nop", "j strcmp_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB_ALT) /* * Returns @@ -43,7 +43,7 @@ SYM_FUNC_START(strcmp) * The code was published as part of the bitmanip manual * in Appendix A. */ -#ifdef CONFIG_RISCV_ISA_ZBB +#ifdef CONFIG_RISCV_ISA_ZBB_ALT strcmp_zbb: .option push diff --git a/arch/riscv/lib/strlen.S b/arch/riscv/lib/strlen.S index 8ae3064e45ff..3ab1310a7b83 100644 --- a/arch/riscv/lib/strlen.S +++ b/arch/riscv/lib/strlen.S @@ -8,7 +8,7 @@ /* int strlen(const char *s) */ SYM_FUNC_START(strlen) - ALTERNATIVE("nop", "j strlen_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB) + ALTERNATIVE("nop", "j strlen_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB_ALT) /* * Returns @@ -33,7 +33,7 @@ SYM_FUNC_START(strlen) /* * Variant of strlen using the ZBB extension if available */ -#ifdef CONFIG_RISCV_ISA_ZBB +#ifdef CONFIG_RISCV_ISA_ZBB_ALT strlen_zbb: #ifdef CONFIG_CPU_BIG_ENDIAN diff --git a/arch/riscv/lib/strncmp.S b/arch/riscv/lib/strncmp.S index aba5b3148621..aeed830804d7 100644 --- a/arch/riscv/lib/strncmp.S +++ b/arch/riscv/lib/strncmp.S @@ -8,7 +8,7 @@ /* int strncmp(const char *cs, const char *ct, size_t count) */ SYM_FUNC_START(strncmp) - ALTERNATIVE("nop", "j strncmp_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB) + ALTERNATIVE("nop", "j strncmp_zbb", 0, RISCV_ISA_EXT_ZBB, CONFIG_RISCV_ISA_ZBB_ALT) /* * Returns @@ -46,7 +46,7 @@ SYM_FUNC_START(strncmp) /* * Variant of strncmp using the ZBB extension if available */ -#ifdef CONFIG_RISCV_ISA_ZBB +#ifdef CONFIG_RISCV_ISA_ZBB_ALT strncmp_zbb: .option push