Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add bpf2c support for non-inlined local function calls #3506

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

add bpf2c support for non-inlined local function calls #3506

wants to merge 16 commits into from

Conversation

dthaler
Copy link
Collaborator

@dthaler dthaler commented Apr 28, 2024 •
edited

Description

Previous to this PR, bpf2c only supported inlined function calls, not bpf2bpf function calls.
PREVAIL support for non-inlined function calls is in vbpf/ebpf-verifier#608 which should be merged before this PR.
This PR adds support in bpf2c, as well as a bindmonitor_bpf2bpf.c test sample.

A few changes are prerequisites towards the ability to have multiple programs in the same ELF section (issue #1440), such as the fact that there can be multiple program_t structures per section_t in bpf2c. But #1440 also requires support in PREVAIL for vbpf/ebpf-verifier#393 which hasn't been done yet so the rest of #1440 is left for after that.

Fixes #3388

Testing

This PR adds a bindmonitor_bpf2bpf.c test sample

Documentation

This PR includes a doc update to isa-support.rst

Installation

No impact

@dthaler dthaler marked this pull request as draft April 28, 2024 22:30
hawkinsw
hawkinsw reviewed Apr 29, 2024
View reviewed changes
Copy link
Collaborator

@hawkinsw hawkinsw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment about ubpf support for calling local functions.

docs/isa-support.rst Outdated Show resolved Hide resolved
@dthaler dthaler force-pushed the calllocal branch 4 times, most recently from 56ad36d to e72b0a8 Compare April 30, 2024 22:14
dthaler added 11 commits May 6, 2024 11:24
@dthaler
Add bpf2c support for non-inlined local function calls
388f1ad 
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
@dthaler
Generate code for subprograms
8abc1db 
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
@dthaler
Add more of implementation
a6bae3e 
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
@dthaler
Add test sample
d4cda3c 
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
@dthaler
Enable conformance test for call local
f61ecf6 
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
@dthaler
uBPF supports call local
0a2a912 
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
@dthaler
Bug fix
86ea4e7 
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
@dthaler
Temporarily switch to dthaler fork
4187eaa 
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
@dthaler
Bug fix
01c334e 
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
@dthaler
Update expected output
c8b5fa8 
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
@dthaler
More fixes
8e55999 
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
@dthaler
Fix negative bpf2c tests
88c5ce8 
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
@dthaler
Cleanup
c3486b3 
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
@dthaler dthaler marked this pull request as ready for review May 7, 2024 15:03
@Alan-Jowett
Copy link
Member

I strongly suspect this change won't pass conformance tests once Alan-Jowett/bpf_conformance#245 merges.

The BPF ISA appears to assume that the JIT compiler adjusts R10 when calling the child function. The code needs to be modified to do one of two things:

  1. Each frame gets its own 512 of stack space.
  2. Caller and callee share stack space, but caller adjusts R10 to highest used address before calling child.

@dthaler
Copy link
Collaborator Author

dthaler commented May 11, 2024

I strongly suspect this change won't pass conformance tests once Alan-Jowett/bpf_conformance#245 merges.

If so, I think that PR should be fixed.

The BPF ISA appears to assume that the JIT compiler adjusts R10 when calling the child function.

The BPF ISA has no such assumption. See https://datatracker.ietf.org/doc/draft-ietf-bpf-isa/
The ISA itself does not define any specific meaning for R10 or any specific stack size.
That is the psABI not the ISA and in my opinion any conformance tests for ISA vs psABI should be separate.
That's because per https://datatracker.ietf.org/wg/bpf/about/ the ABI is intended to be a standard but the psABI is just informational so may vary by runtime.

The code needs to be modified to do one of two things:

  1. Each frame gets its own 512 of stack space.
  2. Caller and callee share stack space, but caller adjusts R10 to highest used address before calling child.

This might be needed to match the Linux psABI but not for conformance to the ABI. As such, I'd treat them as part of a separate pull request that may need additional verifier support.

@dthaler dthaler mentioned this pull request May 11, 2024
Closed
@dthaler
Add test for bpf2bpf behavior
1e1a63f 
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
@dthaler
Fix bug to make new test pass
10649e5 
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
@dthaler
Update isa-support.rst
b120970 
uBPF supports call local for raw byte code but not when loaded from ELF

Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkinsw hawkinsw hawkinsw left review comments

@Alan-Jowett Alan-Jowett Alan-Jowett approved these changes

@poornagmsft poornagmsft Awaiting requested review from poornagmsft poornagmsft is a code owner

@saxena-anurag saxena-anurag Awaiting requested review from saxena-anurag saxena-anurag is a code owner

@shankarseal shankarseal Awaiting requested review from shankarseal shankarseal is a code owner

@dv-msft dv-msft Awaiting requested review from dv-msft dv-msft is a code owner

@gtrevi gtrevi Awaiting requested review from gtrevi gtrevi is a code owner

@shpalani shpalani Awaiting requested review from shpalani shpalani is a code owner

@matthewige matthewige Awaiting requested review from matthewige matthewige is a code owner

@mtfriesen mtfriesen Awaiting requested review from mtfriesen mtfriesen is a code owner

@rectified95 rectified95 Awaiting requested review from rectified95 rectified95 is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bpf2c should support bpf2bpf function calls
3 participants
@dthaler @Alan-Jowett @hawkinsw