Varun R Mallya <[email protected]> writes: > This patchset aims to add BPF exceptions supports for riscv64 by > implementing the arch_bpf_stack_walk function and then updating the > prologue and epilogue for BPF JIT on riscv. Also, > bpf_jit_supports_exceptions() returns true now so that the verifier does > not reject programs containing BPF exceptions on riscv64. > > On riscv the unwinder used by arch_bpf_stack_walk() is the frame-pointer > unwinder, so exception support is gated on CONFIG_FRAME_POINTER. > > In the prologue and epilogue of the RISC-V JIT, I saved the > return address and then the frame-pointer according to [1]. Also, > according to [2], s0 to s11 are callee saved registers, which is why a > new array (rv_exception_csave_regs) has been created to save these > registers which contains all the required registers along with the frame > pointer as well as return address.
... > Since all exception tests pass now, the denylist file has been removed > as it is empty now. > > Changes in v1->v2: > v1: > https://lore.kernel.org/bpf/[email protected]/ > - resolve the ftrace trampoline address with ftrace_graph_ret_addr() > inside arch_bpf_stack_walk() > - Remove duplicated code in bpf_jit_build_prologue() > > Changes in v2->v3: > v2: > https://lore.kernel.org/bpf/[email protected]/ > - extend walk_stackframe to use sp/fp similar to how arm64 does it. (Björn > Töpel) > - refactor prologue/epilogue into helpers. (Björn Töpel) > - fix comments to make them netdev style. > - save/restore the tail-call counter in the exception prologue/epilogue > - exception_tail_call now passes since bpf2bpf+tailcall mixing landed [3], so > drop the (now-empty) DENYLIST.riscv64 > - (!aux->exception_cb) has been simplified to only one condition (Pu Lehui) Varun! Thanks for the rework. The overall structure is much better now, and the main v2 comments around reusing the stack walker and splitting the prologue/epilogue have been addressed! I tried to make sense of Sashiko's reports as well... Sashiko seems correct about sp being stale after crossing an exception frame. We restore pc and fp from pt_regs, so sp should be restored from there as well. Would you agree? The full pt_regs should also be validated before dereferencing it...? I *think* the instrumentation concern also looks valid. The new stack-walk adapter callbacks should not be instrumentable. I don't think the !CONFIG_FRAME_POINTER report is an exception-support bug. Exceptions require a valid boundary frame pointer, so the current gating looks correct. Sashiko is also correct that deleting DENYLIST.riscv64 breaks the documented selftest command. Please update the README or retain an empty file. Minor wording issue: ra is not callee-saved. Please say “RA and the callee-saved registers s0-s11.” Thanks, Björn
