Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughReworks VM and VM-exit abstractions: moves the Changes
Sequence Diagram(s)sequenceDiagram
participant Vcpu as Vcpu
participant VmExitHandler as VmExitHandler
participant DeviceMgr as DeviceManager
participant Pio as PIO
participant Mmio as MMIO
participant Psci as PSCI
Vcpu->>VmExitHandler: on VM exit (reason, vcpu state)
alt IO port access
VmExitHandler->>DeviceMgr: pio lookup (port)
DeviceMgr->>Pio: call io_in/io_out (with mutex inside)
Pio-->>VmExitHandler: result / error
else MMIO access
VmExitHandler->>DeviceMgr: mmio lookup (addr)
DeviceMgr->>Mmio: mmio_read/mmio_write (bounds checked)
Mmio-->>VmExitHandler: result / error
else SMC (aarch64)
VmExitHandler->>Psci: call_smc(vcpu)
Psci-->>VmExitHandler: result / error
end
VmExitHandler-->>Vcpu: apply results / resume or propagate error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 1
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.rs (1)
1-17:⚠️ Potential issue | 🔴 CriticalMissing re-exports break downstream imports.
The module declares
pub mod vm;but doesn't re-exportVmorSetUserMemoryRegionFlags. This breakshvp.rswhich importsuse crate::virt::Vm;(line 27 per the relevant code snippet).Add re-exports for API convenience and to fix the broken import:
🔧 Proposed fix
use std::sync::Arc; use crate::error::Error; use crate::virt::vm::Vm; #[cfg(feature = "kvm")] pub mod kvm; #[cfg(feature = "hvp")] pub mod hvp; pub mod vcpu; pub mod vm; +pub use vm::SetUserMemoryRegionFlags; +pub use vm::Vm; + pub trait Virt { fn create_vm(&self) -> Result<Arc<dyn Vm>, Error>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm-core/src/virt.rs` around lines 1 - 17, The downstream import fails because virt.rs exposes pub mod vm but does not re-export the Vm type or the SetUserMemoryRegionFlags symbol; update virt.rs to add public re-exports for these API symbols (e.g., add pub use for Vm and SetUserMemoryRegionFlags) so files like hvp.rs can import use crate::virt::Vm; and use crate::virt::SetUserMemoryRegionFlags; without breaking.
🤖 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/virt/hvp.rs`:
- Around line 26-31: The import for Vm is broken because virt does not re-export
Vm; update the use statement to import the concrete module path (use
crate::virt::vm::Vm) instead of use crate::virt::Vm so references to Vm in this
file (and similarly in crates/vm-core/src/virt/kvm.rs where Vm is imported the
same way) resolve correctly; modify the import near the top of hvp.rs
referencing symbols Virt, Vm, HvpGicV3, and HvpVcpu to use crate::virt::vm::Vm,
and make the same change in kvm.rs to fix the compilation error.
---
Outside diff comments:
In `@crates/vm-core/src/virt.rs`:
- Around line 1-17: The downstream import fails because virt.rs exposes pub mod
vm but does not re-export the Vm type or the SetUserMemoryRegionFlags symbol;
update virt.rs to add public re-exports for these API symbols (e.g., add pub use
for Vm and SetUserMemoryRegionFlags) so files like hvp.rs can import use
crate::virt::Vm; and use crate::virt::SetUserMemoryRegionFlags; without
breaking.
🪄 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: bee93dfa-1197-4e5d-baed-8f8749dc444e
📒 Files selected for processing (9)
crates/vm-core/src/arch/x86_64.rscrates/vm-core/src/vcpu/vcpu_manager.rscrates/vm-core/src/virt.rscrates/vm-core/src/virt/hvp.rscrates/vm-core/src/virt/kvm/vcpu/arch/aarch64.rscrates/vm-core/src/virt/kvm/vcpu/arch/x86_64.rscrates/vm-core/src/virt/vm.rscrates/vm-machine/src/vm.rscrates/vm-machine/src/vmm.rs
💤 Files with no reviewable changes (1)
- crates/vm-core/src/arch/x86_64.rs
Comment on lines
26
to
+31
| use crate::virt::Virt; | ||
| use crate::virt::Vm; | ||
| use crate::virt::hvp::irq_chip::HvpGicV3; | ||
| use crate::virt::hvp::vcpu::HvpVcpu; | ||
| use crate::virt::vcpu::Vcpu; | ||
| use crate::virt::vm::SetUserMemoryRegionFlags; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if there's a re-export of Vm in virt.rs
rg -n 'pub use.*Vm' crates/vm-core/src/virt.rs
# Check all files importing crate::virt::Vm (without ::vm::)
rg -n 'use crate::virt::Vm;' crates/Repository: junyu0312/rust-vm
Length of output: 170
🏁 Script executed:
cat -n crates/vm-core/src/virt.rsRepository: junyu0312/rust-vm
Length of output: 428
🏁 Script executed:
rg -n 'pub use' crates/vm-core/src/virt.rsRepository: junyu0312/rust-vm
Length of output: 43
🏁 Script executed:
# Check if there's a Cargo.lock or build artifacts
ls -la crates/vm-core/ | grep -E '(Cargo|target)'
# Also verify the exact structure - maybe there's a re-export I missed
cat -n crates/vm-core/src/virt/vm.rs | head -50Repository: junyu0312/rust-vm
Length of output: 845
Broken import: crate::virt::Vm is not re-exported from the virt module.
Line 27 imports use crate::virt::Vm;, but virt.rs only contains a private use crate::virt::vm::Vm; without a public re-export. This will fail to compile.
Update the import to use the full path:
Proposed fix
use crate::virt::Virt;
-use crate::virt::Vm;
+use crate::virt::vm::Vm;
use crate::virt::hvp::irq_chip::HvpGicV3;Note: The same issue exists in crates/vm-core/src/virt/kvm.rs line 10, which also imports use crate::virt::Vm;.
📝 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
| use crate::virt::Virt; | |
| use crate::virt::Vm; | |
| use crate::virt::hvp::irq_chip::HvpGicV3; | |
| use crate::virt::hvp::vcpu::HvpVcpu; | |
| use crate::virt::vcpu::Vcpu; | |
| use crate::virt::vm::SetUserMemoryRegionFlags; | |
| use crate::virt::Virt; | |
| use crate::virt::vm::Vm; | |
| use crate::virt::hvp::irq_chip::HvpGicV3; | |
| use crate::virt::hvp::vcpu::HvpVcpu; | |
| use crate::virt::vcpu::Vcpu; | |
| use crate::virt::vm::SetUserMemoryRegionFlags; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm-core/src/virt/hvp.rs` around lines 26 - 31, The import for Vm is
broken because virt does not re-export Vm; update the use statement to import
the concrete module path (use crate::virt::vm::Vm) instead of use
crate::virt::Vm so references to Vm in this file (and similarly in
crates/vm-core/src/virt/kvm.rs where Vm is imported the same way) resolve
correctly; modify the import near the top of hvp.rs referencing symbols Virt,
Vm, HvpGicV3, and HvpVcpu to use crate::virt::vm::Vm, and make the same change
in kvm.rs to fix the compilation error.
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