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.

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

link,kprobe: add arch prefixes for portable syscall wrapper support #304

Merged
merged 3 commits into from
Jun 10, 2021

Conversation

nickolaev
Copy link
Contributor

@nickolaev nickolaev commented May 14, 2021

Adds basic developer support for ARM64, fixed the tests and added some instructions/notes.

Modify kprobe to be smart about syscall naming, depending on the platform.
For example, it will take sys_getpid and turn it into __arm64_sys_getpid when running on an ARM64 host.

Fixes #266

Signed-off-by: Nikolay Nikolaev nicknickolaev@gmail.com

Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

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

Thanks for making the PR :) Please see my comments, I think changing the ELF builder is not necessary. It's nice that we have working tests on arm64, but we can't claim to support arm64 until we have a working CI.

Makefile Outdated Show resolved Hide resolved
examples/kprobe/fn_amd64.go Outdated Show resolved Hide resolved
link/platform.go Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
examples/kprobe/main.go Outdated Show resolved Hide resolved
link/platform.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Collaborator

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! We're seeing arm being used more and more frequently, but I think this needs some more work in order to become useful.

  • Please remove all merge commits. git fetch origin && git rebase origin/master is the way to keep your branch up-to-date with changes that have landed on master.
  • I don't feel like the additions to CONTRIBUTING.md are necessary. If we want to provide llvm builder images going forward, testdata/docker should output images for both arm64 and x86, which will automatically be picked on hosts/dev machines of those respective architectures.
  • link.K(ret)probe should handle __x64_ syscall fn prefixes transparently, but implementing this might be annoying given the difference between tracefs and PMU, see other comment on the code itself. Please reach out through Cilium Slack (#libbpf-go) if you need more input.

Marking this as draft in the mean time.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
link/perf_event_test.go Show resolved Hide resolved
link/tracepoint_test.go Show resolved Hide resolved
examples/kprobe/main.go Outdated Show resolved Hide resolved
@ti-mo ti-mo marked this pull request as draft May 21, 2021 14:52
@nickolaev nickolaev force-pushed the arm64 branch 2 times, most recently from d15407a to 00b8c66 Compare May 27, 2021 21:43
@nickolaev
Copy link
Contributor Author

@lmb and @ti-mo, I've tried to be creative here and figure a better way to handle syscall naming cross platforms and kernels. Let me know if that aligns with your ideas here.

Copy link
Collaborator

@lmb lmb left a comment

Choose a reason for hiding this comment

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

Please take a look at #304 (comment) which outlines a simpler API.

internal/kernel.go Outdated Show resolved Hide resolved
link/platform.go Outdated Show resolved Hide resolved
link/platform.go Outdated Show resolved Hide resolved
@lmb lmb marked this pull request as ready for review June 7, 2021 11:00
examples/go.mod Outdated Show resolved Hide resolved
link/platform.go Show resolved Hide resolved
link/platform.go Outdated Show resolved Hide resolved
link/platform.go Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Add a basic developer support for ARM64, and possibly other
non-x86 platforms.

Make sure test are passing and the used syscalls are available
on non-x86 hosts.

Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
@lmb lmb merged commit 1e7f01c into cilium:master Jun 10, 2021
@ti-mo
Copy link
Collaborator

ti-mo commented Jun 11, 2021

@nickolaev Thanks for the work!

@ti-mo ti-mo mentioned this pull request Jun 11, 2021
@ti-mo ti-mo changed the title arch: support arm64 link,kprobe: add arch prefixes for portable syscall wrapper support Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: test on arm64
3 participants