[PATCH v2 1/2] powerpc/bpf: ensure module addresses are supported

[PATCH v2 1/2] powerpc/bpf: ensure module addresses are supported

[Hari Bathini]

Currently, bpf jit code on powerpc assumes all the bpf functions and
helpers to be kernel text. This is false for kfunc case, as function
addresses are mostly module addresses in that case. Ensure module
addresses are supported to enable kfunc support.

Assume kernel text address for programs with no kfunc call to optimize
instruction sequence in that case. Add a check to error out if this
assumption ever changes in the future.

Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
---

Changes in v2:
* Using bpf_prog_has_kfunc_call() to decide whether to use optimized
  instruction sequence or not as suggested by Naveen.


 arch/powerpc/net/bpf_jit.h        |   5 +-
 arch/powerpc/net/bpf_jit_comp.c   |   4 +-
 arch/powerpc/net/bpf_jit_comp32.c |   8 ++-
 arch/powerpc/net/bpf_jit_comp64.c | 109 ++++++++++++++++++++++++------
 4 files changed, 97 insertions(+), 29 deletions(-)

diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h
index cdea5dccaefe..fc56ee0ee9c5 100644
--- a/arch/powerpc/net/bpf_jit.h
+++ b/arch/powerpc/net/bpf_jit.h
@@ -160,10 +160,11 @@ static inline void bpf_clear_seen_register(struct codegen_context *ctx, int i)
 }
 
 void bpf_jit_init_reg_mapping(struct codegen_context *ctx);
-int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func);
+int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func,
+			       bool has_kfunc_call);
 int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct codegen_context *ctx,
 		       u32 *addrs, int pass, bool extra_pass);
-void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx);
+void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx, bool has_kfunc_call);
 void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx);
 void bpf_jit_realloc_regs(struct codegen_context *ctx);
 int bpf_jit_emit_exit_insn(u32 *image, struct codegen_context *ctx, int tmp_reg, long exit_addr);
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index 0f9a21783329..7b4103b4c929 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -163,7 +163,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
 	 * update ctgtx.idx as it pretends to output instructions, then we can
 	 * calculate total size from idx.
 	 */
-	bpf_jit_build_prologue(NULL, &cgctx);
+	bpf_jit_build_prologue(NULL, &cgctx, bpf_prog_has_kfunc_call(fp));
 	addrs[fp->len] = cgctx.idx * 4;
 	bpf_jit_build_epilogue(NULL, &cgctx);
 
@@ -192,7 +192,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
 		/* Now build the prologue, body code & epilogue for real. */
 		cgctx.idx = 0;
 		cgctx.alt_exit_addr = 0;
-		bpf_jit_build_prologue(code_base, &cgctx);
+		bpf_jit_build_prologue(code_base, &cgctx, bpf_prog_has_kfunc_call(fp));
 		if (bpf_jit_build_body(fp, code_base, fcode_base, &cgctx, addrs, pass,
 				       extra_pass)) {
 			bpf_arch_text_copy(&fhdr->size, &hdr->size, sizeof(hdr->size));
diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
index 2f39c50ca729..447747e51a58 100644
--- a/arch/powerpc/net/bpf_jit_comp32.c
+++ b/arch/powerpc/net/bpf_jit_comp32.c
@@ -123,7 +123,7 @@ void bpf_jit_realloc_regs(struct codegen_context *ctx)
 	}
 }
 
-void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx)
+void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx, bool has_kfunc_call)
 {
 	int i;
 
@@ -201,7 +201,8 @@ void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)
 }
 
 /* Relative offset needs to be calculated based on final image location */
-int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func)
+int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func,
+			       bool has_kfunc_call)
 {
 	s32 rel = (s32)func - (s32)(fimage + ctx->idx);
 
@@ -1054,7 +1055,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code
 				EMIT(PPC_RAW_STW(bpf_to_ppc(BPF_REG_5), _R1, 12));
 			}
 
-			ret = bpf_jit_emit_func_call_rel(image, fimage, ctx, func_addr);
+			ret = bpf_jit_emit_func_call_rel(image, fimage, ctx, func_addr,
+							 bpf_prog_has_kfunc_call(fp));
 			if (ret)
 				return ret;
 
diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
index 79f23974a320..385a5df1670c 100644
--- a/arch/powerpc/net/bpf_jit_comp64.c
+++ b/arch/powerpc/net/bpf_jit_comp64.c
@@ -122,12 +122,17 @@ void bpf_jit_realloc_regs(struct codegen_context *ctx)
 {
 }
 
-void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx)
+void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx, bool has_kfunc_call)
 {
 	int i;
 
 #ifndef CONFIG_PPC_KERNEL_PCREL
-	if (IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
+	/*
+	 * If the program doesn't have a kfunc call, all BPF helpers are part of kernel text
+	 * and all BPF programs/functions utilize the kernel TOC. So, optimize the
+	 * instruction sequence by using kernel toc in r2 for that case.
+	 */
+	if (!has_kfunc_call && IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
 		EMIT(PPC_RAW_LD(_R2, _R13, offsetof(struct paca_struct, kernel_toc)));
 #endif
 
@@ -202,12 +207,17 @@ void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)
 	EMIT(PPC_RAW_BLR());
 }
 
-static int bpf_jit_emit_func_call_hlp(u32 *image, struct codegen_context *ctx, u64 func)
+static int bpf_jit_emit_func_call_hlp(u32 *image, struct codegen_context *ctx, u64 func,
+				      bool has_kfunc_call)
 {
 	unsigned long func_addr = func ? ppc_function_entry((void *)func) : 0;
 	long reladdr;
 
-	if (WARN_ON_ONCE(!core_kernel_text(func_addr)))
+	/*
+	 * If the program doesn't have a kfunc call, all BPF helpers are assumed to be part of
+	 * kernel text. Don't proceed if that assumption ever changes in future.
+	 */
+	if (!has_kfunc_call && WARN_ON_ONCE(!core_kernel_text(func_addr)))
 		return -EINVAL;
 
 	if (IS_ENABLED(CONFIG_PPC_KERNEL_PCREL)) {
@@ -225,30 +235,55 @@ static int bpf_jit_emit_func_call_hlp(u32 *image, struct codegen_context *ctx, u
 		EMIT(PPC_RAW_BCTR());
 
 	} else {
-		reladdr = func_addr - kernel_toc_addr();
-		if (reladdr > 0x7FFFFFFF || reladdr < -(0x80000000L)) {
-			pr_err("eBPF: address of %ps out of range of kernel_toc.\n", (void *)func);
-			return -ERANGE;
-		}
+		if (has_kfunc_call) {
+#ifdef PPC64_ELF_ABI_v1
+			/* func points to the function descriptor */
+			PPC_LI64(b2p[TMP_REG_2], func);
+			/* Load actual entry point from function descriptor */
+			PPC_BPF_LL(b2p[TMP_REG_1], b2p[TMP_REG_2], 0);
+			/* ... and move it to CTR */
+			EMIT(PPC_RAW_MTCTR(b2p[TMP_REG_1]));
+			/*
+			 * Load TOC from function descriptor at offset 8.
+			 * We can clobber r2 since we get called through a
+			 * function pointer (so caller will save/restore r2)
+			 * and since we don't use a TOC ourself.
+			 */
+			PPC_BPF_LL(2, b2p[TMP_REG_2], 8);
+#else
+			/* We can clobber r12 */
+			PPC_LI64(12, func);
+			EMIT(PPC_RAW_MTCTR(12));
+#endif
+		} else {
+			reladdr = func_addr - kernel_toc_addr();
+			if (reladdr > 0x7FFFFFFF || reladdr < -(0x80000000L)) {
+				pr_err("eBPF: address of %ps out of range of kernel_toc.\n",
+				       (void *)func);
+				return -ERANGE;
+			}
 
-		EMIT(PPC_RAW_ADDIS(_R12, _R2, PPC_HA(reladdr)));
-		EMIT(PPC_RAW_ADDI(_R12, _R12, PPC_LO(reladdr)));
-		EMIT(PPC_RAW_MTCTR(_R12));
+			EMIT(PPC_RAW_ADDIS(_R12, _R2, PPC_HA(reladdr)));
+			EMIT(PPC_RAW_ADDI(_R12, _R12, PPC_LO(reladdr)));
+			EMIT(PPC_RAW_MTCTR(_R12));
+		}
 		EMIT(PPC_RAW_BCTRL());
 	}
 
 	return 0;
 }
 
-int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func)
+int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func,
+			       bool has_kfunc_call)
 {
 	unsigned int i, ctx_idx = ctx->idx;
 
-	if (WARN_ON_ONCE(func && is_module_text_address(func)))
+	if (WARN_ON_ONCE(func && !has_kfunc_call && is_module_text_address(func)))
 		return -EINVAL;
 
 	/* skip past descriptor if elf v1 */
-	func += FUNCTION_DESCR_SIZE;
+	if (!has_kfunc_call)
+		func += FUNCTION_DESCR_SIZE;
 
 	/* Load function address into r12 */
 	PPC_LI64(_R12, func);
@@ -267,13 +302,28 @@ int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *
 		for (i = ctx->idx - ctx_idx; i < 5; i++)
 			EMIT(PPC_RAW_NOP());
 
+#ifdef PPC64_ELF_ABI_v1
+	if (has_kfunc_call) {
+		/*
+		 * Load TOC from function descriptor at offset 8.
+		 * We can clobber r2 since we get called through a
+		 * function pointer (so caller will save/restore r2)
+		 * and since we don't use a TOC ourself.
+		 */
+		PPC_BPF_LL(2, 12, 8);
+		/* Load actual entry point from function descriptor */
+		PPC_BPF_LL(12, 12, 0);
+	}
+#endif
+
 	EMIT(PPC_RAW_MTCTR(_R12));
 	EMIT(PPC_RAW_BCTRL());
 
 	return 0;
 }
 
