Re: [RFC bpf-next v1 3/8] bpf, x86: no_caller_saved_registers for bpf_get_smp_processor_id()

[Puranjay Mohan <puranjay@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2280 bytes --]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Tue, Jul 2, 2024 at 1:44 PM Eduard Zingerman <eddyz87@gmail.com> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 255 bytes --]