[PATCH bpf-next 0/3] bpf: fsession support for s390

[PATCH bpf-next 0/3] bpf: fsession support for s390

[Menglong Dong]

Implement bpf fsession for the s390 architecture.

Menglong Dong (3):
  bpf,s390: introduce emit_store_stack_imm64() for trampoline
  bpf,s390: add fsession support for trampolines
  selftests/bpf: enable fsession_test on s390

 arch/s390/net/bpf_jit_comp.c                  | 97 ++++++++++++++-----
 .../selftests/bpf/progs/get_func_args_test.c  |  2 +-
 .../selftests/bpf/progs/get_func_ip_test.c    |  2 +-
 3 files changed, 75 insertions(+), 26 deletions(-)

-- 
2.53.0

[PATCH bpf-next 1/3] bpf,s390: introduce emit_store_stack_imm64() for trampoline

[Menglong Dong]

Introduce a helper to store 64-bit immediate on the trampoline stack with
a help of a register.

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 arch/s390/net/bpf_jit_comp.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index 579461d471bb..763d2491dfa3 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -2506,6 +2506,13 @@ static void load_imm64(struct bpf_jit *jit, int dst_reg, u64 val)
 	EMIT6_IMM(0xc00d0000, dst_reg, val);
 }
 
+static void emit_store_stack_imm64(struct bpf_jit *jit, int tmp_reg, int stack_off, u64 imm)
+{
+	load_imm64(jit, tmp_reg, imm);
+	/* stg %tmp_reg,stack_off(%r15) */
+	EMIT6_DISP_LH(0xe3000000, 0x0024, tmp_reg, REG_0, REG_15, stack_off);
+}
+
 static int invoke_bpf_prog(struct bpf_tramp_jit *tjit,
 			   const struct btf_func_model *m,
 			   struct bpf_tramp_link *tlink, bool save_ret)
