From: Conor Dooley <[email protected]>
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 <[email protected]>
---
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: [email protected]
CC: Andrew Jones <[email protected]>
CC: [email protected]
CC: Charlie Jenkins <[email protected]>
CC: Paul Walmsley <[email protected]>
CC: Palmer Dabbelt <[email protected]>
CC: Conor Dooley <[email protected]>
CC: [email protected]
CC: [email protected]
---
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)
/*
* 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 04:27:40PM +0100, Conor Dooley wrote:
> From: Conor Dooley <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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: [email protected]
> CC: Andrew Jones <[email protected]>
> CC: [email protected]
> CC: Charlie Jenkins <[email protected]>
> CC: Paul Walmsley <[email protected]>
> CC: Palmer Dabbelt <[email protected]>
> CC: Conor Dooley <[email protected]>
> CC: [email protected]
> CC: [email protected]
> ---
> 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 <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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: [email protected]
> CC: Andrew Jones <[email protected]>
> CC: [email protected]
> CC: Charlie Jenkins <[email protected]>
> CC: Paul Walmsley <[email protected]>
> CC: Palmer Dabbelt <[email protected]>
> CC: Conor Dooley <[email protected]>
> CC: [email protected]
> CC: [email protected]
> ---
> 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 <[email protected]> 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.