feat: Add bpf to bpf call support #122
Conversation
qmonnet
left a comment
qmonnet
left a comment
There was a problem hiding this comment.
Thanks! Looks good, although I haven't looked into the details yet.
I'd really like to have support in the JIT before merging function calls support though, or we'll lose feature parity.
|
Ok, maybe I can try to add JIT related code although I am not very familiar with JIT. |
|
It's not trivial, you'll have to look into some assembly code - but it would be awesome if you could do that, thanks for looking! uBPF supports it, you're welcome to base your code on theirs (the JIT in rbpf is largely adapted from uBPF already); no kernel code though (GPL). |
|
Thank you, it doesn't look very difficult. I should be able to solve it. |
Godones
force-pushed
the
dev
branch
2 times, most recently
from
5b0baf2 to
5cf8564
Compare
|
Hi, I've added support for function calls to jit and written two simple tests. |
|
Thanks a lot! I need to find some time to look at this in details, this will likely take me a few days. Don't hesitate to ping me next week if I haven't reviewed. |
|
Hi, do you have time to take a look at this PR. I would like to add some features after this PR is received. |
qmonnet
left a comment
qmonnet
left a comment
There was a problem hiding this comment.
Great work, thank you!
I've got a number of comments, don't worry - most of them are minor. A few are more important:
- I think we miss a check on remaining stack size when doing a function call, in the interpreter
- Would be nice to get a few more tests, to check what happens when we mess up with stack size
- We miss the update for the assembler, disassembler, crane-based JIT compiler (but that's OK if they're not in this PR)
- I'm not sure I understand the use case for the custom stack size validator, would you mind explaining please?
As a side note, you mentioned you had other features in mind, what are they? And if I may ask, what's your use case for rbpf?
|
The new feature I would like to add is to allow jit to be used in a no_std environment. In this way, we can use rbpf in rust-implemented os. |
Like ubpf, we can check the correctness of ebpf calls and add support for ebpf to ebpf calls. To do this, a `StackFrame` data structure is added to save the context when making function calls. Currently, the maximum nesting level of function calls is 8. In addition, function calls require the stack to be protected. Therefore, when entering a new function, the stack pointer will be moved. Since it is impossible to accurately calculate the stack size used by the current function, the current stack usage calculator gives a default value: 256 bytes. Users can customize `StackUsageCalculator` to override the current default value. For a simple example: when executing multiple simple function calls, since these functions usually do not use the stack, in theory the stack space will not be exhausted, the user can return 0 in the customized `StackUsageCalculator`, so that the execution of the ebpf program will not cause a stack overflow. Signed-off-by: Godones <[email protected]>
Signed-off-by: Godones <[email protected]>
Signed-off-by: Godones <[email protected]>
qmonnet
left a comment
qmonnet
left a comment
There was a problem hiding this comment.
Looks in good shape, thanks! Could I ask you to move the fixes from commits 4 and 5 to the relevant, previous commits, so that we end up with only 4 commits, and commit 4 just contains the assembler/disassembler bits (maybe adjust the commit title as well), please?
Let's also give uBPF folks a couple days to reply. This 16-bytes alignment is bothering me, in particular because they also make sure to preserve it across function calls, which is not the case in this PR.
Move my tests into misc. add comment and fix the tests error Signed-off-by: Godones <[email protected]>
Signed-off-by: Godones <[email protected]>
qmonnet
left a comment
qmonnet
left a comment
There was a problem hiding this comment.
Looks good, thank you!
Just one question:
I think a custom stack calculator is useful. Of course, if we had an accurate way to calculate the stack used by a function, we wouldn't need it.
Do you actually use the custom stack calculator? If not, and especially now that we removed the alignment requirement and don't do any checks, I consider dropping it until someone comes up with a use case for it (I can do the change after we've merged the PR in its current state though).
|
Sorry, I don't have an example right now, but I think one will come soon. Let's keep it for now, shall we? |
2 participants
Like ubpf, we can check the correctness of bpf calls and add support for bpf to bpf calls.