Hi! Thank you for doing this, please see some comments inline.
> @@ -3491,6 +3531,7 @@ run_test(const struct bpf_test *tst)
> if (ret != 0) {
> printf("%s@%d: check_result(%s) failed, error: %d(%s);\n",
> __func__, __LINE__, tst->name, ret, strerror(ret));
> + return -1;
> }
>
> /* repeat the same test with jit, when possible */
> @@ -3506,6 +3547,7 @@ run_test(const struct bpf_test *tst)
> "error: %d(%s);\n",
> __func__, __LINE__, tst->name,
> rv, strerror(rv));
> + return -1;
> }
> }
>
All return values were OR-ed, so adding early return is not strictly necessary.
The real problem is JIT tests being skipped altogether if jit.func == NULL, and
it still remains. We need to disallow empty jit.func on platforms with known
working JIT.
(I'd ask for many more tests, but not sure it's fair since they aren't really
platform-dependent.)
// snip to implementation
> +static void
> +emit_ld_mbuf(struct a64_jit_ctx *ctx, uint32_t op, uint8_t tmp1, uint8_t
> tmp2,
> + uint8_t src, uint32_t imm)
Handling immediate as unsigned is questionable, especially in the BPF_IND case
it may produce incorrect results.
To make things worse, `__rte_pktmbuf_read` is also buggy when passed very large
lengths (again, technically not ARM eBPF fault).
> +{
> + uint8_t r0 = ebpf_to_a64_reg(ctx, EBPF_REG_0);
> + uint8_t r6 = ebpf_to_a64_reg(ctx, EBPF_REG_6);
> + uint32_t mode = BPF_MODE(op);
> + uint32_t opsz = BPF_SIZE(op);
> + uint32_t sz = bpf_size(opsz);
> + int16_t jump_to_epilogue;
> +
> + /* r0 = mbuf (R6) */
> + emit_mov_64(ctx, A64_R(0), r6);
> +
> + /* r1 = off: for ABS use imm, for IND use src + imm */
> + if (mode == BPF_ABS) {
> + emit_mov_imm(ctx, 1, A64_R(1), imm);
> + } else {
> + emit_mov_imm(ctx, 1, tmp2, imm);
> + emit_add(ctx, 1, tmp2, src);
> + emit_mov_64(ctx, A64_R(1), tmp2);
> + }
> +
> + /* r2 = len */
> + emit_mov_imm(ctx, 1, A64_R(2), sz);
> +
> + /* r3 = buf (SP) */
> + emit_mov_64(ctx, A64_R(3), A64_SP);
> +
> + /* call __rte_pktmbuf_read */
> + emit_call(ctx, tmp1, __rte_pktmbuf_read);
> + /* check return value of __rte_pktmbuf_read */
> + emit_cbnz(ctx, 1, A64_R(0), 3);
> + emit_mov_imm(ctx, 1, r0, 0);
> + jump_to_epilogue = (ctx->program_start + ctx->program_sz) - ctx->idx;
> + emit_b(ctx, jump_to_epilogue);
Could we call emit_return_zero_if_src_zero here instead?
> +
> + /* r0 points to the data, load 1/2/4 bytes */
> + emit_ldr(ctx, opsz, A64_R(0), A64_R(0), A64_ZR);
> + if (sz != sizeof(uint8_t))
> + emit_be(ctx, A64_R(0), sz * CHAR_BIT);
> + emit_mov_64(ctx, r0, A64_R(0));
> +}
> +
I would also pass final verdict on ARM code to ARM folks. To my untrained eye
it looks correct apart from the signed immediate issue. Optimizations are
possible, but since we're only implementing slow path for now maybe not worth
the effort.
