From: Daniel Borkmann <daniel@iogearbox.net>
To: "Björn Töpel" <bjorn.topel@gmail.com>
Cc: Netdev <netdev@vger.kernel.org>,
linux-riscv@lists.infradead.org,
Palmer Dabbelt <palmer@sifive.com>,
davidlee@sifive.com
Subject: Re: [RFC PATCH 3/3] bpf, riscv: added eBPF JIT for RV64G
Date: Wed, 16 Jan 2019 16:41:55 +0100 [thread overview]
Message-ID: <912c38fc-e75d-a7f5-30bc-592e7498e958@iogearbox.net> (raw)
In-Reply-To: <CAJ+HfNg7Ud0RRZQHmbh_DmQDQYNRbSd3nTmNfjO-fX=0o0x-2A@mail.gmail.com>
On 01/16/2019 08:23 AM, Björn Töpel wrote:
> Den ons 16 jan. 2019 kl 00:50 skrev Daniel Borkmann <daniel@iogearbox.net>:
>>
>> 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 <bjorn.topel@gmail.com>
>>> ---
>>> 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 <bjorn.topel@gmail.com>
>>> + *
>>> + */
>>> +
>>> +#include <linux/bpf.h>
>>> +#include <linux/filter.h>
>>> +#include <asm/cacheflush.h>
>>> +
>>> +#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(<reg>) 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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2019-01-16 15:42 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-15 8:35 [RFC PATCH 0/3] RV64G eBPF JIT Björn Töpel
2019-01-15 8:35 ` [RFC PATCH 1/3] riscv: set HAVE_EFFICIENT_UNALIGNED_ACCESS Björn Töpel
2019-01-15 15:39 ` Christoph Hellwig
2019-01-15 16:06 ` Björn Töpel
2019-01-25 20:21 ` Palmer Dabbelt
2019-01-26 1:33 ` Jim Wilson
2019-01-29 2:43 ` Palmer Dabbelt
2019-01-15 8:35 ` [RFC PATCH 2/3] riscv: add build infra for JIT compiler Björn Töpel
2019-01-15 15:43 ` Christoph Hellwig
2019-01-15 16:09 ` Björn Töpel
2019-01-15 8:35 ` [RFC PATCH 3/3] bpf, riscv: added eBPF JIT for RV64G Björn Töpel
2019-01-15 23:49 ` Daniel Borkmann
2019-01-16 7:23 ` Björn Töpel
2019-01-16 15:41 ` Daniel Borkmann [this message]
2019-01-16 19:06 ` Björn Töpel
2019-01-15 15:40 ` [RFC PATCH 0/3] RV64G eBPF JIT Christoph Hellwig
2019-01-15 16:03 ` Björn Töpel
2019-01-25 19:02 ` Palmer Dabbelt
2019-01-25 19:54 ` Paul Walmsley
2019-01-27 12:28 ` Björn Töpel
2019-01-30 2:02 ` Palmer Dabbelt
Reply instructions:
You may reply publicly 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=912c38fc-e75d-a7f5-30bc-592e7498e958@iogearbox.net \
--to=daniel@iogearbox.net \
--cc=bjorn.topel@gmail.com \
--cc=davidlee@sifive.com \
--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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).