> riscv: bpf: Add BPF private stack support
TL;DR: Commit message only: it says the guard layout follows x86-64, but
x86-64 uses an 8 byte single-copy guard, and it may overstate why
__per_cpu_offset needs absolute moves.
> riscv: bpf: Add BPF private stack support
[ ... ]
> The base is materialized with absolute moves, as on arm64, x86-64 and
> powerpc64: the dynamic per-CPU area is vmalloc'd, beyond a PC-relative
> auipc pair from the JIT region, and riscv64 has no segment or register
> holding the per-CPU offset, so __per_cpu_offset needs materializing the
> same way. Both constants are fixed before the offset convergence loop,
> so the prologue length is stable across passes.
>
> The guard layout and value follow arm64, x86-64 and powerpc64: a 16
> byte region at each end holding two copies of the pattern. A jited
> program has them checked when it is freed, reporting the first
> corrupted CPU only; failure paths release the area unchecked since the
> image never ran, which also clears priv_stack_ptr for the later
> bpf_jit_free().
Is the statement that the guard layout follows x86-64 accurate?
arm64 and powerpc64 use a 16 byte guard with two copies of the pattern at
each end, but x86-64 differs:
arch/x86/net/bpf_jit_comp.c:
#define PRIV_STACK_GUARD_SZ 8
...
stack_ptr[0] = PRIV_STACK_GUARD_VAL;
stack_ptr[underflow_idx] = PRIV_STACK_GUARD_VAL;
It uses an 8 byte guard with a single copy at each end, and it rounds the
stack depth to 8 rather than 16. Only the guard value (0xEB9F12345678eb9f)
is shared by all three. Could the changelog say that the 16 byte, two-copy
layout follows arm64 and powerpc64, and that only the guard value follows
x86-64?
Separately, the changelog says __per_cpu_offset "needs materializing the
same way", meaning with absolute moves. The existing BPF_MOV64_PERCPU_REG
handler in bpf_jit_emit_insn() already loads the same symbol PC-relatively:
/* Load address of __per_cpu_offset array in T2 */
emit_addr(RV_REG_T2, (u64)&__per_cpu_offset, extra_pass, ctx);
Is emit_imm() for __per_cpu_offset in emit_priv_stack_frame() required, or a
choice made so that the prologue length stays fixed across passes? The
changelog could say which.
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/38671177164
