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=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 AA29DC43387 for ; Wed, 16 Jan 2019 15:42:01 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 7459A20675 for ; Wed, 16 Jan 2019 15:42:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2394148AbfAPPmA (ORCPT ); Wed, 16 Jan 2019 10:42:00 -0500 Received: from www62.your-server.de ([213.133.104.62]:48072 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728412AbfAPPmA (ORCPT ); Wed, 16 Jan 2019 10:42:00 -0500 Received: from [78.46.172.3] (helo=sslproxy06.your-server.de) by www62.your-server.de with esmtpsa (TLSv1.2:DHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89_1) (envelope-from ) id 1gjnKG-0004YI-Hj; Wed, 16 Jan 2019 16:41:56 +0100 Received: from [2a02:1203:ecb1:b710:c81f:d2d6:50a9:c2d] (helo=linux.home) by sslproxy06.your-server.de with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89) (envelope-from ) id 1gjnKG-000106-Bo; Wed, 16 Jan 2019 16:41:56 +0100 Subject: Re: [RFC PATCH 3/3] bpf, riscv: added eBPF JIT for RV64G To: =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= Cc: linux-riscv@lists.infradead.org, Palmer Dabbelt , davidlee@sifive.com, Netdev References: <20190115083518.10149-1-bjorn.topel@gmail.com> <20190115083518.10149-4-bjorn.topel@gmail.com> From: Daniel Borkmann Message-ID: <912c38fc-e75d-a7f5-30bc-592e7498e958@iogearbox.net> Date: Wed, 16 Jan 2019 16:41:55 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Authenticated-Sender: daniel@iogearbox.net X-Virus-Scanned: Clear (ClamAV 0.100.2/25303/Wed Jan 16 11:51:22 2019) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 01/16/2019 08:23 AM, Björn Töpel wrote: > Den ons 16 jan. 2019 kl 00:50 skrev Daniel Borkmann : >> >> On 01/15/2019 09:35 AM, Björn Töpel 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 yourself >> under BPF JIT section, and update Documentation/sysctl/net.txt with riscv64. >> > > Ah! Yes, I'll fix that. > >>> Signed-off-by: Björn Töpel >>> --- >>> 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_comp.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örn Töpel >>> + * >>> + */ >>> + >>> +#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 = 0, /* The constant value 0 */ >>> + RV_REG_RA = 1, /* Return address */ >>> + RV_REG_SP = 2, /* Stack pointer */ >>> + RV_REG_GP = 3, /* Global pointer */ >>> + RV_REG_TP = 4, /* Thread pointer */ >>> + RV_REG_T0 = 5, /* Temporaries */ >>> + RV_REG_T1 = 6, >>> + RV_REG_T2 = 7, >>> + RV_REG_FP = 8, >>> + RV_REG_S1 = 9, /* Saved registers */ >>> + RV_REG_A0 = 10, /* Function argument/return values */ >>> + RV_REG_A1 = 11, /* Function arguments */ >>> + RV_REG_A2 = 12, >>> + RV_REG_A3 = 13, >>> + RV_REG_A4 = 14, >>> + RV_REG_A5 = 15, >>> + RV_REG_A6 = 16, >>> + RV_REG_A7 = 17, >>> + RV_REG_S2 = 18, /* Saved registers */ >>> + RV_REG_S3 = 19, >>> + RV_REG_S4 = 20, >>> + RV_REG_S5 = 21, >>> + RV_REG_S6 = 22, >>> + RV_REG_S7 = 23, >>> + RV_REG_S8 = 24, >>> + RV_REG_S9 = 25, >>> + RV_REG_S10 = 26, >>> + RV_REG_S11 = 27, >>> + RV_REG_T3 = 28, /* Temporaries */ >>> + RV_REG_T4 = 29, >>> + RV_REG_T5 = 30, >>> + RV_REG_T6 = 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 = 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. Right. > 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! :-)) Basically in this case verifier sets stack size to MAX_BPF_STACK when it finds a tail call in the prog, meaning the callee will be reusing <= stack size than the caller and then upon exit unwinds it via leave+ret. Cheers, Daniel