Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> writes: > On Tue, Jul 2, 2024 at 1:44 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: >> >> On Mon, 2024-07-01 at 17:41 -0700, Andrii Nakryiko wrote: >> >> [...] >> >> > > @@ -158,6 +158,7 @@ const struct bpf_func_proto bpf_get_smp_processor_id_proto = { >> > > .func = bpf_get_smp_processor_id, >> > > .gpl_only = false, >> > > .ret_type = RET_INTEGER, >> > > + .nocsr = true, >> > >> > I'm wondering if we should call this flag in such a way that it's >> > clear that this is more of an request, while the actual nocsr cleanup >> > and stuff is done only if BPF verifier/BPF JIT support that for >> > specific architecture/config/etc? >> >> Can change to .allow_nocsr. On the other hand, can remove this flag >> completely and rely on call_csr_mask(). > > I like the declaration that helper is eligible to be close to helper > definition, so I'd definitely keep it, but yeah "allow_nocsr" seems > betterto me > >> >> [...] >> >> > > @@ -16030,7 +16030,14 @@ static u8 get_helper_reg_mask(const struct bpf_func_proto *fn) >> > > */ >> > > static bool verifier_inlines_helper_call(struct bpf_verifier_env *env, s32 imm) >> > > { >> > > - return false; >> > > + switch (imm) { >> > > +#ifdef CONFIG_X86_64 >> > > + case BPF_FUNC_get_smp_processor_id: >> > > + return env->prog->jit_requested && bpf_jit_supports_percpu_insn(); >> > > +#endif >> > >> > please see bpf_jit_inlines_helper_call(), arm64 and risc-v inline it >> > in JIT, so we need to validate they don't assume any of R1-R5 register >> > to be a scratch register They don't assume any register to be scratch (except R0) so we can enable this on arm64 and riscv. >> >> At the moment I return false for this archs. Yes, verifier_inlines_helper_call() should keep returning false for arm64 and risc-v. >> Or do you suggest these to be added in the current patch-set? The correct way to do this would be to change call_csr_mask() to have: verifier_inlines_helper_call(env, insn->imm) || bpf_jit_inlines_helper_call(insn->imm) > I'd add them from the get go. CC Puranjay to double-check? Thanks, Puranjay
Attachment:
signature.asc
Description: PGP signature