Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRefactors device/MMIO ownership and concurrency: DeviceManager is wrapped in Arc<Mutex<...>>, DeviceVmExitHandler gains mmio_layout(), MMIO layout is exposed as Arc, Vcpu run paths now accept &MmioLayout, and several device-related traits gained a Send bound. Changes
Sequence Diagram(s)sequenceDiagram
participant Vm
participant Virt
participant Vcpu
participant DeviceMgr as DeviceManager\n(Arc<Mutex>)
participant MMIO as MmioLayout\n(Arc)
Vm->>Virt: run(device_manager: Arc<Mutex<DeviceManager>>)
Virt->>DeviceMgr: lock()
DeviceMgr-->>Virt: guard
Virt->>MMIO: mmio_layout() -> Arc<MmioLayout>
MMIO-->>Virt: Arc<MmioLayout>
Virt->>Vcpu: run(&MmioLayout)
Vcpu->>MMIO: in_mmio_region(addr)
MMIO-->>Vcpu: bool
alt is_mmio
Vcpu->>DeviceMgr: lock()
Vcpu->>DeviceMgr: handle_vm_exit(Arc<Mutex<DeviceManager>>)
DeviceMgr-->>Vcpu: HandleVmExitResult
end
Vcpu-->>Virt: VmExitReason
Virt-->>Vm: Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 unit tests (beta)
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
🧹 Nitpick comments (1)
crates/vm-machine/src/vm.rs (1)
114-124: Extract irq_chip and mmio_devices before calling load, then drop the lock.
The device_manager lock is held for the entireboot_loader.load()call but is only needed to extractget_irq_chip()andmmio_devices(). Extract these handles before invokingloadand drop the lock immediately to reduce contention and avoid potential future re-entrance issues (line 114).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm-machine/src/vm.rs` around lines 114 - 124, Hold the device_manager lock only long enough to extract the needed handles: call self.device_manager.lock().unwrap(), call get_irq_chip() (and convert to the expected Result/error as done now) and collect mmio_devices() into local variables, then immediately drop the lock before calling boot_loader.load; pass the extracted irq_chip (use as_ref() if load expects a reference) and the collected mmio_devices to boot_loader.load so the mutex is not held during the load call (symbols: self.device_manager.lock().unwrap(), device_manager.get_irq_chip(), device_manager.mmio_devices(), boot_loader.load).
🤖 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/vm_exit/aarch64.rs`:
- Around line 57-58: The VM-exit handlers use device.lock().unwrap(), which will
panic on mutex poisoning; change both occurrences in the MMIORead and MMIOWrite
handlers to handle locking errors by replacing unwrap() with a fallible lock
(e.g., device.lock().map_err(|_| Error::MmioErr)?) and propagate
Err(Error::MmioErr) so the VM-exit returns an MmioErr instead of crashing;
update the code paths in the MMIORead and MMIOWrite functions that currently
call device.lock().unwrap() to map the poisoned error into Error::MmioErr and
use the resulting locked guard for subsequent operations.
In `@crates/vm-core/src/device/vm_exit.rs`:
- Around line 1-13: The DeviceVmExitHandler trait is used as a trait object
inside Arc<Mutex<dyn DeviceVmExitHandler>> across threads, so add a Send
supertrait to the trait definition: change the declaration of
DeviceVmExitHandler to require Send (i.e., pub trait DeviceVmExitHandler: Send {
... }) so that dyn DeviceVmExitHandler is eligible to be sent between threads;
update the trait declaration in vm_exit.rs accordingly.
---
Duplicate comments:
In `@crates/vm-core/src/virt/hvp.rs`:
- Around line 256-262: The code currently unwraps the device_manager mutex
(device_manager.lock().unwrap()) which can panic if the lock is poisoned; update
the lock acquisition to map the PoisonError into a proper error return instead
of unwraping, e.g., replace the unwrap with using lock().map_err(|e| /* convert
to appropriate error type */)? and propagate that error through the function so
mmio_layout is obtained safely before calling get_vcpu_mut(0), vcpu.run(...),
and handle_vm_exit; ensure the same approach is applied to any other
lock().unwrap() usages in this module for consistency (references:
device_manager, mmio_layout, get_vcpu_mut, handle_vm_exit).
---
Nitpick comments:
In `@crates/vm-machine/src/vm.rs`:
- Around line 114-124: Hold the device_manager lock only long enough to extract
the needed handles: call self.device_manager.lock().unwrap(), call
get_irq_chip() (and convert to the expected Result/error as done now) and
collect mmio_devices() into local variables, then immediately drop the lock
before calling boot_loader.load; pass the extracted irq_chip (use as_ref() if
load expects a reference) and the collected mmio_devices to boot_loader.load so
the mutex is not held during the load call (symbols:
self.device_manager.lock().unwrap(), device_manager.get_irq_chip(),
device_manager.mmio_devices(), boot_loader.load).
Comment on lines
+57
to
+58
| let device = device.lock().unwrap(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file and the specific lines mentioned
cat -n crates/vm-core/src/arch/vm_exit/aarch64.rs | head -80 | tail -30Repository: junyu0312/rust-vm
Length of output: 1509
🏁 Script executed:
# Check the Error type definition to understand MmioErr
rg -n 'enum Error|fn MmioErr' crates/vm-core/src/arch/vm_exit/aarch64.rs -A 3Repository: junyu0312/rust-vm
Length of output: 160
🏁 Script executed:
# Search for all lock().unwrap() patterns in the file
rg -n 'lock\(\)\.unwrap\(\)' crates/vm-core/src/arch/vm_exit/aarch64.rsRepository: junyu0312/rust-vm
Length of output: 164
🏁 Script executed:
# Check the return type and Error definition in the entire vm-core crate
rg -n 'enum Error' crates/vm-core/src/ -A 10 | head -50Repository: junyu0312/rust-vm
Length of output: 2788
Handle mutex poisoning gracefully in VM-exit handlers.
lock().unwrap() will panic if the device handler mutex is poisoned. Replace with error mapping to Error::MmioErr so VM-exit handling fails gracefully instead of crashing (lines 57–58 and 68–69).
Suggested fix
- let device = device.lock().unwrap();
+ let device = device
+ .lock()
+ .map_err(|_| Error::MmioErr("device handler lock poisoned".into()))?;Apply the same change at both lines 57–58 (MMIORead) and 68–69 (MMIOWrite).
📝 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
| let device = device.lock().unwrap(); | |
| let device = device | |
| .lock() | |
| .map_err(|_| Error::MmioErr("device handler lock poisoned".into()))?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm-core/src/arch/vm_exit/aarch64.rs` around lines 57 - 58, The VM-exit
handlers use device.lock().unwrap(), which will panic on mutex poisoning; change
both occurrences in the MMIORead and MMIOWrite handlers to handle locking errors
by replacing unwrap() with a fallible lock (e.g., device.lock().map_err(|_|
Error::MmioErr)?) and propagate Err(Error::MmioErr) so the VM-exit returns an
MmioErr instead of crashing; update the code paths in the MMIORead and MMIOWrite
functions that currently call device.lock().unwrap() to map the poisoned error
into Error::MmioErr and use the resulting locked guard for subsequent
operations.
Comment on lines
+1
to
13
| use std::sync::Arc; | ||
|
|
||
| use crate::device::Result; | ||
| use crate::device::mmio::MmioLayout; | ||
|
|
||
| pub trait DeviceVmExitHandler { | ||
| fn io_in(&mut self, port: u16, data: &mut [u8]) -> Result<()>; | ||
| fn io_out(&mut self, port: u16, data: &[u8]) -> Result<()>; | ||
| fn mmio_read(&self, addr: u64, len: usize, data: &mut [u8]) -> Result<()>; | ||
| fn mmio_write(&self, addr: u64, len: usize, data: &[u8]) -> Result<()>; | ||
| fn in_mmio_range(&self, addr: u64) -> bool; | ||
| fn mmio_layout(&self) -> Arc<MmioLayout>; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for DeviceVmExitHandler usages and context
rg -n "DeviceVmExitHandler" -g "*.rs" -C 2Repository: junyu0312/rust-vm
Length of output: 4126
🏁 Script executed:
#!/bin/bash
# Search for Arc<Mutex<dyn DeviceVmExitHandler patterns specifically
rg -n "Arc.*Mutex.*DeviceVmExitHandler" -g "*.rs" -C 2Repository: junyu0312/rust-vm
Length of output: 1424
🏁 Script executed:
#!/bin/bash
# Check the trait definition more thoroughly to see if Send is already bound
cat -n crates/vm-core/src/device/vm_exit.rsRepository: junyu0312/rust-vm
Length of output: 651
Add Send bound to DeviceVmExitHandler trait.
The trait is used in Arc<Mutex<dyn DeviceVmExitHandler>> across multiple VM implementations (kvm, hvp) and architecture handlers (aarch64), which suggests cross-thread usage. Without a Send supertrait bound, the trait object cannot be safely sent across threads. Add : Send to the trait definition:
pub trait DeviceVmExitHandler: Send {
fn io_in(&mut self, port: u16, data: &mut [u8]) -> Result<()>;
fn io_out(&mut self, port: u16, data: &[u8]) -> Result<()>;
fn mmio_read(&self, addr: u64, len: usize, data: &mut [u8]) -> Result<()>;
fn mmio_write(&self, addr: u64, len: usize, data: &[u8]) -> Result<()>;
fn in_mmio_range(&self, addr: u64) -> bool;
fn mmio_layout(&self) -> Arc<MmioLayout>;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm-core/src/device/vm_exit.rs` around lines 1 - 13, The
DeviceVmExitHandler trait is used as a trait object inside Arc<Mutex<dyn
DeviceVmExitHandler>> across threads, so add a Send supertrait to the trait
definition: change the declaration of DeviceVmExitHandler to require Send (i.e.,
pub trait DeviceVmExitHandler: Send { ... }) so that dyn DeviceVmExitHandler is
eligible to be sent between threads; update the trait declaration in vm_exit.rs
accordingly.
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