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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87o7d2ohdi.fsf@all.your.base.are.belong.to.us>
Date: Tue, 30 Jan 2024 18:34:01 +0100
From: Björn Töpel <bjorn@...nel.org>
To: Pu Lehui <pulehui@...wei.com>, Daniel Borkmann <daniel@...earbox.net>,
 Pu Lehui <pulehui@...weicloud.com>, bpf@...r.kernel.org,
 linux-riscv@...ts.infradead.org, netdev@...r.kernel.org
Cc: Alexei Starovoitov <ast@...nel.org>, Andrii Nakryiko
 <andrii@...nel.org>, Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu
 <song@...nel.org>, Yonghong
 Song <yhs@...com>, John Fastabend <john.fastabend@...il.com>, KP Singh
 <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...gle.com>, Hao Luo
 <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, Palmer Dabbelt
 <palmer@...belt.com>, Conor Dooley <conor@...nel.org>, Luke Nelson
 <luke.r.nels@...il.com>, Andrew Jones <ajones@...tanamicro.com>
Subject: Re: [PATCH RESEND bpf-next v3 4/6] riscv, bpf: Add necessary Zbb
 instructions

Pu Lehui <pulehui@...wei.com> writes:

> On 2024/1/30 14:18, Björn Töpel wrote:
>> Daniel Borkmann <daniel@...earbox.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@...weicloud.com> writes:
>>>>>
>>>>>> From: Pu Lehui <pulehui@...wei.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@...wei.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