The Zba extension provides add.uw insn which can be used to implement
zext.w with rs2 set as ZERO.
Signed-off-by: Xiao Wang <[email protected]>
---
v2:
* Add Zba description in the Kconfig. (Lehui)
* Reword the Kconfig help message to make it clearer. (Conor)
---
arch/riscv/Kconfig | 22 ++++++++++++++++++++++
arch/riscv/net/bpf_jit.h | 18 ++++++++++++++++++
2 files changed, 40 insertions(+)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 6bec1bce6586..e262a8668b41 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -586,6 +586,14 @@ config RISCV_ISA_V_PREEMPTIVE
preemption. Enabling this config will result in higher memory
consumption due to the allocation of per-task's kernel Vector context.
+config TOOLCHAIN_HAS_ZBA
+ bool
+ default y
+ depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zba)
+ depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zba)
+ depends on LLD_VERSION >= 150000 || LD_VERSION >= 23900
+ depends on AS_HAS_OPTION_ARCH
+
config TOOLCHAIN_HAS_ZBB
bool
default y
@@ -601,6 +609,20 @@ config TOOLCHAIN_HAS_VECTOR_CRYPTO
def_bool $(as-instr, .option arch$(comma) +v$(comma) +zvkb)
depends on AS_HAS_OPTION_ARCH
+config RISCV_ISA_ZBA
+ bool "Zba extension support for bit manipulation instructions"
+ depends on TOOLCHAIN_HAS_ZBA
+ depends on RISCV_ALTERNATIVE
+ default y
+ help
+ Add support for enabling optimisations in the kernel when the Zba
+ extension is detected at boot.
+
+ The Zba extension provides instructions to accelerate the generation
+ of addresses that index into arrays of basic data types.
+
+ If you don't know what to do here, say Y.
+
config RISCV_ISA_ZBB
bool "Zbb extension support for bit manipulation instructions"
depends on TOOLCHAIN_HAS_ZBB
diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
index f4b6b3b9edda..18a7885ba95e 100644
--- a/arch/riscv/net/bpf_jit.h
+++ b/arch/riscv/net/bpf_jit.h
@@ -18,6 +18,11 @@ static inline bool rvc_enabled(void)
return IS_ENABLED(CONFIG_RISCV_ISA_C);
}
+static inline bool rvzba_enabled(void)
+{
+ return IS_ENABLED(CONFIG_RISCV_ISA_ZBA) && riscv_has_extension_likely(RISCV_ISA_EXT_ZBA);
+}
+
static inline bool rvzbb_enabled(void)
{
return IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && riscv_has_extension_likely(RISCV_ISA_EXT_ZBB);
@@ -937,6 +942,14 @@ static inline u16 rvc_sdsp(u32 imm9, u8 rs2)
return rv_css_insn(0x7, imm, rs2, 0x2);
}
+/* RV64-only ZBA instructions. */
+
+static inline u32 rvzba_zextw(u8 rd, u8 rs1)
+{
+ /* add.uw rd, rs1, ZERO */
+ return rv_r_insn(0x04, RV_REG_ZERO, rs1, 0, rd, 0x3b);
+}
+
#endif /* __riscv_xlen == 64 */
/* Helper functions that emit RVC instructions when possible. */
@@ -1159,6 +1172,11 @@ static inline void emit_zexth(u8 rd, u8 rs, struct rv_jit_context *ctx)
static inline void emit_zextw(u8 rd, u8 rs, struct rv_jit_context *ctx)
{
+ if (rvzba_enabled()) {
+ emit(rvzba_zextw(rd, rs), ctx);
+ return;
+ }
+
emit_slli(rd, rs, 32, ctx);
emit_srli(rd, rd, 32, ctx);
}
--
2.25.1
On Sat, May 11, 2024 at 10:34:36AM GMT, Xiao Wang wrote:
> The Zba extension provides add.uw insn which can be used to implement
> zext.w with rs2 set as ZERO.
>
> Signed-off-by: Xiao Wang <[email protected]>
> ---
> v2:
> * Add Zba description in the Kconfig. (Lehui)
> * Reword the Kconfig help message to make it clearer. (Conor)
> ---
> arch/riscv/Kconfig | 22 ++++++++++++++++++++++
> arch/riscv/net/bpf_jit.h | 18 ++++++++++++++++++
> 2 files changed, 40 insertions(+)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 6bec1bce6586..e262a8668b41 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -586,6 +586,14 @@ config RISCV_ISA_V_PREEMPTIVE
> preemption. Enabling this config will result in higher memory
> consumption due to the allocation of per-task's kernel Vector context.
>
> +config TOOLCHAIN_HAS_ZBA
> + bool
> + default y
> + depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zba)
> + depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zba)
> + depends on LLD_VERSION >= 150000 || LD_VERSION >= 23900
> + depends on AS_HAS_OPTION_ARCH
> +
> config TOOLCHAIN_HAS_ZBB
> bool
> default y
> @@ -601,6 +609,20 @@ config TOOLCHAIN_HAS_VECTOR_CRYPTO
> def_bool $(as-instr, .option arch$(comma) +v$(comma) +zvkb)
> depends on AS_HAS_OPTION_ARCH
>
> +config RISCV_ISA_ZBA
> + bool "Zba extension support for bit manipulation instructions"
> + depends on TOOLCHAIN_HAS_ZBA
We handcraft the instruction, so why do we need toolchain support?
> + depends on RISCV_ALTERNATIVE
Also, while riscv_has_extension_likely() will be accelerated with
RISCV_ALTERNATIVE, it's not required.
> + default y
> + help
> + Add support for enabling optimisations in the kernel when the Zba
> + extension is detected at boot.
> +
> + The Zba extension provides instructions to accelerate the generation
> + of addresses that index into arrays of basic data types.
> +
> + If you don't know what to do here, say Y.
> +
> config RISCV_ISA_ZBB
> bool "Zbb extension support for bit manipulation instructions"
> depends on TOOLCHAIN_HAS_ZBB
> diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
> index f4b6b3b9edda..18a7885ba95e 100644
> --- a/arch/riscv/net/bpf_jit.h
> +++ b/arch/riscv/net/bpf_jit.h
> @@ -18,6 +18,11 @@ static inline bool rvc_enabled(void)
> return IS_ENABLED(CONFIG_RISCV_ISA_C);
> }
>
> +static inline bool rvzba_enabled(void)
> +{
> + return IS_ENABLED(CONFIG_RISCV_ISA_ZBA) && riscv_has_extension_likely(RISCV_ISA_EXT_ZBA);
> +}
> +
> static inline bool rvzbb_enabled(void)
> {
> return IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && riscv_has_extension_likely(RISCV_ISA_EXT_ZBB);
> @@ -937,6 +942,14 @@ static inline u16 rvc_sdsp(u32 imm9, u8 rs2)
> return rv_css_insn(0x7, imm, rs2, 0x2);
> }
>
> +/* RV64-only ZBA instructions. */
> +
> +static inline u32 rvzba_zextw(u8 rd, u8 rs1)
> +{
> + /* add.uw rd, rs1, ZERO */
> + return rv_r_insn(0x04, RV_REG_ZERO, rs1, 0, rd, 0x3b);
> +}
> +
> #endif /* __riscv_xlen == 64 */
>
> /* Helper functions that emit RVC instructions when possible. */
> @@ -1159,6 +1172,11 @@ static inline void emit_zexth(u8 rd, u8 rs, struct rv_jit_context *ctx)
>
> static inline void emit_zextw(u8 rd, u8 rs, struct rv_jit_context *ctx)
> {
> + if (rvzba_enabled()) {
> + emit(rvzba_zextw(rd, rs), ctx);
> + return;
> + }
> +
> emit_slli(rd, rs, 32, ctx);
> emit_srli(rd, rd, 32, ctx);
> }
> --
> 2.25.1
>
Thanks,
drew
> -----Original Message-----
> From: Andrew Jones <[email protected]>
> Sent: Tuesday, May 14, 2024 12:53 AM
> To: Wang, Xiao W <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; Li, Haicheng <[email protected]>;
> [email protected]
> Subject: Re: [PATCH v2] riscv, bpf: Optimize zextw insn with Zba extension
>
> On Sat, May 11, 2024 at 10:34:36AM GMT, Xiao Wang wrote:
> > The Zba extension provides add.uw insn which can be used to implement
> > zext.w with rs2 set as ZERO.
> >
> > Signed-off-by: Xiao Wang <[email protected]>
> > ---
> > v2:
> > * Add Zba description in the Kconfig. (Lehui)
> > * Reword the Kconfig help message to make it clearer. (Conor)
> > ---
> > arch/riscv/Kconfig | 22 ++++++++++++++++++++++
> > arch/riscv/net/bpf_jit.h | 18 ++++++++++++++++++
> > 2 files changed, 40 insertions(+)
> >
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 6bec1bce6586..e262a8668b41 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -586,6 +586,14 @@ config RISCV_ISA_V_PREEMPTIVE
> > preemption. Enabling this config will result in higher memory
> > consumption due to the allocation of per-task's kernel Vector
> context.
> >
> > +config TOOLCHAIN_HAS_ZBA
> > + bool
> > + default y
> > + depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zba)
> > + depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zba)
> > + depends on LLD_VERSION >= 150000 || LD_VERSION >= 23900
> > + depends on AS_HAS_OPTION_ARCH
> > +
> > config TOOLCHAIN_HAS_ZBB
> > bool
> > default y
> > @@ -601,6 +609,20 @@ config TOOLCHAIN_HAS_VECTOR_CRYPTO
> > def_bool $(as-instr, .option arch$(comma) +v$(comma) +zvkb)
> > depends on AS_HAS_OPTION_ARCH
> >
> > +config RISCV_ISA_ZBA
> > + bool "Zba extension support for bit manipulation instructions"
> > + depends on TOOLCHAIN_HAS_ZBA
>
> We handcraft the instruction, so why do we need toolchain support?
Good point, we don't need toolchain support for this bpf jit case.
>
> > + depends on RISCV_ALTERNATIVE
>
> Also, while riscv_has_extension_likely() will be accelerated with
> RISCV_ALTERNATIVE, it's not required.
Agree, it's not required. For this bpf jit case, we should drop these two dependencies.
BTW, Zbb is used in bpf jit, the usage there also doesn't depend on toolchain and
RISCV_ALTERNATIVE, but the Kconfig for RISCV_ISA_ZBB has forced the dependencies
due to Zbb assembly programming elsewhere.
Maybe we could just dynamically check the existence of RISCV_ISA_ZB* before jit code
emission? or introduce new config options for bpf jit? I prefer the first method and
welcome any comments.
Thanks,
Xiao
[...]
> > {
> > + if (rvzba_enabled()) {
> > + emit(rvzba_zextw(rd, rs), ctx);
> > + return;
> > + }
> > +
> > emit_slli(rd, rs, 32, ctx);
> > emit_srli(rd, rd, 32, ctx);
> > }
> > --
> > 2.25.1
> >
>
> Thanks,
> drew
On Tue, May 14, 2024 at 07:36:04AM GMT, Wang, Xiao W wrote:
>
>
> > -----Original Message-----
> > From: Andrew Jones <[email protected]>
> > Sent: Tuesday, May 14, 2024 12:53 AM
> > To: Wang, Xiao W <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected]; linux-
> > [email protected]; [email protected]; [email protected];
> > [email protected]; Li, Haicheng <[email protected]>;
> > [email protected]
> > Subject: Re: [PATCH v2] riscv, bpf: Optimize zextw insn with Zba extension
> >
> > On Sat, May 11, 2024 at 10:34:36AM GMT, Xiao Wang wrote:
> > > The Zba extension provides add.uw insn which can be used to implement
> > > zext.w with rs2 set as ZERO.
> > >
> > > Signed-off-by: Xiao Wang <[email protected]>
> > > ---
> > > v2:
> > > * Add Zba description in the Kconfig. (Lehui)
> > > * Reword the Kconfig help message to make it clearer. (Conor)
> > > ---
> > > arch/riscv/Kconfig | 22 ++++++++++++++++++++++
> > > arch/riscv/net/bpf_jit.h | 18 ++++++++++++++++++
> > > 2 files changed, 40 insertions(+)
> > >
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index 6bec1bce6586..e262a8668b41 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -586,6 +586,14 @@ config RISCV_ISA_V_PREEMPTIVE
> > > preemption. Enabling this config will result in higher memory
> > > consumption due to the allocation of per-task's kernel Vector
> > context.
> > >
> > > +config TOOLCHAIN_HAS_ZBA
> > > + bool
> > > + default y
> > > + depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zba)
> > > + depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zba)
> > > + depends on LLD_VERSION >= 150000 || LD_VERSION >= 23900
> > > + depends on AS_HAS_OPTION_ARCH
> > > +
> > > config TOOLCHAIN_HAS_ZBB
> > > bool
> > > default y
> > > @@ -601,6 +609,20 @@ config TOOLCHAIN_HAS_VECTOR_CRYPTO
> > > def_bool $(as-instr, .option arch$(comma) +v$(comma) +zvkb)
> > > depends on AS_HAS_OPTION_ARCH
> > >
> > > +config RISCV_ISA_ZBA
> > > + bool "Zba extension support for bit manipulation instructions"
> > > + depends on TOOLCHAIN_HAS_ZBA
> >
> > We handcraft the instruction, so why do we need toolchain support?
>
> Good point, we don't need toolchain support for this bpf jit case.
>
> >
> > > + depends on RISCV_ALTERNATIVE
> >
> > Also, while riscv_has_extension_likely() will be accelerated with
> > RISCV_ALTERNATIVE, it's not required.
>
> Agree, it's not required. For this bpf jit case, we should drop these two dependencies.
>
> BTW, Zbb is used in bpf jit, the usage there also doesn't depend on toolchain and
> RISCV_ALTERNATIVE, but the Kconfig for RISCV_ISA_ZBB has forced the dependencies
> due to Zbb assembly programming elsewhere.
> Maybe we could just dynamically check the existence of RISCV_ISA_ZB* before jit code
> emission? or introduce new config options for bpf jit? I prefer the first method and
> welcome any comments.
My preferences is to remove as much of the TOOLCHAIN_HAS_ stuff as
possible. We should audit the extensions which have them to see if
they're really necessary. I don't mind depending on RISCV_ALTERNATIVE,
since it's almost required for riscv at this point anyway.
Thanks,
drew
>
> Thanks,
> Xiao
>
> [...]
> > > {
> > > + if (rvzba_enabled()) {
> > > + emit(rvzba_zextw(rd, rs), ctx);
> > > + return;
> > > + }
> > > +
> > > emit_slli(rd, rs, 32, ctx);
> > > emit_srli(rd, rd, 32, ctx);
> > > }
> > > --
> > > 2.25.1
> > >
> >
> > Thanks,
> > drew
> -----Original Message-----
> From: Andrew Jones <[email protected]>
> Sent: Tuesday, May 14, 2024 9:37 PM
> To: Wang, Xiao W <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; Li, Haicheng <[email protected]>;
> [email protected]; Ben Dooks <[email protected]>
> Subject: Re: [PATCH v2] riscv, bpf: Optimize zextw insn with Zba extension
>
> On Tue, May 14, 2024 at 07:36:04AM GMT, Wang, Xiao W wrote:
> >
> >
> > > -----Original Message-----
> > > From: Andrew Jones <[email protected]>
> > > Sent: Tuesday, May 14, 2024 12:53 AM
> > > To: Wang, Xiao W <[email protected]>
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected];
> [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected];
> [email protected];
> > > [email protected]; [email protected]; [email protected]; linux-
> > > [email protected]; [email protected];
> [email protected];
> > > [email protected]; Li, Haicheng <[email protected]>;
> > > [email protected]
> > > Subject: Re: [PATCH v2] riscv, bpf: Optimize zextw insn with Zba extension
> > >
> > > On Sat, May 11, 2024 at 10:34:36AM GMT, Xiao Wang wrote:
> > > > The Zba extension provides add.uw insn which can be used to implement
> > > > zext.w with rs2 set as ZERO.
> > > >
> > > > Signed-off-by: Xiao Wang <[email protected]>
> > > > ---
> > > > v2:
> > > > * Add Zba description in the Kconfig. (Lehui)
> > > > * Reword the Kconfig help message to make it clearer. (Conor)
> > > > ---
> > > > arch/riscv/Kconfig | 22 ++++++++++++++++++++++
> > > > arch/riscv/net/bpf_jit.h | 18 ++++++++++++++++++
> > > > 2 files changed, 40 insertions(+)
> > > >
> > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > index 6bec1bce6586..e262a8668b41 100644
> > > > --- a/arch/riscv/Kconfig
> > > > +++ b/arch/riscv/Kconfig
> > > > @@ -586,6 +586,14 @@ config RISCV_ISA_V_PREEMPTIVE
> > > > preemption. Enabling this config will result in higher memory
> > > > consumption due to the allocation of per-task's kernel Vector
> > > context.
> > > >
> > > > +config TOOLCHAIN_HAS_ZBA
> > > > + bool
> > > > + default y
> > > > + depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zba)
> > > > + depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zba)
> > > > + depends on LLD_VERSION >= 150000 || LD_VERSION >= 23900
> > > > + depends on AS_HAS_OPTION_ARCH
> > > > +
> > > > config TOOLCHAIN_HAS_ZBB
> > > > bool
> > > > default y
> > > > @@ -601,6 +609,20 @@ config TOOLCHAIN_HAS_VECTOR_CRYPTO
> > > > def_bool $(as-instr, .option arch$(comma) +v$(comma) +zvkb)
> > > > depends on AS_HAS_OPTION_ARCH
> > > >
> > > > +config RISCV_ISA_ZBA
> > > > + bool "Zba extension support for bit manipulation instructions"
> > > > + depends on TOOLCHAIN_HAS_ZBA
> > >
> > > We handcraft the instruction, so why do we need toolchain support?
> >
> > Good point, we don't need toolchain support for this bpf jit case.
> >
> > >
> > > > + depends on RISCV_ALTERNATIVE
> > >
> > > Also, while riscv_has_extension_likely() will be accelerated with
> > > RISCV_ALTERNATIVE, it's not required.
> >
> > Agree, it's not required. For this bpf jit case, we should drop these two
> dependencies.
> >
> > BTW, Zbb is used in bpf jit, the usage there also doesn't depend on toolchain
> and
> > RISCV_ALTERNATIVE, but the Kconfig for RISCV_ISA_ZBB has forced the
> dependencies
> > due to Zbb assembly programming elsewhere.
> > Maybe we could just dynamically check the existence of RISCV_ISA_ZB*
> before jit code
> > emission? or introduce new config options for bpf jit? I prefer the first
> method and
> > welcome any comments.
>
> My preferences is to remove as much of the TOOLCHAIN_HAS_ stuff as
> possible. We should audit the extensions which have them to see if
> they're really necessary. I don't mind depending on RISCV_ALTERNATIVE,
> since it's almost required for riscv at this point anyway.
I go through all the existing TOOLCHAIN_HAS_* stuff, all of them are
helpful for compiling the corresponding assembly code. So they're really
necessary.
For this patch, I would drop the two dependencies for RISCV_ISA_ZBA Kconfig,
as the jit doesn't depend on them.
BRs,
Xiao
>
> Thanks,
> drew
>
> >
> > Thanks,
> > Xiao
> >
> > [...]
> > > > {
> > > > + if (rvzba_enabled()) {
> > > > + emit(rvzba_zextw(rd, rs), ctx);
> > > > + return;
> > > > + }
> > > > +
> > > > emit_slli(rd, rs, 32, ctx);
> > > > emit_srli(rd, rd, 32, ctx);
> > > > }
> > > > --
> > > > 2.25.1
> > > >
> > >
> > > Thanks,
> > > drew
On Tue, May 14, 2024 at 03:37:02PM +0200, Andrew Jones wrote:
> On Tue, May 14, 2024 at 07:36:04AM GMT, Wang, Xiao W wrote:
> > > From: Andrew Jones <[email protected]>
>> > > > +config RISCV_ISA_ZBA
> > > > + bool "Zba extension support for bit manipulation instructions"
> > > > + depends on TOOLCHAIN_HAS_ZBA
> > >
> > > We handcraft the instruction, so why do we need toolchain support?
> >
> > Good point, we don't need toolchain support for this bpf jit case.
> >
> > >
> > > > + depends on RISCV_ALTERNATIVE
> > >
> > > Also, while riscv_has_extension_likely() will be accelerated with
> > > RISCV_ALTERNATIVE, it's not required.
> >
> > Agree, it's not required. For this bpf jit case, we should drop these two dependencies.
> >
> > BTW, Zbb is used in bpf jit, the usage there also doesn't depend on toolchain and
> > RISCV_ALTERNATIVE, but the Kconfig for RISCV_ISA_ZBB has forced the dependencies
> > due to Zbb assembly programming elsewhere.
> > Maybe we could just dynamically check the existence of RISCV_ISA_ZB* before jit code
> > emission? or introduce new config options for bpf jit? I prefer the first method and
> > welcome any comments.
>
> My preferences is to remove as much of the TOOLCHAIN_HAS_ stuff as
> possible. We should audit the extensions which have them to see if
> they're really necessary.
While I think it is reasonable to allow the "RISCV_ISA_ZBB" option to
control whether or not bpf is allowed to use it for optimisations, only
allowing bpf to do that if there's toolchain support feels odd to me..
Maybe we need to sorta steal from Charlie's patchset and introduce
some hidden options that have the toolchain dep that are used by the
alternative macros etc?
I'll have a poke at how bad that looks I think.
> I don't mind depending on RISCV_ALTERNATIVE,
> since it's almost required for riscv at this point anyway.
As you say, using on riscv_has_extension_likely() doesn't mean you
depend on alternatives so effectively all this does is rule out use
with XIP, since alternatives are selected when !XIP. Does BPF even work
for XIP?
On Wed, May 15, 2024 at 09:19:46AM +0100, Conor Dooley wrote:
> On Tue, May 14, 2024 at 03:37:02PM +0200, Andrew Jones wrote:
> > On Tue, May 14, 2024 at 07:36:04AM GMT, Wang, Xiao W wrote:
> > > > From: Andrew Jones <[email protected]>
> >> > > > +config RISCV_ISA_ZBA
> > > > > + bool "Zba extension support for bit manipulation instructions"
> > > > > + depends on TOOLCHAIN_HAS_ZBA
> > > >
> > > > We handcraft the instruction, so why do we need toolchain support?
> > >
> > > Good point, we don't need toolchain support for this bpf jit case.
> > >
> > > >
> > > > > + depends on RISCV_ALTERNATIVE
> > > >
> > > > Also, while riscv_has_extension_likely() will be accelerated with
> > > > RISCV_ALTERNATIVE, it's not required.
> > >
> > > Agree, it's not required. For this bpf jit case, we should drop these two dependencies.
> > >
> > > BTW, Zbb is used in bpf jit, the usage there also doesn't depend on toolchain and
> > > RISCV_ALTERNATIVE, but the Kconfig for RISCV_ISA_ZBB has forced the dependencies
> > > due to Zbb assembly programming elsewhere.
> > > Maybe we could just dynamically check the existence of RISCV_ISA_ZB* before jit code
> > > emission? or introduce new config options for bpf jit? I prefer the first method and
> > > welcome any comments.
> >
> > My preferences is to remove as much of the TOOLCHAIN_HAS_ stuff as
> > possible. We should audit the extensions which have them to see if
> > they're really necessary.
>
> While I think it is reasonable to allow the "RISCV_ISA_ZBB" option to
> control whether or not bpf is allowed to use it for optimisations, only
> allowing bpf to do that if there's toolchain support feels odd to me..
> Maybe we need to sorta steal from Charlie's patchset and introduce
> some hidden options that have the toolchain dep that are used by the
> alternative macros etc?
>
> I'll have a poke at how bad that looks I think.
I don't love this, in particular my option naming, but it would allow
the Zbb optimisations in the kernel to not depend on toolchain support
while not muddying the Kconfig waters for users:
https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=riscv-zbb_split
A similar model could be followed if there were to be some
optimisations for Zba in the future that do require toolchain support:
> -----Original Message-----
> From: Conor Dooley <[email protected]>
> Sent: Wednesday, May 15, 2024 5:33 PM
> To: Andrew Jones <[email protected]>
> Cc: Wang, Xiao W <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected]; Li,
> Haicheng <[email protected]>; [email protected]; Ben Dooks
> <[email protected]>
> Subject: Re: [PATCH v2] riscv, bpf: Optimize zextw insn with Zba extension
>
> On Wed, May 15, 2024 at 09:19:46AM +0100, Conor Dooley wrote:
> > On Tue, May 14, 2024 at 03:37:02PM +0200, Andrew Jones wrote:
> > > On Tue, May 14, 2024 at 07:36:04AM GMT, Wang, Xiao W wrote:
> > > > > From: Andrew Jones <[email protected]>
> > >> > > > +config RISCV_ISA_ZBA
> > > > > > + bool "Zba extension support for bit manipulation instructions"
> > > > > > + depends on TOOLCHAIN_HAS_ZBA
> > > > >
> > > > > We handcraft the instruction, so why do we need toolchain support?
> > > >
> > > > Good point, we don't need toolchain support for this bpf jit case.
> > > >
> > > > >
> > > > > > + depends on RISCV_ALTERNATIVE
> > > > >
> > > > > Also, while riscv_has_extension_likely() will be accelerated with
> > > > > RISCV_ALTERNATIVE, it's not required.
> > > >
> > > > Agree, it's not required. For this bpf jit case, we should drop these two
> dependencies.
> > > >
> > > > BTW, Zbb is used in bpf jit, the usage there also doesn't depend on
> toolchain and
> > > > RISCV_ALTERNATIVE, but the Kconfig for RISCV_ISA_ZBB has forced the
> dependencies
> > > > due to Zbb assembly programming elsewhere.
> > > > Maybe we could just dynamically check the existence of RISCV_ISA_ZB*
> before jit code
> > > > emission? or introduce new config options for bpf jit? I prefer the first
> method and
> > > > welcome any comments.
> > >
> > > My preferences is to remove as much of the TOOLCHAIN_HAS_ stuff as
> > > possible. We should audit the extensions which have them to see if
> > > they're really necessary.
> >
> > While I think it is reasonable to allow the "RISCV_ISA_ZBB" option to
> > control whether or not bpf is allowed to use it for optimisations, only
> > allowing bpf to do that if there's toolchain support feels odd to me..
> > Maybe we need to sorta steal from Charlie's patchset and introduce
> > some hidden options that have the toolchain dep that are used by the
> > alternative macros etc?
> >
> > I'll have a poke at how bad that looks I think.
>
> I don't love this, in particular my option naming, but it would allow
> the Zbb optimisations in the kernel to not depend on toolchain support
> while not muddying the Kconfig waters for users:
> https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=ri
> scv-zbb_split
In that patch, I think the bpt jit part should check IS_ENABLED(CONFIG_RISCV_ISA_ZBB)
rather than IS_ENABLED(CONFIG_RISCV_ISA_ZBB_ALT).
> A similar model could be followed if there were to be some
> optimisations for Zba in the future that do require toolchain support:
Though this model introduces extra hidden Kconfig option, it does provide finer
config granularity. This should be a separate patch in the future, we can discuss about
the option naming there.
BRs,
Xiao
On Wed, May 15, 2024 at 11:31:43AM +0000, Wang, Xiao W wrote:
> > From: Conor Dooley <[email protected]>
> > > > My preferences is to remove as much of the TOOLCHAIN_HAS_ stuff as
> > > > possible. We should audit the extensions which have them to see if
> > > > they're really necessary.
> > >
> > > While I think it is reasonable to allow the "RISCV_ISA_ZBB" option to
> > > control whether or not bpf is allowed to use it for optimisations, only
> > > allowing bpf to do that if there's toolchain support feels odd to me..
> > > Maybe we need to sorta steal from Charlie's patchset and introduce
> > > some hidden options that have the toolchain dep that are used by the
> > > alternative macros etc?
> > >
> > > I'll have a poke at how bad that looks I think.
> >
> > I don't love this, in particular my option naming, but it would allow
> > the Zbb optimisations in the kernel to not depend on toolchain support
> > while not muddying the Kconfig waters for users:
> > https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=ri
> > scv-zbb_split
>
> In that patch, I think the bpt jit part should check IS_ENABLED(CONFIG_RISCV_ISA_ZBB)
> rather than IS_ENABLED(CONFIG_RISCV_ISA_ZBB_ALT).
D'oh, you're right. The bpf code being different was meant to be the whole
point of the change...
> > A similar model could be followed if there were to be some
> > optimisations for Zba in the future that do require toolchain support:
>
> Though this model introduces extra hidden Kconfig option, it does provide finer
> config granularity. This should be a separate patch in the future, we can discuss about
> the option naming there.
Yeah, not expecting you to do this as part of this patch.
Thanks,
Conor.