Messages in this thread | | | Date | Fri, 6 Jan 2023 09:57:49 +0800 | Subject | Re: [RFC PATCH RESEND bpf-next 3/4] riscv, bpf: Add bpf_arch_text_poke support for RV64 | From | Pu Lehui <> |
| |
On 2023/1/4 2:12, Björn Töpel wrote: > Pu Lehui <pulehui@huaweicloud.com> writes: > >> From: Pu Lehui <pulehui@huawei.com> >> >> Implement bpf_arch_text_poke for RV64. For call scenario, >> ftrace framework reserve 4 nops for RV64 kernel function >> as function entry, and use auipc+jalr instructions to call >> kernel or module functions. However, since the auipc+jalr >> call instructions is non-atomic operation, we need to use >> stop-machine to make sure instruction patching in atomic >> context. As for jump scenario, since we only jump inside >> the trampoline, a jal instruction is sufficient. > > Hmm, is that really true? More below! > >> >> Signed-off-by: Pu Lehui <pulehui@huawei.com> >> --- >> arch/riscv/net/bpf_jit.h | 5 ++ >> arch/riscv/net/bpf_jit_comp64.c | 131 +++++++++++++++++++++++++++++++- >> 2 files changed, 134 insertions(+), 2 deletions(-) >> >> diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h >> index d926e0f7ef57..bf9802a63061 100644 >> --- a/arch/riscv/net/bpf_jit.h >> +++ b/arch/riscv/net/bpf_jit.h >> @@ -573,6 +573,11 @@ static inline u32 rv_fence(u8 pred, u8 succ) >> return rv_i_insn(imm11_0, 0, 0, 0, 0xf); >> } >> >> +static inline u32 rv_nop(void) >> +{ >> + return rv_i_insn(0, 0, 0, 0, 0x13); >> +} >> + >> /* RVC instrutions. */ >> >> static inline u16 rvc_addi4spn(u8 rd, u32 imm10) >> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c >> index bf4721a99a09..fa8b03c52463 100644 >> --- a/arch/riscv/net/bpf_jit_comp64.c >> +++ b/arch/riscv/net/bpf_jit_comp64.c >> @@ -8,6 +8,8 @@ >> #include <linux/bitfield.h> >> #include <linux/bpf.h> >> #include <linux/filter.h> >> +#include <linux/memory.h> >> +#include <linux/stop_machine.h> >> #include "bpf_jit.h" >> >> #define RV_REG_TCC RV_REG_A6 >> @@ -238,7 +240,7 @@ static void __build_epilogue(bool is_tail_call, struct rv_jit_context *ctx) >> if (!is_tail_call) >> emit_mv(RV_REG_A0, RV_REG_A5, ctx); >> emit_jalr(RV_REG_ZERO, is_tail_call ? RV_REG_T3 : RV_REG_RA, >> - is_tail_call ? 4 : 0, /* skip TCC init */ >> + is_tail_call ? 20 : 0, /* skip reserved nops and TCC init */ >> ctx); >> } >> >> @@ -615,6 +617,127 @@ static int add_exception_handler(const struct bpf_insn *insn, >> return 0; >> } >> >> +struct text_poke_args { >> + void *addr; >> + const void *insns; >> + size_t len; >> + atomic_t cpu_count; >> +}; >> + >> +static int do_text_poke(void *data) >> +{ >> + int ret = 0; >> + struct text_poke_args *patch = data; >> + >> + if (atomic_inc_return(&patch->cpu_count) == num_online_cpus()) { >> + ret = patch_text_nosync(patch->addr, patch->insns, patch->len); >> + atomic_inc(&patch->cpu_count); >> + } else { >> + while (atomic_read(&patch->cpu_count) <= num_online_cpus()) >> + cpu_relax(); >> + smp_mb(); >> + } >> + >> + return ret; >> +} >> + >> +static int bpf_text_poke_stop_machine(void *addr, const void *insns, size_t len) >> +{ >> + struct text_poke_args patch = { >> + .addr = addr, >> + .insns = insns, >> + .len = len, >> + .cpu_count = ATOMIC_INIT(0), >> + }; >> + >> + return stop_machine(do_text_poke, &patch, cpu_online_mask); >> +} >> + >> +static int gen_call_or_nops(void *target, void *ip, u32 *insns) >> +{ >> + int i, ret; >> + s64 rvoff; >> + struct rv_jit_context ctx; >> + >> + ctx.ninsns = 0; >> + ctx.insns = (u16 *)insns; >> + >> + if (!target) { >> + for (i = 0; i < 4; i++) >> + emit(rv_nop(), &ctx); >> + return 0; >> + } >> + >> + rvoff = (s64)(target - ip); >> + emit(rv_sd(RV_REG_SP, -8, RV_REG_RA), &ctx); >> + ret = emit_jump_and_link(RV_REG_RA, rvoff, false, &ctx); >> + if (ret) >> + return ret; >> + emit(rv_ld(RV_REG_RA, -8, RV_REG_SP), &ctx); >> + >> + return 0; >> + >> +} >> + >> +static int bpf_text_poke_call(void *ip, void *old_addr, void *new_addr) >> +{ >> + int ret; >> + u32 old_insns[4], new_insns[4]; >> + >> + ret = gen_call_or_nops(old_addr, ip + 4, old_insns); >> + if (ret) >> + return ret; >> + >> + ret = gen_call_or_nops(new_addr, ip + 4, new_insns); >> + if (ret) >> + return ret; >> + >> + mutex_lock(&text_mutex); >> + if (memcmp(ip, old_insns, sizeof(old_insns))) { >> + ret = -EFAULT; >> + goto out; >> + } >> + >> + if (memcmp(ip, new_insns, sizeof(new_insns))) >> + ret = bpf_text_poke_stop_machine(ip, new_insns, >> sizeof(new_insns)); > > I'd rather see that you added a patch_text variant to > arch/riscv/kernel/patch.c (something like your > bpf_text_poke_stop_machine()), and use that here. Might be other users > of that as well -- Andy's ftrace patch maybe? :-) >
Good idea.
>> +out: >> + mutex_unlock(&text_mutex); >> + return ret; >> +} >> + >> +static int bpf_text_poke_jump(void *ip, void *old_addr, void *new_addr) >> +{ >> + int ret; >> + u32 old_insn, new_insn; >> + >> + old_insn = old_addr ? rv_jal(RV_REG_ZERO, (s64)(old_addr - ip) >> 1) : rv_nop(); >> + new_insn = new_addr ? rv_jal(RV_REG_ZERO, (s64)(new_addr - ip) >> 1) : rv_nop(); >> + >> + mutex_lock(&text_mutex); >> + if (memcmp(ip, &old_insn, sizeof(old_insn))) { >> + ret = -EFAULT; >> + goto out; >> + } >> + >> + if (memcmp(ip, &new_insn, sizeof(new_insn))) >> + ret = patch_text_nosync(ip, &new_insn, sizeof(new_insn)); >> +out: >> + mutex_unlock(&text_mutex); >> + return ret; >> +} >> + >> +int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type poke_type, >> + void *old_addr, void *new_addr) > > AFAIU there's nothing in the bpf_arch_text_poke() API that say that > BPF_MOD_JUMP is jumps within the trampoline. That is one usage, but not > the only one. In general, the jal might not have enough reach. > > I believe that this needs to be an auipc/jalr pair similar to > BPF_MOD_CALL (w/o linked register). >
The initial idea was that currently BPF_MOD_JUMP only serves for bpf_tramp_image_put, and jal, which range is +/- 1MB, is sufficient for the distance between im->ip_after_call and im->ip_epilogue, and try to not use not-atomic auipc/jalr pair. But take deep consideration, this might be extended to other uses, such as tailcall optimization. So agree with your suggestion.
> > And again, thanks for working on the RV trampoline! > Björn
| |