Re: [PATCH bpf-next 2/5] riscv, bpf: Relax restrictions on Zbb instructions

From: "Björn Töpel" <bjorn@kernel.org>
To: Conor Dooley <conor@kernel.org>
Cc: Pu Lehui <pulehui@huaweicloud.com>,
	Stefan O'Rear <sorear@fastmail.com>,
	bpf@vger.kernel.org, linux-riscv@lists.infradead.org,
	netdev@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Martin KaFai Lau <martin.lau@linux.dev>,
	Eduard Zingerman <eddyz87@gmail.com>, Song Liu <song@kernel.org>,
	Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@kernel.org>,
	Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>,
	Jiri Olsa <jolsa@kernel.org>, Mykola Lysenko <mykolal@fb.com>,
	Manu Bretelle <chantr4@gmail.com>, Pu Lehui <pulehui@huawei.com>
Subject: Re: [PATCH bpf-next 2/5] riscv, bpf: Relax restrictions on Zbb instructions
Date: Tue, 02 Apr 2024 21:00:45 +0200	[thread overview]
Message-ID: <871q7nr3mq.fsf@all.your.base.are.belong.to.us> (raw)
In-Reply-To: <20240402-ample-preview-c84edb69db1b@spud>

Hey Conor!

Conor Dooley <conor@kernel.org> writes:

> On Tue, Apr 02, 2024 at 04:25:24PM +0200, Björn Töpel wrote:
>> Pu Lehui <pulehui@huaweicloud.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@huawei.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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv