On Thu, 26 Feb 2026 14:24:43 +0000
Marat Khalili <[email protected]> wrote:
> Thank you for looking into this problem.
>
> Some general comments:
>
> * This needs tests verifying that instructions, both hardcoded in the test and
> converted from pcap filter, actually load expected numbers from the packet
> (or fail as required by specification).
>
> * Instead of hand-coding corresponding logic in machine code for each
> architecture, should we consider writing it once in eBPF and using only
> minimal architecture-dependent code for insertion?
This is for JIT, so it is going to be machine dependent.
The biggest win would be figuring out how to share eBPF to JIT from other
projects. That is the redundancy.
>
> About AI generation. I feel like I spent much more time reading this stuff
> than
> AI spent generating it. It does not feel fair. This needs a lot of work to be
> of production quality, and it is possible that many of us would do it faster
> without AI. Highlighting possible improvements feels very gratifying when part
> of a teaching process with an intern, relatively useless for an AI model.
> Since
> each of us has access to this stuff, I'm not sure what the point is in doing
> it
> this way.
Agree mostly. But AI does a good job of cloning from existing code.
The problem is that as you point out, it rarely spots where having
> Please see my comments inline to understand what I mean.
>
> > diff --git a/lib/bpf/bpf_jit_arm64.c b/lib/bpf/bpf_jit_arm64.c
> > index a04ef33a9c..dff101dbba 100644
> > --- a/lib/bpf/bpf_jit_arm64.c
> > +++ b/lib/bpf/bpf_jit_arm64.c
> > @@ -3,11 +3,13 @@
> > */
> >
> > #include <errno.h>
> > +#include <stddef.h>
> > #include <stdbool.h>
> > #include <stdlib.h>
> >
> > #include <rte_common.h>
> > #include <rte_byteorder.h>
> > +#include <rte_mbuf.h>
> >
> > #include "bpf_impl.h"
> >
> > @@ -43,6 +45,7 @@ struct a64_jit_ctx {
> > uint32_t program_start; /* Program index, Just after prologue */
> > uint32_t program_sz; /* Program size. Found in first pass */
> > uint8_t foundcall; /* Found EBPF_CALL class code in eBPF pgm */
> > + uint8_t foundldmbuf; /* Found BPF_ABS/BPF_IND in eBPF pgm */
> > };
> >
> > static int
> > @@ -1137,12 +1140,293 @@ check_program_has_call(struct a64_jit_ctx *ctx,
> > struct rte_bpf *bpf)
> > switch (op) {
> > /* Call imm */
> > case (BPF_JMP | EBPF_CALL):
> > + /*
> > + * BPF_ABS/BPF_IND instructions may need to call
> > + * __rte_pktmbuf_read() in the slow path, so treat
> > + * them as having a call for register save purposes.
> > + */
> > + case (BPF_LD | BPF_ABS | BPF_B):
> > + case (BPF_LD | BPF_ABS | BPF_H):
> > + case (BPF_LD | BPF_ABS | BPF_W):
> > + case (BPF_LD | BPF_IND | BPF_B):
> > + case (BPF_LD | BPF_IND | BPF_H):
> > + case (BPF_LD | BPF_IND | BPF_W):
> > ctx->foundcall = 1;
> > + ctx->foundldmbuf = 1;
>
> Will set it for calls as well.
>
> > return;
> > }
> > }
> > }
> >
> > +/*
> > + * Emit SUBS (extended register) for comparing with zero-extended halfword.
> > + * Used to compare: data_len - offset against size.
> > + * CMP (imm): SUBS Xd, Xn, #imm
>
> This whole comment confuses more than clarifies:
> * I believe CMP is a valid mnemonic in itself, no need to go too nerdy about
> this.
> * No idea what "zero-extended halfword" means here (maybe someone can help).
> * How it is being used belongs to the place of use, not function declaration.
> * There is no Xd in function arguments, so not sure what last line explains.
>
> Comment to the next one is only marginally better. IMO before writing a
> comment
> AI should ask itself "what will be unclear here to the reader", not "what
> random facts I can add to make it look smarter".
The problem with AI text, is that it always thinks "more is better".
Which is wrong. More is more, and just adds confusion.
>
> > + */
> > +static void
> > +emit_cmp_imm(struct a64_jit_ctx *ctx, bool is64, uint8_t rn, uint16_t
> > imm12)
> > +{
> > + uint32_t insn, imm;
> > + int32_t simm;
> > +
> > + imm = mask_imm(12, imm12);
> > + simm = (int32_t)imm12;
> > + insn = RTE_SHIFT_VAL32(is64, 31);
> > + insn |= RTE_SHIFT_VAL32(1, 30); /* SUB */
> > + insn |= RTE_SHIFT_VAL32(1, 29); /* set flags */
> > + insn |= UINT32_C(0x11000000);
> > + insn |= A64_ZR; /* Rd = ZR for CMP */
> > + insn |= RTE_SHIFT_VAL32(rn, 5);
> > + insn |= RTE_SHIFT_VAL32(imm, 10);
> > +
> > + emit_insn(ctx, insn, check_reg(rn) || check_imm(12, simm));
> > +}
> > +
> > +/*
> > + * Emit a load from [rn + unsigned immediate offset].
> > + * LDR (unsigned offset): size determined by sz parameter.
> > + */
> > +static void
> > +emit_ldr_uimm(struct a64_jit_ctx *ctx, uint8_t sz, uint8_t rt, uint8_t rn,
> > + uint16_t uimm)
> > +{
> > + uint32_t insn, scale, imm;
> > + int32_t simm;
> > +
> > + /* Determine scale from BPF size encoding */
> > + if (sz == BPF_B)
> > + scale = 0;
> > + else if (sz == BPF_H)
> > + scale = 1;
> > + else if (sz == BPF_W)
> > + scale = 2;
> > + else /* EBPF_DW */
> > + scale = 3;
> > +
> > + /* imm12 is the offset divided by the access size */
> > + imm = uimm >> scale;
>
> This is error-prone, need to either assert (maybe even static_assert) that
> uimm
> is a multiple of scale here, or accept already descaled number and deal with
> it
> at the place of call.
>
> > + simm = (int32_t)imm;
>
> This whole uimm -> imm -> simm logic is a bit hard to follow. Maybe my
> personal
> preference, but I'd get rid of at least simm, which is just an exact copy of
> imm with different type. It also makes one question if some edge cases with
> negative numbers are not overlooked.
>
> > +
> > + insn = RTE_SHIFT_VAL32(scale, 30);
> > + insn |= UINT32_C(0x39400000); /* LDR (unsigned offset) base */
> > + insn |= RTE_SHIFT_VAL32(mask_imm(12, imm), 10);
> > + insn |= RTE_SHIFT_VAL32(rn, 5);
> > + insn |= rt;
> > +
> > + emit_insn(ctx, insn, check_reg(rt) || check_reg(rn) ||
> > + check_imm(12, simm) || check_ls_sz(sz));
> > +}
>
> These functions should probably integrate with and follow the style of
> existing
> ones like emit_cmp_tst/emit_cmp and emit_ls/emit_ldr. Even if deliberately
> duplicating some code, it should at least be copied verbatim, not re-invented.
>
> Both function accept immediate as uint16_t for some reason, truncating it from
> uint32_t or size_t. This truncation does not seem to be checked explicitly.
> Should we accept ssize_t instead?
>
All good observations.
The purpose of this patch was to get kickstart the process.
It is kind of what Linus and Andrew have done in the past to
get something out there to motivate a better solution.
