Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
📝 WalkthroughWalkthroughRemoves the ad-hoc ChangesInterruptManager Rollout
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/vm-core/src/virtualization/kvm/vm.rs`:
- Around line 11-13: Add the same target_arch guard to the aarch64
IRQ_ALLOCATION_LEN import in vm:: imports so it only compiles on aarch64,
matching IRQ_ALLOCATION_START and the x86_64 IRQ_ALLOCATION_LEN import. Update
the import block in the vm module to keep the aarch64 symbols under
#[cfg(target_arch = "aarch64")] and leave the x86_64 symbol under its existing
cfg to avoid duplicate-name conflicts and unresolved imports on x86_64 builds.
In `@crates/vm-vfio/src/vfio_pci/function.rs`:
- Around line 147-153: The MSI/MSI-X update path in Function::write and the
related GSI allocation logic currently calls allocate_gsi().unwrap(), which can
panic and abort the VM when the shared GSI pool is exhausted. Update the
interrupt setup/update flow around msi.gsi handling so GSI allocation failures
are returned as a Result instead of unwrapping, and propagate
InterruptManagerError back through the write/update path; if easier, preallocate
per-vector GSIs during interrupt setup and reuse them in Function::write.
In `@crates/vm-virtio/src/transport/pci.rs`:
- Around line 118-124: The non-Linux PCI legacy IRQ setup in
VirtioPciTransport::new currently unwraps allocate_irq() and the IRQ conversion,
which can panic instead of surfacing failures. Update VirtioPciTransport::new to
return Result<Self> and propagate errors from interrupt_manager.allocate_irq()
and the try_into conversion, then adjust into_virtio_pci_device to handle the
new Result-returning constructor path without unwrapping.
🪄 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: d8beb1f0-52f1-4ba9-879f-c5fae42701ed
📒 Files selected for processing (24)
crates/vm-core/src/arch/aarch64/layout.rscrates/vm-core/src/arch/x86_64/layout.rscrates/vm-core/src/interrupt_manager.rscrates/vm-core/src/lib.rscrates/vm-core/src/virtualization.rscrates/vm-core/src/virtualization/hvp/vm.rscrates/vm-core/src/virtualization/irq_allocator.rscrates/vm-core/src/virtualization/kvm/vm.rscrates/vm-core/src/virtualization/vm.rscrates/vm-core/src/virtualization/vm/error.rscrates/vm-vfio/src/vfio_pci/device.rscrates/vm-vfio/src/vfio_pci/function.rscrates/vm-vfio/src/vfio_pci/interrupt/msi.rscrates/vm-vfio/src/vfio_pci/interrupt/msix.rscrates/vm-virtio/src/device.rscrates/vm-virtio/src/result.rscrates/vm-virtio/src/transport/pci.rscrates/vm-vmm/src/device/error.rscrates/vm-vmm/src/vm/config.rscrates/vm-vmm/src/vm/device_builder.rscrates/vm-vmm/src/vm/device_builder/arch/aarch64.rscrates/vm-vmm/src/vm/device_builder/vfio.rscrates/vm-vmm/src/vm/snapshot.rscrates/vm-vmm/src/vmm/error.rs
💤 Files with no reviewable changes (2)
- crates/vm-core/src/virtualization/irq_allocator.rs
- crates/vm-core/src/virtualization.rs
| let gsi = if let Some(gsi) = msi.gsi[vector] { | ||
| gsi | ||
| } else { | ||
| let gsi = self.irq_manager.allocate_gsi().unwrap(); | ||
| msi.gsi[vector] = Some(gsi); | ||
| gsi | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Propagate GSI allocation failures instead of panicking.
allocate_gsi().unwrap() can abort the VM when the shared GSI pool is exhausted. Since these paths are reached from MSI/MSI-X configuration updates, either preallocate per-vector GSIs during interrupt setup or make the update/write path return a Result and surface InterruptManagerError.
Also applies to: 360-366
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm-vfio/src/vfio_pci/function.rs` around lines 147 - 153, The
MSI/MSI-X update path in Function::write and the related GSI allocation logic
currently calls allocate_gsi().unwrap(), which can panic and abort the VM when
the shared GSI pool is exhausted. Update the interrupt setup/update flow around
msi.gsi handling so GSI allocation failures are returned as a Result instead of
unwrapping, and propagate InterruptManagerError back through the write/update
path; if easier, preallocate per-vector GSIs during interrupt setup and reuse
them in Function::write.
| legacy_int = Some( | ||
| interrupt_manager | ||
| .allocate_irq() | ||
| .unwrap() | ||
| .try_into() | ||
| .unwrap(), | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return allocation errors from non-Linux PCI legacy IRQ setup.
This constructor path can panic if allocate_irq() fails. Since into_virtio_pci_device already returns Result, make VirtioPciTransport::new return Result<Self> and propagate the allocation/conversion errors instead of unwrapping.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm-virtio/src/transport/pci.rs` around lines 118 - 124, The non-Linux
PCI legacy IRQ setup in VirtioPciTransport::new currently unwraps allocate_irq()
and the IRQ conversion, which can panic instead of surfacing failures. Update
VirtioPciTransport::new to return Result<Self> and propagate errors from
interrupt_manager.allocate_irq() and the try_into conversion, then adjust
into_virtio_pci_device to handle the new Result-returning constructor path
without unwrapping.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes