Messages in this thread | | | Date | Tue, 6 Dec 2022 16:16:15 +0800 | Subject | Re: [PATCH bpf v2] riscv, bpf: Emit fixed-length instructions for BPF_PSEUDO_FUNC | From | Pu Lehui <> |
| |
On 2022/12/6 15:55, Björn Töpel wrote: > Pu Lehui <pulehui@huaweicloud.com> writes: > >> Sorry for replying so late. For BPF_PSEUDO_FUNC instruction, verifier >> will set insn[0].imm and insn[1].imm to 1 that make addr to 0x100000001 >> before extra pass, and also ctx->insns is NULL in iteration stage, all >> of these make off out of range of AUIPC-ADDI range, and return failed. >> We could add some special handling at different stages, but that seems a >> little weird. By the way, I do not really like emit_addr function with >> return value. > > My rational is that *if* for some reason the jit is passed an address > that auipc/addi can't represent, we'd like to catch that and not emit > broken code. > >> While a proper address is at least 2B alignment, and the valid address >> is from 0xffffffff00000000 to 0xffffffffffffffff, we can make address >> shifed 1 place to right, and addr >> 1 will always in the range of >> AUIPC-ADDI range. We can get rid of the range detection. The >> implementation is as follows: >> >> static void emit_addr(u8 rd, u64 addr, struct rv_jit_context *ctx) >> { >> s64 imm = addr >> 1; >> s64 upper = (imm + (1 << 11)) >> 12; >> s64 lower = imm & 0xfff; >> >> emit(rv_lui(rd, upper), ctx); >> emit(rv_addi(rd, rd, lower), ctx); >> emit(rv_slli(rd, rd, 1), ctx); >> } >> >> What do you think? > > That's a code generation penalty, instead of catching it at code > gen. Don't like! :-) I much prefer the auipc/addi version. > > What do you think about the diff (on-top of your work) below? > > --8<-- > diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c > index aa9410eef77c..7acaf28cb3be 100644 > --- a/arch/riscv/net/bpf_jit_comp64.c > +++ b/arch/riscv/net/bpf_jit_comp64.c > @@ -137,15 +137,21 @@ static bool in_auipc_jalr_range(s64 val) > } > > /* Emit fixed-length instructions for address */ > -static void emit_addr(u8 rd, u64 addr, struct rv_jit_context *ctx) > +static int emit_addr(u8 rd, u64 addr, bool extra_pass, struct rv_jit_context *ctx) > { > u64 ip = (u64)(ctx->insns + ctx->ninsns); > s64 off = addr - ip; > s64 upper = (off + (1 << 11)) >> 12; > s64 lower = ((off & 0xfff) << 52) >> 52; > > + if (extra_pass && !in_auipc_jalr_range(off)) { > + pr_err("bpf-jit: target offset 0x%llx is out of range\n", off); > + return -ERANGE; > + } > + > emit(rv_auipc(rd, upper), ctx); > emit(rv_addi(rd, rd, lower), ctx); > + return 0; > } > > /* Emit variable-length instructions for 32-bit and 64-bit imm */ > @@ -1061,13 +1067,17 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx, > { > struct bpf_insn insn1 = insn[1]; > u64 imm64; > + int ret; > > imm64 = (u64)insn1.imm << 32 | (u32)imm; > - if (bpf_pseudo_func(insn)) > + if (bpf_pseudo_func(insn)) { > /* fixed-length insns for extra jit pass */ > - emit_addr(rd, imm64, ctx); > - else > + ret = emit_addr(rd, imm64, extra_pass, ctx); > + if (ret) > + return ret; > + } else { > emit_imm(rd, imm64, ctx); > + } > > return 1; > } > > --8<-- > > Wouldn't that work? >
It definitely works. But auipc+addi may be some holes, while lui+addi+slli support all the address of kernel and module. And this might be help for the future feature porting.
> > Björn
| |