Daniel Borkmann <daniel@xxxxxxxxxxxxx> writes: > On 1/29/24 10:13 AM, Pu Lehui wrote: >> On 2024/1/28 1:16, Björn Töpel wrote: >>> Pu Lehui <pulehui@xxxxxxxxxxxxxxx> writes: >>> >>>> From: Pu Lehui <pulehui@xxxxxxxxxx> >>>> >>>> Add necessary Zbb instructions introduced by [0] to reduce code size and >>>> improve performance of RV64 JIT. Meanwhile, a runtime deteted helper is >>>> added to check whether the CPU supports Zbb instructions. >>>> >>>> Link: https://github.com/riscv/riscv-bitmanip/releases/download/1.0.0/bitmanip-1.0.0-38-g865e7a7.pdf [0] >>>> Signed-off-by: Pu Lehui <pulehui@xxxxxxxxxx> >>>> --- >>>> arch/riscv/net/bpf_jit.h | 32 ++++++++++++++++++++++++++++++++ >>>> 1 file changed, 32 insertions(+) >>>> >>>> diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h >>>> index e30501b46f8f..51f6d214086f 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 rvzbb_enabled(void) >>>> +{ >>>> + return IS_ENABLED(CONFIG_RISCV_ISA_ZBB) && riscv_has_extension_likely(RISCV_ISA_EXT_ZBB); >>> >>> Hmm, I'm thinking about the IS_ENABLED(CONFIG_RISCV_ISA_ZBB) semantics >>> for a kernel JIT compiler. >>> >>> IS_ENABLED(CONFIG_RISCV_ISA_ZBB) affects the kernel compiler flags. >>> Should it be enough to just have the run-time check? Should a kernel >>> built w/o Zbb be able to emit Zbb from the JIT? >> >> Not enough, because riscv_has_extension_likely(RISCV_ISA_EXT_ZBB) is >> a platform capability check, and the other one is a kernel image >> capability check. We can pass the check >> riscv_has_extension_likely(RISCV_ISA_EXT_ZBB) when >> CONFIG_RISCV_ISA_ZBB=n. And my local test prove it. What I'm trying to say (and drew as well in the other reply) is that "riscv_has_extension_likely(RISCV_ISA_EXT_ZBB) when CONFIG_RISCV_ISA_ZBB=n" should also make the JIT emit Zbb insns. The platform check should be sufficient. > So if I understand you correctly, only relying on the > riscv_has_extension_likely(RISCV_ISA_EXT_ZBB) part would not work - > iow, the IS_ENABLED(CONFIG_RISCV_ISA_ZBB) is mandatory here? > > Thanks, > Daniel > > P.s.: Given Bjorn's review and tests I took the series into bpf-next > now. Thanks everyone! Thanks! Yes, this is mainly a semantic discussion, and it can be further relaxed later with a follow up -- if applicable. Björn
