bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next] bpf, x86: Simplify the parsing logic of structure parameters
@ 2023-01-03  1:31 Pu Lehui
  2023-01-04 18:24 ` Yonghong Song
  0 siblings, 1 reply; 3+ messages in thread
From: Pu Lehui @ 2023-01-03  1:31 UTC (permalink / raw)
  To: bpf, netdev, linux-kernel
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Pu Lehui,
	Pu Lehui

From: Pu Lehui <pulehui@huawei.com>

Extra_nregs of structure parameters and nr_args can be
added directly at the beginning, and using a flip flag
to identifiy structure parameters. Meantime, renaming
some variables to make them more sense.

Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
 arch/x86/net/bpf_jit_comp.c | 99 +++++++++++++++++--------------------
 1 file changed, 46 insertions(+), 53 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index e3e2b57e4e13..e7b72299f5a4 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1839,62 +1839,57 @@ st:			if (is_imm8(insn->off))
 	return proglen;
 }
 
-static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
+static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
 		      int stack_size)
 {
-	int i, j, arg_size, nr_regs;
+	int i, j, arg_size;
+	bool is_struct = false;
+
 	/* Store function arguments to stack.
 	 * For a function that accepts two pointers the sequence will be:
 	 * mov QWORD PTR [rbp-0x10],rdi
 	 * mov QWORD PTR [rbp-0x8],rsi
 	 */
-	for (i = 0, j = 0; i < min(nr_args, 6); i++) {
-		if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG) {
-			nr_regs = (m->arg_size[i] + 7) / 8;
+	for (i = 0, j = 0; i < min(nr_regs, 6); i++) {
+		arg_size = m->arg_size[j];
+		if (arg_size > 8) {
 			arg_size = 8;
-		} else {
-			nr_regs = 1;
-			arg_size = m->arg_size[i];
+			is_struct ^= 1;
 		}
 
-		while (nr_regs) {
-			emit_stx(prog, bytes_to_bpf_size(arg_size),
-				 BPF_REG_FP,
-				 j == 5 ? X86_REG_R9 : BPF_REG_1 + j,
-				 -(stack_size - j * 8));
-			nr_regs--;
-			j++;
-		}
+		emit_stx(prog, bytes_to_bpf_size(arg_size),
+			 BPF_REG_FP,
+			 i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
+			 -(stack_size - i * 8));
+
+		j = is_struct ? j : j + 1;
 	}
 }
 
-static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
+static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
 			 int stack_size)
 {
-	int i, j, arg_size, nr_regs;
+	int i, j, arg_size;
+	bool is_struct = false;
 
 	/* Restore function arguments from stack.
 	 * For a function that accepts two pointers the sequence will be:
 	 * EMIT4(0x48, 0x8B, 0x7D, 0xF0); mov rdi,QWORD PTR [rbp-0x10]
 	 * EMIT4(0x48, 0x8B, 0x75, 0xF8); mov rsi,QWORD PTR [rbp-0x8]
 	 */
-	for (i = 0, j = 0; i < min(nr_args, 6); i++) {
-		if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG) {
-			nr_regs = (m->arg_size[i] + 7) / 8;
+	for (i = 0, j = 0; i < min(nr_regs, 6); i++) {
+		arg_size = m->arg_size[j];
+		if (arg_size > 8) {
 			arg_size = 8;
-		} else {
-			nr_regs = 1;
-			arg_size = m->arg_size[i];
+			is_struct ^= 1;
 		}
 
-		while (nr_regs) {
-			emit_ldx(prog, bytes_to_bpf_size(arg_size),
-				 j == 5 ? X86_REG_R9 : BPF_REG_1 + j,
-				 BPF_REG_FP,
-				 -(stack_size - j * 8));
-			nr_regs--;
-			j++;
-		}
+		emit_ldx(prog, bytes_to_bpf_size(arg_size),
+			 i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
+			 BPF_REG_FP,
+			 -(stack_size - i * 8));
+
+		j = is_struct ? j : j + 1;
 	}
 }
 
@@ -2120,8 +2115,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 				struct bpf_tramp_links *tlinks,
 				void *func_addr)
 {
-	int ret, i, nr_args = m->nr_args, extra_nregs = 0;
-	int regs_off, ip_off, args_off, stack_size = nr_args * 8, run_ctx_off;
+	int i, ret, nr_regs = m->nr_args, stack_size = 0;
+	int regs_off, nregs_off, ip_off, run_ctx_off;
 	struct bpf_tramp_links *fentry = &tlinks[BPF_TRAMP_FENTRY];
 	struct bpf_tramp_links *fexit = &tlinks[BPF_TRAMP_FEXIT];
 	struct bpf_tramp_links *fmod_ret = &tlinks[BPF_TRAMP_MODIFY_RETURN];
@@ -2130,17 +2125,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	u8 *prog;
 	bool save_ret;
 
-	/* x86-64 supports up to 6 arguments. 7+ can be added in the future */
-	if (nr_args > 6)
-		return -ENOTSUPP;
-
-	for (i = 0; i < MAX_BPF_FUNC_ARGS; i++) {
+	/* extra registers for struct arguments */
+	for (i = 0; i < m->nr_args; i++)
 		if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG)
-			extra_nregs += (m->arg_size[i] + 7) / 8 - 1;
-	}
-	if (nr_args + extra_nregs > 6)
+			nr_regs += (m->arg_size[i] + 7) / 8 - 1;
+
+	/* x86-64 supports up to 6 arguments. 7+ can be added in the future */
+	if (nr_regs > 6)
 		return -ENOTSUPP;
-	stack_size += extra_nregs * 8;
 
 	/* Generated trampoline stack layout:
 	 *
@@ -2154,7 +2146,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	 *                 [ ...             ]
 	 * RBP - regs_off  [ reg_arg1        ]  program's ctx pointer
 	 *
-	 * RBP - args_off  [ arg regs count  ]  always
+	 * RBP - nregs_off [ regs count	     ]  always
 	 *
 	 * RBP - ip_off    [ traced function ]  BPF_TRAMP_F_IP_ARG flag
 	 *
@@ -2166,11 +2158,12 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	if (save_ret)
 		stack_size += 8;
 
+	stack_size += nr_regs * 8;
 	regs_off = stack_size;
 
-	/* args count  */
+	/* regs count  */
 	stack_size += 8;
-	args_off = stack_size;
+	nregs_off = stack_size;
 
 	if (flags & BPF_TRAMP_F_IP_ARG)
 		stack_size += 8; /* room for IP address argument */
@@ -2198,11 +2191,11 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	EMIT1(0x53);		 /* push rbx */
 
 	/* Store number of argument registers of the traced function:
-	 *   mov rax, nr_args + extra_nregs
-	 *   mov QWORD PTR [rbp - args_off], rax
+	 *   mov rax, nr_regs
+	 *   mov QWORD PTR [rbp - nregs_off], rax
 	 */
-	emit_mov_imm64(&prog, BPF_REG_0, 0, (u32) nr_args + extra_nregs);
-	emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -args_off);
+	emit_mov_imm64(&prog, BPF_REG_0, 0, (u32) nr_regs);
+	emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -nregs_off);
 
 	if (flags & BPF_TRAMP_F_IP_ARG) {
 		/* Store IP address of the traced function:
@@ -2213,7 +2206,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 		emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -ip_off);
 	}
 
-	save_regs(m, &prog, nr_args, regs_off);
+	save_regs(m, &prog, nr_regs, regs_off);
 
 	if (flags & BPF_TRAMP_F_CALL_ORIG) {
 		/* arg1: mov rdi, im */
@@ -2243,7 +2236,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 	}
 
 	if (flags & BPF_TRAMP_F_CALL_ORIG) {
-		restore_regs(m, &prog, nr_args, regs_off);
+		restore_regs(m, &prog, nr_regs, regs_off);
 
 		if (flags & BPF_TRAMP_F_ORIG_STACK) {
 			emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8);
@@ -2284,7 +2277,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 		}
 
 	if (flags & BPF_TRAMP_F_RESTORE_REGS)
-		restore_regs(m, &prog, nr_args, regs_off);
+		restore_regs(m, &prog, nr_regs, regs_off);
 
 	/* This needs to be done regardless. If there were fmod_ret programs,
 	 * the return value is only updated on the stack and still needs to be
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH bpf-next] bpf, x86: Simplify the parsing logic of structure parameters
  2023-01-03  1:31 [PATCH bpf-next] bpf, x86: Simplify the parsing logic of structure parameters Pu Lehui
@ 2023-01-04 18:24 ` Yonghong Song
  2023-01-05  3:47   ` Pu Lehui
  0 siblings, 1 reply; 3+ messages in thread
From: Yonghong Song @ 2023-01-04 18:24 UTC (permalink / raw)
  To: Pu Lehui, bpf, netdev, linux-kernel
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Pu Lehui



On 1/2/23 5:31 PM, Pu Lehui wrote:
> From: Pu Lehui <pulehui@huawei.com>
> 
> Extra_nregs of structure parameters and nr_args can be
> added directly at the beginning, and using a flip flag
> to identifiy structure parameters. Meantime, renaming
> some variables to make them more sense.
> 
> Signed-off-by: Pu Lehui <pulehui@huawei.com>

Thanks for refactoring. Using nr_regs instead of nr_args indeed
making things easier to understand. Ack with a few nits below.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   arch/x86/net/bpf_jit_comp.c | 99 +++++++++++++++++--------------------
>   1 file changed, 46 insertions(+), 53 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index e3e2b57e4e13..e7b72299f5a4 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1839,62 +1839,57 @@ st:			if (is_imm8(insn->off))
>   	return proglen;
>   }
>   
> -static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
> +static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
>   		      int stack_size)
>   {
> -	int i, j, arg_size, nr_regs;
> +	int i, j, arg_size;
> +	bool is_struct = false;
> +
>   	/* Store function arguments to stack.
>   	 * For a function that accepts two pointers the sequence will be:
>   	 * mov QWORD PTR [rbp-0x10],rdi
>   	 * mov QWORD PTR [rbp-0x8],rsi
>   	 */
> -	for (i = 0, j = 0; i < min(nr_args, 6); i++) {
> -		if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG) {
> -			nr_regs = (m->arg_size[i] + 7) / 8;
> +	for (i = 0, j = 0; i < min(nr_regs, 6); i++) {
> +		arg_size = m->arg_size[j];
> +		if (arg_size > 8) {
>   			arg_size = 8;
> -		} else {
> -			nr_regs = 1;
> -			arg_size = m->arg_size[i];
> +			is_struct ^= 1;
>   		}
>   
> -		while (nr_regs) {
> -			emit_stx(prog, bytes_to_bpf_size(arg_size),
> -				 BPF_REG_FP,
> -				 j == 5 ? X86_REG_R9 : BPF_REG_1 + j,
> -				 -(stack_size - j * 8));
> -			nr_regs--;
> -			j++;
> -		}
> +		emit_stx(prog, bytes_to_bpf_size(arg_size),
> +			 BPF_REG_FP,
> +			 i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
> +			 -(stack_size - i * 8));
> +
> +		j = is_struct ? j : j + 1;
>   	}
>   }
>   
> -static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_args,
> +static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
>   			 int stack_size)
>   {
> -	int i, j, arg_size, nr_regs;
> +	int i, j, arg_size;
> +	bool is_struct = false;

Maybe
	bool next_same_struct = false
to better characterize what it means?

>   
>   	/* Restore function arguments from stack.
>   	 * For a function that accepts two pointers the sequence will be:
>   	 * EMIT4(0x48, 0x8B, 0x7D, 0xF0); mov rdi,QWORD PTR [rbp-0x10]
>   	 * EMIT4(0x48, 0x8B, 0x75, 0xF8); mov rsi,QWORD PTR [rbp-0x8]
>   	 */
> -	for (i = 0, j = 0; i < min(nr_args, 6); i++) {
> -		if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG) {
> -			nr_regs = (m->arg_size[i] + 7) / 8;
> +	for (i = 0, j = 0; i < min(nr_regs, 6); i++) {

Let us put a comment here so the later users can understand the logic
behind 'is_struct ^= 1'.

/* The arg_size is at most 16 bytes, enforced by the verifier. */

> +		arg_size = m->arg_size[j];
> +		if (arg_size > 8) {
>   			arg_size = 8;
> -		} else {
> -			nr_regs = 1;
> -			arg_size = m->arg_size[i];
> +			is_struct ^= 1;

next_same_struct = !next_same_struct;

The same for above save_regs().

>   		}
>   
> -		while (nr_regs) {
> -			emit_ldx(prog, bytes_to_bpf_size(arg_size),
> -				 j == 5 ? X86_REG_R9 : BPF_REG_1 + j,
> -				 BPF_REG_FP,
> -				 -(stack_size - j * 8));
> -			nr_regs--;
> -			j++;
> -		}
> +		emit_ldx(prog, bytes_to_bpf_size(arg_size),
> +			 i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
> +			 BPF_REG_FP,
> +			 -(stack_size - i * 8));
> +
> +		j = is_struct ? j : j + 1;
>   	}
>   }
>   
[...]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH bpf-next] bpf, x86: Simplify the parsing logic of structure parameters
  2023-01-04 18:24 ` Yonghong Song
@ 2023-01-05  3:47   ` Pu Lehui
  0 siblings, 0 replies; 3+ messages in thread
From: Pu Lehui @ 2023-01-05  3:47 UTC (permalink / raw)
  To: Yonghong Song, bpf, netdev, linux-kernel
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Pu Lehui



On 2023/1/5 2:24, Yonghong Song wrote:
> 
> 
> On 1/2/23 5:31 PM, Pu Lehui wrote:
>> From: Pu Lehui <pulehui@huawei.com>
>>
>> Extra_nregs of structure parameters and nr_args can be
>> added directly at the beginning, and using a flip flag
>> to identifiy structure parameters. Meantime, renaming
>> some variables to make them more sense.
>>
>> Signed-off-by: Pu Lehui <pulehui@huawei.com>
> 
> Thanks for refactoring. Using nr_regs instead of nr_args indeed
> making things easier to understand. Ack with a few nits below.
> 
> Acked-by: Yonghong Song <yhs@fb.com>
> 
>> ---
>>   arch/x86/net/bpf_jit_comp.c | 99 +++++++++++++++++--------------------
>>   1 file changed, 46 insertions(+), 53 deletions(-)
>>
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>> index e3e2b57e4e13..e7b72299f5a4 100644
>> --- a/arch/x86/net/bpf_jit_comp.c
>> +++ b/arch/x86/net/bpf_jit_comp.c
>> @@ -1839,62 +1839,57 @@ st:            if (is_imm8(insn->off))
>>       return proglen;
>>   }
>> -static void save_regs(const struct btf_func_model *m, u8 **prog, int 
>> nr_args,
>> +static void save_regs(const struct btf_func_model *m, u8 **prog, int 
>> nr_regs,
>>                 int stack_size)
>>   {
>> -    int i, j, arg_size, nr_regs;
>> +    int i, j, arg_size;
>> +    bool is_struct = false;
>> +
>>       /* Store function arguments to stack.
>>        * For a function that accepts two pointers the sequence will be:
>>        * mov QWORD PTR [rbp-0x10],rdi
>>        * mov QWORD PTR [rbp-0x8],rsi
>>        */
>> -    for (i = 0, j = 0; i < min(nr_args, 6); i++) {
>> -        if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG) {
>> -            nr_regs = (m->arg_size[i] + 7) / 8;
>> +    for (i = 0, j = 0; i < min(nr_regs, 6); i++) {
>> +        arg_size = m->arg_size[j];
>> +        if (arg_size > 8) {
>>               arg_size = 8;
>> -        } else {
>> -            nr_regs = 1;
>> -            arg_size = m->arg_size[i];
>> +            is_struct ^= 1;
>>           }
>> -        while (nr_regs) {
>> -            emit_stx(prog, bytes_to_bpf_size(arg_size),
>> -                 BPF_REG_FP,
>> -                 j == 5 ? X86_REG_R9 : BPF_REG_1 + j,
>> -                 -(stack_size - j * 8));
>> -            nr_regs--;
>> -            j++;
>> -        }
>> +        emit_stx(prog, bytes_to_bpf_size(arg_size),
>> +             BPF_REG_FP,
>> +             i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
>> +             -(stack_size - i * 8));
>> +
>> +        j = is_struct ? j : j + 1;
>>       }
>>   }
>> -static void restore_regs(const struct btf_func_model *m, u8 **prog, 
>> int nr_args,
>> +static void restore_regs(const struct btf_func_model *m, u8 **prog, 
>> int nr_regs,
>>                int stack_size)
>>   {
>> -    int i, j, arg_size, nr_regs;
>> +    int i, j, arg_size;
>> +    bool is_struct = false;
> 
> Maybe
>      bool next_same_struct = false
> to better characterize what it means?
> 

agree, will do as suggested bellow.

>>       /* Restore function arguments from stack.
>>        * For a function that accepts two pointers the sequence will be:
>>        * EMIT4(0x48, 0x8B, 0x7D, 0xF0); mov rdi,QWORD PTR [rbp-0x10]
>>        * EMIT4(0x48, 0x8B, 0x75, 0xF8); mov rsi,QWORD PTR [rbp-0x8]
>>        */
>> -    for (i = 0, j = 0; i < min(nr_args, 6); i++) {
>> -        if (m->arg_flags[i] & BTF_FMODEL_STRUCT_ARG) {
>> -            nr_regs = (m->arg_size[i] + 7) / 8;
>> +    for (i = 0, j = 0; i < min(nr_regs, 6); i++) {
> 
> Let us put a comment here so the later users can understand the logic
> behind 'is_struct ^= 1'.
> 
> /* The arg_size is at most 16 bytes, enforced by the verifier. */
> 
>> +        arg_size = m->arg_size[j];
>> +        if (arg_size > 8) {
>>               arg_size = 8;
>> -        } else {
>> -            nr_regs = 1;
>> -            arg_size = m->arg_size[i];
>> +            is_struct ^= 1;
> 
> next_same_struct = !next_same_struct;
> 
> The same for above save_regs().
> 
>>           }
>> -        while (nr_regs) {
>> -            emit_ldx(prog, bytes_to_bpf_size(arg_size),
>> -                 j == 5 ? X86_REG_R9 : BPF_REG_1 + j,
>> -                 BPF_REG_FP,
>> -                 -(stack_size - j * 8));
>> -            nr_regs--;
>> -            j++;
>> -        }
>> +        emit_ldx(prog, bytes_to_bpf_size(arg_size),
>> +             i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
>> +             BPF_REG_FP,
>> +             -(stack_size - i * 8));
>> +
>> +        j = is_struct ? j : j + 1;
>>       }
>>   }
> [...]


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2023-01-05  4:09 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-03  1:31 [PATCH bpf-next] bpf, x86: Simplify the parsing logic of structure parameters Pu Lehui
2023-01-04 18:24 ` Yonghong Song
2023-01-05  3:47   ` Pu Lehui

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).