| Message ID | 20240511023436.3282285-1-xiao.w.wang@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] riscv, bpf: Optimize zextw insn with Zba extension | expand |
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 <xiao.w.wang@intel.com> > --- > 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 <ajones@ventanamicro.com> > Sent: Tuesday, May 14, 2024 12:53 AM > To: Wang, Xiao W <xiao.w.wang@intel.com> > Cc: paul.walmsley@sifive.com; palmer@dabbelt.com; > aou@eecs.berkeley.edu; luke.r.nels@gmail.com; xi.wang@gmail.com; > bjorn@kernel.org; ast@kernel.org; daniel@iogearbox.net; andrii@kernel.org; > martin.lau@linux.dev; eddyz87@gmail.com; song@kernel.org; > yonghong.song@linux.dev; john.fastabend@gmail.com; kpsingh@kernel.org; > sdf@google.com; haoluo@google.com; jolsa@kernel.org; linux- > riscv@lists.infradead.org; linux-kernel@vger.kernel.org; bpf@vger.kernel.org; > pulehui@huawei.com; Li, Haicheng <haicheng.li@intel.com>; > conor@kernel.org > 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 <xiao.w.wang@intel.com> > > --- > > 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 <ajones@ventanamicro.com> > > Sent: Tuesday, May 14, 2024 12:53 AM > > To: Wang, Xiao W <xiao.w.wang@intel.com> > > Cc: paul.walmsley@sifive.com; palmer@dabbelt.com; > > aou@eecs.berkeley.edu; luke.r.nels@gmail.com; xi.wang@gmail.com; > > bjorn@kernel.org; ast@kernel.org; daniel@iogearbox.net; andrii@kernel.org; > > martin.lau@linux.dev; eddyz87@gmail.com; song@kernel.org; > > yonghong.song@linux.dev; john.fastabend@gmail.com; kpsingh@kernel.org; > > sdf@google.com; haoluo@google.com; jolsa@kernel.org; linux- > > riscv@lists.infradead.org; linux-kernel@vger.kernel.org; bpf@vger.kernel.org; > > pulehui@huawei.com; Li, Haicheng <haicheng.li@intel.com>; > > conor@kernel.org > > 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 <xiao.w.wang@intel.com> > > > --- > > > 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 <ajones@ventanamicro.com> > Sent: Tuesday, May 14, 2024 9:37 PM > To: Wang, Xiao W <xiao.w.wang@intel.com> > Cc: paul.walmsley@sifive.com; palmer@dabbelt.com; > aou@eecs.berkeley.edu; luke.r.nels@gmail.com; xi.wang@gmail.com; > bjorn@kernel.org; ast@kernel.org; daniel@iogearbox.net; andrii@kernel.org; > martin.lau@linux.dev; eddyz87@gmail.com; song@kernel.org; > yonghong.song@linux.dev; john.fastabend@gmail.com; kpsingh@kernel.org; > sdf@google.com; haoluo@google.com; jolsa@kernel.org; linux- > riscv@lists.infradead.org; linux-kernel@vger.kernel.org; bpf@vger.kernel.org; > pulehui@huawei.com; Li, Haicheng <haicheng.li@intel.com>; > conor@kernel.org; Ben Dooks <ben.dooks@codethink.co.uk> > 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 <ajones@ventanamicro.com> > > > Sent: Tuesday, May 14, 2024 12:53 AM > > > To: Wang, Xiao W <xiao.w.wang@intel.com> > > > Cc: paul.walmsley@sifive.com; palmer@dabbelt.com; > > > aou@eecs.berkeley.edu; luke.r.nels@gmail.com; xi.wang@gmail.com; > > > bjorn@kernel.org; ast@kernel.org; daniel@iogearbox.net; > andrii@kernel.org; > > > martin.lau@linux.dev; eddyz87@gmail.com; song@kernel.org; > > > yonghong.song@linux.dev; john.fastabend@gmail.com; > kpsingh@kernel.org; > > > sdf@google.com; haoluo@google.com; jolsa@kernel.org; linux- > > > riscv@lists.infradead.org; linux-kernel@vger.kernel.org; > bpf@vger.kernel.org; > > > pulehui@huawei.com; Li, Haicheng <haicheng.li@intel.com>; > > > conor@kernel.org > > > 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 <xiao.w.wang@intel.com> > > > > --- > > > > 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 <ajones@ventanamicro.com> >> > > > +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 <ajones@ventanamicro.com> > >> > > > +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 <conor.dooley@microchip.com> > Sent: Wednesday, May 15, 2024 5:33 PM > To: Andrew Jones <ajones@ventanamicro.com> > Cc: Wang, Xiao W <xiao.w.wang@intel.com>; paul.walmsley@sifive.com; > palmer@dabbelt.com; aou@eecs.berkeley.edu; luke.r.nels@gmail.com; > xi.wang@gmail.com; bjorn@kernel.org; ast@kernel.org; > daniel@iogearbox.net; andrii@kernel.org; martin.lau@linux.dev; > eddyz87@gmail.com; song@kernel.org; yonghong.song@linux.dev; > john.fastabend@gmail.com; kpsingh@kernel.org; sdf@google.com; > haoluo@google.com; jolsa@kernel.org; linux-riscv@lists.infradead.org; linux- > kernel@vger.kernel.org; bpf@vger.kernel.org; pulehui@huawei.com; Li, > Haicheng <haicheng.li@intel.com>; conor@kernel.org; Ben Dooks > <ben.dooks@codethink.co.uk> > 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 <ajones@ventanamicro.com> > > >> > > > +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 <conor.dooley@microchip.com> > > > > 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.
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); }
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 <xiao.w.wang@intel.com> --- 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(+)