> -----Original Message----- > From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx> > Sent: Wednesday, May 15, 2024 5:33 PM > To: Andrew Jones <ajones@xxxxxxxxxxxxxxxx> > Cc: Wang, Xiao W <xiao.w.wang@xxxxxxxxx>; paul.walmsley@xxxxxxxxxx; > palmer@xxxxxxxxxxx; aou@xxxxxxxxxxxxxxxxx; luke.r.nels@xxxxxxxxx; > xi.wang@xxxxxxxxx; bjorn@xxxxxxxxxx; ast@xxxxxxxxxx; > daniel@xxxxxxxxxxxxx; andrii@xxxxxxxxxx; martin.lau@xxxxxxxxx; > eddyz87@xxxxxxxxx; song@xxxxxxxxxx; yonghong.song@xxxxxxxxx; > john.fastabend@xxxxxxxxx; kpsingh@xxxxxxxxxx; sdf@xxxxxxxxxx; > haoluo@xxxxxxxxxx; jolsa@xxxxxxxxxx; linux-riscv@xxxxxxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; bpf@xxxxxxxxxxxxxxx; pulehui@xxxxxxxxxx; Li, > Haicheng <haicheng.li@xxxxxxxxx>; conor@xxxxxxxxxx; Ben Dooks > <ben.dooks@xxxxxxxxxxxxxxx> > Subject: Re: [PATCH v2] riscv, bpf: Optimize zextw insn with Zba extension > > On Wed, May 15, 2024 at 09:19:46AM +0100, Conor Dooley wrote: > > On Tue, May 14, 2024 at 03:37:02PM +0200, Andrew Jones wrote: > > > On Tue, May 14, 2024 at 07:36:04AM GMT, Wang, Xiao W wrote: > > > > > From: Andrew Jones <ajones@xxxxxxxxxxxxxxxx> > > >> > > > +config RISCV_ISA_ZBA > > > > > > + bool "Zba extension support for bit manipulation instructions" > > > > > > + depends on TOOLCHAIN_HAS_ZBA > > > > > > > > > > We handcraft the instruction, so why do we need toolchain support? > > > > > > > > Good point, we don't need toolchain support for this bpf jit case. > > > > > > > > > > > > > > > + depends on RISCV_ALTERNATIVE > > > > > > > > > > Also, while riscv_has_extension_likely() will be accelerated with > > > > > RISCV_ALTERNATIVE, it's not required. > > > > > > > > Agree, it's not required. For this bpf jit case, we should drop these two > dependencies. > > > > > > > > BTW, Zbb is used in bpf jit, the usage there also doesn't depend on > toolchain and > > > > RISCV_ALTERNATIVE, but the Kconfig for RISCV_ISA_ZBB has forced the > dependencies > > > > due to Zbb assembly programming elsewhere. > > > > Maybe we could just dynamically check the existence of RISCV_ISA_ZB* > before jit code > > > > emission? or introduce new config options for bpf jit? I prefer the first > method and > > > > welcome any comments. > > > > > > My preferences is to remove as much of the TOOLCHAIN_HAS_ stuff as > > > possible. We should audit the extensions which have them to see if > > > they're really necessary. > > > > While I think it is reasonable to allow the "RISCV_ISA_ZBB" option to > > control whether or not bpf is allowed to use it for optimisations, only > > allowing bpf to do that if there's toolchain support feels odd to me.. > > Maybe we need to sorta steal from Charlie's patchset and introduce > > some hidden options that have the toolchain dep that are used by the > > alternative macros etc? > > > > I'll have a poke at how bad that looks I think. > > I don't love this, in particular my option naming, but it would allow > the Zbb optimisations in the kernel to not depend on toolchain support > while not muddying the Kconfig waters for users: > https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=ri > scv-zbb_split In that patch, I think the bpt jit part should check IS_ENABLED(CONFIG_RISCV_ISA_ZBB) rather than IS_ENABLED(CONFIG_RISCV_ISA_ZBB_ALT). > A similar model could be followed if there were to be some > optimisations for Zba in the future that do require toolchain support: Though this model introduces extra hidden Kconfig option, it does provide finer config granularity. This should be a separate patch in the future, we can discuss about the option naming there. BRs, Xiao
