Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1670157 > unrolled thread

Re: [PATCH v2] arm: eBPF JIT compiler

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] arm: eBPF JIT compiler Shubham Bansal <illusionist.neo@gmail.com> - 2017-06-20 03:40 +0200
    Re: [PATCH v2] arm: eBPF JIT compiler Daniel Borkmann <daniel@iogearbox.net> - 2017-06-20 19:00 +0200
      Re: [PATCH v2] arm: eBPF JIT compiler Shubham Bansal <illusionist.neo@gmail.com> - 2017-06-21 16:30 +0200
        Re: [PATCH v2] arm: eBPF JIT compiler Daniel Borkmann <daniel@iogearbox.net> - 2017-06-21 18:40 +0200
          Re: [PATCH v2] arm: eBPF JIT compiler Shubham Bansal <illusionist.neo@gmail.com> - 2017-06-21 21:40 +0200
            Re: [PATCH v2] arm: eBPF JIT compiler Daniel Borkmann <daniel@iogearbox.net> - 2017-06-21 22:00 +0200

#1670157 — Re: [PATCH v2] arm: eBPF JIT compiler

Hi Daniel,

>
> Sorry, had a travel over the weekend, so didn't read it in time.
>
> What is the issue with imitating in JIT what the interpreter is
> doing as a starting point? That should be generic enough to handle
> any case.
>
> Otherwise you'd need some sort of reverse mapping since verifier
> already converted BPF_CALL insns into relative helper addresses
> in imm part.
>
Sorry but I don't get what you are trying to say. Can you explain it
with an example?

-Shubham

[toc] | [next] | [standalone]

#1670957

On 06/20/2017 03:34 AM, Shubham Bansal wrote:
> Hi Daniel,
>
>> Sorry, had a travel over the weekend, so didn't read it in time.
>>
>> What is the issue with imitating in JIT what the interpreter is
>> doing as a starting point? That should be generic enough to handle
>> any case.

Why not proceeding this way first?

>> Otherwise you'd need some sort of reverse mapping since verifier
>> already converted BPF_CALL insns into relative helper addresses
>> in imm part.
>>
> Sorry but I don't get what you are trying to say. Can you explain it
> with an example?

Ok, probably the best is to check fixup_bpf_calls() in the verifier,
see the fn = prog->aux->ops->get_func_proto(insn->imm). It fetches the
helper function specification based on the BPF_FUNC_* enum and converts
the imm field into a relative address for the function such that if
you look at ___bpf_prog_run(), JMP_CALL label, the call address can
be reconstructed again. So you'd need some reverse mapping to get back
to the struct bpf_func_proto, so you can check argX_type that needs to
be extended with whether its JITable on 32bit or not.

[toc] | [prev] | [next] | [standalone]

#1671706

Hi Daniel,

>
> So my question would be, why can't the JIT imitate something
> similar to what we do in the interpreter as well? So looking
> at the disasm of what gcc compiles for the interpreter when it's
> doing the above call could help as well in going forward. Not
> sure if that answers your question, but perhaps not sure if I
> understand your question yet?

I just looked at the code again and I think I completely misunderstood
the logic of  BPF_JMP | BPF_CALL.
I think each helper function is working like this.

____helper_function(u32 a1, u32 a2){
}

helper_function(u64 a1, u64 a2){
     ____helper_function((u32 *)a1, (u32 *)a2);
}

So ultimately, we call helper_function which takes u64 as arguments
only. I know its asking a lot, but can you please confirm this asap? I
would like to start implementing it.

>
> Cheers,
> Daniel

-Shubham

[toc] | [prev] | [next] | [standalone]

#1671788

On 06/21/2017 04:26 PM, Shubham Bansal wrote:
[...]
> So ultimately, we call helper_function which takes u64 as arguments
> only. I know its asking a lot, but can you please confirm this asap? I
> would like to start implementing it.

Yes, that is correct. I think it would be the better, more generic
approach going forward to always assume that.

[toc] | [prev] | [next] | [standalone]

#1671984

Hi Daniel,

Good news. Got the CALL to work.

[  145.670882] test_bpf: Summary: 316 PASSED, 0 FAILED, [287/308 JIT'ed]

Awesome. Do you think with this implementation, the patch could get
accepted? If you think so, then I will send the patch in couple of
days after some refactoring, if not, then do let me know what more is
required?

Best,
Shubham Bansal


On Wed, Jun 21, 2017 at 10:02 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 06/21/2017 04:26 PM, Shubham Bansal wrote:
> [...]
>>
>> So ultimately, we call helper_function which takes u64 as arguments
>> only. I know its asking a lot, but can you please confirm this asap? I
>> would like to start implementing it.
>
>
> Yes, that is correct. I think it would be the better, more generic
> approach going forward to always assume that.

[toc] | [prev] | [next] | [standalone]

#1671993

On 06/21/2017 09:37 PM, Shubham Bansal wrote:
> Hi Daniel,
>
> Good news. Got the CALL to work.
>
> [  145.670882] test_bpf: Summary: 316 PASSED, 0 FAILED, [287/308 JIT'ed]
>
> Awesome. Do you think with this implementation, the patch could get
> accepted? If you think so, then I will send the patch in couple of
> days after some refactoring, if not, then do let me know what more is
> required?

Nice, it's ultimately up to the arm folks to review the set in-depth,
but feel free to send out the patch once you're done refactoring. With
BPF_CALL support that looks quite good from pov of supported insns.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web