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

From: Pu Lehui <pulehui@huaweicloud.com>
To: Stefan O'Rear <sorear@fastmail.com>, Conor Dooley <conor@kernel.org>
Cc: bpf@vger.kernel.org, linux-riscv@lists.infradead.org,
	netdev@vger.kernel.org, "Björn Töpel" <bjorn@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: Fri, 29 Mar 2024 18:05:41 +0800	[thread overview]
Message-ID: <ed3debc9-f2a9-41fb-9cf9-dc6419de5c01@huaweicloud.com> (raw)
In-Reply-To: <20240328-ferocity-repose-c554f75a676c@spud>

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?

Link: 
https://lore.kernel.org/bpf/20240129-d06c79a17a5091b3403fc5b6@orel/ [0]

>>
>> If BPF needs to become an exception (why?), this should be mentioned in
>> Kconfig.
> 
> And in the commit message. On one hand I think this could be a reasonable
> thing to do in bpf as it is acting as a jit here, and doesn't actually
> need the alternatives that we are using elsewhere to enable the
> optimisations nor the compiler support. On the other the intention of
> that kconfig option is to control optimisations like rvzbb_enabled()
> gates, so this is gonna need a proper justification as to
> 
> As I said on IRC to you earlier, I think the Kconfig options here are in
> need of a bit of a spring cleaning - they should be modified to explain
> their individual purposes, be that enabling optimisations in the kernel
> or being required for userspace. I'll try to send a patch for that if
> I remember tomorrow.
> 
> Thanks,
> Conor.
> 
>>> Signed-off-by: Pu Lehui <pulehui@huawei.com>
>>> ---
>>>   arch/riscv/net/bpf_jit.h | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
>>> index 5fc374ed98ea..bcf109b88df5 100644
>>> --- a/arch/riscv/net/bpf_jit.h
>>> +++ b/arch/riscv/net/bpf_jit.h
>>> @@ -20,7 +20,7 @@ static inline bool rvc_enabled(void)
>>>
>>>   static inline bool rvzbb_enabled(void)
>>>   {
>>> -	return IS_ENABLED(CONFIG_RISCV_ISA_ZBB) &&
>>> riscv_has_extension_likely(RISCV_ISA_EXT_ZBB);
>>> +	return riscv_has_extension_likely(RISCV_ISA_EXT_ZBB);
>>>   }
>>>
>>>   enum {
>>> -- 
>>> 2.34.1
>>>
>>>
>>> _______________________________________________
>>> linux-riscv mailing list
>>> linux-riscv@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

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