-static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 out)
+static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 out,
+				  bool has_kfunc_call)
 {
 	/*
 	 * By now, the eBPF program has already setup parameters in r3, r4 and r5
@@ -285,7 +335,7 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o
 	int b2p_index = bpf_to_ppc(BPF_REG_3);
 	int bpf_tailcall_prologue_size = 8;
 
-	if (IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
+	if (!has_kfunc_call && IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
 		bpf_tailcall_prologue_size += 4; /* skip past the toc load */
 
 	/*
@@ -325,8 +375,20 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o
 
 	/* goto *(prog->bpf_func + prologue_size); */
 	EMIT(PPC_RAW_LD(bpf_to_ppc(TMP_REG_1), bpf_to_ppc(TMP_REG_1), offsetof(struct bpf_prog, bpf_func)));
-	EMIT(PPC_RAW_ADDI(bpf_to_ppc(TMP_REG_1), bpf_to_ppc(TMP_REG_1),
-			FUNCTION_DESCR_SIZE + bpf_tailcall_prologue_size));
+	if (has_kfunc_call) {
+#ifdef PPC64_ELF_ABI_v1
+		/* skip past the function descriptor */
+		EMIT(PPC_RAW_ADDI(bpf_to_ppc(TMP_REG_1), bpf_to_ppc(TMP_REG_1),
+				FUNCTION_DESCR_SIZE + bpf_tailcall_prologue_size));
+#else
+		EMIT(PPC_RAW_ADDI(bpf_to_ppc(TMP_REG_1), bpf_to_ppc(TMP_REG_1),
+				bpf_tailcall_prologue_size));
+#endif
+	} else {
+		EMIT(PPC_RAW_ADDI(bpf_to_ppc(TMP_REG_1), bpf_to_ppc(TMP_REG_1),
+				FUNCTION_DESCR_SIZE + bpf_tailcall_prologue_size));
+	}
+
 	EMIT(PPC_RAW_MTCTR(bpf_to_ppc(TMP_REG_1)));
 
 	/* tear down stack, restore NVRs, ... */
@@ -365,6 +427,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code
 		       u32 *addrs, int pass, bool extra_pass)
 {
 	enum stf_barrier_type stf_barrier = stf_barrier_type_get();
+	bool has_kfunc_call = bpf_prog_has_kfunc_call(fp);
 	const struct bpf_insn *insn = fp->insnsi;
 	int flen = fp->len;
 	int i, ret;
@@ -993,9 +1056,11 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code
 				return ret;
 
 			if (func_addr_fixed)
-				ret = bpf_jit_emit_func_call_hlp(image, ctx, func_addr);
+				ret = bpf_jit_emit_func_call_hlp(image, ctx, func_addr,
+								 has_kfunc_call);
 			else
-				ret = bpf_jit_emit_func_call_rel(image, fimage, ctx, func_addr);
+				ret = bpf_jit_emit_func_call_rel(image, fimage, ctx, func_addr,
+								 has_kfunc_call);
 
 			if (ret)
 				return ret;
@@ -1204,7 +1269,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code
 		 */
 		case BPF_JMP | BPF_TAIL_CALL:
 			ctx->seen |= SEEN_TAILCALL;
-			ret = bpf_jit_emit_tail_call(image, ctx, addrs[i + 1]);
+			ret = bpf_jit_emit_tail_call(image, ctx, addrs[i + 1], has_kfunc_call);
 			if (ret < 0)
 				return ret;
 			break;
-- 
2.43.0

[PATCH v2 2/2] powerpc/bpf: enable kfunc call

[Hari Bathini]

With module addresses supported, override bpf_jit_supports_kfunc_call()
to enable kfunc support. Module address offsets can be more than 32-bit
long, so override bpf_jit_supports_far_kfunc_call() to enable 64-bit
pointers.

Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
---

* No changes since v1.


 arch/powerpc/net/bpf_jit_comp.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index 7b4103b4c929..f896a4213696 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -359,3 +359,13 @@ void bpf_jit_free(struct bpf_prog *fp)
 
 	bpf_prog_unlock_free(fp);
 }
+
+bool bpf_jit_supports_kfunc_call(void)
+{
+	return true;
+}
+
+bool bpf_jit_supports_far_kfunc_call(void)
+{
+	return true;
+}
-- 
2.43.0

Re: [PATCH v2 1/2] powerpc/bpf: ensure module addresses are supported

[Christophe Leroy]

