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

[Yonghong Song <yonghong.song@linux.dev>]

On 9/26/24 9:58 PM, Leon Hwang wrote:
> 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 for pointing this out. I will make the change after waiting some
other reviews.

>
> Thanks,
> Leon
>