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

Linux loader integration #2470

Merged
merged 6 commits into from
Sep 27, 2021
Merged

Linux loader integration #2470

merged 6 commits into from
Sep 27, 2021

Conversation

AlexandruCihodaru
Copy link
Contributor

@AlexandruCihodaru AlexandruCihodaru commented Feb 23, 2021 •
edited by dianpopa
Loading

Reason for This PR

Replaced kernel crate with rust-vmm/linux-loader
See #1182.

Description of Changes

Added github dependency for rust-vmm/linux-loader
Deleted kernel crate from Firecracker
Use linux-loader to write bootparams
Integrated rust-vmm/linux-loader in vmm crate

As for right now the snapshot tests will not pass since the rust compiler uses upstream implementations of traits from vm-memory.

  • This functionality can be added in rust-vmm.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any newly added unsafe code is properly documented.
  • Any API changes are reflected in firecracker/swagger.yaml.
  • Any user-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

@AlexandruCihodaru AlexandruCihodaru self-assigned this Feb 24, 2021
@andreeaflorescu
Copy link
Member

We just yanked v0.2.0 of linux-loader. Before merging the PR, please make sure to use the latest available version v0.3.0.

@alindima alindima self-requested a review February 26, 2021 10:34
@dianpopa dianpopa self-requested a review March 2, 2021 18:56
dianpopa
dianpopa reviewed Mar 11, 2021
View reviewed changes
Copy link
Contributor

@dianpopa dianpopa left a comment

Choose a reason for hiding this comment

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

The changes where you make use of the boot_params defined in the linux-loader from rust-vmm could be placed in another commit for better readability.

src/arch/Cargo.toml Outdated Show resolved Hide resolved
src/arch/src/x86_64/mod.rs Outdated Show resolved Hide resolved
src/vmm/src/builder.rs Outdated Show resolved Hide resolved
src/vmm/src/builder.rs Outdated Show resolved Hide resolved
src/vmm/src/lib.rs Outdated Show resolved Hide resolved
@AlexandruCihodaru AlexandruCihodaru force-pushed the linux_loader branch 10 times, most recently from 80f5097 to 00c17de Compare April 28, 2021 07:24
@acatangiu acatangiu added Status: Blocked Indicates that an issue or pull request cannot currently be worked on and removed Status: Author labels May 5, 2021
@AlexandruCihodaru AlexandruCihodaru force-pushed the linux_loader branch 3 times, most recently from d50e020 to 366e38c Compare May 31, 2021 10:35
@dianpopa dianpopa marked this pull request as ready for review May 31, 2021 12:59
@dianpopa dianpopa changed the base branch from feature/rust_vmm_adoption to main May 31, 2021 13:00
@dianpopa dianpopa changed the base branch from main to feature/rust_vmm_adoption May 31, 2021 13:01
@dianpopa dianpopa marked this pull request as draft May 31, 2021 13:03
alindima
alindima reviewed Sep 24, 2021
View reviewed changes
Copy link
Contributor

@alindima alindima left a comment

Choose a reason for hiding this comment

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

Looks good! Only a couple of nits

src/arch/src/x86_64/mod.rs Show resolved Hide resolved
src/vmm/src/builder.rs Outdated Show resolved Hide resolved
src/vmm/src/builder.rs Show resolved Hide resolved
alindima
alindima previously approved these changes Sep 24, 2021
View reviewed changes
alindima
alindima reviewed Sep 27, 2021
View reviewed changes
src/vmm/src/vstate/vcpu/mod.rs Outdated Show resolved Hide resolved
src/vmm/src/resources.rs Show resolved Hide resolved
@alindima alindima self-requested a review September 27, 2021 06:31
alindima
alindima previously approved these changes Sep 27, 2021
View reviewed changes
@AlexandruCihodaru
Deleted kernel crate
2f0aeba 
Signed-off-by: AlexandruCihodaru <[email protected]>
@AlexandruCihodaru
Add linux-loader dependency
199869f 
Signed-off-by: AlexandruCihodaru <[email protected]>
@AlexandruCihodaru
linux-loader boot parameters
c76e779 
Signed-off-by: AlexandruCihodaru <[email protected]>
@AlexandruCihodaru
Integrated linux loader
b6541f7 
Signed-off-by: AlexandruCihodaru <[email protected]>
@AlexandruCihodaru
[CI]: Updated coverage
2a5e23e 
Signed-off-by: AlexandruCihodaru <[email protected]>
@AlexandruCihodaru
[CI]: Updated binary size
c85b8fb 
Signed-off-by: AlexandruCihodaru <[email protected]>
@dianpopa dianpopa merged commit 690ad4c into firecracker-microvm:main Sep 27, 2021
@dianpopa dianpopa mentioned this pull request Oct 4, 2021
Closed
@AlexandruCihodaru AlexandruCihodaru deleted the linux_loader branch December 7, 2021 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dianpopa dianpopa dianpopa approved these changes

@alindima alindima alindima approved these changes

Assignees

@AlexandruCihodaru AlexandruCihodaru

Labels
Status: Awaiting review Indicates that a pull request is ready to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AlexandruCihodaru @andreeaflorescu @dianpopa @alindima @acatangiu