Le 01/02/2024 à 18:12, Hari Bathini a écrit :
> Currently, bpf jit code on powerpc assumes all the bpf functions and
> helpers to be kernel text. This is false for kfunc case, as function
> addresses are mostly module addresses in that case. Ensure module
> addresses are supported to enable kfunc support.
> 
> Assume kernel text address for programs with no kfunc call to optimize
> instruction sequence in that case. Add a check to error out if this
> assumption ever changes in the future.
> 
> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
> ---
> 
> Changes in v2:
> * Using bpf_prog_has_kfunc_call() to decide whether to use optimized
>    instruction sequence or not as suggested by Naveen.
> 
> 
>   arch/powerpc/net/bpf_jit.h        |   5 +-
>   arch/powerpc/net/bpf_jit_comp.c   |   4 +-
>   arch/powerpc/net/bpf_jit_comp32.c |   8 ++-
>   arch/powerpc/net/bpf_jit_comp64.c | 109 ++++++++++++++++++++++++------
>   4 files changed, 97 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h
> index cdea5dccaefe..fc56ee0ee9c5 100644
> --- a/arch/powerpc/net/bpf_jit.h
> +++ b/arch/powerpc/net/bpf_jit.h
> @@ -160,10 +160,11 @@ static inline void bpf_clear_seen_register(struct codegen_context *ctx, int i)
>   }
>   
>   void bpf_jit_init_reg_mapping(struct codegen_context *ctx);
> -int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func);
> +int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func,
> +			       bool has_kfunc_call);
>   int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct codegen_context *ctx,
>   		       u32 *addrs, int pass, bool extra_pass);
> -void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx);
> +void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx, bool has_kfunc_call);
>   void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx);
>   void bpf_jit_realloc_regs(struct codegen_context *ctx);
>   int bpf_jit_emit_exit_insn(u32 *image, struct codegen_context *ctx, int tmp_reg, long exit_addr);
> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index 0f9a21783329..7b4103b4c929 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
> @@ -163,7 +163,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>   	 * update ctgtx.idx as it pretends to output instructions, then we can
>   	 * calculate total size from idx.
>   	 */
> -	bpf_jit_build_prologue(NULL, &cgctx);
> +	bpf_jit_build_prologue(NULL, &cgctx, bpf_prog_has_kfunc_call(fp));
>   	addrs[fp->len] = cgctx.idx * 4;
>   	bpf_jit_build_epilogue(NULL, &cgctx);
>   
> @@ -192,7 +192,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>   		/* Now build the prologue, body code & epilogue for real. */
>   		cgctx.idx = 0;
>   		cgctx.alt_exit_addr = 0;
> -		bpf_jit_build_prologue(code_base, &cgctx);
> +		bpf_jit_build_prologue(code_base, &cgctx, bpf_prog_has_kfunc_call(fp));
>   		if (bpf_jit_build_body(fp, code_base, fcode_base, &cgctx, addrs, pass,
>   				       extra_pass)) {
>   			bpf_arch_text_copy(&fhdr->size, &hdr->size, sizeof(hdr->size));
> diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
> index 2f39c50ca729..447747e51a58 100644
> --- a/arch/powerpc/net/bpf_jit_comp32.c
> +++ b/arch/powerpc/net/bpf_jit_comp32.c
> @@ -123,7 +123,7 @@ void bpf_jit_realloc_regs(struct codegen_context *ctx)
>   	}
>   }
>   
> -void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx)
> +void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx, bool has_kfunc_call)
>   {
>   	int i;
>   
> @@ -201,7 +201,8 @@ void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)
>   }
>   
>   /* Relative offset needs to be calculated based on final image location */
> -int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func)
> +int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func,
> +			       bool has_kfunc_call)
>   {
>   	s32 rel = (s32)func - (s32)(fimage + ctx->idx);
>   
> @@ -1054,7 +1055,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code
>   				EMIT(PPC_RAW_STW(bpf_to_ppc(BPF_REG_5), _R1, 12));
>   			}
>   
> -			ret = bpf_jit_emit_func_call_rel(image, fimage, ctx, func_addr);
> +			ret = bpf_jit_emit_func_call_rel(image, fimage, ctx, func_addr,
> +							 bpf_prog_has_kfunc_call(fp));
>   			if (ret)
>   				return ret;
>   
> diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
> index 79f23974a320..385a5df1670c 100644
> --- a/arch/powerpc/net/bpf_jit_comp64.c
> +++ b/arch/powerpc/net/bpf_jit_comp64.c
> @@ -122,12 +122,17 @@ void bpf_jit_realloc_regs(struct codegen_context *ctx)
>   {
>   }
>   
> -void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx)
> +void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx, bool has_kfunc_call)
>   {
>   	int i;
>   
>   #ifndef CONFIG_PPC_KERNEL_PCREL
> -	if (IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
> +	/*
> +	 * If the program doesn't have a kfunc call, all BPF helpers are part of kernel text
> +	 * and all BPF programs/functions utilize the kernel TOC. So, optimize the
> +	 * instruction sequence by using kernel toc in r2 for that case.
> +	 */
> +	if (!has_kfunc_call && IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
>   		EMIT(PPC_RAW_LD(_R2, _R13, offsetof(struct paca_struct, kernel_toc)));
>   #endif
>   
> @@ -202,12 +207,17 @@ void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)
>   	EMIT(PPC_RAW_BLR());
>   }
>   
> -static int bpf_jit_emit_func_call_hlp(u32 *image, struct codegen_context *ctx, u64 func)
> +static int bpf_jit_emit_func_call_hlp(u32 *image, struct codegen_context *ctx, u64 func,
> +				      bool has_kfunc_call)
>   {
>   	unsigned long func_addr = func ? ppc_function_entry((void *)func) : 0;
>   	long reladdr;
>   
> -	if (WARN_ON_ONCE(!core_kernel_text(func_addr)))
> +	/*
> +	 * If the program doesn't have a kfunc call, all BPF helpers are assumed to be part of
> +	 * kernel text. Don't proceed if that assumption ever changes in future.
> +	 */
> +	if (!has_kfunc_call && WARN_ON_ONCE(!core_kernel_text(func_addr)))
>   		return -EINVAL;
>   
>   	if (IS_ENABLED(CONFIG_PPC_KERNEL_PCREL)) {
> @@ -225,30 +235,55 @@ static int bpf_jit_emit_func_call_hlp(u32 *image, struct codegen_context *ctx, u
>   		EMIT(PPC_RAW_BCTR());
>   
>   	} else {
> -		reladdr = func_addr - kernel_toc_addr();
> -		if (reladdr > 0x7FFFFFFF || reladdr < -(0x80000000L)) {
> -			pr_err("eBPF: address of %ps out of range of kernel_toc.\n", (void *)func);
> -			return -ERANGE;
> -		}
> +		if (has_kfunc_call) {
> +#ifdef PPC64_ELF_ABI_v1

I can't see a reason for a #ifdef here, why not use IS_ENABLED() like 
other places ?

> +			/* func points to the function descriptor */
> +			PPC_LI64(b2p[TMP_REG_2], func);
> +			/* Load actual entry point from function descriptor */
> +			PPC_BPF_LL(b2p[TMP_REG_1], b2p[TMP_REG_2], 0);
> +			/* ... and move it to CTR */
> +			EMIT(PPC_RAW_MTCTR(b2p[TMP_REG_1]));
> +			/*
> +			 * Load TOC from function descriptor at offset 8.
> +			 * We can clobber r2 since we get called through a
> +			 * function pointer (so caller will save/restore r2)
> +			 * and since we don't use a TOC ourself.
> +			 */
> +			PPC_BPF_LL(2, b2p[TMP_REG_2], 8);
> +#else
> +			/* We can clobber r12 */
> +			PPC_LI64(12, func);
> +			EMIT(PPC_RAW_MTCTR(12));
> +#endif
> +		} else {
> +			reladdr = func_addr - kernel_toc_addr();
> +			if (reladdr > 0x7FFFFFFF || reladdr < -(0x80000000L)) {
> +				pr_err("eBPF: address of %ps out of range of kernel_toc.\n",
> +				       (void *)func);
> +				return -ERANGE;
> +			}
>   
> -		EMIT(PPC_RAW_ADDIS(_R12, _R2, PPC_HA(reladdr)));
> -		EMIT(PPC_RAW_ADDI(_R12, _R12, PPC_LO(reladdr)));
> -		EMIT(PPC_RAW_MTCTR(_R12));
> +			EMIT(PPC_RAW_ADDIS(_R12, _R2, PPC_HA(reladdr)));
> +			EMIT(PPC_RAW_ADDI(_R12, _R12, PPC_LO(reladdr)));
> +			EMIT(PPC_RAW_MTCTR(_R12));
> +		}
>   		EMIT(PPC_RAW_BCTRL());
>   	}
>   
>   	return 0;
>   }
>   
> -int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func)
> +int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func,
> +			       bool has_kfunc_call)
>   {
>   	unsigned int i, ctx_idx = ctx->idx;
>   
> -	if (WARN_ON_ONCE(func && is_module_text_address(func)))
> +	if (WARN_ON_ONCE(func && !has_kfunc_call && is_module_text_address(func)))
>   		return -EINVAL;
>   
>   	/* skip past descriptor if elf v1 */
> -	func += FUNCTION_DESCR_SIZE;
> +	if (!has_kfunc_call)
> +		func += FUNCTION_DESCR_SIZE;
>   
>   	/* Load function address into r12 */
>   	PPC_LI64(_R12, func);
> @@ -267,13 +302,28 @@ int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *
>   		for (i = ctx->idx - ctx_idx; i < 5; i++)
>   			EMIT(PPC_RAW_NOP());
>   
> +#ifdef PPC64_ELF_ABI_v1

I can't see a reason for a #ifdef here, why not use IS_ENABLED() like 
other places ?


> +	if (has_kfunc_call) {
> +		/*
> +		 * Load TOC from function descriptor at offset 8.
> +		 * We can clobber r2 since we get called through a
> +		 * function pointer (so caller will save/restore r2)
> +		 * and since we don't use a TOC ourself.
> +		 */
> +		PPC_BPF_LL(2, 12, 8);
> +		/* Load actual entry point from function descriptor */
> +		PPC_BPF_LL(12, 12, 0);
> +	}
> +#endif
> +
>   	EMIT(PPC_RAW_MTCTR(_R12));
>   	EMIT(PPC_RAW_BCTRL());
>   
>   	return 0;
>   }
>   
> -static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 out)
> +static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 out,
> +				  bool has_kfunc_call)
>   {
>   	/*
>   	 * By now, the eBPF program has already setup parameters in r3, r4 and r5
> @@ -285,7 +335,7 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o
>   	int b2p_index = bpf_to_ppc(BPF_REG_3);
>   	int bpf_tailcall_prologue_size = 8;
>   
> -	if (IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
> +	if (!has_kfunc_call && IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
>   		bpf_tailcall_prologue_size += 4; /* skip past the toc load */
>   
>   	/*
> @@ -325,8 +375,20 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o
>   
>   	/* goto *(prog->bpf_func + prologue_size); */
>   	EMIT(PPC_RAW_LD(bpf_to_ppc(TMP_REG_1), bpf_to_ppc(TMP_REG_1), offsetof(struct bpf_prog, bpf_func)));
> -	EMIT(PPC_RAW_ADDI(bpf_to_ppc(TMP_REG_1), bpf_to_ppc(TMP_REG_1),
> -			FUNCTION_DESCR_SIZE + bpf_tailcall_prologue_size));
> +	if (has_kfunc_call) {
> +#ifdef PPC64_ELF_ABI_v1

I can't see a reason for a #ifdef here, why not use IS_ENABLED() like 
other places ?

> +		/* skip past the function descriptor */
> +		EMIT(PPC_RAW_ADDI(bpf_to_ppc(TMP_REG_1), bpf_to_ppc(TMP_REG_1),
> +				FUNCTION_DESCR_SIZE + bpf_tailcall_prologue_size));
> +#else
> +		EMIT(PPC_RAW_ADDI(bpf_to_ppc(TMP_REG_1), bpf_to_ppc(TMP_REG_1),
> +				bpf_tailcall_prologue_size));
> +#endif
> +	} else {
> +		EMIT(PPC_RAW_ADDI(bpf_to_ppc(TMP_REG_1), bpf_to_ppc(TMP_REG_1),
> +				FUNCTION_DESCR_SIZE + bpf_tailcall_prologue_size));
> +	}
> +
>   	EMIT(PPC_RAW_MTCTR(bpf_to_ppc(TMP_REG_1)));
>   
>   	/* tear down stack, restore NVRs, ... */
> @@ -365,6 +427,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code
>   		       u32 *addrs, int pass, bool extra_pass)
>   {
>   	enum stf_barrier_type stf_barrier = stf_barrier_type_get();
> +	bool has_kfunc_call = bpf_prog_has_kfunc_call(fp);
>   	const struct bpf_insn *insn = fp->insnsi;
>   	int flen = fp->len;
>   	int i, ret;
> @@ -993,9 +1056,11 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code
>   				return ret;
>   
>   			if (func_addr_fixed)
> -				ret = bpf_jit_emit_func_call_hlp(image, ctx, func_addr);
> +				ret = bpf_jit_emit_func_call_hlp(image, ctx, func_addr,
> +								 has_kfunc_call);

Doesn't this fit on a single line ?

>   			else
> -				ret = bpf_jit_emit_func_call_rel(image, fimage, ctx, func_addr);
> +				ret = bpf_jit_emit_func_call_rel(image, fimage, ctx, func_addr,
> +								 has_kfunc_call);

Same. Nowadays lines up to 100 chars are the norm.

>   
>   			if (ret)
>   				return ret;
> @@ -1204,7 +1269,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code
>   		 */
>   		case BPF_JMP | BPF_TAIL_CALL:
>   			ctx->seen |= SEEN_TAILCALL;
> -			ret = bpf_jit_emit_tail_call(image, ctx, addrs[i + 1]);
> +			ret = bpf_jit_emit_tail_call(image, ctx, addrs[i + 1], has_kfunc_call);
>   			if (ret < 0)
>   				return ret;
>   			break;

Re: [PATCH v2 2/2] powerpc/bpf: enable kfunc call

[Christophe Leroy]

Le 01/02/2024 à 18:12, Hari Bathini a écrit :
> With module addresses supported, override bpf_jit_supports_kfunc_call()
> to enable kfunc support. Module address offsets can be more than 32-bit
> long, so override bpf_jit_supports_far_kfunc_call() to enable 64-bit
> pointers.

What's the impact on PPC32 ? There are no 64-bit pointers on PPC32.

> 
> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
> ---
> 
> * No changes since v1.
> 
> 
>   arch/powerpc/net/bpf_jit_comp.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index 7b4103b4c929..f896a4213696 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
> @@ -359,3 +359,13 @@ void bpf_jit_free(struct bpf_prog *fp)
>   
>   	bpf_prog_unlock_free(fp);
>   }
> +
> +bool bpf_jit_supports_kfunc_call(void)
> +{
> +	return true;
> +}
> +
> +bool bpf_jit_supports_far_kfunc_call(void)
> +{
> +	return true;
> +}

