diff mbox series

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

Message ID 20240629094733.3863850-4-eddyz87@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series no_caller_saved_registers attribute for helper calls | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 pending Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 fail Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 850 this patch: 850
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 6 maintainers not CCed: song@kernel.org haoluo@google.com jolsa@kernel.org kpsingh@kernel.org john.fastabend@gmail.com sdf@google.com
netdev/build_clang success Errors and warnings before: 854 this patch: 854
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 917 this patch: 917
netdev/checkpatch warning WARNING: line length of 82 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 9 this patch: 9
netdev/source_inline success Was 0 now: 0

Commit Message

Eduard Zingerman June 29, 2024, 9:47 a.m. UTC
On x86 verifier replaces call to bpf_get_smp_processor_id() with a
sequence of instructions that modify only R0.
This satisfies attribute no_caller_saved_registers contract.
Allow rewrite of no_caller_saved_registers patterns for
bpf_get_smp_processor_id() in order to use this function
as a canary for no_caller_saved_registers tests.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 kernel/bpf/helpers.c  |  1 +
 kernel/bpf/verifier.c | 11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

Comments

Andrii Nakryiko July 2, 2024, 12:41 a.m. UTC | #1
On Sat, Jun 29, 2024 at 2:48 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On x86 verifier replaces call to bpf_get_smp_processor_id() with a
> sequence of instructions that modify only R0.
> This satisfies attribute no_caller_saved_registers contract.
> Allow rewrite of no_caller_saved_registers patterns for
> bpf_get_smp_processor_id() in order to use this function
> as a canary for no_caller_saved_registers tests.
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  kernel/bpf/helpers.c  |  1 +
>  kernel/bpf/verifier.c | 11 +++++++++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 229396172026..1df01f454590 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -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?

>  };
>
>  BPF_CALL_0(bpf_get_numa_node_id)
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1340d3e60d30..d68ed70186c8 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -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


