From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,FROM_EXCESS_BASE64, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AD41FC43387 for ; Wed, 16 Jan 2019 07:24:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5EA8B20840 for ; Wed, 16 Jan 2019 07:24:11 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="C/k5rc1J" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2388197AbfAPHYK (ORCPT ); Wed, 16 Jan 2019 02:24:10 -0500 Received: from mail-qt1-f196.google.com ([209.85.160.196]:36999 "EHLO mail-qt1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727247AbfAPHYJ (ORCPT ); Wed, 16 Jan 2019 02:24:09 -0500 Received: by mail-qt1-f196.google.com with SMTP id t33so6094202qtt.4 for ; Tue, 15 Jan 2019 23:24:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=dKvOC089S4I4I8Vt8+CwS++1M4BEvZ0vmrrEcdxHbUw=; b=C/k5rc1JX9xpGQQUn3w2hfLxAP/ogBlRhzbxFHbJytbiIxCt+ddNJMQqrKjVTHy7LZ Ixb7P4ECUpZs2YsoP9gHrUhDIy1Qix7DaJ3S9rfxK46Cr10WbkpHAtqTyrV49MThYGQy qxA8pklYnw48wv2KM4IQBuSbHXDuBoLHLqW6UthofxItE54Vj9YSAB9/RZusxldEz2dT cH/z9Rp2uvC/k93n0OzM88xfpvO8d1hYv2DwkEBM8KcALcDPO0VamTUCJ63iIJNO8GpJ Eoy+4hePwyivOITyW9jho4c+hT1eNsquSC7H8PH6waA7pLyv8x4sSxzp9uvXhNUQxBFu vzGA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=dKvOC089S4I4I8Vt8+CwS++1M4BEvZ0vmrrEcdxHbUw=; b=W28ntJbgpzNpNNLVlzlWaqtn+vT58eCGqKyXzInacM0fPs+SMNZyFpqMRGYrQgw6Se Fr6p0X6CI7G8BfPvOPJ7PnaeKN6Pzc/Rcqs+GuLbM6V70I30frk3DLJ5GgyK9lo8anxO qNdouiwFkHIJyjfVUtUF0lTpnEi5d0c9cyLg3iGfIfjJlcDQxwJpS87ZMC4AO5MJb8kg Q75A/YTo0vUUapjeYYyAasL3yCo9t62n9bpnJ5nMve9JhESQsU4ie0J7ELnviKs+Eyhk 9KRvc76HoMLvzisNSkK/uzpD5L45yWnu7GNt0hV0f7khkikkVaqPqWtpNZibheR2izLw udRw== X-Gm-Message-State: AJcUukfyPDfNXc7aCbTmRCKXwU9o/VioWKg/DYwF7h0xhYRFARSKm+pn NnjRTpBObqJtMrZaoYKnVHyynxCwwQr3N+jo4bc= X-Google-Smtp-Source: ALg8bN7lXkTCdRVcq3nyQaWvyRgMRL/n/8NwjIxi4wR54Ulu8atR/rBgOz2+MQXfYOstc2xA6XucBy4o1PDrtwi2wJA= X-Received: by 2002:ac8:4454:: with SMTP id m20mr5809055qtn.381.1547623448004; Tue, 15 Jan 2019 23:24:08 -0800 (PST) MIME-Version: 1.0 References: <20190115083518.10149-1-bjorn.topel@gmail.com> <20190115083518.10149-4-bjorn.topel@gmail.com> In-Reply-To: From: =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= Date: Wed, 16 Jan 2019 08:23:56 +0100 Message-ID: Subject: Re: [RFC PATCH 3/3] bpf, riscv: added eBPF JIT for RV64G To: Daniel Borkmann Cc: linux-riscv@lists.infradead.org, Palmer Dabbelt , davidlee@sifive.com, Netdev Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Den ons 16 jan. 2019 kl 00:50 skrev Daniel Borkmann : > > On 01/15/2019 09:35 AM, Bj=C3=B6rn T=C3=B6pel wrote: > > This commit adds eBPF JIT for RV64G. > > > > Codewise, it needs some refactoring. Currently there's a bit too much > > copy-and-paste going on, and I know some places where I could optimize > > the code generation a bit (mostly BPF_K type of instructions, dealing > > with immediates). > > Nice work! :) > > > From a features perspective, two things are missing: > > > > * tail calls > > * "far-branches", i.e. conditional branches that reach beyond 13b. > > > > The test_bpf.ko passes all tests. > > Did you also check test_verifier under jit with/without jit hardening > enabled? That one contains lots of runtime tests as well. Probably makes > sense to check under CONFIG_BPF_JIT_ALWAYS_ON to see what fails the JIT; > the test_verifier also contains various tail call tests targeted at JITs, > for example. > Good point! I will do that. The only selftests/bpf program that I ran (and passed) was "test_progs". I'll make sure that the complete bpf selftests suite passes as well! > Nit: please definitely also add a MAINTAINERS entry with at least yoursel= f > under BPF JIT section, and update Documentation/sysctl/net.txt with riscv= 64. > Ah! Yes, I'll fix that. > > Signed-off-by: Bj=C3=B6rn T=C3=B6pel > > --- > > arch/riscv/net/bpf_jit_comp.c | 1608 +++++++++++++++++++++++++++++++++ > > 1 file changed, 1608 insertions(+) > > > > diff --git a/arch/riscv/net/bpf_jit_comp.c b/arch/riscv/net/bpf_jit_com= p.c > > index 7e359d3249ee..562d56eb8d23 100644 > > --- a/arch/riscv/net/bpf_jit_comp.c > > +++ b/arch/riscv/net/bpf_jit_comp.c > > @@ -1,4 +1,1612 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * BPF JIT compiler for RV64G > > + * > > + * Copyright(c) 2019 Bj=C3=B6rn T=C3=B6pel > > + * > > + */ > > + > > +#include > > +#include > > +#include > > + > > +#define TMP_REG_0 (MAX_BPF_JIT_REG + 0) > > +#define TMP_REG_1 (MAX_BPF_JIT_REG + 1) > > Not used? > Correct! I'll get rid of them. > > +#define TAIL_CALL_REG (MAX_BPF_JIT_REG + 2) > > + > > +enum rv_register { > > + RV_REG_ZERO =3D 0, /* The constant value 0 */ > > + RV_REG_RA =3D 1, /* Return address */ > > + RV_REG_SP =3D 2, /* Stack pointer */ > > + RV_REG_GP =3D 3, /* Global pointer */ > > + RV_REG_TP =3D 4, /* Thread pointer */ > > + RV_REG_T0 =3D 5, /* Temporaries */ > > + RV_REG_T1 =3D 6, > > + RV_REG_T2 =3D 7, > > + RV_REG_FP =3D 8, > > + RV_REG_S1 =3D 9, /* Saved registers */ > > + RV_REG_A0 =3D 10, /* Function argument/return values */ > > + RV_REG_A1 =3D 11, /* Function arguments */ > > + RV_REG_A2 =3D 12, > > + RV_REG_A3 =3D 13, > > + RV_REG_A4 =3D 14, > > + RV_REG_A5 =3D 15, > > + RV_REG_A6 =3D 16, > > + RV_REG_A7 =3D 17, > > + RV_REG_S2 =3D 18, /* Saved registers */ > > + RV_REG_S3 =3D 19, > > + RV_REG_S4 =3D 20, > > + RV_REG_S5 =3D 21, > > + RV_REG_S6 =3D 22, > > + RV_REG_S7 =3D 23, > > + RV_REG_S8 =3D 24, > > + RV_REG_S9 =3D 25, > > + RV_REG_S10 =3D 26, > > + RV_REG_S11 =3D 27, > > + RV_REG_T3 =3D 28, /* Temporaries */ > > + RV_REG_T4 =3D 29, > > + RV_REG_T5 =3D 30, > > + RV_REG_T6 =3D 31, > > +}; > > + > > +struct rv_jit_context { > > + struct bpf_prog *prog; > > + u32 *insns; /* RV insns */ > > + int ninsns; > > + int epilogue_offset; > > + int *offset; /* BPF to RV */ > > + unsigned long seen_reg_bits; > > + 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) > > +{ > > This one can also be simplified by having a simple mapping as in > other JITs and then mark __set_bit() in the small bpf_to_rv_reg() > helper. > Yeah, I agree. Much better. I'll take that route. > > + switch (bpf_reg) { > > + /* Return value */ > > + case BPF_REG_0: > > + __set_bit(RV_REG_A5, &ctx->seen_reg_bits); > > + return RV_REG_A5; > > + /* Function arguments */ > > + case BPF_REG_1: > > + __set_bit(RV_REG_A0, &ctx->seen_reg_bits); > > + return RV_REG_A0; > > + case BPF_REG_2: > > + __set_bit(RV_REG_A1, &ctx->seen_reg_bits); > > + return RV_REG_A1; > > + case BPF_REG_3: > > + __set_bit(RV_REG_A2, &ctx->seen_reg_bits); > > + return RV_REG_A2; > > + case BPF_REG_4: > > + __set_bit(RV_REG_A3, &ctx->seen_reg_bits); > > + return RV_REG_A3; > > + case BPF_REG_5: > > + __set_bit(RV_REG_A4, &ctx->seen_reg_bits); > > + return RV_REG_A4; > > + /* Callee saved registers */ > > + case BPF_REG_6: > > + __set_bit(RV_REG_S1, &ctx->seen_reg_bits); > > + return RV_REG_S1; > > + case BPF_REG_7: > > + __set_bit(RV_REG_S2, &ctx->seen_reg_bits); > > + return RV_REG_S2; > > + case BPF_REG_8: > > + __set_bit(RV_REG_S3, &ctx->seen_reg_bits); > > + return RV_REG_S3; > > + case BPF_REG_9: > > + __set_bit(RV_REG_S4, &ctx->seen_reg_bits); > > + return RV_REG_S4; > > + /* Stack read-only frame pointer to access stack */ > > + case BPF_REG_FP: > > + __set_bit(RV_REG_S5, &ctx->seen_reg_bits); > > + return RV_REG_S5; > > + /* Temporary register */ > > + case BPF_REG_AX: > > + __set_bit(RV_REG_T0, &ctx->seen_reg_bits); > > + return RV_REG_T0; > > + /* Tail call counter */ > > + case TAIL_CALL_REG: > > + __set_bit(RV_REG_S6, &ctx->seen_reg_bits); > > + return RV_REG_S6; > > + default: > > + return 0; > > + } > > +}; > [...] > > + /* tail call */ > > + case BPF_JMP | BPF_TAIL_CALL: > > + rd =3D bpf_to_rv_reg(TAIL_CALL_REG, ctx); > > + pr_err("bpf-jit: tail call not supported yet!\n"); > > + return -1; > > There are two options here, either fixed size prologue where you can > then jump over it in tail call case, or dynamic one which would make > it slower due to reg restore but shrinks image for non-tail calls. > So, it would be the latter then, which is pretty much like a more expensive (due to the tail call depth checks) function call. For the fixed prologue: how does, say x86, deal with BPF stack usage in the tail call case? If the caller doesn't use the bpf stack, but the callee does. From a quick glance in the code, the x86 prologue still uses aux->stack_depth. If the callee has a different stack usage that the caller, and then the callee does a function call, wouldn't this mess up the frame? (Yeah, obviously missing something! :-)) > > + /* function return */ > > + case BPF_JMP | BPF_EXIT: > > + if (i =3D=3D ctx->prog->len - 1) > > + break; > > + > > + rvoff =3D epilogue_offset(ctx); > > + if (!is_21b_int(rvoff)) { > > + pr_err("bpf-jit: %d offset=3D%d not supported yet= !\n", > > + __LINE__, rvoff); > > + return -1; > > + } > > + > > + emit(rv_jal(RV_REG_ZERO, rvoff >> 1), ctx); > > + break; > > + > > + /* dst =3D imm64 */ > > + case BPF_LD | BPF_IMM | BPF_DW: > > + { > > + struct bpf_insn insn1 =3D insn[1]; > > + u64 imm64; > > + > [...] > > + > > +static void build_prologue(struct rv_jit_context *ctx) > > +{ > > + int stack_adjust =3D 0, store_offset, bpf_stack_adjust; > > + > > + if (seen_reg(RV_REG_RA, ctx)) > > + stack_adjust +=3D 8; > > + stack_adjust +=3D 8; /* RV_REG_FP */ > > + if (seen_reg(RV_REG_S1, ctx)) > > + stack_adjust +=3D 8; > > + if (seen_reg(RV_REG_S2, ctx)) > > + stack_adjust +=3D 8; > > + if (seen_reg(RV_REG_S3, ctx)) > > + stack_adjust +=3D 8; > > + if (seen_reg(RV_REG_S4, ctx)) > > + stack_adjust +=3D 8; > > + if (seen_reg(RV_REG_S5, ctx)) > > + stack_adjust +=3D 8; > > + if (seen_reg(RV_REG_S6, ctx)) > > + stack_adjust +=3D 8; > > + > > + stack_adjust =3D round_up(stack_adjust, 16); > > + bpf_stack_adjust =3D round_up(ctx->prog->aux->stack_depth, 16); > > + stack_adjust +=3D bpf_stack_adjust; > > + > > + store_offset =3D stack_adjust - 8; > > + > > + emit(rv_addi(RV_REG_SP, RV_REG_SP, -stack_adjust), ctx); > > + > > + if (seen_reg(RV_REG_RA, ctx)) { > > + emit(rv_sd(RV_REG_SP, store_offset, RV_REG_RA), ctx); > > + store_offset -=3D 8; > > + } > > + emit(rv_sd(RV_REG_SP, store_offset, RV_REG_FP), ctx); > > + store_offset -=3D 8; > > + if (seen_reg(RV_REG_S1, ctx)) { > > + emit(rv_sd(RV_REG_SP, store_offset, RV_REG_S1), ctx); > > + store_offset -=3D 8; > > + } > > + if (seen_reg(RV_REG_S2, ctx)) { > > + emit(rv_sd(RV_REG_SP, store_offset, RV_REG_S2), ctx); > > + store_offset -=3D 8; > > + } > > + if (seen_reg(RV_REG_S3, ctx)) { > > + emit(rv_sd(RV_REG_SP, store_offset, RV_REG_S3), ctx); > > + store_offset -=3D 8; > > + } > > + if (seen_reg(RV_REG_S4, ctx)) { > > + emit(rv_sd(RV_REG_SP, store_offset, RV_REG_S4), ctx); > > + store_offset -=3D 8; > > + } > > + if (seen_reg(RV_REG_S5, ctx)) { > > + emit(rv_sd(RV_REG_SP, store_offset, RV_REG_S5), ctx); > > + store_offset -=3D 8; > > + } > > + if (seen_reg(RV_REG_S6, ctx)) { > > + emit(rv_sd(RV_REG_SP, store_offset, RV_REG_S6), ctx); > > + store_offset -=3D 8; > > + } > > + > > + emit(rv_addi(RV_REG_FP, RV_REG_SP, stack_adjust), ctx); > > + > > + if (bpf_stack_adjust) { > > + if (!seen_reg(RV_REG_S5, ctx)) > > + pr_warn("bpf-jit: not seen BPF_REG_FP, stack is %= d\n", > > + bpf_stack_adjust); > > + emit(rv_addi(RV_REG_S5, RV_REG_SP, bpf_stack_adjust), ctx= ); > > + } > > + > > + ctx->stack_size =3D stack_adjust; > > +} > > + > > +static void build_epilogue(struct rv_jit_context *ctx) > > +{ > > + int stack_adjust =3D ctx->stack_size, store_offset =3D stack_adju= st - 8; > > + > > + if (seen_reg(RV_REG_RA, ctx)) { > > + emit(rv_ld(RV_REG_RA, store_offset, RV_REG_SP), ctx); > > + store_offset -=3D 8; > > + } > > + emit(rv_ld(RV_REG_FP, store_offset, RV_REG_SP), ctx); > > + store_offset -=3D 8; > > + if (seen_reg(RV_REG_S1, ctx)) { > > + emit(rv_ld(RV_REG_S1, store_offset, RV_REG_SP), ctx); > > + store_offset -=3D 8; > > + } > > + if (seen_reg(RV_REG_S2, ctx)) { > > + emit(rv_ld(RV_REG_S2, store_offset, RV_REG_SP), ctx); > > + store_offset -=3D 8; > > + } > > + if (seen_reg(RV_REG_S3, ctx)) { > > + emit(rv_ld(RV_REG_S3, store_offset, RV_REG_SP), ctx); > > + store_offset -=3D 8; > > + } > > + if (seen_reg(RV_REG_S4, ctx)) { > > + emit(rv_ld(RV_REG_S4, store_offset, RV_REG_SP), ctx); > > + store_offset -=3D 8; > > + } > > + if (seen_reg(RV_REG_S5, ctx)) { > > + emit(rv_ld(RV_REG_S5, store_offset, RV_REG_SP), ctx); > > + store_offset -=3D 8; > > + } > > + if (seen_reg(RV_REG_S6, ctx)) { > > + emit(rv_ld(RV_REG_S6, store_offset, RV_REG_SP), ctx); > > + store_offset -=3D 8; > > + } > > + > > + emit(rv_addi(RV_REG_SP, RV_REG_SP, stack_adjust), ctx); > > + /* Set return value. */ > > + emit(rv_addi(RV_REG_A0, RV_REG_A5, 0), ctx); > > + emit(rv_jalr(RV_REG_ZERO, RV_REG_RA, 0), ctx); > > +} > > + > > +static int build_body(struct rv_jit_context *ctx, bool extra_pass) > > +{ > > + const struct bpf_prog *prog =3D ctx->prog; > > + int i; > > + > > + for (i =3D 0; i < prog->len; i++) { > > + const struct bpf_insn *insn =3D &prog->insnsi[i]; > > + int ret; > > + > > + ret =3D emit_insn(insn, ctx, extra_pass); > > + if (ret > 0) { > > + i++; > > + if (ctx->insns =3D=3D NULL) > > + ctx->offset[i] =3D ctx->ninsns; > > + continue; > > + } > > + if (ctx->insns =3D=3D NULL) > > + ctx->offset[i] =3D ctx->ninsns; > > + if (ret) > > + return ret; > > + } > > + return 0; > > +} > > + > > +static void bpf_fill_ill_insns(void *area, unsigned int size) > > +{ > > + memset(area, 0, size); > > Needs update as well? > No, bitpattern of all zeros is an illegal instruction, but a comment would be good! > > +} > > + > > +static void bpf_flush_icache(void *start, void *end) > > +{ > > + flush_icache_range((unsigned long)start, (unsigned long)end); > > +} > > + Thanks a lot for the comments, Daniel! I'll get back with a v2. Bj=C3=B6rn