Re: [PATCH v2 3/8] bpf powerpc: refactor JIT compiler code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 :
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..




Signed-off-by: Hari Bathini <hbathini@xxxxxxxxxxxxx>
---

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.

+        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.

+            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



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux