[PATCH bpf-next 0/2] bpf, riscv: Add support for indirect jumps

Chen Pei posted 2 patches 22 hours ago
arch/riscv/net/bpf_jit_comp64.c                  |  5 +++++
arch/riscv/net/bpf_jit_core.c                    | 16 ++++++++++++++--
.../testing/selftests/bpf/progs/verifier_gotox.c |  8 ++++++--
3 files changed, 25 insertions(+), 4 deletions(-)
[PATCH bpf-next 0/2] bpf, riscv: Add support for indirect jumps
Posted by Chen Pei 22 hours ago
The indirect jump instruction (BPF_JMP | BPF_JA | BPF_X, "gotox") and the
BPF_MAP_TYPE_INSN_ARRAY jump tables it consumes are core features: the
verifier accepts them on every architecture, and the x86-64, arm64 and
powerpc JITs implement them. On riscv64 a program using gotox passes
verification and then fails to load, because the JIT does not know the
opcode and CONFIG_BPF_JIT_ALWAYS_ON leaves no interpreter to fall back
onto.

This series adds the riscv64 support and turns the existing selftests on
for that architecture.

Patch 1 emits "jalr zero, rd, 0" for gotox and publishes the xlated to
jitted offsets through bpf_prog_update_insn_ptrs(), which is what fills
in the jump table addresses. Patch 2 widens the arch guard in
verifier_gotox.c to riscv64, the same way arm64 and powerpc were enabled.

Testing
=======

Environment: QEMU virt rv64, with CONFIG_BPF_JIT=y,
CONFIG_BPF_JIT_ALWAYS_ON=y, CONFIG_DEBUG_INFO_BTF=y; selftests
cross-built with clang 22.

- test_progs -t verifier_gotox: 27/27 subtests pass on bpf-next, 13 of
  them executed through BPF_PROG_TEST_RUN.
- test_progs-cpuv4 -t bpf_gotox: 14/14 subtests pass, none skipped.
  The cpuv4 flavor is needed here because bpf_gotox gates its subtests
  on __BPF_FEATURE_GOTOX, which clang only defines for -mcpu=v4.

Chen Pei (2):
  bpf, riscv: Add support for indirect jumps
  selftests/bpf: Enable gotox tests for riscv64

 arch/riscv/net/bpf_jit_comp64.c                  |  5 +++++
 arch/riscv/net/bpf_jit_core.c                    | 16 ++++++++++++++--
 .../testing/selftests/bpf/progs/verifier_gotox.c |  8 ++++++--
 3 files changed, 25 insertions(+), 4 deletions(-)

-- 
2.50.1
Re: [PATCH bpf-next 0/2] bpf, riscv: Add support for indirect jumps
Posted by Chen Pei 9 hours ago
Hi all,

Thanks for the reviews. Patch 1 is a plain wording fix and will go into
v2. For patch 2 I need your input on one thing before I respin: which
form of the #endif comment you would prefer.

Patch 1, on the comment above bpf_prog_update_insn_ptrs():

> This isn't a bug, but could the comment say "shift ctx->offset[] by one"
> rather than "shift the linfo array by one"? The jited_linfo was already
> filled in by the bpf_prog_fill_jited_linfo() call above, and it is the
> JIT's own offset table that is being rewritten here.

Agreed. Fixed in v2.

Patch 2, on the #endif marker in verifier_gotox.c:

> [Severity: Low]
> This isn't a bug, but does this newly introduced multi-line comment
> follow the BPF subsystem style guide? The subsystem guidelines
> explicitly require that multi-line comments have the opening '/*' on
> its own line, rather than beginning text on the same line as the
> opening marker.

I would rather collapse it to a single line, but there is more than one
way to spell it:

  a) 89 columns, clean under scripts/checkpatch.pl --strict:

#endif /* __TARGET_ARCH_x86 || __TARGET_ARCH_arm64 || __TARGET_ARCH_powerpc || riscv64 */

  b) 101 columns, keeps every macro name verbatim, but checkpatch then
     reports "WARNING: line length of 101 exceeds 100 columns":

#endif /* __TARGET_ARCH_x86 || __TARGET_ARCH_arm64 || __TARGET_ARCH_powerpc || __TARGET_ARCH_riscv */

  c) or a short marker that does not repeat the condition at all:

#endif /* gotox-capable arch */

I lean towards (a): tools/testing/selftests/bpf/progs/ otherwise only
uses single-line #endif markers, and "riscv64" matches the guard, which
is __TARGET_ARCH_riscv && __riscv_xlen == 64.

I will send v2 with the patch 1 fix and whichever form you pick, together
with any other feedback, so please let me know if you would like anything
else changed. If nobody has a preference, I will go with (a) in a few
days.

Thanks,
Pei