Re: [PATCH bpf-next v3 2/2] bpf, mips: Add support for BPF_MOVSX in the JITs

[Johan Almbladh]

Hi Nicholas,

Thanks for the v3 series. Looks good, with one minor comment below.

I have applied all three of your patch series together, along with the
two fixes I just submitted, on top of the base commit 15972a29ac91.

Nicholas Dudar (6):
  bpf, mips: Factor out div/mod emission helpers
  MIPS: uasm: Add signed div/mod and sign-extension emitters
  bpf, mips: Add support for BPF_SDIV and BPF_SMOD
  bpf, mips: Factor register moves into helpers
  bpf, mips: Add support for BPF_MOVSX in the JITs
  bpf, mips: Add BPF_MEMSX support to the JITs

Johan Almbladh (2):
  bpf, mips: Fix immediate JMP JEQ/JNE on MIPS32
  bpf, mips: Fix BSWAP 32 and 16 on MIPS64

I ran test_bpf on QEMU targets, with the following results. All tests
pass on all 12 targets tested.

MIPS32r1/r2/r6, big and little endian:
test_bpf: Summary: 1062 PASSED, 0 FAILED, [1041/1050 JIT'ed]

MIPS64r1/r2/r6, big and little endian:
test_bpf: Summary: 1061 PASSED, 0 FAILED, [1040/1049 JIT'ed]

> +/* Register move operation (32-bit), optionally with sign extension */
>  static void emit_mov_r32(struct jit_context *ctx, const u8 dst[],
> -                        const u8 src[])
> +                        const u8 src[], s16 off)
>  {
> -       emit_mov_r(ctx, lo(dst), lo(src));
> +       switch (off) {
> +       case 8:
> +       case 16:
> +               emit_movsx_r(ctx, lo(dst), lo(src), off);
> +               break;
> +       default:
> +               /* off == 0 is MOV; the verifier rejects other offsets. */
> +               emit_mov_r(ctx, lo(dst), lo(src));
> +               break;
> +       }
> +       clobber_reg(ctx, lo(dst));
>         emit_zext_ver(ctx, dst);
>  }

The naming is a bit confusing. The name "mov" means two different
things here: 1) an instruction with opcode BPF_MOV, 2) a MOV
instruction (as opposed to MOVSX). I would suggest the following name
changes, applied to both 32-bit and 64-bit JIT variants. This would
make it consistent with the rest of the JIT and solve the MOV naming
ambiguity.

emit_mov_r32 -> emit_mov_movsx_r
emit_mov_r64 -> emit_mov_movsx_r64

I would also put the clobber_reg call last, after all emitter calls.

For the series:
Tested-by: Johan Almbladh <johan.almbladh@xxxxxxxxxxxxxxxxx>
Acked-by: Johan Almbladh <johan.almbladh@xxxxxxxxxxxxxxxxx>

Thanks,
Johan