@@ -2520,10 +2527,7 @@ static int invoke_bpf_prog(struct bpf_tramp_jit *tjit,
 	 * run_ctx.cookie = tlink->cookie;
 	 */
 
-	/* %r0 = tlink->cookie */
-	load_imm64(jit, REG_W0, tlink->cookie);
-	/* stg %r0,cookie_off(%r15) */
-	EMIT6_DISP_LH(0xe3000000, 0x0024, REG_W0, REG_0, REG_15, cookie_off);
+	emit_store_stack_imm64(jit, REG_W0, cookie_off, tlink->cookie);
 
 	/*
 	 * if ((start = __bpf_prog_enter(p, &run_ctx)) == 0)
@@ -2743,18 +2747,9 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
 	 * arg_cnt = m->nr_args;
 	 */
 
-	if (flags & BPF_TRAMP_F_IP_ARG) {
-		/* %r0 = func_addr */
-		load_imm64(jit, REG_0, (u64)func_addr);
-		/* stg %r0,ip_off(%r15) */
-		EMIT6_DISP_LH(0xe3000000, 0x0024, REG_0, REG_0, REG_15,
-			      tjit->ip_off);
-	}
-	/* lghi %r0,nr_bpf_args */
-	EMIT4_IMM(0xa7090000, REG_0, nr_bpf_args);
-	/* stg %r0,arg_cnt_off(%r15) */
-	EMIT6_DISP_LH(0xe3000000, 0x0024, REG_0, REG_0, REG_15,
-		      tjit->arg_cnt_off);
+	if (flags & BPF_TRAMP_F_IP_ARG)
+		emit_store_stack_imm64(jit, REG_0, tjit->ip_off, (u64)func_addr);
+	emit_store_stack_imm64(jit, REG_0, tjit->arg_cnt_off, nr_bpf_args);
 
 	if (flags & BPF_TRAMP_F_CALL_ORIG) {
 		/*
-- 
2.53.0

Re: [PATCH bpf-next 1/3] bpf,s390: introduce emit_store_stack_imm64() for trampoline

[Ilya Leoshkevich]

On 2/23/26 09:40, Menglong Dong wrote:
> Introduce a helper to store 64-bit immediate on the trampoline stack with
> a help of a register.
>
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> ---
>   arch/s390/net/bpf_jit_comp.c | 27 +++++++++++----------------
>   1 file changed, 11 insertions(+), 16 deletions(-)

[...]

> @@ -2743,18 +2747,9 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
>   	 * arg_cnt = m->nr_args;
>   	 */
>   
> -	if (flags & BPF_TRAMP_F_IP_ARG) {
> -		/* %r0 = func_addr */
> -		load_imm64(jit, REG_0, (u64)func_addr);
> -		/* stg %r0,ip_off(%r15) */
> -		EMIT6_DISP_LH(0xe3000000, 0x0024, REG_0, REG_0, REG_15,
> -			      tjit->ip_off);
> -	}
> -	/* lghi %r0,nr_bpf_args */
> -	EMIT4_IMM(0xa7090000, REG_0, nr_bpf_args);

LGHI is more space-efficient than load_imm64(), can we keep it open-coded?

With this changed:

Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>

Re: [PATCH bpf-next 1/3] bpf,s390: introduce emit_store_stack_imm64() for trampoline

[Menglong Dong]

On 2026/2/23 19:44 Ilya Leoshkevich <iii@linux.ibm.com> write:
> 
> On 2/23/26 09:40, Menglong Dong wrote:
> > Introduce a helper to store 64-bit immediate on the trampoline stack with
> > a help of a register.
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > ---
> >   arch/s390/net/bpf_jit_comp.c | 27 +++++++++++----------------
> >   1 file changed, 11 insertions(+), 16 deletions(-)
> 
> [...]
> 
> > @@ -2743,18 +2747,9 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
> >   	 * arg_cnt = m->nr_args;
> >   	 */
> >   
> > -	if (flags & BPF_TRAMP_F_IP_ARG) {
> > -		/* %r0 = func_addr */
> > -		load_imm64(jit, REG_0, (u64)func_addr);
> > -		/* stg %r0,ip_off(%r15) */
> > -		EMIT6_DISP_LH(0xe3000000, 0x0024, REG_0, REG_0, REG_15,
> > -			      tjit->ip_off);
> > -	}
> > -	/* lghi %r0,nr_bpf_args */
> > -	EMIT4_IMM(0xa7090000, REG_0, nr_bpf_args);
> 
> LGHI is more space-efficient than load_imm64(), can we keep it open-coded?

OK, I can keep this lghi instruction as it was.

Thanks for your reviewing :)

> 
> With this changed:
> 
> Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
> 
> 
> 

[PATCH bpf-next 2/3] bpf,s390: add fsession support for trampolines

[Menglong Dong]

Implement BPF_TRACE_FSESSION support for s390. The logic here is similar
to what we did in x86_64.

In order to simply the logic, we factor out the function invoke_bpf() for
fentry and fexit.

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 arch/s390/net/bpf_jit_comp.c | 68 +++++++++++++++++++++++++++++++-----
 1 file changed, 59 insertions(+), 9 deletions(-)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index 763d2491dfa3..ea0c81f18ece 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -2480,8 +2480,8 @@ struct bpf_tramp_jit {
 	int ip_off;		/* For bpf_get_func_ip(), has to be at
 				 * (ctx - 16)
 				 */
-	int arg_cnt_off;	/* For bpf_get_func_arg_cnt(), has to be at
-				 * (ctx - 8)
+	int func_meta_off;	/* For bpf_get_func_arg_cnt()/fsession, has
+				 * to be at (ctx - 8)
 				 */
 	int bpf_args_off;	/* Offset of BPF_PROG context, which consists
 				 * of BPF arguments followed by return value
@@ -2585,6 +2585,28 @@ static int invoke_bpf_prog(struct bpf_tramp_jit *tjit,
 	return 0;
 }
 
+static int invoke_bpf(struct bpf_tramp_jit *tjit,
+		      const struct btf_func_model *m,
+		      struct bpf_tramp_links *tl, bool save_ret,
+		      u64 func_meta, int cookie_off)
+{
+	int i, cur_cookie = (tjit->bpf_args_off - cookie_off) / sizeof(u64);
+	struct bpf_jit *jit = &tjit->common;
+
+	for (i = 0; i < tl->nr_links; i++) {
+		if (bpf_prog_calls_session_cookie(tl->links[i])) {
+			u64 meta = func_meta | ((u64)cur_cookie << BPF_TRAMP_COOKIE_INDEX_SHIFT);
+
+			emit_store_stack_imm64(jit, REG_0, tjit->func_meta_off, meta);
+			cur_cookie--;
+		}
+		if (invoke_bpf_prog(tjit, m, tl->links[i], save_ret))
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int alloc_stack(struct bpf_tramp_jit *tjit, size_t size)
 {
 	int stack_offset = tjit->stack_size;
@@ -2614,8 +2636,10 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
 	struct bpf_tramp_links *fentry = &tlinks[BPF_TRAMP_FENTRY];
 	struct bpf_tramp_links *fexit = &tlinks[BPF_TRAMP_FEXIT];
 	int nr_bpf_args, nr_reg_args, nr_stack_args;
+	int cookie_cnt, cookie_off, fsession_cnt;
 	struct bpf_jit *jit = &tjit->common;
 	int arg, bpf_arg_off;
+	u64 func_meta;
 	int i, j;
 
 	/* Support as many stack arguments as "mvc" instruction can handle. */
@@ -2647,6 +2671,9 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
 			return -ENOTSUPP;
 	}
 
+	cookie_cnt = bpf_fsession_cookie_cnt(tlinks);
+	fsession_cnt = bpf_fsession_cnt(tlinks);
+
 	/*
 	 * Calculate the stack layout.
 	 */
@@ -2659,8 +2686,9 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
 	tjit->backchain_off = tjit->stack_size - sizeof(u64);
 	tjit->stack_args_off = alloc_stack(tjit, nr_stack_args * sizeof(u64));
 	tjit->reg_args_off = alloc_stack(tjit, nr_reg_args * sizeof(u64));
+	cookie_off = alloc_stack(tjit, cookie_cnt * sizeof(u64));
 	tjit->ip_off = alloc_stack(tjit, sizeof(u64));
-	tjit->arg_cnt_off = alloc_stack(tjit, sizeof(u64));
+	tjit->func_meta_off = alloc_stack(tjit, sizeof(u64));
 	tjit->bpf_args_off = alloc_stack(tjit, nr_bpf_args * sizeof(u64));
 	tjit->retval_off = alloc_stack(tjit, sizeof(u64));
 	tjit->r7_r8_off = alloc_stack(tjit, 2 * sizeof(u64));
@@ -2749,7 +2777,8 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
 
 	if (flags & BPF_TRAMP_F_IP_ARG)
 		emit_store_stack_imm64(jit, REG_0, tjit->ip_off, (u64)func_addr);
-	emit_store_stack_imm64(jit, REG_0, tjit->arg_cnt_off, nr_bpf_args);
+	func_meta = nr_bpf_args;
+	emit_store_stack_imm64(jit, REG_0, tjit->func_meta_off, func_meta);
 
 	if (flags & BPF_TRAMP_F_CALL_ORIG) {
 		/*
@@ -2762,10 +2791,19 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
 		EMIT6_PCREL_RILB_PTR(0xc0050000, REG_14, __bpf_tramp_enter);
 	}
 
-	for (i = 0; i < fentry->nr_links; i++)
-		if (invoke_bpf_prog(tjit, m, fentry->links[i],
-				    flags & BPF_TRAMP_F_RET_FENTRY_RET))
+	if (fsession_cnt) {
+		/* clear all the session cookies' value */
+		for (i = 0; i < cookie_cnt; i++)
+			emit_store_stack_imm64(jit, REG_0, cookie_off + 8 * i, 0);
+		/* clear the return value to make sure fentry always gets 0 */
+		emit_store_stack_imm64(jit, REG_0, tjit->retval_off, 0);
+	}
+
+	if (fentry->nr_links) {
+		if (invoke_bpf(tjit, m, fentry, flags & BPF_TRAMP_F_RET_FENTRY_RET,
+			       func_meta, cookie_off))
 			return -EINVAL;
+	}
 
 	if (fmod_ret->nr_links) {
 		/*
@@ -2842,11 +2880,18 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
 		EMIT6_PCREL_RILC(0xc0040000, 0, (u64)im->ip_epilogue);
 	}
 
+	/* set the "is_return" flag for fsession */
+	func_meta |= (1ULL << BPF_TRAMP_IS_RETURN_SHIFT);
+	if (fsession_cnt)
+		emit_store_stack_imm64(jit, REG_W0, tjit->func_meta_off,
+				       func_meta);
+
 	/* do_fexit: */
 	tjit->do_fexit = jit->prg;
-	for (i = 0; i < fexit->nr_links; i++)
-		if (invoke_bpf_prog(tjit, m, fexit->links[i], false))
+	if (fexit->nr_links) {
+		if (invoke_bpf(tjit, m, fexit, false, func_meta, cookie_off))
 			return -EINVAL;
+	}
 
 	if (flags & BPF_TRAMP_F_CALL_ORIG) {
 		im->ip_epilogue = jit->prg_buf + jit->prg;
@@ -2951,6 +2996,11 @@ bool bpf_jit_supports_arena(void)
 	return true;
 }
 
+bool bpf_jit_supports_fsession(void)
+{
+	return true;
+}
+
 bool bpf_jit_supports_insn(struct bpf_insn *insn, bool in_arena)
 {
 	if (!in_arena)
-- 
2.53.0

Re: [PATCH bpf-next 2/3] bpf,s390: add fsession support for trampolines

[Ilya Leoshkevich]

On 2/23/26 09:40, Menglong Dong wrote:
> Implement BPF_TRACE_FSESSION support for s390. The logic here is similar
> to what we did in x86_64.
>
> In order to simply the logic, we factor out the function invoke_bpf() for
> fentry and fexit.
>
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> ---
>   arch/s390/net/bpf_jit_comp.c | 68 +++++++++++++++++++++++++++++++-----
>   1 file changed, 59 insertions(+), 9 deletions(-)


Thank you for contributing this series!


In general this all looks very reasonable; I believe I found a few nits, 
please take a look at my comments below.


>
> diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
> index 763d2491dfa3..ea0c81f18ece 100644
> --- a/arch/s390/net/bpf_jit_comp.c
> +++ b/arch/s390/net/bpf_jit_comp.c
> @@ -2480,8 +2480,8 @@ struct bpf_tramp_jit {
>   	int ip_off;		/* For bpf_get_func_ip(), has to be at
>   				 * (ctx - 16)
>   				 */
> -	int arg_cnt_off;	/* For bpf_get_func_arg_cnt(), has to be at
> -				 * (ctx - 8)
> +	int func_meta_off;	/* For bpf_get_func_arg_cnt()/fsession, has
> +				 * to be at (ctx - 8)
>   				 */
>   	int bpf_args_off;	/* Offset of BPF_PROG context, which consists
>   				 * of BPF arguments followed by return value
> @@ -2585,6 +2585,28 @@ static int invoke_bpf_prog(struct bpf_tramp_jit *tjit,
>   	return 0;
>   }
>   
> +static int invoke_bpf(struct bpf_tramp_jit *tjit,
> +		      const struct btf_func_model *m,
> +		      struct bpf_tramp_links *tl, bool save_ret,
> +		      u64 func_meta, int cookie_off)
> +{
> +	int i, cur_cookie = (tjit->bpf_args_off - cookie_off) / sizeof(u64);
> +	struct bpf_jit *jit = &tjit->common;
> +
> +	for (i = 0; i < tl->nr_links; i++) {
> +		if (bpf_prog_calls_session_cookie(tl->links[i])) {
> +			u64 meta = func_meta | ((u64)cur_cookie << BPF_TRAMP_COOKIE_INDEX_SHIFT);
> +
> +			emit_store_stack_imm64(jit, REG_0, tjit->func_meta_off, meta);
> +			cur_cookie--;
> +		}
> +		if (invoke_bpf_prog(tjit, m, tl->links[i], save_ret))
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>   static int alloc_stack(struct bpf_tramp_jit *tjit, size_t size)
>   {
>   	int stack_offset = tjit->stack_size;
> @@ -2614,8 +2636,10 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
>   	struct bpf_tramp_links *fentry = &tlinks[BPF_TRAMP_FENTRY];
>   	struct bpf_tramp_links *fexit = &tlinks[BPF_TRAMP_FEXIT];
>   	int nr_bpf_args, nr_reg_args, nr_stack_args;
> +	int cookie_cnt, cookie_off, fsession_cnt;
>   	struct bpf_jit *jit = &tjit->common;
>   	int arg, bpf_arg_off;
> +	u64 func_meta;
>   	int i, j;
>   
>   	/* Support as many stack arguments as "mvc" instruction can handle. */
> @@ -2647,6 +2671,9 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
>   			return -ENOTSUPP;
>   	}
>   
> +	cookie_cnt = bpf_fsession_cookie_cnt(tlinks);
> +	fsession_cnt = bpf_fsession_cnt(tlinks);
> +
>   	/*
>   	 * Calculate the stack layout.
>   	 */
> @@ -2659,8 +2686,9 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
>   	tjit->backchain_off = tjit->stack_size - sizeof(u64);
>   	tjit->stack_args_off = alloc_stack(tjit, nr_stack_args * sizeof(u64));
>   	tjit->reg_args_off = alloc_stack(tjit, nr_reg_args * sizeof(u64));
> +	cookie_off = alloc_stack(tjit, cookie_cnt * sizeof(u64));
>   	tjit->ip_off = alloc_stack(tjit, sizeof(u64));
> -	tjit->arg_cnt_off = alloc_stack(tjit, sizeof(u64));
> +	tjit->func_meta_off = alloc_stack(tjit, sizeof(u64));
>   	tjit->bpf_args_off = alloc_stack(tjit, nr_bpf_args * sizeof(u64));
>   	tjit->retval_off = alloc_stack(tjit, sizeof(u64));
>   	tjit->r7_r8_off = alloc_stack(tjit, 2 * sizeof(u64));
> @@ -2749,7 +2777,8 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
>   
>   	if (flags & BPF_TRAMP_F_IP_ARG)
>   		emit_store_stack_imm64(jit, REG_0, tjit->ip_off, (u64)func_addr);
> -	emit_store_stack_imm64(jit, REG_0, tjit->arg_cnt_off, nr_bpf_args);
> +	func_meta = nr_bpf_args;
> +	emit_store_stack_imm64(jit, REG_0, tjit->func_meta_off, func_meta);
>   
>   	if (flags & BPF_TRAMP_F_CALL_ORIG) {
>   		/*
> @@ -2762,10 +2791,19 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
>   		EMIT6_PCREL_RILB_PTR(0xc0050000, REG_14, __bpf_tramp_enter);
>   	}
>   
> -	for (i = 0; i < fentry->nr_links; i++)
> -		if (invoke_bpf_prog(tjit, m, fentry->links[i],
> -				    flags & BPF_TRAMP_F_RET_FENTRY_RET))
> +	if (fsession_cnt) {
> +		/* clear all the session cookies' value */

Here and below: please use the existing style for single-line comments: 
full sentence starting with a capital letter and ending with a dot. 
Unfortunately the JIT is somewhat inconsistent in this area, but let's 
at least not introduce a new comment style here.

> +		for (i = 0; i < cookie_cnt; i++)
> +			emit_store_stack_imm64(jit, REG_0, cookie_off + 8 * i, 0);
> +		/* clear the return value to make sure fentry always gets 0 */
> +		emit_store_stack_imm64(jit, REG_0, tjit->retval_off, 0);
> +	}

Would it make sense to clear cookies right after invoke_bpf_prog() and 
only if bpf_prog_calls_session_cookie() is true?

Going one step further and reducing the size of cookies array would 
probably be ideal, but I guess this will complicate things 
significantly, so I'm not suggesting to do this.

> +
> +	if (fentry->nr_links) {

I think it's okay to drop this condition, invoke_bpf() is most likely 
inlined and the loop will automatically have 0 iterations in this case.

> +		if (invoke_bpf(tjit, m, fentry, flags & BPF_TRAMP_F_RET_FENTRY_RET,
> +			       func_meta, cookie_off))
>   			return -EINVAL;
> +	}
>   
>   	if (fmod_ret->nr_links) {
>   		/*
> @@ -2842,11 +2880,18 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
>   		EMIT6_PCREL_RILC(0xc0040000, 0, (u64)im->ip_epilogue);
>   	}
>   
> +	/* set the "is_return" flag for fsession */
> +	func_meta |= (1ULL << BPF_TRAMP_IS_RETURN_SHIFT);
> +	if (fsession_cnt)
> +		emit_store_stack_imm64(jit, REG_W0, tjit->func_meta_off,
> +				       func_meta);
> +
>   	/* do_fexit: */
>   	tjit->do_fexit = jit->prg;
> -	for (i = 0; i < fexit->nr_links; i++)
> -		if (invoke_bpf_prog(tjit, m, fexit->links[i], false))
> +	if (fexit->nr_links) {
Same as for fentry.
> +		if (invoke_bpf(tjit, m, fexit, false, func_meta, cookie_off))
>   			return -EINVAL;
> +	}
>   
>   	if (flags & BPF_TRAMP_F_CALL_ORIG) {
>   		im->ip_epilogue = jit->prg_buf + jit->prg;
> @@ -2951,6 +2996,11 @@ bool bpf_jit_supports_arena(void)
>   	return true;
>   }
>   
> +bool bpf_jit_supports_fsession(void)
> +{
> +	return true;
> +}
> +
>   bool bpf_jit_supports_insn(struct bpf_insn *insn, bool in_arena)
>   {
>   	if (!in_arena)

Re: [PATCH bpf-next 2/3] bpf,s390: add fsession support for trampolines

[Menglong Dong]

On 2026/2/23 20:14 Ilya Leoshkevich <iii@linux.ibm.com> write:
> 
> On 2/23/26 09:40, Menglong Dong wrote:
> > Implement BPF_TRACE_FSESSION support for s390. The logic here is similar
> > to what we did in x86_64.
> >
> > In order to simply the logic, we factor out the function invoke_bpf() for
> > fentry and fexit.
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > ---
> >   arch/s390/net/bpf_jit_comp.c | 68 +++++++++++++++++++++++++++++++-----
> >   1 file changed, 59 insertions(+), 9 deletions(-)
> 
> 
> Thank you for contributing this series!
> 
> 
> In general this all looks very reasonable; I believe I found a few nits, 
> please take a look at my comments below.
> 
> 
> >
> > diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
> > index 763d2491dfa3..ea0c81f18ece 100644
[...]
> >   
> > -	for (i = 0; i < fentry->nr_links; i++)
> > -		if (invoke_bpf_prog(tjit, m, fentry->links[i],
> > -				    flags & BPF_TRAMP_F_RET_FENTRY_RET))
> > +	if (fsession_cnt) {
> > +		/* clear all the session cookies' value */
> 
> Here and below: please use the existing style for single-line comments: 
> full sentence starting with a capital letter and ending with a dot. 
> Unfortunately the JIT is somewhat inconsistent in this area, but let's 
> at least not introduce a new comment style here.

OK, I'll fix up the comments style in the next version.

> 
> > +		for (i = 0; i < cookie_cnt; i++)
> > +			emit_store_stack_imm64(jit, REG_0, cookie_off + 8 * i, 0);
> > +		/* clear the return value to make sure fentry always gets 0 */
> > +		emit_store_stack_imm64(jit, REG_0, tjit->retval_off, 0);
> > +	}
> 
> Would it make sense to clear cookies right after invoke_bpf_prog() and 
> only if bpf_prog_calls_session_cookie() is true?

I think it doesn't make much sense. We only reserve session cookies for
the fsession progs that called bpf_session_cookie(), which means that
all the cookies that we reserved here will be used. Right?

> 
> Going one step further and reducing the size of cookies array would 
> probably be ideal, but I guess this will complicate things 
> significantly, so I'm not suggesting to do this.
> 
> > +
> > +	if (fentry->nr_links) {
> 
> I think it's okay to drop this condition, invoke_bpf() is most likely 
> inlined and the loop will automatically have 0 iterations in this case.

OK, I'll drop this checking.

> 
> > +		if (invoke_bpf(tjit, m, fentry, flags & BPF_TRAMP_F_RET_FENTRY_RET,
> > +			       func_meta, cookie_off))
> >   			return -EINVAL;
> > +	}
> >   
> >   	if (fmod_ret->nr_links) {
> >   		/*
> > @@ -2842,11 +2880,18 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
> >   		EMIT6_PCREL_RILC(0xc0040000, 0, (u64)im->ip_epilogue);
> >   	}
> >   
> > +	/* set the "is_return" flag for fsession */
> > +	func_meta |= (1ULL << BPF_TRAMP_IS_RETURN_SHIFT);
> > +	if (fsession_cnt)
> > +		emit_store_stack_imm64(jit, REG_W0, tjit->func_meta_off,
> > +				       func_meta);
> > +
> >   	/* do_fexit: */
> >   	tjit->do_fexit = jit->prg;
> > -	for (i = 0; i < fexit->nr_links; i++)
> > -		if (invoke_bpf_prog(tjit, m, fexit->links[i], false))
> > +	if (fexit->nr_links) {
> Same as for fentry.

ACK.

Thanks!
Menglong Dong

> > +		if (invoke_bpf(tjit, m, fexit, false, func_meta, cookie_off))
> >   			return -EINVAL;
> > +	}
> >   
> >   	if (flags & BPF_TRAMP_F_CALL_ORIG) {
> >   		im->ip_epilogue = jit->prg_buf + jit->prg;
> > @@ -2951,6 +2996,11 @@ bool bpf_jit_supports_arena(void)
> >   	return true;
> >   }
> >   
> > +bool bpf_jit_supports_fsession(void)
> > +{
> > +	return true;
> > +}
> > +
> >   bool bpf_jit_supports_insn(struct bpf_insn *insn, bool in_arena)
> >   {
> >   	if (!in_arena)
> 
> 

[PATCH bpf-next 3/3] selftests/bpf: factor out get_func_* tests for fsession

[Menglong Dong]

The fsession is already supported by x86_64, arm64, riscv and s390, so we
don't need to disable it in the compile time according to the
architecture. Factor out the testings for it. Therefore, the testing can
be disabled for the architecture that doesn't support it manually.

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 .../bpf/prog_tests/get_func_args_test.c       | 25 +++++++++++-
 .../bpf/prog_tests/get_func_ip_test.c         | 28 +++++++++++++-
 .../bpf/progs/get_func_args_fsession_test.c   | 37 ++++++++++++++++++
 .../selftests/bpf/progs/get_func_args_test.c  | 38 -------------------
 .../bpf/progs/get_func_ip_fsession_test.c     | 21 ++++++++++
 .../selftests/bpf/progs/get_func_ip_test.c    | 23 -----------
 6 files changed, 108 insertions(+), 64 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/get_func_args_fsession_test.c
 create mode 100644 tools/testing/selftests/bpf/progs/get_func_ip_fsession_test.c

diff --git a/tools/testing/selftests/bpf/prog_tests/get_func_args_test.c b/tools/testing/selftests/bpf/prog_tests/get_func_args_test.c
index 96b27de05524..7bf8adc41e99 100644
--- a/tools/testing/selftests/bpf/prog_tests/get_func_args_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/get_func_args_test.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
 #include "get_func_args_test.skel.h"
+#include "get_func_args_fsession_test.skel.h"
 
 void test_get_func_args_test(void)
 {
@@ -41,8 +42,30 @@ void test_get_func_args_test(void)
 	ASSERT_EQ(skel->bss->test4_result, 1, "test4_result");
 	ASSERT_EQ(skel->bss->test5_result, 1, "test5_result");
 	ASSERT_EQ(skel->bss->test6_result, 1, "test6_result");
-	ASSERT_EQ(skel->bss->test7_result, 1, "test7_result");
 
 cleanup:
 	get_func_args_test__destroy(skel);
 }
+
+void test_get_func_args_fsession_test(void)
+{
+	struct get_func_args_fsession_test *skel = NULL;
+	int err;
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
+
+	skel = get_func_args_fsession_test__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "get_func_args_fsession_test__open_and_load"))
+		return;
+
+	err = get_func_args_fsession_test__attach(skel);
+	if (!ASSERT_OK(err, "get_func_args_fsession_test__attach"))
+		goto cleanup;
+
+	err = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.test1), &topts);
+	ASSERT_OK(err, "test_run");
+	ASSERT_EQ(topts.retval, 0, "test_run");
+
+	ASSERT_EQ(skel->bss->test1_result, 1, "test1_result");
+cleanup:
+	get_func_args_fsession_test__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c b/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c
index 7772a0f288d3..357fdedfea93 100644
--- a/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c
@@ -2,6 +2,7 @@
 #include <test_progs.h>
 #include "get_func_ip_test.skel.h"
 #include "get_func_ip_uprobe_test.skel.h"
+#include "get_func_ip_fsession_test.skel.h"
 
 static noinline void uprobe_trigger(void)
 {
@@ -46,8 +47,6 @@ static void test_function_entry(void)
 	ASSERT_EQ(skel->bss->test5_result, 1, "test5_result");
 	ASSERT_EQ(skel->bss->test7_result, 1, "test7_result");
 	ASSERT_EQ(skel->bss->test8_result, 1, "test8_result");
-	ASSERT_EQ(skel->bss->test9_entry_result, 1, "test9_entry_result");
-	ASSERT_EQ(skel->bss->test9_exit_result, 1, "test9_exit_result");
 
 cleanup:
 	get_func_ip_test__destroy(skel);
@@ -139,3 +138,28 @@ void test_get_func_ip_test(void)
 	test_function_entry();
 	test_function_body();
 }
+
+void test_get_func_ip_fsession_test(void)
+{
+	struct get_func_ip_fsession_test *skel = NULL;
+	int err;
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
+
+	skel = get_func_ip_fsession_test__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "get_func_ip_fsession_test__open_and_load"))
+		return;
+
+	err = get_func_ip_fsession_test__attach(skel);
+	if (!ASSERT_OK(err, "get_func_ip_fsession_test__attach"))
+		goto cleanup;
+
+	err = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.test1), &topts);
+	ASSERT_OK(err, "test_run");
+	ASSERT_EQ(topts.retval, 0, "test_run");
+
+	ASSERT_EQ(skel->bss->test1_entry_result, 1, "test1_entry_result");
+	ASSERT_EQ(skel->bss->test1_exit_result, 1, "test1_exit_result");
+
+cleanup:
+	get_func_ip_fsession_test__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/get_func_args_fsession_test.c b/tools/testing/selftests/bpf/progs/get_func_args_fsession_test.c
new file mode 100644
index 000000000000..bb597f24b659
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/get_func_args_fsession_test.c
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <errno.h>
+
+char _license[] SEC("license") = "GPL";
+
+__u64 test1_result = 0;
+
+SEC("fsession/bpf_fentry_test1")
+int BPF_PROG(test1)
+{
+	__u64 cnt = bpf_get_func_arg_cnt(ctx);
+	__u64 a = 0, z = 0, ret = 0;
+	__s64 err;
+
+	test1_result = cnt == 1;
+
+	/* valid arguments */
+	err = bpf_get_func_arg(ctx, 0, &a);
+	test1_result &= err == 0 && ((int) a == 1);
+
+	/* not valid argument */
+	err = bpf_get_func_arg(ctx, 1, &z);
+	test1_result &= err == -EINVAL;
+
+	if (bpf_session_is_return(ctx)) {
+		err = bpf_get_func_ret(ctx, &ret);
+		test1_result &= err == 0 && ret == 2;
+	} else {
+		err = bpf_get_func_ret(ctx, &ret);
+		test1_result &= err == 0 && ret == 0;
+	}
+
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/get_func_args_test.c b/tools/testing/selftests/bpf/progs/get_func_args_test.c
index 075a1180ec26..1bf47f64d096 100644
--- a/tools/testing/selftests/bpf/progs/get_func_args_test.c
+++ b/tools/testing/selftests/bpf/progs/get_func_args_test.c
@@ -165,41 +165,3 @@ int BPF_PROG(tp_test2)
 
 	return 0;
 }
-
-__u64 test7_result = 0;
-#if defined(bpf_target_x86) || defined(bpf_target_arm64) || defined(bpf_target_riscv)
-SEC("fsession/bpf_fentry_test1")
-int BPF_PROG(test7)
-{
-	__u64 cnt = bpf_get_func_arg_cnt(ctx);
-	__u64 a = 0, z = 0, ret = 0;
-	__s64 err;
-
-	test7_result = cnt == 1;
-
-	/* valid arguments */
-	err = bpf_get_func_arg(ctx, 0, &a);
-	test7_result &= err == 0 && ((int) a == 1);
-
-	/* not valid argument */
-	err = bpf_get_func_arg(ctx, 1, &z);
-	test7_result &= err == -EINVAL;
-
-	if (bpf_session_is_return(ctx)) {
-		err = bpf_get_func_ret(ctx, &ret);
-		test7_result &= err == 0 && ret == 2;
-	} else {
-		err = bpf_get_func_ret(ctx, &ret);
-		test7_result &= err == 0 && ret == 0;
-	}
-
-	return 0;
-}
-#else
-SEC("fentry/bpf_fentry_test1")
-int BPF_PROG(test7)
-{
-	test7_result = 1;
-	return 0;
-}
-#endif
diff --git a/tools/testing/selftests/bpf/progs/get_func_ip_fsession_test.c b/tools/testing/selftests/bpf/progs/get_func_ip_fsession_test.c
new file mode 100644
index 000000000000..bbeea0d512e3
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/get_func_ip_fsession_test.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+__u64 test1_entry_result = 0;
+__u64 test1_exit_result = 0;
+
+SEC("fsession/bpf_fentry_test1")
+int BPF_PROG(test1, int a)
+{
+	__u64 addr = bpf_get_func_ip(ctx);
+
+	if (bpf_session_is_return(ctx))
+		test1_exit_result = (const void *) addr == &bpf_fentry_test1;
+	else
+		test1_entry_result = (const void *) addr == &bpf_fentry_test1;
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/get_func_ip_test.c b/tools/testing/selftests/bpf/progs/get_func_ip_test.c
index 45eaa54d1ac7..2011cacdeb18 100644
--- a/tools/testing/selftests/bpf/progs/get_func_ip_test.c
+++ b/tools/testing/selftests/bpf/progs/get_func_ip_test.c
@@ -103,26 +103,3 @@ int BPF_URETPROBE(test8, int ret)
 	test8_result = (const void *) addr == (const void *) uprobe_trigger;
 	return 0;
 }
-
-__u64 test9_entry_result = 0;
-__u64 test9_exit_result = 0;
-#if defined(bpf_target_x86) || defined(bpf_target_arm64) || defined(bpf_target_riscv)
-SEC("fsession/bpf_fentry_test1")
-int BPF_PROG(test9, int a)
-{
-	__u64 addr = bpf_get_func_ip(ctx);
-
-	if (bpf_session_is_return(ctx))
-		test9_exit_result = (const void *) addr == &bpf_fentry_test1;
-	else
-		test9_entry_result = (const void *) addr == &bpf_fentry_test1;
-	return 0;
-}
-#else
-SEC("fentry/bpf_fentry_test1")
-int BPF_PROG(test9, int a)
-{
-	test9_entry_result = test9_exit_result = 1;
-	return 0;
-}
-#endif
-- 
2.53.0

Re: [PATCH bpf-next 3/3] selftests/bpf: factor out get_func_* tests for fsession

[Ilya Leoshkevich]

On 2/23/26 09:40, Menglong Dong wrote:
> The fsession is already supported by x86_64, arm64, riscv and s390, so we
> don't need to disable it in the compile time according to the
> architecture. Factor out the testings for it. Therefore, the testing can
> be disabled for the architecture that doesn't support it manually.
>
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> ---
>   .../bpf/prog_tests/get_func_args_test.c       | 25 +++++++++++-
>   .../bpf/prog_tests/get_func_ip_test.c         | 28 +++++++++++++-
>   .../bpf/progs/get_func_args_fsession_test.c   | 37 ++++++++++++++++++
>   .../selftests/bpf/progs/get_func_args_test.c  | 38 -------------------
>   .../bpf/progs/get_func_ip_fsession_test.c     | 21 ++++++++++
>   .../selftests/bpf/progs/get_func_ip_test.c    | 23 -----------
>   6 files changed, 108 insertions(+), 64 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/progs/get_func_args_fsession_test.c
>   create mode 100644 tools/testing/selftests/bpf/progs/get_func_ip_fsession_test.c


I personally like the "let the test fail on unsupported architectures" 
philosophy, since it lets architecture maintainers notice missing 
features more easily and take action (even if it's just adding the 
respective test to DENYLIST), so:


Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>