| Message ID | 20221220021319.1655871-4-pulehui@huaweicloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support bpf trampoline for RV64 | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Guessing tree name failed |
On 12/20/2022 10:13 AM, Pu Lehui wrote: > 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. > > 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()) { seems this sync is not needed, why not calling stop machine like this: stop_machine(do_text_poke, &patch, NULL); > + 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)); since there are 4 instructions being replaced, it is possible that serveral old instructions were already executed before stop machine. > +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) > +{ > + if (!is_kernel_text((unsigned long)ip) && > + !is_bpf_text_address((unsigned long)ip)) > + return -ENOTSUPP; > + > + return poke_type == BPF_MOD_CALL ? > + bpf_text_poke_call(ip, old_addr, new_addr) : > + bpf_text_poke_jump(ip, old_addr, new_addr); > +} > + > int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx, > bool extra_pass) > { > @@ -1266,7 +1389,7 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx, > > void bpf_jit_build_prologue(struct rv_jit_context *ctx) > { > - int stack_adjust = 0, store_offset, bpf_stack_adjust; > + int i, stack_adjust = 0, store_offset, bpf_stack_adjust; > bool is_main_prog = ctx->prog->aux->func_idx == 0; > > bpf_stack_adjust = round_up(ctx->prog->aux->stack_depth, 16); > @@ -1294,6 +1417,10 @@ void bpf_jit_build_prologue(struct rv_jit_context *ctx) > > store_offset = stack_adjust - 8; > > + /* reserve 4 nop insns */ > + for (i = 0; i < 4; i++) > + emit(rv_nop(), ctx); > + > /* First instruction is always setting the tail-call-counter > * (TCC) register. This instruction is skipped for tail calls. > * Force using a 4-byte (non-compressed) instruction.
On 2022/12/20 11:00, Xu Kuohai wrote: > On 12/20/2022 10:13 AM, Pu Lehui wrote: >> 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. >> >> 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()) { > > seems this sync is not needed, why not calling stop machine like this: > > stop_machine(do_text_poke, &patch, NULL); > I think both version should be ok. While the NULL version uses first cpu as master to patch_text and the others touch nmi watchdog, this version attempts to let other cpus enter cpu_relax to calm down, and let master cpu owns system resources as possible. >> + 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)); > > since there are 4 instructions being replaced, it is possible that > serveral old instructions were already executed before stop machine. > Yes, there are unpredictable problems after stop machine in that situation. In addition, probing stop machine sub-functions may lead to loop nesting problems. And this is the current problem with riscv ftrace. But the surprise was that Andy's work [1] was about to solve these problems. We can synchronize changes in the future. [1] https://lore.kernel.org/linux-riscv/20220913094252.3555240-1-andy.chiu@sifive.com >> +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) >> +{ >> + if (!is_kernel_text((unsigned long)ip) && >> + !is_bpf_text_address((unsigned long)ip)) >> + return -ENOTSUPP; >> + >> + return poke_type == BPF_MOD_CALL ? >> + bpf_text_poke_call(ip, old_addr, new_addr) : >> + bpf_text_poke_jump(ip, old_addr, new_addr); >> +} >> + >> int bpf_jit_emit_insn(const struct bpf_insn *insn, struct >> rv_jit_context *ctx, >> bool extra_pass) >> { >> @@ -1266,7 +1389,7 @@ int bpf_jit_emit_insn(const struct bpf_insn >> *insn, struct rv_jit_context *ctx, >> void bpf_jit_build_prologue(struct rv_jit_context *ctx) >> { >> - int stack_adjust = 0, store_offset, bpf_stack_adjust; >> + int i, stack_adjust = 0, store_offset, bpf_stack_adjust; >> bool is_main_prog = ctx->prog->aux->func_idx == 0; >> bpf_stack_adjust = round_up(ctx->prog->aux->stack_depth, 16); >> @@ -1294,6 +1417,10 @@ void bpf_jit_build_prologue(struct >> rv_jit_context *ctx) >> store_offset = stack_adjust - 8; >> + /* reserve 4 nop insns */ >> + for (i = 0; i < 4; i++) >> + emit(rv_nop(), ctx); >> + >> /* First instruction is always setting the tail-call-counter >> * (TCC) register. This instruction is skipped for tail calls. >> * Force using a 4-byte (non-compressed) instruction.
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? :-) > +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). And again, thanks for working on the RV trampoline! Björn
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
On 2023/1/6 9:57, Pu Lehui wrote: > > > 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 small nit，the current riscv BPF_MOD_JUMP > 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 >
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)); +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) +{ + if (!is_kernel_text((unsigned long)ip) && + !is_bpf_text_address((unsigned long)ip)) + return -ENOTSUPP; + + return poke_type == BPF_MOD_CALL ? + bpf_text_poke_call(ip, old_addr, new_addr) : + bpf_text_poke_jump(ip, old_addr, new_addr); +} + int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx, bool extra_pass) { @@ -1266,7 +1389,7 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx, void bpf_jit_build_prologue(struct rv_jit_context *ctx) { - int stack_adjust = 0, store_offset, bpf_stack_adjust; + int i, stack_adjust = 0, store_offset, bpf_stack_adjust; bool is_main_prog = ctx->prog->aux->func_idx == 0; bpf_stack_adjust = round_up(ctx->prog->aux->stack_depth, 16); @@ -1294,6 +1417,10 @@ void bpf_jit_build_prologue(struct rv_jit_context *ctx) store_offset = stack_adjust - 8; + /* reserve 4 nop insns */ + for (i = 0; i < 4; i++) + emit(rv_nop(), ctx); + /* First instruction is always setting the tail-call-counter * (TCC) register. This instruction is skipped for tail calls. * Force using a 4-byte (non-compressed) instruction.