Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

michael2012z
Copy link

@michael2012z michael2012z commented Nov 26, 2019

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

@michael2012z
Copy link
Author

Hi, @andreeaflorescu @sameo , no reviewer was assigned automatically. Could you help review this PR?

Copy link
Member

@alxiord alxiord left a 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?)

@alxiord
Copy link
Member

alxiord commented Mar 6, 2020

@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 aarch64 that would need cleaning up too. Would you like to take a look at those as well?

@michael2012z
Copy link
Author

@aghecenco Thank you very much for review and adding the binary. I will come back soon to update this long-waiting PR.

@michael2012z
Copy link
Author

Hi, @aghecenco , the file test_pe.bin turns a HTML text file: https://github.com/aghecenco/linux-loader/blob/aarch64pe/src/loader/test_pe.bin

Could you help again?

@alxiord
Copy link
Member

alxiord commented Mar 9, 2020

Hi, @aghecenco , the file test_pe.bin turns a HTML text file: https://github.com/aghecenco/linux-loader/blob/aarch64pe/src/loader/test_pe.bin

Could you help again?

Oops! Cherry-pick alxiord@2175ff8 instead. I updated it.

@dianpopa
Copy link

dianpopa commented Mar 9, 2020

@aghecenco @michael2012z i strongly encourage you to implement the kernel loading protocols in separate sources (even folders) per architecture.
Right now the hierarchy of the crate looks like this:
root
-> mod.rs (contains kernel loading protocol for all supported archs including unit tests)
-> elf.rs (x86_64 specific)
-> struct_util.rs (x86_64 specific)
-> bootparam.rs (x86_64 specific)
-> test_elf.bin (x86_64 specific)

root
-> x86_64 -> mod.rs, elf.rs, struct_util.rs, bootparam.rs
-> aarhc64 -> mod.rs, test_arm64_pe.bin

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,
Copy link
Author

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.

@michael2012z
Copy link
Author

@aghecenco @michael2012z i strongly encourage you to implement the kernel loading protocols in separate sources (even folders) per architecture.

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.

@alxiord alxiord mentioned this pull request Mar 9, 2020
Signed-off-by: Alexandra Iordache <aghecen@amazon.com>
@michael2012z michael2012z force-pushed the support_aarch64 branch 3 times, most recently from 05f9eae to 726b36c Compare March 10, 2020 05:16
Signed-off-by: Michael Zhao <michael.zhao@arm.com>
@michael2012z
Copy link
Author

michael2012z commented Mar 10, 2020

@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.

@alxiord
Copy link
Member

alxiord commented Mar 12, 2020

@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?

@michael2012z
Copy link
Author

Hi, @aghecenco , no problem, I will close it. Sure, I will continue working in this crate.
And thank you very much for reviewing this PR.

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.

3 participants