Message-ID: <871q7nr3mq.fsf@all.your.base.are.belong.to.us>
Date: Tue, 02 Apr 2024 21:00:45 +0200
From: Björn Töpel <bjorn@...nel.org>
To: Conor Dooley <conor@...nel.org>
Cc: Pu Lehui <pulehui@...weicloud.com>, Stefan O'Rear <sorear@...tmail.com>,
 bpf@...r.kernel.org, linux-riscv@...ts.infradead.org,
 netdev@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>, Daniel
 Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
 Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman
 <eddyz87@...il.com>, 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>, Mykola Lysenko
 <mykolal@...com>, Manu Bretelle <chantr4@...il.com>, Pu Lehui
 <pulehui@...wei.com>
Subject: Re: [PATCH bpf-next 2/5] riscv, bpf: Relax restrictions on Zbb
 instructions

Hey Conor!

Conor Dooley <conor@...nel.org> writes:

> On Tue, Apr 02, 2024 at 04:25:24PM +0200, Björn Töpel wrote:
>> Pu Lehui <pulehui@...weicloud.com> writes:
>> 
>> > On 2024/3/29 6:07, Conor Dooley wrote:
>> >> On Thu, Mar 28, 2024 at 03:34:31PM -0400, Stefan O'Rear wrote:
>> >>> On Thu, Mar 28, 2024, at 8:49 AM, Pu Lehui wrote:
>> >>>> From: Pu Lehui <pulehui@...wei.com>
>> >>>>
>> >>>> This patch relaxes the restrictions on the Zbb instructions. The hardware
>> >>>> is capable of recognizing the Zbb instructions independently, eliminating
>> >>>> the need for reliance on kernel compile configurations.
>> >>>
>> >>> This doesn't make sense to me.
>> >> 
>> >> It doesn't make sense to me either. Of course the hardware's capability
>> >> to understand an instruction is independent of whether or not a
>> >> toolchain is capable of actually emitting the instruction.
>> >> 
>> >>> RISCV_ISA_ZBB is defined as:
>> >>>
>> >>>             Adds support to dynamically detect the presence of the ZBB
>> >>>             extension (basic bit manipulation) and enable its usage.
>> >>>
>> >>> In other words, RISCV_ISA_ZBB=n should disable everything that attempts
>> >>> to detect Zbb at runtime. It is mostly relevant for code size reduction,
>> >>> which is relevant for BPF since if RISCV_ISA_ZBB=n all rvzbb_enabled()
>> >>> checks can be constant-folded.
>> >
>> > Thanks for review. My initial thought was the same as yours, but after 
>> > discussions [0] and test verifications, the hardware can indeed 
>> > recognize the zbb instruction even if the kernel has not enabled 
>> > CONFIG_RISCV_ISA_ZBB. As Conor mentioned, we are just acting as a JIT to 
>> > emit zbb instruction here. Maybe is_hw_zbb_capable() will be better?
>> 
>> I still think Lehui's patch is correct; Building a kernel that can boot
>> on multiple platforms (w/ or w/o Zbb support) and not having Zbb insn in
>> the kernel proper, and iff Zbb is available at run-time the BPF JIT will
>> emit Zbb.
>
> This sentence is -ENOPARSE to me, did you accidentally omit some words?
> Additionally he config option has nothing to do with building kernels that
> boot on multiple platforms, it only controls whether optimisations for Zbb
> are built so that if Zbb is detected they can be used.

Ugh, sorry about that! I'm probably confused myself.

>> For these kind of optimizations, (IMO) it's better to let the BPF JIT
>> decide at run-time.
>
> Why is bpf a different case to any other user in this regard?
> I think that the commit message is misleading and needs to be changed,
> because the point "the hardware is capable of recognising the Zbb
> instructions independently..." is completely unrelated to the purpose
> of the config option. Of course the hardware understanding the option
> has nothing to do with kernel configuration. The commit message needs to
> explain why bpf is a special case and is exempt from an 
>
> I totally understand any point about bpf being different in terms of
> needing toolchain support, but IIRC it was I who pointed out up-thread.
> The part of the conversation that you're replying to here is about the
> semantics of the Kconfig option and the original patch never mentioned
> trying to avoid a dependency on toolchains at all, just kernel
> configurations. The toolchain requirements I don't think are even super
> hard to fulfill either - the last 3 versions of ld and lld all meet the
> criteria.

Thanks for making it more clear, and I agree that the toolchain
requirements are not hard to fulfull.

My view has been that "BPF is like userland", but I realize now that's
odd. Let's make BPF similar to the rest of the RV kernel. If ZBB=n, then
the BPF JIT doesn't know about emitting Zbb.


Björn

Powered by blists - more mailing lists