Re: [PATCH bpf-next v9 06/11] bpf,x86: introduce emit_store_stack_imm64() for trampoline

[Menglong Dong <menglong.dong@linux.dev>]

On 2026/1/14 09:22 Andrii Nakryiko <andrii.nakryiko@gmail.com> write:
> On Sat, Jan 10, 2026 at 6:12 AM Menglong Dong <menglong8.dong@gmail.com> wrote:
> >
> > Introduce the helper emit_store_stack_imm64(), which is used to store a
> > imm64 to the stack with the help of r0.
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > ---
> > v9:
> > - rename emit_st_r0_imm64() to emit_store_stack_imm64()
> > ---
> >  arch/x86/net/bpf_jit_comp.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > index e3b1c4b1d550..d94f7038c441 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -1300,6 +1300,15 @@ static void emit_st_r12(u8 **pprog, u32 size, u32 dst_reg, int off, int imm)
> >         emit_st_index(pprog, size, dst_reg, X86_REG_R12, off, imm);
> >  }
> >
> > +static void emit_store_stack_imm64(u8 **pprog, int stack_off, u64 imm64)
> > +{
> > +       /* mov rax, imm64
> > +        * mov QWORD PTR [rbp - stack_off], rax
> > +        */
> > +       emit_mov_imm64(pprog, BPF_REG_0, imm64 >> 32, (u32) imm64);
> 
> maybe make the caller pass BPF_REG_0 explicitly, it will be more
> generic but also more explicit that BPF_REG_0 is used as temporary
> register?

OK! I were worried about that it wasn't explicit that BPF_REG_0 is
used too.

> 
> > +       emit_stx(pprog, BPF_DW, BPF_REG_FP, BPF_REG_0, -stack_off);
> 
> why are you negating stack offset here and not in the caller?..
> 
> > +}
> > +
> >  static int emit_atomic_rmw(u8 **pprog, u32 atomic_op,
> >                            u32 dst_reg, u32 src_reg, s16 off, u8 bpf_size)
> >  {
> > @@ -3352,16 +3361,14 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *rw_im
> >          *   mov rax, nr_regs
> >          *   mov QWORD PTR [rbp - nregs_off], rax
> >          */
> > -       emit_mov_imm64(&prog, BPF_REG_0, 0, (u32) nr_regs);
> > -       emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -nregs_off);
> > +       emit_store_stack_imm64(&prog, nregs_off, nr_regs);
> >
> >         if (flags & BPF_TRAMP_F_IP_ARG) {
> >                 /* Store IP address of the traced function:
> >                  * movabsq rax, func_addr
> >                  * mov QWORD PTR [rbp - ip_off], rax
> >                  */
> > -               emit_mov_imm64(&prog, BPF_REG_0, (long) func_addr >> 32, (u32) (long) func_addr);
> > -               emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -ip_off);
> > +               emit_store_stack_imm64(&prog, ip_off, (long)func_addr);
> 
> see above, I'd pass BPF_REG_0 and -ip_off (and -nregs_off) explicitly,
> too many small transformations are hidden inside
> emit_store_stack_imm64(), IMO

ACK. The negating offset in emit_store_stack_imm64() indeed implicit.
I'll use this way in the next version.

Thanks!
Menglong Dong

> 
> 
> >         }
> >
> >         save_args(m, &prog, regs_off, false, flags);
> > --
> > 2.52.0
> >
>