Re: [PATCH 03/13] powerpc/bpf: Update ldimm64 instructions during extra pass

2022-01-10 Thread Naveen N. Rao 

Christophe Leroy wrote:



Le 06/01/2022 à 12:45, Naveen N. Rao a écrit :

These instructions are updated after the initial JIT, so redo codegen
during the extra pass. Rename bpf_jit_fixup_subprog_calls() to clarify
that this is more than just subprog calls.

Fixes: 69c087ba6225b5 ("bpf: Add bpf_for_each_map_elem() helper")
Cc: sta...@vger.kernel.org # v5.15
Signed-off-by: Naveen N. Rao 
---
  arch/powerpc/net/bpf_jit_comp.c   | 29 +++--
  arch/powerpc/net/bpf_jit_comp32.c |  6 ++
  arch/powerpc/net/bpf_jit_comp64.c |  7 ++-
  3 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index d6ffdd0f2309d0..56dd1f4e3e4447 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -23,15 +23,15 @@ static void bpf_jit_fill_ill_insns(void *area, unsigned int 
size)
memset32(area, BREAKPOINT_INSTRUCTION, size / 4);
  }
  
-/* Fix the branch target addresses for subprog calls */

-static int bpf_jit_fixup_subprog_calls(struct bpf_prog *fp, u32 *image,
-  struct codegen_context *ctx, u32 *addrs)
+/* Fix updated addresses (for subprog calls, ldimm64, et al) during extra pass 
*/
+static int bpf_jit_fixup_addresses(struct bpf_prog *fp, u32 *image,
+  struct codegen_context *ctx, u32 *addrs)
  {
const struct bpf_insn *insn = fp->insnsi;
bool func_addr_fixed;
u64 func_addr;
u32 tmp_idx;
-   int i, ret;
+   int i, j, ret;
  
  	for (i = 0; i < fp->len; i++) {

/*
@@ -66,6 +66,23 @@ static int bpf_jit_fixup_subprog_calls(struct bpf_prog *fp, 
u32 *image,
 * of the JITed sequence remains unchanged.
 */
ctx->idx = tmp_idx;
+   } else if (insn[i].code == (BPF_LD | BPF_IMM | BPF_DW)) {
+   tmp_idx = ctx->idx;
+   ctx->idx = addrs[i] / 4;
+#ifdef CONFIG_PPC32
+   PPC_LI32(ctx->b2p[insn[i].dst_reg] - 1, (u32)insn[i + 
1].imm);
+   PPC_LI32(ctx->b2p[insn[i].dst_reg], (u32)insn[i].imm);
+   for (j = ctx->idx - addrs[i] / 4; j < 4; j++)
+   EMIT(PPC_RAW_NOP());
+#else
+   func_addr = ((u64)(u32)insn[i].imm) | (((u64)(u32)insn[i + 
1].imm) << 32);
+   PPC_LI64(b2p[insn[i].dst_reg], func_addr);
+   /* overwrite rest with nops */
+   for (j = ctx->idx - addrs[i] / 4; j < 5; j++)
+   EMIT(PPC_RAW_NOP());
+#endif


#ifdefs should be avoided as much as possible.

Here it seems we could easily do an

if (IS_ENABLED(CONFIG_PPC32)) {
} else {
}

And it looks like the CONFIG_PPC64 alternative would in fact also work 
on PPC32, wouldn't it ?


We never implemented PPC_LI64() for ppc32:
 /linux/arch/powerpc/net/bpf_jit_comp.c: In function 
 'bpf_jit_fixup_addresses':

 /linux/arch/powerpc/net/bpf_jit_comp.c:81:5: error: this decimal constant is 
unsigned only in ISO C90 [-Werror]
81 | PPC_LI64(b2p[insn[i].dst_reg], func_addr);
| ^~~~
 /linux/arch/powerpc/net/bpf_jit_comp.c:81:5: error: this decimal constant is 
unsigned only in ISO C90 [-Werror]
 In file included from /linux/arch/powerpc/net/bpf_jit_comp.c:19:
 /linux/arch/powerpc/net/bpf_jit.h:78:40: error: right shift count >= width of 
type [-Werror=shift-count-overflow]
78 | EMIT(PPC_RAW_LI(d, ((uintptr_t)(i) >> 32) &   \
|^~


We should move that out from bpf_jit.h


- Naveen

Re: [PATCH 03/13] powerpc/bpf: Update ldimm64 instructions during extra pass

2022-01-10 Thread Christophe Leroy 


Le 06/01/2022 à 12:45, Naveen N. Rao a écrit :
> These instructions are updated after the initial JIT, so redo codegen
> during the extra pass. Rename bpf_jit_fixup_subprog_calls() to clarify
> that this is more than just subprog calls.
> 
> Fixes: 69c087ba6225b5 ("bpf: Add bpf_for_each_map_elem() helper")
> Cc: sta...@vger.kernel.org # v5.15
> Signed-off-by: Naveen N. Rao 
> ---
>   arch/powerpc/net/bpf_jit_comp.c   | 29 +++--
>   arch/powerpc/net/bpf_jit_comp32.c |  6 ++
>   arch/powerpc/net/bpf_jit_comp64.c |  7 ++-
>   3 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index d6ffdd0f2309d0..56dd1f4e3e4447 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
> @@ -23,15 +23,15 @@ static void bpf_jit_fill_ill_insns(void *area, unsigned 
> int size)
>   memset32(area, BREAKPOINT_INSTRUCTION, size / 4);
>   }
>   
> -/* Fix the branch target addresses for subprog calls */
> -static int bpf_jit_fixup_subprog_calls(struct bpf_prog *fp, u32 *image,
> -struct codegen_context *ctx, u32 *addrs)
> +/* Fix updated addresses (for subprog calls, ldimm64, et al) during extra 
> pass */
> +static int bpf_jit_fixup_addresses(struct bpf_prog *fp, u32 *image,
> +struct codegen_context *ctx, u32 *addrs)
>   {
>   const struct bpf_insn *insn = fp->insnsi;
>   bool func_addr_fixed;
>   u64 func_addr;
>   u32 tmp_idx;
> - int i, ret;
> + int i, j, ret;
>   
>   for (i = 0; i < fp->len; i++) {
>   /*
> @@ -66,6 +66,23 @@ static int bpf_jit_fixup_subprog_calls(struct bpf_prog 
> *fp, u32 *image,
>* of the JITed sequence remains unchanged.
>*/
>   ctx->idx = tmp_idx;
> + } else if (insn[i].code == (BPF_LD | BPF_IMM | BPF_DW)) {
> + tmp_idx = ctx->idx;
> + ctx->idx = addrs[i] / 4;
> +#ifdef CONFIG_PPC32
> + PPC_LI32(ctx->b2p[insn[i].dst_reg] - 1, (u32)insn[i + 
> 1].imm);
> + PPC_LI32(ctx->b2p[insn[i].dst_reg], (u32)insn[i].imm);
> + for (j = ctx->idx - addrs[i] / 4; j < 4; j++)
> + EMIT(PPC_RAW_NOP());
> +#else
> + func_addr = ((u64)(u32)insn[i].imm) | 
> (((u64)(u32)insn[i + 1].imm) << 32);
> + PPC_LI64(b2p[insn[i].dst_reg], func_addr);
> + /* overwrite rest with nops */
> + for (j = ctx->idx - addrs[i] / 4; j < 5; j++)
> + EMIT(PPC_RAW_NOP());
> +#endif

#ifdefs should be avoided as much as possible.

Here it seems we could easily do an

if (IS_ENABLED(CONFIG_PPC32)) {
} else {
}

And it looks like the CONFIG_PPC64 alternative would in fact also work 
on PPC32, wouldn't it ?


> + ctx->idx = tmp_idx;
> + i++;
>   }
>   }
>   
> @@ -200,13 +217,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog 
> *fp)
>   /*
>* Do not touch the prologue and epilogue as they will remain
>* unchanged. Only fix the branch target address for subprog
> -  * calls in the body.
> +  * calls in the body, and ldimm64 instructions.
>*
>* This does not change the offsets and lengths of the subprog
>* call instruction sequences and hence, the size of the JITed
>* image as well.
>*/
> - bpf_jit_fixup_subprog_calls(fp, code_base, , addrs);
> + bpf_jit_fixup_addresses(fp, code_base, , addrs);
>   
>   /* There is no need to perform the usual passes. */
>   goto skip_codegen_passes;
> diff --git a/arch/powerpc/net/bpf_jit_comp32.c 
> b/arch/powerpc/net/bpf_jit_comp32.c
> index 997a47fa615b30..2258d3886d02ec 100644
> --- a/arch/powerpc/net/bpf_jit_comp32.c
> +++ b/arch/powerpc/net/bpf_jit_comp32.c
> @@ -293,6 +293,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, 
> struct codegen_context *
>   bool func_addr_fixed;
>   u64 func_addr;
>   u32 true_cond;
> + u32 tmp_idx;
> + int j;
>   
>   /*
>* addrs[] maps a BPF bytecode address into a real offset from
> @@ -908,8 +910,12 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, 
> struct codegen_context *
>* 16 byte instruction that uses two 'struct bpf_insn'
>*/
>   case BPF_LD | BPF_IMM | BPF_DW: /* dst = (u64) imm */
> + tmp_idx = ctx->idx;
>   PPC_LI32(dst_reg_h, (u32)insn[i + 1].imm);
>   PPC_LI32(dst_reg, (u32)insn[i].imm);
> + /*

Re: [PATCH 03/13] powerpc/bpf: Update ldimm64 instructions during extra pass

2022-01-08 Thread Jiri Olsa 
On Thu, Jan 06, 2022 at 05:15:07PM +0530, Naveen N. Rao wrote:
> These instructions are updated after the initial JIT, so redo codegen
> during the extra pass. Rename bpf_jit_fixup_subprog_calls() to clarify
> that this is more than just subprog calls.
> 
> Fixes: 69c087ba6225b5 ("bpf: Add bpf_for_each_map_elem() helper")
> Cc: sta...@vger.kernel.org # v5.15
> Signed-off-by: Naveen N. Rao 

Tested-by: Jiri Olsa 

thanks,
jirka

> ---
>  arch/powerpc/net/bpf_jit_comp.c   | 29 +++--
>  arch/powerpc/net/bpf_jit_comp32.c |  6 ++
>  arch/powerpc/net/bpf_jit_comp64.c |  7 ++-
>  3 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index d6ffdd0f2309d0..56dd1f4e3e4447 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
> @@ -23,15 +23,15 @@ static void bpf_jit_fill_ill_insns(void *area, unsigned 
> int size)
>   memset32(area, BREAKPOINT_INSTRUCTION, size / 4);
>  }
>  
> -/* Fix the branch target addresses for subprog calls */
> -static int bpf_jit_fixup_subprog_calls(struct bpf_prog *fp, u32 *image,
> -struct codegen_context *ctx, u32 *addrs)
> +/* Fix updated addresses (for subprog calls, ldimm64, et al) during extra 
> pass */
> +static int bpf_jit_fixup_addresses(struct bpf_prog *fp, u32 *image,
> +struct codegen_context *ctx, u32 *addrs)
>  {
>   const struct bpf_insn *insn = fp->insnsi;
>   bool func_addr_fixed;
>   u64 func_addr;
>   u32 tmp_idx;
> - int i, ret;
> + int i, j, ret;
>  
>   for (i = 0; i < fp->len; i++) {
>   /*
> @@ -66,6 +66,23 @@ static int bpf_jit_fixup_subprog_calls(struct bpf_prog 
> *fp, u32 *image,
>* of the JITed sequence remains unchanged.
>*/
>   ctx->idx = tmp_idx;
> + } else if (insn[i].code == (BPF_LD | BPF_IMM | BPF_DW)) {
> + tmp_idx = ctx->idx;
> + ctx->idx = addrs[i] / 4;
> +#ifdef CONFIG_PPC32
> + PPC_LI32(ctx->b2p[insn[i].dst_reg] - 1, (u32)insn[i + 
> 1].imm);
> + PPC_LI32(ctx->b2p[insn[i].dst_reg], (u32)insn[i].imm);
> + for (j = ctx->idx - addrs[i] / 4; j < 4; j++)
> + EMIT(PPC_RAW_NOP());
> +#else
> + func_addr = ((u64)(u32)insn[i].imm) | 
> (((u64)(u32)insn[i + 1].imm) << 32);
> + PPC_LI64(b2p[insn[i].dst_reg], func_addr);
> + /* overwrite rest with nops */
> + for (j = ctx->idx - addrs[i] / 4; j < 5; j++)
> + EMIT(PPC_RAW_NOP());
> +#endif
> + ctx->idx = tmp_idx;
> + i++;
>   }
>   }
>  
> @@ -200,13 +217,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog 
> *fp)
>   /*
>* Do not touch the prologue and epilogue as they will remain
>* unchanged. Only fix the branch target address for subprog
> -  * calls in the body.
> +  * calls in the body, and ldimm64 instructions.
>*
>* This does not change the offsets and lengths of the subprog
>* call instruction sequences and hence, the size of the JITed
>* image as well.
>*/
> - bpf_jit_fixup_subprog_calls(fp, code_base, , addrs);
> + bpf_jit_fixup_addresses(fp, code_base, , addrs);
>  
>   /* There is no need to perform the usual passes. */
>   goto skip_codegen_passes;
> diff --git a/arch/powerpc/net/bpf_jit_comp32.c 
> b/arch/powerpc/net/bpf_jit_comp32.c
> index 997a47fa615b30..2258d3886d02ec 100644
> --- a/arch/powerpc/net/bpf_jit_comp32.c
> +++ b/arch/powerpc/net/bpf_jit_comp32.c
> @@ -293,6 +293,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, 
> struct codegen_context *
>   bool func_addr_fixed;
>   u64 func_addr;
>   u32 true_cond;
> + u32 tmp_idx;
> + int j;
>  
>   /*
>* addrs[] maps a BPF bytecode address into a real offset from
> @@ -908,8 +910,12 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, 
> struct codegen_context *
>* 16 byte instruction that uses two 'struct bpf_insn'
>*/
>   case BPF_LD | BPF_IMM | BPF_DW: /* dst = (u64) imm */
> + tmp_idx = ctx->idx;
>   PPC_LI32(dst_reg_h, (u32)insn[i + 1].imm);
>   PPC_LI32(dst_reg, (u32)insn[i].imm);
> + /* padding to allow full 4 instructions for later 
> patching */
> + for (j = ctx->idx - tmp_idx; j < 4; j++)
> + EMIT(PPC_RAW_NOP());
>   /*

[PATCH 03/13] powerpc/bpf: Update ldimm64 instructions during extra pass

2022-01-06 Thread Naveen N. Rao 
These instructions are updated after the initial JIT, so redo codegen
during the extra pass. Rename bpf_jit_fixup_subprog_calls() to clarify
that this is more than just subprog calls.

Fixes: 69c087ba6225b5 ("bpf: Add bpf_for_each_map_elem() helper")
Cc: sta...@vger.kernel.org # v5.15
Signed-off-by: Naveen N. Rao 
---
 arch/powerpc/net/bpf_jit_comp.c   | 29 +++--
 arch/powerpc/net/bpf_jit_comp32.c |  6 ++
 arch/powerpc/net/bpf_jit_comp64.c |  7 ++-
 3 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index d6ffdd0f2309d0..56dd1f4e3e4447 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -23,15 +23,15 @@ static void bpf_jit_fill_ill_insns(void *area, unsigned int 
size)
memset32(area, BREAKPOINT_INSTRUCTION, size / 4);
 }
 
-/* Fix the branch target addresses for subprog calls */
-static int bpf_jit_fixup_subprog_calls(struct bpf_prog *fp, u32 *image,
-  struct codegen_context *ctx, u32 *addrs)
+/* Fix updated addresses (for subprog calls, ldimm64, et al) during extra pass 
*/
+static int bpf_jit_fixup_addresses(struct bpf_prog *fp, u32 *image,
+  struct codegen_context *ctx, u32 *addrs)
 {
const struct bpf_insn *insn = fp->insnsi;
bool func_addr_fixed;
u64 func_addr;
u32 tmp_idx;
-   int i, ret;
+   int i, j, ret;
 
for (i = 0; i < fp->len; i++) {
/*
@@ -66,6 +66,23 @@ static int bpf_jit_fixup_subprog_calls(struct bpf_prog *fp, 
u32 *image,
 * of the JITed sequence remains unchanged.
 */
ctx->idx = tmp_idx;
+   } else if (insn[i].code == (BPF_LD | BPF_IMM | BPF_DW)) {
+   tmp_idx = ctx->idx;
+   ctx->idx = addrs[i] / 4;
+#ifdef CONFIG_PPC32
+   PPC_LI32(ctx->b2p[insn[i].dst_reg] - 1, (u32)insn[i + 
1].imm);
+   PPC_LI32(ctx->b2p[insn[i].dst_reg], (u32)insn[i].imm);
+   for (j = ctx->idx - addrs[i] / 4; j < 4; j++)
+   EMIT(PPC_RAW_NOP());
+#else
+   func_addr = ((u64)(u32)insn[i].imm) | 
(((u64)(u32)insn[i + 1].imm) << 32);
+   PPC_LI64(b2p[insn[i].dst_reg], func_addr);
+   /* overwrite rest with nops */
+   for (j = ctx->idx - addrs[i] / 4; j < 5; j++)
+   EMIT(PPC_RAW_NOP());
+#endif
+   ctx->idx = tmp_idx;
+   i++;
}
}
 
@@ -200,13 +217,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
/*
 * Do not touch the prologue and epilogue as they will remain
 * unchanged. Only fix the branch target address for subprog
-* calls in the body.
+* calls in the body, and ldimm64 instructions.
 *
 * This does not change the offsets and lengths of the subprog
 * call instruction sequences and hence, the size of the JITed
 * image as well.
 */
-   bpf_jit_fixup_subprog_calls(fp, code_base, , addrs);
+   bpf_jit_fixup_addresses(fp, code_base, , addrs);
 
/* There is no need to perform the usual passes. */
goto skip_codegen_passes;
diff --git a/arch/powerpc/net/bpf_jit_comp32.c 
b/arch/powerpc/net/bpf_jit_comp32.c
index 997a47fa615b30..2258d3886d02ec 100644
--- a/arch/powerpc/net/bpf_jit_comp32.c
+++ b/arch/powerpc/net/bpf_jit_comp32.c
@@ -293,6 +293,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, 
struct codegen_context *
bool func_addr_fixed;
u64 func_addr;
u32 true_cond;
+   u32 tmp_idx;
+   int j;
 
/*
 * addrs[] maps a BPF bytecode address into a real offset from
@@ -908,8 +910,12 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, 
struct codegen_context *
 * 16 byte instruction that uses two 'struct bpf_insn'
 */
case BPF_LD | BPF_IMM | BPF_DW: /* dst = (u64) imm */
+   tmp_idx = ctx->idx;
PPC_LI32(dst_reg_h, (u32)insn[i + 1].imm);
PPC_LI32(dst_reg, (u32)insn[i].imm);
+   /* padding to allow full 4 instructions for later 
patching */
+   for (j = ctx->idx - tmp_idx; j < 4; j++)
+   EMIT(PPC_RAW_NOP());
/* Adjust for two bpf instructions */
addrs[++i] = ctx->idx * 4;
break;
diff --git a/arch/powerpc/net/bpf_jit_comp64.c 
b/arch/powerpc/net/bpf_jit_comp64.c
index