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

loader: Re-organize module layout #197

Merged
merged 4 commits into from
Oct 21, 2024

Conversation

TimePrinciple
Copy link
Contributor

Summary of the PR

loader: Re-organize module layout

Re-organize modules of loader to follow image format, enable reuse of
code across different architectures.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@TimePrinciple
Copy link
Contributor Author

Should we consider modify our CI to ignore changes not committed, or change our bench test code to skip download, use local binary?

This setup is confusing while having a binary in our repo and download it from internet when testing 😵‍💫

@roypat
Copy link
Collaborator

roypat commented Oct 17, 2024

Should we consider modify our CI to ignore changes not committed, or change our bench test code to skip download, use local binary?

This setup is confusing while having a binary in our repo and download it from internet when testing 😵‍💫

I agree its a confusing/annoying, but I'm not sure about the license implications of including the kernel binary in the repository, so let's keep it the way it is for now :/

@TimePrinciple
Copy link
Contributor Author

Should we consider modify our CI to ignore changes not committed, or change our bench test code to skip download, use local binary?
This setup is confusing while having a binary in our repo and download it from internet when testing 😵‍💫

I agree its a confusing/annoying, but I'm not sure about the license implications of including the kernel binary in the repository, so let's keep it the way it is for now :/

I don't know why after re-organizing the layout makes the bzimage binary different, see https://buildkite.com/rust-vmm/linux-loader-ci/builds/764#01929877-c73e-4d57-9a9c-1bb5ef78e105

In that run, I haven't remove the binary :( but it's changed somehow after the test downloaded it

Re-organize modules of `loader` to follow image format, enable reuse of
code across different architectures. Its interface is unchanged.

Signed-off-by: Ruoqing He <heruoqing@iscas.ac.cn>
@TimePrinciple TimePrinciple force-pushed the repurpose-loader branch 3 times, most recently from 51e1a11 to a099c14 Compare October 20, 2024 14:21
Code in `fdt.rs` do not contain architecture specific code as of
`aarch64`, it's only used by `aarch64`, move it one level up to allow
reusing on riscv64 platform.

Signed-off-by: Ruoqing He <heruoqing@iscas.ac.cn>
@TimePrinciple TimePrinciple force-pushed the repurpose-loader branch 2 times, most recently from cc8dec1 to d5b8810 Compare October 20, 2024 14:49
Code in `mod.rs` of `aarch64` do not contain architecture specific code
as of `aarch64`, it's only used by `aarch64` to run benchmark tests,
move it one level up and rename it to `fdt.rs` to allow reusing on
riscv64 platform.

Signed-off-by: Ruoqing He <heruoqing@iscas.ac.cn>
@TimePrinciple
Copy link
Contributor Author

Should we consider modify our CI to ignore changes not committed, or change our bench test code to skip download, use local binary?
This setup is confusing while having a binary in our repo and download it from internet when testing 😵‍💫

I agree its a confusing/annoying, but I'm not sure about the license implications of including the kernel binary in the repository, so let's keep it the way it is for now :/

I don't know why after re-organizing the layout makes the bzimage binary different, see https://buildkite.com/rust-vmm/linux-loader-ci/builds/764#01929877-c73e-4d57-9a9c-1bb5ef78e105

In that run, I haven't remove the binary :( but it's changed somehow after the test downloaded it

Update: fixed by @roypat in rust-vmm/rust-vmm-ci#169

Copy link
Collaborator

@roypat roypat left a comment

Choose a reason for hiding this comment

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

In light of #190, I'm very much in favor of this change, but we need to definitely mention it in the changelog, pointing out how the modules were moved. It might even make sense to create some re-exports according to the old module structure, to avoid downstream users of this crate having to update, e.g.

pub mod x86_64 {
    pub mod elf { 
        pub use crate::loader::elf::*;
    }
}

and so on (which I just realize is what Rob also suggested in #190 :D)

@TimePrinciple
Copy link
Contributor Author

In light of #190, I'm very much in favor of this change, but we need to definitely mention it in the changelog, pointing out how the modules were moved. It might even make sense to create some re-exports according to the old module structure, to avoid downstream users of this crate having to update, e.g.

pub mod x86_64 {
    pub mod elf { 
        pub use crate::loader::elf::*;
    }
}

and so on (which I just realize is what Rob also suggested in #190 :D)

Noted, I failed to understand that intent initially, I will work it out :)

@TimePrinciple
Copy link
Contributor Author

In light of #190, I'm very much in favor of this change, but we need to definitely mention it in the changelog, pointing out how the modules were moved. It might even make sense to create some re-exports according to the old module structure, to avoid downstream users of this crate having to update, e.g.

pub mod x86_64 {
    pub mod elf { 
        pub use crate::loader::elf::*;
    }
}

and so on (which I just realize is what Rob also suggested in #190 :D)

I tried to add this re-export, and I just find out the aarch64 and x86_64 are both declared as private module and thus transparent to users, i.e. they won't be able to write something like use linux_loader::x86_64::... that kind of thing :O

Update CHANGELOG.md to document the re-organization of `loader`,
`configurator` and `benches` modules.

Signed-off-by: Ruoqing He <heruoqing@iscas.ac.cn>
Copy link
Collaborator

@roypat roypat left a comment

Choose a reason for hiding this comment

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

Ahh, very neat, thanks for checking!

(verified by compiling Firecracker against this, and everything indeed still works without changes)

@roypat roypat merged commit ce95912 into rust-vmm:main Oct 21, 2024
2 checks passed
TimePrinciple added a commit to TimePrinciple/linux-loader that referenced this pull request Oct 22, 2024
Fix wrongly put rust-vmm#197 changelog and add entry for rust-vmm#190.

Signed-off-by: Ruoqing He <heruoqing@iscas.ac.cn>
TimePrinciple added a commit to TimePrinciple/linux-loader that referenced this pull request Oct 22, 2024
Fix wrongly put rust-vmm#197 changelog and add entry for rust-vmm#190.

Signed-off-by: Ruoqing He <heruoqing@iscas.ac.cn>
TimePrinciple added a commit to TimePrinciple/linux-loader that referenced this pull request Oct 22, 2024
Fix wrongly put rust-vmm#197 changelog and add entry for rust-vmm#190.

Signed-off-by: Ruoqing He <heruoqing@iscas.ac.cn>
TimePrinciple added a commit to TimePrinciple/linux-loader that referenced this pull request Oct 22, 2024
Fix wrongly put rust-vmm#197 changelog and add entry for rust-vmm#190.

Signed-off-by: Ruoqing He <heruoqing@iscas.ac.cn>
rbradford pushed a commit that referenced this pull request Oct 22, 2024
Fix wrongly put #197 changelog and add entry for #190.

Signed-off-by: Ruoqing He <heruoqing@iscas.ac.cn>
@TimePrinciple TimePrinciple deleted the repurpose-loader branch October 22, 2024 14:02
ita93 pushed a commit to ita93/linux-loader that referenced this pull request Nov 27, 2024
Fix wrongly put rust-vmm#197 changelog and add entry for rust-vmm#190.

Signed-off-by: Ruoqing He <heruoqing@iscas.ac.cn>
ita93 pushed a commit to ita93/linux-loader that referenced this pull request Nov 27, 2024
Fix wrongly put rust-vmm#197 changelog and add entry for rust-vmm#190.

Signed-off-by: Ruoqing He <heruoqing@iscas.ac.cn>
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