Hi Christophe,

Thanks for reviewing the series.

On 17/09/21 9:40 pm, Christophe Leroy wrote:

Le 17/09/2021 à 17:30, Hari Bathini a écrit :

quoted

Refactor powerpc JITing. This simplifies adding BPF_PROBE_MEM support.

Could you describe a bit more what you are refactoring exactly ?

I am trying to do more than BPF_PROBE_MEM needs. Will keep the changes 
minimal (BPF_PROBE_MEM specific) and update the changelog..

quoted

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

Changes in v2:
* New patch to refactor a bit of JITing code.


  arch/powerpc/net/bpf_jit_comp32.c | 50 +++++++++++---------
  arch/powerpc/net/bpf_jit_comp64.c | 76 ++++++++++++++++---------------
  2 files changed, 68 insertions(+), 58 deletions(-)

diff --git a/arch/powerpc/net/bpf_jit_comp32.c 

b/arch/powerpc/net/bpf_jit_comp32.c
index b60b59426a24..c8ae14c316e3 100644

--- a/arch/powerpc/net/bpf_jit_comp32.c
+++ b/arch/powerpc/net/bpf_jit_comp32.c

@@ -276,17 +276,17 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 

*image, struct codegen_context *
      u32 exit_addr = addrs[flen];
      for (i = 0; i < flen; i++) {
-        u32 code = insn[i].code;
          u32 dst_reg = bpf_to_ppc(ctx, insn[i].dst_reg);
-        u32 dst_reg_h = dst_reg - 1;
          u32 src_reg = bpf_to_ppc(ctx, insn[i].src_reg);
-        u32 src_reg_h = src_reg - 1;
          u32 tmp_reg = bpf_to_ppc(ctx, TMP_REG);
+        u32 true_cond, code = insn[i].code;
+        u32 dst_reg_h = dst_reg - 1;
+        u32 src_reg_h = src_reg - 1;

All changes above seems unneeded and not linked to the current patch. 
Please leave cosmetic changes outside and focus on necessary changes.

quoted

+        u32 size = BPF_SIZE(code);
          s16 off = insn[i].off;
          s32 imm = insn[i].imm;
          bool func_addr_fixed;
          u64 func_addr;
-        u32 true_cond;
          /*
           * addrs[] maps a BPF bytecode address into a real offset from

@@ -809,25 +809,33 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 

*image, struct codegen_context *
          /*
           * BPF_LDX
           */
-        case BPF_LDX | BPF_MEM | BPF_B: /* dst = *(u8 *)(ul) (src + 
off) */
-            EMIT(PPC_RAW_LBZ(dst_reg, src_reg, off));
-            if (!fp->aux->verifier_zext)
-                EMIT(PPC_RAW_LI(dst_reg_h, 0));
-            break;
-        case BPF_LDX | BPF_MEM | BPF_H: /* dst = *(u16 *)(ul) (src + 
off) */
-            EMIT(PPC_RAW_LHZ(dst_reg, src_reg, off));
-            if (!fp->aux->verifier_zext)
-                EMIT(PPC_RAW_LI(dst_reg_h, 0));
-            break;
-        case BPF_LDX | BPF_MEM | BPF_W: /* dst = *(u32 *)(ul) (src + 
off) */
-            EMIT(PPC_RAW_LWZ(dst_reg, src_reg, off));
-            if (!fp->aux->verifier_zext)
+        /* dst = *(u8 *)(ul) (src + off) */
+        case BPF_LDX | BPF_MEM | BPF_B:
+        /* dst = *(u16 *)(ul) (src + off) */
+        case BPF_LDX | BPF_MEM | BPF_H:
+        /* dst = *(u32 *)(ul) (src + off) */
+        case BPF_LDX | BPF_MEM | BPF_W:
+        /* dst = *(u64 *)(ul) (src + off) */
+        case BPF_LDX | BPF_MEM | BPF_DW:

Why changing the location of the comments ? I found it more readable 
before.

Sure. I will revert that change.

quoted

+            switch (size) {
+            case BPF_B:
+                EMIT(PPC_RAW_LBZ(dst_reg, src_reg, off));
+                break;
+            case BPF_H:
+                EMIT(PPC_RAW_LHZ(dst_reg, src_reg, off));
+                break;
+            case BPF_W:
+                EMIT(PPC_RAW_LWZ(dst_reg, src_reg, off));
+                break;
+            case BPF_DW:
+                EMIT(PPC_RAW_LWZ(dst_reg_h, src_reg, off));
+                EMIT(PPC_RAW_LWZ(dst_reg, src_reg, off + 4));
+                break;
+            }

BPF_B, BPF_H, ... are not part of an enum. Are you sure GCC is happy to 
have no default ?

I used gcc 10.3 for ppc32 & gcc 8.3 for ppc64. No warnings.
Though, no harm adding the below, I guess..

	default:
		break;

Thanks
Hari