netdev - Re: [PATCH bpf-next 2/5] riscv, bpf: Relax restrictions on 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]

Date: Tue, 2 Apr 2024 18:38:59 +0100
From: Conor Dooley <conor@...nel.org>
To: Björn Töpel <bjorn@...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

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.

> 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.


Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)