> +       default:
> +               return false;
> +       }
>  }
>
>  /* If 'insn' is a call that follows no_caller_saved_registers contract
> @@ -20666,7 +20673,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
>  #ifdef CONFIG_X86_64
>                 /* Implement bpf_get_smp_processor_id() inline. */
>                 if (insn->imm == BPF_FUNC_get_smp_processor_id &&
> -                   prog->jit_requested && bpf_jit_supports_percpu_insn()) {
> +                   verifier_inlines_helper_call(env, insn->imm)) {
>                         /* BPF_FUNC_get_smp_processor_id inlining is an
>                          * optimization, so if pcpu_hot.cpu_number is ever
>                          * changed in some incompatible and hard to support
> --
> 2.45.2
>
Eduard Zingerman July 2, 2024, 8:43 p.m. UTC | #2
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().

[...]

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

At the moment I return false for this archs.
Or do you suggest these to be added in the current patch-set?

[...]
Andrii Nakryiko July 2, 2024, 9:11 p.m. UTC | #3
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
>
> At the moment I return false for this archs.
> Or do you suggest these to be added in the current patch-set?

I'd add them from the get go. CC Puranjay to double-check?

>
> [...]
Eduard Zingerman July 2, 2024, 9:25 p.m. UTC | #4
On Tue, 2024-07-02 at 14:11 -0700, Andrii Nakryiko wrote:

[...]

> > > > +#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
> > 
> > At the moment I return false for this archs.
> > Or do you suggest these to be added in the current patch-set?
> 
> I'd add them from the get go. CC Puranjay to double-check?

I'll give it a try.
Puranjay Mohan July 3, 2024, 11:27 a.m. UTC | #5
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
Eduard Zingerman July 3, 2024, 11:14 p.m. UTC | #6
On Wed, 2024-07-03 at 11:27 +0000, Puranjay Mohan wrote:

[...]

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

Puranjay, just out of curiosity and tangential to this patch-set,
I see that get_smp_processor_id is implemented as follows in riscv jit:

  emit_ld(bpf_to_rv_reg(BPF_REG_0, ctx), offsetof(struct thread_info, cpu),
	  RV_REG_TP, ctx);

Where bpf_to_rv_reg() refers to regmap, which in turn has the following line:

  static const int regmap[] = {
	[BPF_REG_0] =	RV_REG_A5,
	...
  }

At the same time, [1] says:

> 18.2 RVG Calling Convention
> ...
> Values are returned from functions in integer registers a0 and a1 and
> floating-point registers fa0 and fa1.

[1] https://riscv.org/wp-content/uploads/2015/01/riscv-calling.pdf

So, I would expect r0 to be mapped to a0, do you happen to know why is it a5?

[...]
Puranjay Mohan July 4, 2024, 11:19 a.m. UTC | #7
Eduard Zingerman <eddyz87@gmail.com> writes:

> On Wed, 2024-07-03 at 11:27 +0000, Puranjay Mohan wrote:
>
> [...]
>
>> > > > > @@ -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.
>
> Puranjay, just out of curiosity and tangential to this patch-set,
> I see that get_smp_processor_id is implemented as follows in riscv jit:
>
>   emit_ld(bpf_to_rv_reg(BPF_REG_0, ctx), offsetof(struct thread_info, cpu),
> 	  RV_REG_TP, ctx);
>
> Where bpf_to_rv_reg() refers to regmap, which in turn has the following line:
>
>   static const int regmap[] = {
> 	[BPF_REG_0] =	RV_REG_A5,
> 	...
>   }
>
> At the same time, [1] says:
>
>> 18.2 RVG Calling Convention
>> ...
>> Values are returned from functions in integer registers a0 and a1 and
>> floating-point registers fa0 and fa1.
>
> [1] https://riscv.org/wp-content/uploads/2015/01/riscv-calling.pdf
>
> So, I would expect r0 to be mapped to a0, do you happen to know why is it a5?

I had the same question when I started working with the JITs. This is
seen on both risc-v and arm64, where as you said on risc-v R0 should be
mapped to A0 but is mapped to A5. Similarly, on ARM64, BPF_R0 should be
mapped to ARM64_R0 but is mapped to ARM64_R7.

Here is my understanding of this:

The reason for this quirk is the usage of BPF register R0 as defined by
BPF Registers and calling convention [1]

[1] says:

```
* R0: return value from function calls, and exit value for BPF programs
* R1 - R5: arguments for function calls
```

On arm64 and risc-v the first argument and the return value are
passed/returned in the same register, A0 on risc-v and R0 on arm64.

In BPF, the first argument to a function is passed in R1 and not in R0.
So when we map these registers to riscv or arm64 calling convention, we
have to map BPF_R1 to A0 on risc-v and to R0 on ARM64. This is to make
argument passing easy. Therefore BPF_R0 is mapped to A5 on risc-v and
ARM64_R7 on arm64.

And when we JIT the 'BPF_JMP | BPF_CALL' we add a mov instruction at the
end to move A0 to A5 on risc-v and R0 to R7 on arm64.

But when inlining the call we can directly put the result in A5 or R7. 

Thanks,
Puranjay

[1] https://kernel.googlesource.com/pub/scm/linux/kernel/git/bpf/bpf-next/+/refs/heads/master/Documentation/bpf/standardization/abi.rst
Eduard Zingerman July 4, 2024, 4:39 p.m. UTC | #8
On Thu, 2024-07-04 at 11:19 +0000, Puranjay Mohan wrote:

[...]

> And when we JIT the 'BPF_JMP | BPF_CALL' we add a mov instruction at the
> end to move A0 to A5 on risc-v and R0 to R7 on arm64.
> 
> But when inlining the call we can directly put the result in A5 or R7. 

Oh, I see that additional move, thank you for explaining!

Best regards,
Eduard
Eduard Zingerman July 4, 2024, 5 p.m. UTC | #9
On Wed, 2024-07-03 at 11:27 +0000, Puranjay Mohan wrote:

[...]

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

Hi Puranjay,

I've added bpf_jit_inlines_helper_call() logic in RFC v2 [1].
If you have a riscv setup at hand, would it be possible to ask you to
run test case 'verifier_nocsr/canary_arm64_riscv64' on it?
I verified that it works for arm64 in [2,3] but it would be nice to
have it checked on riscv, which is currently not a part of the CI.

Thanks,
Eduard

[1] https://lore.kernel.org/bpf/20240704102402.1644916-1-eddyz87@gmail.com/
[2] https://github.com/kernel-patches/bpf/actions/runs/9792217835/job/27037905408?pr=7274
[3] https://productionresultssa19.blob.core.windows.net/actions-results/6fb742ca-e78f-420e-9f1c-66e1e23365ef/workflow-job-run-0caa83a1-3221-5ca7-0e7d-7eb42ae68938/logs/job/job-logs.txt?rsct=text%2Fplain&se=2024-07-04T10%3A32%3A00Z&sig=gb0wAZ%2FPMOfZwCCtJyTqAogIyKZ%2F3lCZCfPUpMdN8rE%3D&ske=2024-07-04T18%3A36%3A08Z&skoid=ca7593d4-ee42-46cd-af88-8b886a2f84eb&sks=b&skt=2024-07-04T06%3A36%3A08Z&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skv=2023-11-03&sp=r&spr=https&sr=b&st=2024-07-04T10%3A21%3A55Z&sv=2023-11-03
Puranjay Mohan July 4, 2024, 5:24 p.m. UTC | #10
Eduard Zingerman <eddyz87@gmail.com> writes:

> On Wed, 2024-07-03 at 11:27 +0000, Puranjay Mohan wrote:
>
> [...]
>
>> 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)
>
> Hi Puranjay,
>
> I've added bpf_jit_inlines_helper_call() logic in RFC v2 [1].
> If you have a riscv setup at hand, would it be possible to ask you to
> run test case 'verifier_nocsr/canary_arm64_riscv64' on it?
> I verified that it works for arm64 in [2,3] but it would be nice to
> have it checked on riscv, which is currently not a part of the CI.

Hi Eduard,

I have qemu setup for risc-v. I will test this and let you know the
results.

Thanks,
Puranjay
Eduard Zingerman July 4, 2024, 5:39 p.m. UTC | #11
On Thu, 2024-07-04 at 17:24 +0000, Puranjay Mohan wrote:

[...]

> I have qemu setup for risc-v. I will test this and let you know the
> results.

Great, thank you!
diff mbox series

Patch

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 229396172026..1df01f454590 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -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,
 };
 
 BPF_CALL_0(bpf_get_numa_node_id)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1340d3e60d30..d68ed70186c8 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -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
+	default:
+		return false;
+	}
 }
 
 /* If 'insn' is a call that follows no_caller_saved_registers contract
@@ -20666,7 +20673,7 @@  static int do_misc_fixups(struct bpf_verifier_env *env)
 #ifdef CONFIG_X86_64
 		/* Implement bpf_get_smp_processor_id() inline. */
 		if (insn->imm == BPF_FUNC_get_smp_processor_id &&
-		    prog->jit_requested && bpf_jit_supports_percpu_insn()) {
+		    verifier_inlines_helper_call(env, insn->imm)) {
 			/* BPF_FUNC_get_smp_processor_id inlining is an
 			 * optimization, so if pcpu_hot.cpu_number is ever
 			 * changed in some incompatible and hard to support