Re: [PATCH v2 2/2] powerpc/bpf: enable kfunc call

[Hari Bathini]

On 13/02/24 1:24 pm, Christophe Leroy wrote:
> 
> 
> Le 01/02/2024 à 18:12, Hari Bathini a écrit :
>> With module addresses supported, override bpf_jit_supports_kfunc_call()
>> to enable kfunc support. Module address offsets can be more than 32-bit
>> long, so override bpf_jit_supports_far_kfunc_call() to enable 64-bit
>> pointers.
> 
> What's the impact on PPC32 ? There are no 64-bit pointers on PPC32.

Yeah. Not required to return true for PPC32 case and probably not a
good thing to claim support for far kfunc calls for PPC32. Changing to:

+bool bpf_jit_supports_far_kfunc_call(void)
+{
+	return IS_ENABLED(CONFIG_PPC64);
+}

>>
>> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
>> ---
>>
>> * No changes since v1.
>>
>>
>>    arch/powerpc/net/bpf_jit_comp.c | 10 ++++++++++
>>    1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
>> index 7b4103b4c929..f896a4213696 100644
>> --- a/arch/powerpc/net/bpf_jit_comp.c
>> +++ b/arch/powerpc/net/bpf_jit_comp.c
>> @@ -359,3 +359,13 @@ void bpf_jit_free(struct bpf_prog *fp)
>>    
>>    	bpf_prog_unlock_free(fp);
>>    }
>> +
>> +bool bpf_jit_supports_kfunc_call(void)
>> +{
>> +	return true;
>> +}
>> +
>> +bool bpf_jit_supports_far_kfunc_call(void)
>> +{
>> +	return true;
>> +}

