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

Add support for custom data relocation #170

Closed
wants to merge 4 commits into from

Conversation

Alan-Jowett
Copy link
Collaborator

@Alan-Jowett Alan-Jowett commented Oct 21, 2022

Adding support for R_BPF_64_64 relocations via custom callback.

Resolves: #45
Resolves: #54

With this change, uBPF can be used with an external map implementation.

Signed-off-by: Alan Jowett alanjo@microsoft.com

@coveralls
Copy link

coveralls commented Oct 21, 2022

Coverage Status

Coverage: 81.321% (-2.5%) from 83.784% when pulling 799c0d2 on Alan-Jowett:issue45 into e17e8bb on iovisor:main.

@Alan-Jowett Alan-Jowett changed the title [WIP] Add support for custom data relocation Add support for custom data relocation Oct 21, 2022
@Alan-Jowett Alan-Jowett marked this pull request as ready for review October 21, 2022 20:03
@Alan-Jowett
Copy link
Collaborator Author

Marking this as ready for review. Codecov.io is having an outage, so coverage upload is currently failing.

@Alan-Jowett Alan-Jowett enabled auto-merge (squash) December 3, 2022 19:50
@hawkinsw hawkinsw mentioned this pull request May 3, 2023
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.

I think that the PR you have here will work really well for the use case targeted by #245. I hope the little bit of feedback that I gave will be helpful!

Will

vm/inc/ubpf.h Show resolved Hide resolved
vm/ubpf_loader.c Show resolved Hide resolved
@hawkinsw
Copy link
Collaborator

hawkinsw commented May 9, 2023

I am more than happy to adjust the PR and shepherd it through final preparation if that would help you in any way, @Alan-Jowett !! Just let me know if/how I can help!
Will

@hawkinsw
Copy link
Collaborator

Hello @Alan-Jowett -- I don't mean to bother you but I wanted to see if there was anything that I could do to help! Just let me know if I can!
Will

@Alan-Jowett
Copy link
Collaborator Author

@hawkinsw Thanks for reviewing this. I will try to set aside some time and update this PR sometime this week.

@hawkinsw
Copy link
Collaborator

@hawkinsw Thanks for reviewing this. I will try to set aside some time and update this PR sometime this week.

Please don't worry! I just wanted offer help if I can!

Alan-Jowett and others added 3 commits May 19, 2023 14:33
Signed-off-by: Alan Jowett <alanjo@microsoft.com>
Signed-off-by: Alan Jowett <alanjo@microsoft.com>
Signed-off-by: Alan Jowett <atjowet@gmail.com>
Signed-off-by: Alan Jowett <atjowet@gmail.com>
@Alan-Jowett
Copy link
Collaborator Author

@hawkinsw if the changes look good, could you marked the review as approved? As long as I get at least 1 reviewer, I am willing to merge this.

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.

Everything is looking good! I just left a few questions. I am on an international flight finishing up a work trip and hope that the feedback was cogent. Thanks again for all your work and for letting me participate!

vm/inc/ubpf.h Show resolved Hide resolved
vm/ubpf_loader.c Show resolved Hide resolved
uint64_t size = sym.st_size;
uint64_t imm =
vm->data_relocation_function(vm->data_relocation_user_data, data, size, sym_name, sym.st_value);
inst->imm = (uint32_t)imm;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wanted to confirm: It looks like you are passing the size of the symbol here and not the size of the section. (see above).


/**
* @brief Data relocation function that is called by the VM when it encounters a
* R_BPF_64_64 relocation in the maps section of the ELF file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to be this specific in the documentation? It may be that the use case is completely orthogonal to maps. For instance, what I am using it for is independent of maps (it just targets some data). (see below)

@hawkinsw
Copy link
Collaborator

@Alan-Jowett See #260 -- I hope it helps!! Thank you so much for letting me work on this project with you!

auto-merge was automatically disabled May 22, 2023 19:29

Pull request was closed

@Alan-Jowett Alan-Jowett deleted the issue45 branch May 22, 2023 19:29
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.

What is the simplest prog.c can be passed to vm/test? ubpf needs to support maps
3 participants