[PATCH RESEND bpf-next v3 4/6] riscv, bpf: Add necessary Zbb instructions

[Björn Töpel]

Pu Lehui <pulehui at huawei.com> writes:

> On 2024/1/30 14:18, Björn Töpel wrote:
>> Daniel Borkmann <daniel at iogearbox.net> 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 at huaweicloud.com> writes:
>>>>>
>>>>>> From: Pu Lehui <pulehui at huawei.com>
>>>>>>
>>>>>> 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 at huawei.com>
>>>>>> ---
>>>>>>    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.
>
> Ooh, this is really beyond my expectation. The test_progs can pass when 
> with only platform check and it can recognize the zbb instructions. Now 
> I know it. Sorry for misleading.🙁
>
> Curious if CONFIG_RISCV_ISA_ZBB is still necessary?

You don't need IS_ENABLED(CONFIG_RISCV_ISA_ZBB) for the JIT, but the
kernel needs it.

Feel free to follow up with a patch to remove it.


Cheers,
Björn