Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR removes runtime layout dependencies from bootloading: BootLoader::load no longer takes a mutable Virt but accepts explicit Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(150,200,255,0.5)
participant VM as Vm
end
rect rgba(200,255,180,0.5)
participant BL as BootLoader
end
rect rgba(255,200,180,0.5)
participant KL as KernelLoader
end
rect rgba(255,220,200,0.5)
participant VIRT as Virt
end
VM->>BL: load(ram_size, vcpus, memory, irq_chip, devices)
BL->>KL: load_image/initrd/dtb (use RAM_BASE, INITRD_START, DTB_START)
KL-->>BL: LoadResult(s) (start_pc, initrd bounds, ...)
BL-->>VM: Result<u64> (start_pc)
VM->>VIRT: run(start_pc, device_vm_exit_handler)
VIRT-->>VM: execution (VM run path)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm-core/src/virt/kvm.rs (1)
64-79:⚠️ Potential issue | 🔴 Critical
_start_pcparameter is unused because x86_64 bootloader support is incomplete.The parameter is prefixed with underscore to suppress the unused variable warning. However, this is NOT an intentional design choice—the x86_64 bootloader's
load()method (both at theBootLoadertrait level and in theKernelLoader) is markedtodo!(), meaning the x86_64 implementation is incomplete.In contrast, the aarch64 implementation properly uses
start_pc, passing it tosetup_cpu()to set thePCregister before execution. The x86_64 side needs to be completed to properly utilize this parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm-core/src/virt/kvm.rs` around lines 64 - 79, The run() method currently ignores the _start_pc parameter; remove the underscore to make it start_pc and ensure the x86_64 path sets the guest PC before executing the vCPU (mirror aarch64 behavior): locate run(), the vcpus collection and the single vcpu obtained via vcpus.get_mut(0).unwrap(), and call the appropriate CPU setup on that vcpu (e.g., a setup_cpu or set_guest_rip equivalent) with start_pc before invoking .run(device_vm_exit_handler); also complete the x86_64 BootLoader::load()/KernelLoader implementation to return or apply the start PC so run() receives a meaningful value.
🧹 Nitpick comments (5)
crates/vm-bootloader/src/boot_loader/arch/aarch64.rs (5)
241-246: The inner block is unnecessary.The block containing the
kernel_loaderandinitrd_loaderassignments serves no purpose since the variables are declared outside. This appears to be a refactoring artifact.🧹 Proposed cleanup
) -> Result<u64> { - let kernel_loader; - let initrd_loader; - { - kernel_loader = self.load_image(ram_size, memory)?; - initrd_loader = self.load_initrd(memory)?; - } + let kernel_loader = self.load_image(ram_size, memory)?; + let initrd_loader = self.load_initrd(memory)?; { let dtb = self.generate_dtb(ram_size, initrd_loader, vcpus, irq_chip, devices)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm-bootloader/src/boot_loader/arch/aarch64.rs` around lines 241 - 246, Remove the redundant inner block around the assignments to kernel_loader and initrd_loader: instead of declaring kernel_loader and initrd_loader beforehand and then assigning them inside a separate block, directly assign them by calling self.load_image(ram_size, memory)? and self.load_initrd(memory)? without the enclosing braces; update the code around the kernel_loader and initrd_loader variables (references: kernel_loader, initrd_loader, load_image, load_initrd, self, ram_size, memory) so the block is eliminated and behavior remains unchanged.
253-254: Remove commented-out dead code.These commented lines are refactoring artifacts and should be removed.
🧹 Proposed cleanup
self.load_dtb(memory, dtb)?; } - // let layout = virt.get_layout(); - // layout.validate()?; - Ok(kernel_loader.start_pc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm-bootloader/src/boot_loader/arch/aarch64.rs` around lines 253 - 254, Remove the dead commented-out refactoring artifacts in aarch64.rs by deleting the two commented lines that reference the unused local "layout" and calls "// let layout = virt.get_layout();" and "// layout.validate()?"; ensure no remaining commented references to "layout" or "virt.get_layout()" remain in the function so the codebase contains only active, relevant code.
70-70: Remove commented-out dead code.This commented line is a leftover from the refactor and should be removed to keep the codebase clean.
🧹 Proposed cleanup
assert_eq!(result.initrd_start, addr); - // layout.set_initrd_len(result.initrd_len)?; Ok(Some(result))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm-bootloader/src/boot_loader/arch/aarch64.rs` at line 70, Remove the dead commented-out line containing the call to layout.set_initrd_len(result.initrd_len)? in the aarch64 bootloader code; locate the commented line in the aarch64.rs file (inside the boot_loader::arch module) and delete that single commented statement so the refactor has no leftover dead code, then run cargo build/test to verify no unintended changes.
198-204: Useinitrd_load_resultdirectly for the conditional to avoid fragile coupling.The current code checks
self.initrd.is_some()but then unwrapsinitrd_load_result. While these are currently kept in sync byload_initrd, directly checkinginitrd_load_resultwould be more robust and self-documenting.♻️ Proposed refactor
- if self.initrd.is_some() { + if let Some(initrd_result) = initrd_load_result.as_ref() { fdt.property_u64("linux,initrd-start", INITRD_START)?; fdt.property_u64( "linux,initrd-end", - initrd_load_result.as_ref().unwrap().initrd_start - + initrd_load_result.as_ref().unwrap().initrd_len as u64, + initrd_result.initrd_start + initrd_result.initrd_len as u64, )?; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm-bootloader/src/boot_loader/arch/aarch64.rs` around lines 198 - 204, The conditional uses self.initrd.is_some() but then unwraps initrd_load_result, which is fragile; change the check to inspect initrd_load_result (e.g., initrd_load_result.is_some()) and then use the bound/as_ref value from initrd_load_result when calling fdt.property_u64 for "linux,initrd-start" and "linux,initrd-end"; update the code paths around load_initrd and the property_u64 calls so they rely on initrd_load_result rather than self.initrd to avoid coupling.
92-92: Remove commented-out dead code.Same as above - this is a refactoring artifact that should be cleaned up.
🧹 Proposed cleanup
.copy_from_slice(dtb_start, &dtb) .map_err(|_| Error::LoadDtbFailed("failed to copy".to_string()))?; - // layout.set_dtb_len(dtb.len())?; - Ok(())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm-bootloader/src/boot_loader/arch/aarch64.rs` at line 92, Remove the commented-out dead code line "layout.set_dtb_len(dtb.len())?;" from the aarch64 boot loader source (it's a refactoring artifact); delete the commented line entirely (not just uncomment) so there is no leftover dead code or trailing comment, then run the repo formatter/linter and commit the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm-core/src/arch/x86_64/layout.rs`:
- Around line 4-7: The constants MMIO_START, MMIO_LEN, and RAM_BASE in layout.rs
are placeholders set to zero which causes MmioLayout::new(MMIO_START, MMIO_LEN)
-> address_space.try_insert(...).unwrap() to panic with InvalidLen at runtime;
either replace these with correct x86_64 values now (set a non-zero MMIO_LEN and
appropriate MMIO_START/RAM_BASE matching the platform layout used by MmioLayout
and vm_builder) or add a compile-time guard (e.g., a cfg error or compile_error!
when target_arch = "x86_64") so x86_64 builds fail to compile until proper
values are implemented; locate and update the constants MMIO_START, MMIO_LEN,
RAM_BASE in crates/vm-core/src/arch/x86_64/layout.rs and ensure tests or
vm_builder paths that call MmioLayout::new no longer receive a zero length.
In `@crates/vm-machine/src/vm_builder.rs`:
- Around line 3-14: The x86_64 imports (MMIO_LEN, MMIO_START, RAM_BASE) pull in
placeholder zero constants, causing MmioLayout::new(0, 0) to panic; update the
architecture selection so the real x86_64 layout values are used instead of the
zero placeholders: adjust the conditional imports or the code that constructs
MmioLayout (reference: MMIO_LEN, MMIO_START, RAM_BASE and the call to
MmioLayout::new) to use the correct runtime/proper constants for x86_64 (e.g.,
source the real layout values from the proper x86_64 layout module or compute
them before calling MmioLayout::new) so MmioLayout::new is never invoked with
zeros.
---
Outside diff comments:
In `@crates/vm-core/src/virt/kvm.rs`:
- Around line 64-79: The run() method currently ignores the _start_pc parameter;
remove the underscore to make it start_pc and ensure the x86_64 path sets the
guest PC before executing the vCPU (mirror aarch64 behavior): locate run(), the
vcpus collection and the single vcpu obtained via vcpus.get_mut(0).unwrap(), and
call the appropriate CPU setup on that vcpu (e.g., a setup_cpu or set_guest_rip
equivalent) with start_pc before invoking .run(device_vm_exit_handler); also
complete the x86_64 BootLoader::load()/KernelLoader implementation to return or
apply the start PC so run() receives a meaningful value.
---
Nitpick comments:
In `@crates/vm-bootloader/src/boot_loader/arch/aarch64.rs`:
- Around line 241-246: Remove the redundant inner block around the assignments
to kernel_loader and initrd_loader: instead of declaring kernel_loader and
initrd_loader beforehand and then assigning them inside a separate block,
directly assign them by calling self.load_image(ram_size, memory)? and
self.load_initrd(memory)? without the enclosing braces; update the code around
the kernel_loader and initrd_loader variables (references: kernel_loader,
initrd_loader, load_image, load_initrd, self, ram_size, memory) so the block is
eliminated and behavior remains unchanged.
- Around line 253-254: Remove the dead commented-out refactoring artifacts in
aarch64.rs by deleting the two commented lines that reference the unused local
"layout" and calls "// let layout = virt.get_layout();" and "//
layout.validate()?"; ensure no remaining commented references to "layout" or
"virt.get_layout()" remain in the function so the codebase contains only active,
relevant code.
- Line 70: Remove the dead commented-out line containing the call to
layout.set_initrd_len(result.initrd_len)? in the aarch64 bootloader code; locate
the commented line in the aarch64.rs file (inside the boot_loader::arch module)
and delete that single commented statement so the refactor has no leftover dead
code, then run cargo build/test to verify no unintended changes.
- Around line 198-204: The conditional uses self.initrd.is_some() but then
unwraps initrd_load_result, which is fragile; change the check to inspect
initrd_load_result (e.g., initrd_load_result.is_some()) and then use the
bound/as_ref value from initrd_load_result when calling fdt.property_u64 for
"linux,initrd-start" and "linux,initrd-end"; update the code paths around
load_initrd and the property_u64 calls so they rely on initrd_load_result rather
than self.initrd to avoid coupling.
- Line 92: Remove the commented-out dead code line
"layout.set_dtb_len(dtb.len())?;" from the aarch64 boot loader source (it's a
refactoring artifact); delete the commented line entirely (not just uncomment)
so there is no leftover dead code or trailing comment, then run the repo
formatter/linter and commit the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28f02b7a-06c0-4c67-bac8-6f8969a812b6
📒 Files selected for processing (10)
crates/vm-bootloader/src/boot_loader.rscrates/vm-bootloader/src/boot_loader/arch/aarch64.rscrates/vm-bootloader/src/boot_loader/arch/x86_64.rscrates/vm-core/src/arch/aarch64/layout.rscrates/vm-core/src/arch/x86_64/layout.rscrates/vm-core/src/virt.rscrates/vm-core/src/virt/hvp.rscrates/vm-core/src/virt/kvm.rscrates/vm-machine/src/vm.rscrates/vm-machine/src/vm_builder.rs
Comment on lines
+4
to
+7
| // TODO | ||
| pub const MMIO_START: u64 = 0x0000_0000; | ||
| pub const MMIO_LEN: usize = 0x0000_0000; | ||
| pub const RAM_BASE: u64 = 0x0000_0000; |
There was a problem hiding this comment.
Zero-value constants will cause runtime panic on x86_64.
Per crates/vm-core/src/device/mmio/layout.rs:11-24, MmioLayout::new(MMIO_START, MMIO_LEN) calls address_space.try_insert(...).unwrap(). When MMIO_LEN=0, try_insert() returns an InvalidLen error, causing an immediate panic in vm_builder.rs line 82.
These placeholder values break x86_64 builds at runtime. Either implement proper x86_64 layout values now or add a compile-time guard to prevent x86_64 builds until implementation is complete.
Suggested compile-time guard (if deferring implementation)
// TODO
-pub const MMIO_START: u64 = 0x0000_0000;
-pub const MMIO_LEN: usize = 0x0000_0000;
-pub const RAM_BASE: u64 = 0x0000_0000;
+// x86_64 layout constants - not yet implemented
+// These values will cause a compile error until properly defined
+compile_error!("x86_64 memory layout constants are not yet implemented");
+
+pub const MMIO_START: u64 = 0x0000_0000;
+pub const MMIO_LEN: usize = 0x0000_0000;
+pub const RAM_BASE: u64 = 0x0000_0000;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // TODO | |
| pub const MMIO_START: u64 = 0x0000_0000; | |
| pub const MMIO_LEN: usize = 0x0000_0000; | |
| pub const RAM_BASE: u64 = 0x0000_0000; | |
| // TODO: x86_64 layout constants - not yet implemented | |
| compile_error!("x86_64 memory layout constants are not yet implemented. Please define proper values for MMIO_START, MMIO_LEN, and RAM_BASE before enabling x86_64 builds."); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm-core/src/arch/x86_64/layout.rs` around lines 4 - 7, The constants
MMIO_START, MMIO_LEN, and RAM_BASE in layout.rs are placeholders set to zero
which causes MmioLayout::new(MMIO_START, MMIO_LEN) ->
address_space.try_insert(...).unwrap() to panic with InvalidLen at runtime;
either replace these with correct x86_64 values now (set a non-zero MMIO_LEN and
appropriate MMIO_START/RAM_BASE matching the platform layout used by MmioLayout
and vm_builder) or add a compile-time guard (e.g., a cfg error or compile_error!
when target_arch = "x86_64") so x86_64 builds fail to compile until proper
values are implemented; locate and update the constants MMIO_START, MMIO_LEN,
RAM_BASE in crates/vm-core/src/arch/x86_64/layout.rs and ensure tests or
vm_builder paths that call MmioLayout::new no longer receive a zero length.
Comment on lines
+3
to
+14
| #[cfg(target_arch = "aarch64")] | ||
| use vm_core::arch::aarch64::layout::MMIO_LEN; | ||
| #[cfg(target_arch = "aarch64")] | ||
| use vm_core::arch::aarch64::layout::MMIO_START; | ||
| #[cfg(target_arch = "aarch64")] | ||
| use vm_core::arch::aarch64::layout::RAM_BASE; | ||
| #[cfg(target_arch = "x86_64")] | ||
| use vm_core::arch::x86_64::layout::MMIO_LEN; | ||
| #[cfg(target_arch = "x86_64")] | ||
| use vm_core::arch::x86_64::layout::MMIO_START; | ||
| #[cfg(target_arch = "x86_64")] | ||
| use vm_core::arch::x86_64::layout::RAM_BASE; |
There was a problem hiding this comment.
Compile-time architecture selection is clean, but x86_64 imports broken constants.
The conditional import pattern is correct. However, on x86_64, these imports reference the zero-value placeholder constants from x86_64/layout.rs, which will cause a panic at line 82 when MmioLayout::new(0, 0) is called.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm-machine/src/vm_builder.rs` around lines 3 - 14, The x86_64 imports
(MMIO_LEN, MMIO_START, RAM_BASE) pull in placeholder zero constants, causing
MmioLayout::new(0, 0) to panic; update the architecture selection so the real
x86_64 layout values are used instead of the zero placeholders: adjust the
conditional imports or the code that constructs MmioLayout (reference: MMIO_LEN,
MMIO_START, RAM_BASE and the call to MmioLayout::new) to use the correct
runtime/proper constants for x86_64 (e.g., source the real layout values from
the proper x86_64 layout module or compute them before calling MmioLayout::new)
so MmioLayout::new is never invoked with zeros.
This was referenced Apr 1, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit