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

Bring back bzImage direct kernel boot support for x86_64 #6200

Merged
merged 3 commits into from
Feb 19, 2024

Conversation

snue
Copy link

@snue snue commented Feb 15, 2024

Fixes: #5766

This feature was previously removed in favor of just supporting uncompressed PVH ELF binaries for simplicity. However, direct bzImage support has a couple benefits that would be nice.

  • No need to keep uncompressed images around
  • Ability to use the distro provided kernel from /boot directly

By using the regular 32 bit entry point, the necessary code changes are rather small. Most of the machine setup can be shared with the PVH setup. We just need to provide the zero page and point to it in the initial register state. The boot variant is auto-detected with no further switches needed. I opted to just use the availability of the setup_header as a marker for the boot protocol instead of an explicit enum (as was done before). While being explicit is mostly good, here it would lead to more code churn and more special cases to handle.

Most of this code is a partial revert of the previously existing code.

I ran the x86_64 integration tests using both vmlinux and bzImage as the direct boot kernel with the same results as far as I could get those to work (using ./scripts/dev_cli.sh docker setup). I added one simple boot test that uses the bzImage explicitly in the integration tests.

For visibility: @parthy @blitz

@snue snue requested a review from a team as a code owner February 15, 2024 17:28
Comment on lines +82 to +97
/// * `entry_point` - Description of the boot entry to set up.
pub fn setup_regs(vcpu: &Arc<dyn hypervisor::Vcpu>, entry_point: EntryPoint) -> Result<()> {
let regs = match entry_point.setup_header {
None => StandardRegisters {
rflags: 0x0000000000000002u64,
rip: entry_point.entry_addr.raw_value(),
rbx: PVH_INFO_START.raw_value(),
..Default::default()
},
Some(_) => StandardRegisters {
rflags: 0x0000000000000002u64,
rip: entry_point.entry_addr.raw_value(),
rsp: BOOT_STACK_POINTER.raw_value(),
rsi: ZERO_PAGE_START.raw_value(),
..Default::default()
},
Copy link
Author

Choose a reason for hiding this comment

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

Note: It is possible to combine the initial register state to point to the PVH start info and the zero page at the same time. But this violates some of the stated assumptions on initial register state from the protocols. It works just fine with regular Linux kernels, but someone somewhere might get very unhappy, so I decided to switch it correctly.

@@ -1195,8 +1221,10 @@ impl Vm {
arch::configure_system(
&mem,
arch::layout::CMDLINE_START,
arch::layout::CMDLINE_MAX_SIZE,
Copy link
Author

Choose a reason for hiding this comment

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

This was providing the actual cmdline length previously, but that's harder to get now, since the code changed a bit. This value only goes into the zero page for the guest kernel to interpret, and I verified the kernels see a correct cmdline. There might still be concerns about that, though.

@snue
Copy link
Author

snue commented Feb 15, 2024

I obviously forgot to fix some tests after the rebase from LTS. I'll take care of those (as well as the missing sign-off). But let me know if you have other fundamental issues with bringing back bzImage support this way.

@snue snue force-pushed the bzimage branch 2 times, most recently from ab61921 to 3334d39 Compare February 16, 2024 09:59
@blitz
Copy link
Contributor

blitz commented Feb 16, 2024

This is so nice! I've booted the NixOS netboot binaries without issue:

cloud-hypervisor on  bzimage [?] is 📦 v37.0.0 via 🦀 v1.76.0 took 7s 
❯ gh pr checkout 6200
cloud-hypervisor on  bzimage [?] is 📦 v37.0.0 via 🦀 v1.76.0 took 2s 
❯ cargo run -- --memory size=8192M --kernel ~/src/own/nixpkgs/nixos/result/bzImage --initramfs ~/src/own/nixpkgs/nixos/result/initrd --serial file=stdio --cmdline "init=/nix/store/x4b3wwnjg6nnw5qmwya3lrkaq3mc3341-nixos-system-nixos-24.05pre130979.gfedcba/init initrd=initrd nohibernate loglevel=4"
   Compiling cloud-hypervisor v37.0.0 (/home/julian/src/own/cloud-hypervisor)
    Finished dev [unoptimized + debuginfo] target(s) in 1.35s
     Running `target/debug/cloud-hypervisor --memory size=8192M --kernel /home/julian/src/own/nixpkgs/nixos/result/bzImage --initramfs /home/julian/src/own/nixpkgs/nixos/result/initrd --serial file=stdio --cmdline 'init=/nix/store/x4b3wwnjg6nnw5qmwya3lrkaq3mc3341-nixos-system-nixos-24.05pre130979.gfedcba/init initrd=initrd nohibernate loglevel=4'`


<<< Welcome to NixOS 24.05pre130979.gfedcba (x86_64) - hvc0 >>>
The "nixos" and "root" accounts have empty passwords.

To log in over ssh you must set a password for either "nixos" or "root"
with `passwd` (prefix with `sudo` for "root"), or add your public key to
/home/nixos/.ssh/authorized_keys or /root/.ssh/authorized_keys.

If you need a wireless connection, type
`sudo systemctl start wpa_supplicant` and configure a
network using `wpa_cli`. See the NixOS manual for details.


nixos login: nixos (automatic login)


[nixos@nixos:~]$

@blitz
Copy link
Contributor

blitz commented Feb 16, 2024

@rbradford Do you have a suggestion how to workaround the commit message check failure?

 7: B1 Line exceeds max length (76>72): "Signed-off-by: Stefan Nuernberger <stefan.nuernberger@cyberus-technology.de>"

@rbradford
Copy link
Member

@rbradford Do you have a suggestion how to workaround the commit message check failure?

 7: B1 Line exceeds max length (76>72): "Signed-off-by: Stefan Nuernberger <stefan.nuernberger@cyberus-technology.de>"

The commit message check is not a blocking check - it's only there to check that nothing has gone wrong. You don't need to do anything.

@rbradford
Copy link
Member

Thanks for your PR. FYI, we don't generally review a PR until most of the checks are green - I think you have a build issue under the snp_sev feature.

@snue
Copy link
Author

snue commented Feb 17, 2024

Thanks for your PR. FYI, we don't generally review a PR until most of the checks are green - I think you have a build issue under the snp_sev feature.

Thanks for letting me know. Yes I spotted the build issue. I'll fix it in time and will be looking forward to further feedback.

Allow cloud-hypervisor to direct boot the bzImage kernel format using
the regular 32 bit entry point. This can share the memory and vcpu
setup with the regular PVH boot code, but requires the setup of the
'zero page'.

Signed-off-by: Stefan Nuernberger <stefan.nuernberger@cyberus-technology.de>
Signed-off-by: Stefan Nuernberger <stefan.nuernberger@cyberus-technology.de>
Signed-off-by: Stefan Nuernberger <stefan.nuernberger@cyberus-technology.de>
Copy link
Member

@rbradford rbradford left a comment

Choose a reason for hiding this comment

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

I'm fine with this - thank you for your contribution!

@rbradford rbradford merged commit 6362b71 into cloud-hypervisor:main Feb 19, 2024
23 of 25 checks passed
copybara-service bot pushed a commit to project-oak/oak that referenced this pull request Apr 9, 2024
Oak switched to using nix to compile linux kernel which produces a
single bzImage. PR6200 brings bzImage support to CH.

PR6200: cloud-hypervisor/cloud-hypervisor#6200

Change-Id: I663c9bd7a281d3d49a47d221e31d7e9eead790c4
Signed-off-by: Yu Ding <dingelish@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

Support for Booting bzImages
3 participants