Re: [PATCH bpf-next v3 4/5] bpf, x86: Add jit support for private stack

[Leon Hwang <hffilwlqm@gmail.com>]

Hi Yonghong,

A brief review about the usage of this_cpu_off on non-SMP systems.

On 27/9/24 07:45, Yonghong Song wrote:
> Add jit support for private stack. For a particular subtree, e.g.,
>   subtree_root <== stack depth 120
>    subprog1    <== stack depth 80
>     subprog2   <== stack depth 40
>    subprog3    <== stack depth 160
> 
> Let us say that private_stack_ptr is the memory address allocated for
> private stack. The frame pointer for each above is calculated like below:
>   subtree_root  <== subtree_root_fp = private_stack_ptr + 120
>    subprog1     <== subtree_subprog1_fp = subtree_root_fp + 80
>     subprog2    <== subtree_subprog2_fp = subtree_subprog1_fp + 40
>    subprog3     <== subtree_subprog1_fp = subtree_root_fp + 160
> 
> For any function call to helper/kfunc, push/pop prog frame pointer
> is needed in order to preserve frame pointer value.
> 
> To deal with exception handling, push/pop frame pointer is also used
> surrounding call to subsequent subprog. For example,
>   subtree_root
>    subprog1
>      ...
>      insn: call bpf_throw
>      ...
> 
> After jit, we will have
>   subtree_root
>    insn: push r9
>    subprog1
>      ...
>      insn: push r9
>      insn: call bpf_throw
>      insn: pop r9
>      ...
>    insn: pop r9
> 
>   exception_handler
>      pop r9
>      ...
> where r9 represents the fp for each subprog.
> 
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>  arch/x86/net/bpf_jit_comp.c | 87 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 81 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 06b080b61aa5..c264822c926b 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -325,6 +325,22 @@ struct jit_context {
>  /* Number of bytes that will be skipped on tailcall */
>  #define X86_TAIL_CALL_OFFSET	(12 + ENDBR_INSN_SIZE)
>  
> +static void push_r9(u8 **pprog)
> +{
> +	u8 *prog = *pprog;
> +
> +	EMIT2(0x41, 0x51);   /* push r9 */
> +	*pprog = prog;
> +}
> +
> +static void pop_r9(u8 **pprog)
> +{
> +	u8 *prog = *pprog;
> +
> +	EMIT2(0x41, 0x59);   /* pop r9 */
> +	*pprog = prog;
> +}
> +
>  static void push_r12(u8 **pprog)
>  {
>  	u8 *prog = *pprog;
> @@ -491,7 +507,7 @@ static void emit_prologue_tail_call(u8 **pprog, bool is_subprog)
>   */
>  static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf,
>  			  bool tail_call_reachable, bool is_subprog,
> -			  bool is_exception_cb)
> +			  bool is_exception_cb, enum bpf_pstack_state  pstack)
>  {
>  	u8 *prog = *pprog;
>  
> @@ -518,6 +534,8 @@ static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf,
>  		 * first restore those callee-saved regs from stack, before
>  		 * reusing the stack frame.
>  		 */
> +		if (pstack)
> +			pop_r9(&prog);
>  		pop_callee_regs(&prog, all_callee_regs_used);
>  		pop_r12(&prog);
>  		/* Reset the stack frame. */
> @@ -1404,6 +1422,22 @@ static void emit_shiftx(u8 **pprog, u32 dst_reg, u8 src_reg, bool is64, u8 op)
>  	*pprog = prog;
>  }
>  
> +static void emit_private_frame_ptr(u8 **pprog, void *private_frame_ptr)
> +{
> +	u8 *prog = *pprog;
> +
> +	/* movabs r9, private_frame_ptr */
> +	emit_mov_imm64(&prog, X86_REG_R9, (long) private_frame_ptr >> 32,
> +		       (u32) (long) private_frame_ptr);
> +
> +	/* add <r9>, gs:[<off>] */
> +	EMIT2(0x65, 0x4c);
> +	EMIT3(0x03, 0x0c, 0x25);
> +	EMIT((u32)(unsigned long)&this_cpu_off, 4);

It should check CONFIG_SMP here like this commit:
1e9e0b85255e ("bpf: handle CONFIG_SMP=n configuration in x86 BPF JIT").

So, it seems better to reuse the code snippet of the commit.

Thanks,
Leon