Linux-RISC-V Archive on lore.kernel.org
 help / color / Atom feed
From: "Björn Töpel" <bjorn.topel@gmail.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: Christoph Hellwig <hch@infradead.org>,
	Netdev <netdev@vger.kernel.org>,
	linux-riscv@lists.infradead.org,
	Palmer Dabbelt <palmer@sifive.com>,
	ast@kernel.org
Subject: Re: [PATCH bpf-next 1/3] bpf, riscv: add BPF JIT for RV64G
Date: Mon, 4 Feb 2019 21:27:30 +0100
Message-ID: <CAJ+HfNhNL9N1GihRBBPei5eD_jA0tigORHE82TrgN5mdUFnMhQ@mail.gmail.com> (raw)
In-Reply-To: <88cdae60-494e-6294-b2c1-10b9cbeb95ac@iogearbox.net>

Den mån 4 feb. 2019 kl 21:06 skrev Daniel Borkmann <daniel@iogearbox.net>:
>
> On 02/03/2019 12:51 PM, bjorn.topel@gmail.com wrote:
> > From: Björn Töpel <bjorn.topel@gmail.com>
> >
> > This commit adds BPF JIT for RV64G.
> >
> > The JIT is a two-pass JIT, and has a dynamic prolog/epilogue (similar
> > to the MIPS64 BPF JIT) instead of static ones (e.g. x86_64).
> >
> > At the moment the RISC-V Linux port does not support HAVE_KPROBES,
> > which means that CONFIG_BPF_EVENTS is not supported. Thus, no tests
> > involving BPF_PROG_TYPE_TRACEPOINT passes.
> >
> > Further, the implementation does not support "far branching" (>4KiB).
> >
> > The implementation passes all the test_bpf.ko tests:
> >   test_bpf: Summary: 378 PASSED, 0 FAILED, [366/366 JIT'ed]
> >
> > All the tail_call tests in the selftest/bpf/test_verifier program
> > passes.
> >
> > All tests where done on QEMU (QEMU emulator version 3.1.50
> > (v3.1.0-688-g8ae951fbc106)).
> >
> > Signed-off-by: Björn Töpel <bjorn.topel@gmail.com>
>
> Some minor comments:
>
> Looks like all the BPF_JMP32 instructions are missing. Would probably
> make sense to include these into the initial merge as well unless there
> is some good reason not to; presumably the test_verifier parts with
> BPF_JMP32 haven't been tried out?
>

Yes indeed. My bad, I didn't realize that Jiong's patches were in the
tree! BPF_JMP32 should definitely be in the initial merge.

> [...]
> > +
> > +enum {
> > +     RV_CTX_F_SEEN_TAIL_CALL =       0,
> > +     RV_CTX_F_SEEN_CALL =            RV_REG_RA,
> > +     RV_CTX_F_SEEN_S1 =              RV_REG_S1,
> > +     RV_CTX_F_SEEN_S2 =              RV_REG_S2,
> > +     RV_CTX_F_SEEN_S3 =              RV_REG_S3,
> > +     RV_CTX_F_SEEN_S4 =              RV_REG_S4,
> > +     RV_CTX_F_SEEN_S5 =              RV_REG_S5,
> > +     RV_CTX_F_SEEN_S6 =              RV_REG_S6,
> > +};
> > +
> > +struct rv_jit_context {
> > +     struct bpf_prog *prog;
> > +     u32 *insns; /* RV insns */
> > +     int ninsns;
> > +     int epilogue_offset;
> > +     int *offset; /* BPF to RV */
> > +     unsigned long flags;
> > +     int stack_size;
> > +};
> > +
> > +struct rv_jit_data {
> > +     struct bpf_binary_header *header;
> > +     u8 *image;
> > +     struct rv_jit_context ctx;
> > +};
> > +
> > +static u8 bpf_to_rv_reg(int bpf_reg, struct rv_jit_context *ctx)
> > +{
> > +     u8 reg = regmap[bpf_reg];
> > +
> > +     switch (reg) {
> > +     case RV_CTX_F_SEEN_S1:
> > +     case RV_CTX_F_SEEN_S2:
> > +     case RV_CTX_F_SEEN_S3:
> > +     case RV_CTX_F_SEEN_S4:
> > +     case RV_CTX_F_SEEN_S5:
> > +     case RV_CTX_F_SEEN_S6:
> > +             __set_bit(reg, &ctx->flags);
> > +     }
> > +     return reg;
> > +};
> > +
> > +static bool seen_reg(int reg, struct rv_jit_context *ctx)
> > +{
> > +     switch (reg) {
> > +     case RV_CTX_F_SEEN_CALL:
> > +     case RV_CTX_F_SEEN_S1:
> > +     case RV_CTX_F_SEEN_S2:
> > +     case RV_CTX_F_SEEN_S3:
> > +     case RV_CTX_F_SEEN_S4:
> > +     case RV_CTX_F_SEEN_S5:
> > +     case RV_CTX_F_SEEN_S6:
> > +             return test_bit(reg, &ctx->flags);
> > +     }
> > +     return false;
> > +}
> > +
> > +static void mark_call(struct rv_jit_context *ctx)
> > +{
> > +     __set_bit(RV_CTX_F_SEEN_CALL, &ctx->flags);
> > +}
> > +
> > +static bool seen_call(struct rv_jit_context *ctx)
> > +{
> > +     return seen_reg(RV_REG_RA, ctx);
> > +}
>
> Just nit: probably might be more obvious to remove this asymmetry in
> seen_reg() and do __set_bit()/test_bit() for RV_CTX_F_SEEN_CALL similar
> like below.
>

Yeah, let's do that.

> > +static void mark_tail_call(struct rv_jit_context *ctx)
> > +{
> > +     __set_bit(RV_CTX_F_SEEN_TAIL_CALL, &ctx->flags);
> > +}
> > +
> > +static bool seen_tail_call(struct rv_jit_context *ctx)
> > +{
> > +     return test_bit(RV_CTX_F_SEEN_TAIL_CALL, &ctx->flags);
> > +}
> > +
> > +static u8 rv_tail_call_reg(struct rv_jit_context *ctx)
> > +{
> > +     mark_tail_call(ctx);
> > +
> > +     if (seen_call(ctx)) {
> > +             __set_bit(RV_CTX_F_SEEN_S6, &ctx->flags);
> > +             return RV_REG_S6;
> > +     }
> > +     return RV_REG_A6;
> > +}
> > +
> > +static void emit(const u32 insn, struct rv_jit_context *ctx)
> > +{
> > +     if (ctx->insns)
> > +             ctx->insns[ctx->ninsns] = insn;
> > +
> > +     ctx->ninsns++;
> > +}
> > +
> > +static u32 rv_r_insn(u8 funct7, u8 rs2, u8 rs1, u8 funct3, u8 rd, u8 opcode)
> > +{
> [...]
> > +     /* Allocate image, now that we know the size. */
> > +     image_size = sizeof(u32) * ctx->ninsns;
> > +     jit_data->header = bpf_jit_binary_alloc(image_size, &jit_data->image,
> > +                                             sizeof(u32),
> > +                                             bpf_fill_ill_insns);
> > +     if (!jit_data->header) {
> > +             prog = orig_prog;
> > +             goto out_offset;
> > +     }
> > +
> > +     /* Second, real pass, that acutally emits the image. */
> > +     ctx->insns = (u32 *)jit_data->image;
> > +skip_init_ctx:
> > +     ctx->ninsns = 0;
> > +
> > +     build_prologue(ctx);
> > +     if (build_body(ctx, extra_pass)) {
> > +             bpf_jit_binary_free(jit_data->header);
> > +             prog = orig_prog;
> > +             goto out_offset;
> > +     }
> > +     build_epilogue(ctx);
> > +
> > +     if (bpf_jit_enable > 1)
> > +             bpf_jit_dump(prog->len, image_size, 2, ctx->insns);
> > +
> > +     prog->bpf_func = (void *)ctx->insns;
> > +     prog->jited = 1;
> > +     prog->jited_len = image_size;
> > +
> > +     bpf_flush_icache(jit_data->header, (u8 *)ctx->insns + ctx->ninsns);
>
> Shouldn't this be '(u32 *)ctx->insns + ctx->ninsns' to cover the range?
>

Yikes! Indeed so, I'll make sure this is corrected!

Thanks for the comments!


Björn

> > +
> > +     if (!prog->is_func || extra_pass) {
> > +out_offset:
> > +             kfree(ctx->offset);
> > +             kfree(jit_data);
> > +             prog->aux->jit_data = NULL;
> > +     }
> > +out:
> > +     if (tmp_blinded)
> > +             bpf_jit_prog_release_other(prog, prog == orig_prog ?
> > +                                        tmp : orig_prog);
> > +     return prog;
> > +}
> >
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

  reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-03 11:51 [PATCH bpf-next 0/3] Add RISC-V (RV64G) BPF JIT bjorn.topel
2019-02-03 11:51 ` [PATCH bpf-next 1/3] bpf, riscv: add BPF JIT for RV64G bjorn.topel
2019-02-03 17:08   ` David Miller
2019-02-04 20:06   ` Daniel Borkmann
2019-02-04 20:27     ` Björn Töpel [this message]
2019-02-03 11:51 ` [PATCH bpf-next 2/3] MAINTAINERS: add RISC-V BPF JIT maintainer bjorn.topel
2019-02-03 17:08   ` David Miller
2019-02-03 11:51 ` [PATCH bpf-next 3/3] bpf, doc: add RISC-V to filter.txt bjorn.topel
2019-02-03 17:08   ` David Miller
2019-02-04 20:09   ` Daniel Borkmann
2019-02-04 20:16     ` Björn Töpel

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAJ+HfNhNL9N1GihRBBPei5eD_jA0tigORHE82TrgN5mdUFnMhQ@mail.gmail.com \
    --to=bjorn.topel@gmail.com \
    --cc=ast@kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=hch@infradead.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=netdev@vger.kernel.org \
    --cc=palmer@sifive.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-RISC-V Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-riscv/0 linux-riscv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-riscv linux-riscv/ https://lore.kernel.org/linux-riscv \
		linux-riscv@lists.infradead.org infradead-linux-riscv@archiver.kernel.org
	public-inbox-index linux-riscv


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-riscv


AGPL code for this site: git clone https://public-inbox.org/ public-inbox