Re: [PATCH v2 1/2] powerpc/bpf: ensure module addresses are supported

[Naveen N Rao]

On Thu, Feb 01, 2024 at 10:42:48PM +0530, Hari Bathini wrote:
> Currently, bpf jit code on powerpc assumes all the bpf functions and
> helpers to be kernel text. This is false for kfunc case, as function
> addresses are mostly module addresses in that case. Ensure module
> addresses are supported to enable kfunc support.

I don't think that statement is entirely accurate. Our current 
assumptions are still correct in the sense that:
1. all BPF helpers are part of core kernel text, calls to which are 
generated in bpf_jit_emit_func_call_hlp().
2. bpf-to-bpf calls go out to module area where the JIT output of BPF 
subprogs are written to, handled by bpf_jit_emit_func_call_rel().

kfunc calls add another variant where BPF progs can call directly into a 
kernel function (similar to a BPF helper call), except for the fact that 
the said function could be in a kernel module.

> 
> Assume kernel text address for programs with no kfunc call to optimize
> instruction sequence in that case. Add a check to error out if this
> assumption ever changes in the future.
> 
> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
> ---
> 
> Changes in v2:
> * Using bpf_prog_has_kfunc_call() to decide whether to use optimized
>   instruction sequence or not as suggested by Naveen.
> 
> 
>  arch/powerpc/net/bpf_jit.h        |   5 +-
>  arch/powerpc/net/bpf_jit_comp.c   |   4 +-
>  arch/powerpc/net/bpf_jit_comp32.c |   8 ++-
>  arch/powerpc/net/bpf_jit_comp64.c | 109 ++++++++++++++++++++++++------
>  4 files changed, 97 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h
> index cdea5dccaefe..fc56ee0ee9c5 100644
> --- a/arch/powerpc/net/bpf_jit.h
> +++ b/arch/powerpc/net/bpf_jit.h
> @@ -160,10 +160,11 @@ static inline void bpf_clear_seen_register(struct codegen_context *ctx, int i)
>  }
>  
>  void bpf_jit_init_reg_mapping(struct codegen_context *ctx);
> -int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func);
> +int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func,
> +			       bool has_kfunc_call);
>  int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct codegen_context *ctx,
>  		       u32 *addrs, int pass, bool extra_pass);
> -void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx);
> +void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx, bool has_kfunc_call);
>  void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx);
>  void bpf_jit_realloc_regs(struct codegen_context *ctx);
>  int bpf_jit_emit_exit_insn(u32 *image, struct codegen_context *ctx, int tmp_reg, long exit_addr);
> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index 0f9a21783329..7b4103b4c929 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
> @@ -163,7 +163,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>  	 * update ctgtx.idx as it pretends to output instructions, then we can
>  	 * calculate total size from idx.
>  	 */
> -	bpf_jit_build_prologue(NULL, &cgctx);
> +	bpf_jit_build_prologue(NULL, &cgctx, bpf_prog_has_kfunc_call(fp));
>  	addrs[fp->len] = cgctx.idx * 4;
>  	bpf_jit_build_epilogue(NULL, &cgctx);
>  
> @@ -192,7 +192,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>  		/* Now build the prologue, body code & epilogue for real. */
>  		cgctx.idx = 0;
>  		cgctx.alt_exit_addr = 0;
> -		bpf_jit_build_prologue(code_base, &cgctx);
> +		bpf_jit_build_prologue(code_base, &cgctx, bpf_prog_has_kfunc_call(fp));
>  		if (bpf_jit_build_body(fp, code_base, fcode_base, &cgctx, addrs, pass,
>  				       extra_pass)) {
>  			bpf_arch_text_copy(&fhdr->size, &hdr->size, sizeof(hdr->size));
> diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
> index 2f39c50ca729..447747e51a58 100644
> --- a/arch/powerpc/net/bpf_jit_comp32.c
> +++ b/arch/powerpc/net/bpf_jit_comp32.c
> @@ -123,7 +123,7 @@ void bpf_jit_realloc_regs(struct codegen_context *ctx)
>  	}
>  }
>  
> -void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx)
> +void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx, bool has_kfunc_call)
>  {
>  	int i;
>  
> @@ -201,7 +201,8 @@ void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)
>  }
>  
>  /* Relative offset needs to be calculated based on final image location */
> -int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func)
> +int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func,
> +			       bool has_kfunc_call)
>  {
>  	s32 rel = (s32)func - (s32)(fimage + ctx->idx);
>  
> @@ -1054,7 +1055,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code
>  				EMIT(PPC_RAW_STW(bpf_to_ppc(BPF_REG_5), _R1, 12));
>  			}
>  
> -			ret = bpf_jit_emit_func_call_rel(image, fimage, ctx, func_addr);
> +			ret = bpf_jit_emit_func_call_rel(image, fimage, ctx, func_addr,
> +							 bpf_prog_has_kfunc_call(fp));
>  			if (ret)
>  				return ret;
>  
> diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
> index 79f23974a320..385a5df1670c 100644
> --- a/arch/powerpc/net/bpf_jit_comp64.c
> +++ b/arch/powerpc/net/bpf_jit_comp64.c
> @@ -122,12 +122,17 @@ void bpf_jit_realloc_regs(struct codegen_context *ctx)
>  {
>  }
>  
> -void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx)
> +void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx, bool has_kfunc_call)
>  {
>  	int i;
>  
>  #ifndef CONFIG_PPC_KERNEL_PCREL
> -	if (IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
> +	/*
> +	 * If the program doesn't have a kfunc call, all BPF helpers are part of kernel text
> +	 * and all BPF programs/functions utilize the kernel TOC. So, optimize the
> +	 * instruction sequence by using kernel toc in r2 for that case.
> +	 */
> +	if (!has_kfunc_call && IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
>  		EMIT(PPC_RAW_LD(_R2, _R13, offsetof(struct paca_struct, kernel_toc)));

I don't think this is the correct approach. As far as I can see, we can 
still retain the current function calling sequence utilizing TOC even 
when a kfunc call is present. More on that below...

>  #endif
>  
> @@ -202,12 +207,17 @@ void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)
>  	EMIT(PPC_RAW_BLR());
>  }
>  
> -static int bpf_jit_emit_func_call_hlp(u32 *image, struct codegen_context *ctx, u64 func)
> +static int bpf_jit_emit_func_call_hlp(u32 *image, struct codegen_context *ctx, u64 func,
> +				      bool has_kfunc_call)
>  {
>  	unsigned long func_addr = func ? ppc_function_entry((void *)func) : 0;
>  	long reladdr;
>  
> -	if (WARN_ON_ONCE(!core_kernel_text(func_addr)))
> +	/*
> +	 * If the program doesn't have a kfunc call, all BPF helpers are assumed to be part of
> +	 * kernel text. Don't proceed if that assumption ever changes in future.
> +	 */
> +	if (!has_kfunc_call && WARN_ON_ONCE(!core_kernel_text(func_addr)))
>  		return -EINVAL;

Rather than pass around the presence of a kfunc call in a particular BPF 
prog, I think we should be able to update bpf_jit_emit_func_call_hlp() 
to handle module addresses. That way, existing calling sequence can 
continue to be used for BPF helpers and other BPF functions.

bpf_jit_get_func_addr() should return the correct function address for a 
kfunc call. So, all we need here is to check if that address belongs to 
a module and use a different instruction sequence for that:
- for pcrel and elf abiv2, we can simply load the full address using 
  PPC_LI64() and use that, rather than utilizing the TOC. Note that we 
  will need to use the GEP here so the target function can setup its own 
  TOC. For elf abiv2, we will need an additional instruction after the 
  bctrl() to load the kernel toc from paca.
- for elf abiv1, we will need to load the function address and toc value 
  from the function descriptor, followed by restoring kernel toc like 
  above.

For kfunc calls to core kernel functions, we should be able to retain 
existing sequence using TOC.

>  
>  	if (IS_ENABLED(CONFIG_PPC_KERNEL_PCREL)) {
> @@ -225,30 +235,55 @@ static int bpf_jit_emit_func_call_hlp(u32 *image, struct codegen_context *ctx, u
>  		EMIT(PPC_RAW_BCTR());
>  
>  	} else {
> -		reladdr = func_addr - kernel_toc_addr();
> -		if (reladdr > 0x7FFFFFFF || reladdr < -(0x80000000L)) {
> -			pr_err("eBPF: address of %ps out of range of kernel_toc.\n", (void *)func);
> -			return -ERANGE;
> -		}
> +		if (has_kfunc_call) {
> +#ifdef PPC64_ELF_ABI_v1
> +			/* func points to the function descriptor */
> +			PPC_LI64(b2p[TMP_REG_2], func);
> +			/* Load actual entry point from function descriptor */
> +			PPC_BPF_LL(b2p[TMP_REG_1], b2p[TMP_REG_2], 0);
> +			/* ... and move it to CTR */
> +			EMIT(PPC_RAW_MTCTR(b2p[TMP_REG_1]));
> +			/*
> +			 * Load TOC from function descriptor at offset 8.
> +			 * We can clobber r2 since we get called through a
> +			 * function pointer (so caller will save/restore r2)
> +			 * and since we don't use a TOC ourself.
> +			 */
> +			PPC_BPF_LL(2, b2p[TMP_REG_2], 8);
> +#else
> +			/* We can clobber r12 */
> +			PPC_LI64(12, func);
> +			EMIT(PPC_RAW_MTCTR(12));
> +#endif
> +		} else {
> +			reladdr = func_addr - kernel_toc_addr();
> +			if (reladdr > 0x7FFFFFFF || reladdr < -(0x80000000L)) {
> +				pr_err("eBPF: address of %ps out of range of kernel_toc.\n",
> +				       (void *)func);
> +				return -ERANGE;
> +			}
>  
> -		EMIT(PPC_RAW_ADDIS(_R12, _R2, PPC_HA(reladdr)));
> -		EMIT(PPC_RAW_ADDI(_R12, _R12, PPC_LO(reladdr)));
> -		EMIT(PPC_RAW_MTCTR(_R12));
> +			EMIT(PPC_RAW_ADDIS(_R12, _R2, PPC_HA(reladdr)));
> +			EMIT(PPC_RAW_ADDI(_R12, _R12, PPC_LO(reladdr)));
> +			EMIT(PPC_RAW_MTCTR(_R12));
> +		}
>  		EMIT(PPC_RAW_BCTRL());
>  	}
>  
>  	return 0;
>  }
>  
> -int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func)
> +int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func,
> +			       bool has_kfunc_call)
>  {

This function shouldn't need any changes since this is only for calling 
other bpf progs.

- Naveen

Re: [PATCH v2 1/2] powerpc/bpf: ensure module addresses are supported

[Hari Bathini]

On 13/02/24 1:23 pm, Christophe Leroy wrote:
> 
> 
> Le 01/02/2024 à 18:12, Hari Bathini a écrit :
>> Currently, bpf jit code on powerpc assumes all the bpf functions and
>> helpers to be kernel text. This is false for kfunc case, as function
>> addresses are mostly module addresses in that case. Ensure module
>> addresses are supported to enable kfunc support.
>>
>> Assume kernel text address for programs with no kfunc call to optimize
>> instruction sequence in that case. Add a check to error out if this
>> assumption ever changes in the future.
>>
>> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
>> ---
>>
>> Changes in v2:
>> * Using bpf_prog_has_kfunc_call() to decide whether to use optimized
>>     instruction sequence or not as suggested by Naveen.
>>
>>
>>    arch/powerpc/net/bpf_jit.h        |   5 +-
>>    arch/powerpc/net/bpf_jit_comp.c   |   4 +-
>>    arch/powerpc/net/bpf_jit_comp32.c |   8 ++-
>>    arch/powerpc/net/bpf_jit_comp64.c | 109 ++++++++++++++++++++++++------
>>    4 files changed, 97 insertions(+), 29 deletions(-)
>>
>> diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h
>> index cdea5dccaefe..fc56ee0ee9c5 100644
>> --- a/arch/powerpc/net/bpf_jit.h
>> +++ b/arch/powerpc/net/bpf_jit.h
>> @@ -160,10 +160,11 @@ static inline void bpf_clear_seen_register(struct codegen_context *ctx, int i)
>>    }
>>    
>>    void bpf_jit_init_reg_mapping(struct codegen_context *ctx);
>> -int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func);
>> +int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func,
>> +			       bool has_kfunc_call);
>>    int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct codegen_context *ctx,
>>    		       u32 *addrs, int pass, bool extra_pass);
>> -void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx);
>> +void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx, bool has_kfunc_call);
>>    void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx);
>>    void bpf_jit_realloc_regs(struct codegen_context *ctx);
>>    int bpf_jit_emit_exit_insn(u32 *image, struct codegen_context *ctx, int tmp_reg, long exit_addr);
>> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
>> index 0f9a21783329..7b4103b4c929 100644
>> --- a/arch/powerpc/net/bpf_jit_comp.c
>> +++ b/arch/powerpc/net/bpf_jit_comp.c
>> @@ -163,7 +163,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>>    	 * update ctgtx.idx as it pretends to output instructions, then we can
>>    	 * calculate total size from idx.
>>    	 */
>> -	bpf_jit_build_prologue(NULL, &cgctx);
>> +	bpf_jit_build_prologue(NULL, &cgctx, bpf_prog_has_kfunc_call(fp));
>>    	addrs[fp->len] = cgctx.idx * 4;
>>    	bpf_jit_build_epilogue(NULL, &cgctx);
>>    
>> @@ -192,7 +192,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>>    		/* Now build the prologue, body code & epilogue for real. */
>>    		cgctx.idx = 0;
>>    		cgctx.alt_exit_addr = 0;
>> -		bpf_jit_build_prologue(code_base, &cgctx);
>> +		bpf_jit_build_prologue(code_base, &cgctx, bpf_prog_has_kfunc_call(fp));
>>    		if (bpf_jit_build_body(fp, code_base, fcode_base, &cgctx, addrs, pass,
>>    				       extra_pass)) {
>>    			bpf_arch_text_copy(&fhdr->size, &hdr->size, sizeof(hdr->size));
>> diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
>> index 2f39c50ca729..447747e51a58 100644
>> --- a/arch/powerpc/net/bpf_jit_comp32.c
>> +++ b/arch/powerpc/net/bpf_jit_comp32.c
>> @@ -123,7 +123,7 @@ void bpf_jit_realloc_regs(struct codegen_context *ctx)
>>    	}
>>    }
>>    
>> -void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx)
>> +void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx, bool has_kfunc_call)
>>    {
>>    	int i;
>>    
>> @@ -201,7 +201,8 @@ void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)
>>    }
>>    
>>    /* Relative offset needs to be calculated based on final image location */
>> -int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func)
>> +int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func,
>> +			       bool has_kfunc_call)
>>    {
>>    	s32 rel = (s32)func - (s32)(fimage + ctx->idx);
>>    
>> @@ -1054,7 +1055,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code
>>    				EMIT(PPC_RAW_STW(bpf_to_ppc(BPF_REG_5), _R1, 12));
>>    			}
>>    
>> -			ret = bpf_jit_emit_func_call_rel(image, fimage, ctx, func_addr);
>> +			ret = bpf_jit_emit_func_call_rel(image, fimage, ctx, func_addr,
>> +							 bpf_prog_has_kfunc_call(fp));
>>    			if (ret)
>>    				return ret;
>>    
>> diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
>> index 79f23974a320..385a5df1670c 100644
>> --- a/arch/powerpc/net/bpf_jit_comp64.c
>> +++ b/arch/powerpc/net/bpf_jit_comp64.c
>> @@ -122,12 +122,17 @@ void bpf_jit_realloc_regs(struct codegen_context *ctx)
>>    {
>>    }
>>    
>> -void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx)
>> +void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx, bool has_kfunc_call)
>>    {
>>    	int i;
>>    
>>    #ifndef CONFIG_PPC_KERNEL_PCREL
>> -	if (IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
>> +	/*
>> +	 * If the program doesn't have a kfunc call, all BPF helpers are part of kernel text
>> +	 * and all BPF programs/functions utilize the kernel TOC. So, optimize the
>> +	 * instruction sequence by using kernel toc in r2 for that case.
>> +	 */
>> +	if (!has_kfunc_call && IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
>>    		EMIT(PPC_RAW_LD(_R2, _R13, offsetof(struct paca_struct, kernel_toc)));
>>    #endif
>>    
>> @@ -202,12 +207,17 @@ void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx)
>>    	EMIT(PPC_RAW_BLR());
>>    }
>>    
>> -static int bpf_jit_emit_func_call_hlp(u32 *image, struct codegen_context *ctx, u64 func)
>> +static int bpf_jit_emit_func_call_hlp(u32 *image, struct codegen_context *ctx, u64 func,
>> +				      bool has_kfunc_call)
>>    {
>>    	unsigned long func_addr = func ? ppc_function_entry((void *)func) : 0;
>>    	long reladdr;
>>    
>> -	if (WARN_ON_ONCE(!core_kernel_text(func_addr)))
>> +	/*
>> +	 * If the program doesn't have a kfunc call, all BPF helpers are assumed to be part of
>> +	 * kernel text. Don't proceed if that assumption ever changes in future.
>> +	 */
>> +	if (!has_kfunc_call && WARN_ON_ONCE(!core_kernel_text(func_addr)))
>>    		return -EINVAL;
>>    
>>    	if (IS_ENABLED(CONFIG_PPC_KERNEL_PCREL)) {
>> @@ -225,30 +235,55 @@ static int bpf_jit_emit_func_call_hlp(u32 *image, struct codegen_context *ctx, u
>>    		EMIT(PPC_RAW_BCTR());
>>    
>>    	} else {
>> -		reladdr = func_addr - kernel_toc_addr();
>> -		if (reladdr > 0x7FFFFFFF || reladdr < -(0x80000000L)) {
>> -			pr_err("eBPF: address of %ps out of range of kernel_toc.\n", (void *)func);
>> -			return -ERANGE;
>> -		}
>> +		if (has_kfunc_call) {
>> +#ifdef PPC64_ELF_ABI_v1
> 
> I can't see a reason for a #ifdef here, why not use IS_ENABLED() like
> other places ?
> 
>> +			/* func points to the function descriptor */
>> +			PPC_LI64(b2p[TMP_REG_2], func);
>> +			/* Load actual entry point from function descriptor */
>> +			PPC_BPF_LL(b2p[TMP_REG_1], b2p[TMP_REG_2], 0);
>> +			/* ... and move it to CTR */
>> +			EMIT(PPC_RAW_MTCTR(b2p[TMP_REG_1]));
>> +			/*
>> +			 * Load TOC from function descriptor at offset 8.
>> +			 * We can clobber r2 since we get called through a
>> +			 * function pointer (so caller will save/restore r2)
>> +			 * and since we don't use a TOC ourself.
>> +			 */
>> +			PPC_BPF_LL(2, b2p[TMP_REG_2], 8);
>> +#else
>> +			/* We can clobber r12 */
>> +			PPC_LI64(12, func);
>> +			EMIT(PPC_RAW_MTCTR(12));
>> +#endif
>> +		} else {
>> +			reladdr = func_addr - kernel_toc_addr();
>> +			if (reladdr > 0x7FFFFFFF || reladdr < -(0x80000000L)) {
>> +				pr_err("eBPF: address of %ps out of range of kernel_toc.\n",
>> +				       (void *)func);
>> +				return -ERANGE;
>> +			}
>>    
>> -		EMIT(PPC_RAW_ADDIS(_R12, _R2, PPC_HA(reladdr)));
>> -		EMIT(PPC_RAW_ADDI(_R12, _R12, PPC_LO(reladdr)));
>> -		EMIT(PPC_RAW_MTCTR(_R12));
>> +			EMIT(PPC_RAW_ADDIS(_R12, _R2, PPC_HA(reladdr)));
>> +			EMIT(PPC_RAW_ADDI(_R12, _R12, PPC_LO(reladdr)));
>> +			EMIT(PPC_RAW_MTCTR(_R12));
>> +		}
>>    		EMIT(PPC_RAW_BCTRL());
>>    	}
>>    
>>    	return 0;
>>    }
>>    
>> -int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func)
>> +int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *ctx, u64 func,
>> +			       bool has_kfunc_call)
>>    {
>>    	unsigned int i, ctx_idx = ctx->idx;
>>    
>> -	if (WARN_ON_ONCE(func && is_module_text_address(func)))
>> +	if (WARN_ON_ONCE(func && !has_kfunc_call && is_module_text_address(func)))
>>    		return -EINVAL;
>>    
>>    	/* skip past descriptor if elf v1 */
>> -	func += FUNCTION_DESCR_SIZE;
>> +	if (!has_kfunc_call)
>> +		func += FUNCTION_DESCR_SIZE;
>>    
>>    	/* Load function address into r12 */
>>    	PPC_LI64(_R12, func);
>> @@ -267,13 +302,28 @@ int bpf_jit_emit_func_call_rel(u32 *image, u32 *fimage, struct codegen_context *
>>    		for (i = ctx->idx - ctx_idx; i < 5; i++)
>>    			EMIT(PPC_RAW_NOP());
>>    
>> +#ifdef PPC64_ELF_ABI_v1
> 
> I can't see a reason for a #ifdef here, why not use IS_ENABLED() like
> other places ?

Will update.

> 
> 
>> +	if (has_kfunc_call) {
>> +		/*
>> +		 * Load TOC from function descriptor at offset 8.
>> +		 * We can clobber r2 since we get called through a
>> +		 * function pointer (so caller will save/restore r2)
>> +		 * and since we don't use a TOC ourself.
>> +		 */
>> +		PPC_BPF_LL(2, 12, 8);
>> +		/* Load actual entry point from function descriptor */
>> +		PPC_BPF_LL(12, 12, 0);
>> +	}
>> +#endif
>> +
>>    	EMIT(PPC_RAW_MTCTR(_R12));
>>    	EMIT(PPC_RAW_BCTRL());
>>    
>>    	return 0;
>>    }
>>    
>> -static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 out)
>> +static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 out,
>> +				  bool has_kfunc_call)
>>    {
>>    	/*
>>    	 * By now, the eBPF program has already setup parameters in r3, r4 and r5
>> @@ -285,7 +335,7 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o
>>    	int b2p_index = bpf_to_ppc(BPF_REG_3);
>>    	int bpf_tailcall_prologue_size = 8;
>>    
>> -	if (IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
>> +	if (!has_kfunc_call && IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
>>    		bpf_tailcall_prologue_size += 4; /* skip past the toc load */
>>    
>>    	/*
>> @@ -325,8 +375,20 @@ static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 o
>>    
>>    	/* goto *(prog->bpf_func + prologue_size); */
>>    	EMIT(PPC_RAW_LD(bpf_to_ppc(TMP_REG_1), bpf_to_ppc(TMP_REG_1), offsetof(struct bpf_prog, bpf_func)));
>> -	EMIT(PPC_RAW_ADDI(bpf_to_ppc(TMP_REG_1), bpf_to_ppc(TMP_REG_1),
>> -			FUNCTION_DESCR_SIZE + bpf_tailcall_prologue_size));
>> +	if (has_kfunc_call) {
>> +#ifdef PPC64_ELF_ABI_v1
> 
> I can't see a reason for a #ifdef here, why not use IS_ENABLED() like
> other places ?
> 
>> +		/* skip past the function descriptor */
>> +		EMIT(PPC_RAW_ADDI(bpf_to_ppc(TMP_REG_1), bpf_to_ppc(TMP_REG_1),
>> +				FUNCTION_DESCR_SIZE + bpf_tailcall_prologue_size));
>> +#else
>> +		EMIT(PPC_RAW_ADDI(bpf_to_ppc(TMP_REG_1), bpf_to_ppc(TMP_REG_1),
>> +				bpf_tailcall_prologue_size));
>> +#endif
>> +	} else {
>> +		EMIT(PPC_RAW_ADDI(bpf_to_ppc(TMP_REG_1), bpf_to_ppc(TMP_REG_1),
>> +				FUNCTION_DESCR_SIZE + bpf_tailcall_prologue_size));
>> +	}
>> +
>>    	EMIT(PPC_RAW_MTCTR(bpf_to_ppc(TMP_REG_1)));
>>    
>>    	/* tear down stack, restore NVRs, ... */
>> @@ -365,6 +427,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code
>>    		       u32 *addrs, int pass, bool extra_pass)
>>    {
>>    	enum stf_barrier_type stf_barrier = stf_barrier_type_get();
>> +	bool has_kfunc_call = bpf_prog_has_kfunc_call(fp);
>>    	const struct bpf_insn *insn = fp->insnsi;
>>    	int flen = fp->len;
>>    	int i, ret;
>> @@ -993,9 +1056,11 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code
>>    				return ret;
>>    
>>    			if (func_addr_fixed)
>> -				ret = bpf_jit_emit_func_call_hlp(image, ctx, func_addr);
>> +				ret = bpf_jit_emit_func_call_hlp(image, ctx, func_addr,
>> +								 has_kfunc_call);
> 
> Doesn't this fit on a single line ?
> 
>>    			else
>> -				ret = bpf_jit_emit_func_call_rel(image, fimage, ctx, func_addr);
>> +				ret = bpf_jit_emit_func_call_rel(image, fimage, ctx, func_addr,
>> +								 has_kfunc_call);
> 
> Same. Nowadays lines up to 100 chars are the norm.

Goes beyond 100 chars.

> 
>>    
>>    			if (ret)
>>    				return ret;
>> @@ -1204,7 +1269,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, u32 *fimage, struct code
>>    		 */
>>    		case BPF_JMP | BPF_TAIL_CALL:
>>    			ctx->seen |= SEEN_TAILCALL;
>> -			ret = bpf_jit_emit_tail_call(image, ctx, addrs[i + 1]);
>> +			ret = bpf_jit_emit_tail_call(image, ctx, addrs[i + 1], has_kfunc_call);
>>    			if (ret < 0)
>>    				return ret;
>>    			break;

Thanks
Hari

Re: [PATCH v2 2/2] powerpc/bpf: enable kfunc call

[Naveen N Rao]

On Tue, Feb 13, 2024 at 07:54:27AM +0000, Christophe Leroy wrote:
> 
> 
> Le 01/02/2024 à 18:12, Hari Bathini a écrit :
> > With module addresses supported, override bpf_jit_supports_kfunc_call()
> > to enable kfunc support. Module address offsets can be more than 32-bit
> > long, so override bpf_jit_supports_far_kfunc_call() to enable 64-bit
> > pointers.
> 
> What's the impact on PPC32 ? There are no 64-bit pointers on PPC32.

Looking at commit 1cf3bfc60f98 ("bpf: Support 64-bit pointers to 
kfuncs"), which added bpf_jit_supports_far_kfunc_call(), that does look 
to be very specific to platforms where module addresses are farther than 
s32. This is true for powerpc 64-bit, but shouldn't be needed for 
32-bit.

> 
> > 
> > Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
> > ---
> > 
> > * No changes since v1.
> > 
> > 
> >   arch/powerpc/net/bpf_jit_comp.c | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> > 
> > diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> > index 7b4103b4c929..f896a4213696 100644
> > --- a/arch/powerpc/net/bpf_jit_comp.c
> > +++ b/arch/powerpc/net/bpf_jit_comp.c
> > @@ -359,3 +359,13 @@ void bpf_jit_free(struct bpf_prog *fp)
> >   
> >   	bpf_prog_unlock_free(fp);
> >   }
> > +
> > +bool bpf_jit_supports_kfunc_call(void)
> > +{
> > +	return true;
> > +}
> > +
> > +bool bpf_jit_supports_far_kfunc_call(void)
> > +{
> > +	return true;
> > +}

I am not sure there is value in keeping this as a separate patch since 
all support code for kfunc calls is introduced in an earlier patch.  
Consider folding this into the previous patch.

- Naveen