-
Notifications
You must be signed in to change notification settings - Fork 57
Support raw image (vmlinux) loading for Aarch64 #9
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
Conversation
Hi, @andreeaflorescu @sameo , no reviewer was assigned automatically. Could you help review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Late to the party, here goes round 1: I'm still considering whether we should have separate sources per-arch 🤔 In any case, I have a bunch of nits, including the commit message with what looks like an autogenerated change ID (can you please remove it?)
@michael2012z you can cherry pick alxiord@d4b9720 for the kernel binary. The unit tests pass with this commit, however there are a bunch of build warnings on |
@aghecenco Thank you very much for review and adding the binary. I will come back soon to update this long-waiting PR. |
Hi, @aghecenco , the file Could you help again? |
Oops! Cherry-pick alxiord@2175ff8 instead. I updated it. |
6b0576f
to
093dec3
Compare
@aghecenco @michael2012z i strongly encourage you to implement the kernel loading protocols in separate sources (even folders) per architecture. root I think the second proposal would reflect a much cleaner hierarchy, would make the code much easier to read since it gets rid of lots of cfg syntax and it is also encourages extensibility. |
/// Invalid ELF magic number | ||
InvalidElfMagicNumber, | ||
/// Invalid magic number | ||
InvalidMagicNumber, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed it to from InvalidElfMagicNumber
to InvalidMagicNumber
, a more generic one. Because an error message is needed when parsing ARM kernel, but defining a new InvalidPeMagicNumber
looks too verbose.
This echos an earlier comment here by @aghecenco : #9 (review) That sounds a good idea. Now ARM code has very little in common with X86 staff. |
Signed-off-by: Alexandra Iordache <aghecen@amazon.com>
05f9eae
to
726b36c
Compare
Signed-off-by: Michael Zhao <michael.zhao@arm.com>
726b36c
to
db8e815
Compare
@dianpopa , @aghecenco , Thanks for reviewing the code. I handled the comments about code that have been placed so far. And errors and warnings were all cleared. Next I am going to refactor the repo to divide source code files by architecture. I tried that a bit, looks like some effort is needed to finish. Considering now there are 2 PR's doing the same job (#16 and this), I think it's better to merge one of them at first to avoid investing more effort (to code and review) in a PR but it is finally given up. |
@michael2012z I compared the changes with #16 and to me the image loading part looks identical functionality-wise, but it's slightly compact in the newer PR and I expect it to be easier to rebase #26 on top of it. There's also the added device tree blob loader. Would you mind closing this in favor of #16 and reviewing it, as you're clearly more familiar with the topic? |
Hi, @aghecenco , no problem, I will close it. Sure, I will continue working in this crate. |
Add support for Aarch64 to load vmlinux in PE format.
Reference to this Firecracker commit.
Note: In unit test, a binary file ("src/loader/test_arm64_pe.bin") is loaded. (For x86 test, it is "test_elf.bin") But as per our company's policy, I can't upload any binary file to a public repository. Please any maintainer help download the file from here and upload. Thanks.
Signed-off-by: Michael Zhao michael.zhao@arm.com