Skip to content

refine: Refine device_manager #89

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.

Sign up for GitHub

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

Merged
junyu0312 merged 1 commit into from
Mar 1, 2026
Merged

refine: Refine device_manager #89

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions crates/vm-core/src/arch/aarch64/vm_exit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,16 +54,14 @@ pub fn handle_vm_exit(
vcpu: &dyn AArch64Vcpu,
exit_reason: VmExitReason,
psci: Arc<Mutex<dyn Psci>>,
device: Arc<Mutex<dyn DeviceVmExitHandler>>,
device: Arc<dyn DeviceVmExitHandler>,
) -> Result<HandleVmExitResult, Error> {
trace!(?exit_reason);

match exit_reason {
VmExitReason::Unknown => Ok(HandleVmExitResult::Continue),
VmExitReason::Wf => Ok(HandleVmExitResult::NextInstruction),
VmExitReason::MMIORead { gpa, srt, len } => {
let device = device.lock().unwrap();

let mut buf = [0; 8];
device
.mmio_read(gpa, len, &mut buf[0..len])
Expand All @@ -73,8 +71,6 @@ pub fn handle_vm_exit(
Ok(HandleVmExitResult::NextInstruction)
}
VmExitReason::MMIOWrite { gpa, buf, len } => {
let device = device.lock().unwrap();

device
.mmio_write(gpa, len, &buf)
.map_err(|err| Error::MmioErr(err.to_string()))?;
Expand Down
4 changes: 1 addition & 3 deletions crates/vm-core/src/device.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,10 @@ pub enum Error {
mmio_len: usize,
addr: u64,
},
#[error("irq_chip already exists")]
IrqChipAlreadyExists,
}

pub type Result<T> = core::result::Result<T, Error>;

pub trait Device: Send {
pub trait Device: Send + Sync {
fn name(&self) -> String;
}
14 changes: 0 additions & 14 deletions crates/vm-core/src/device/device_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
use std::cell::OnceCell;
use std::slice::Iter;
use std::sync::Arc;

use crate::arch::irq::InterruptController;
use crate::device::Error;
use crate::device::Result;
use crate::device::mmio::MmioLayout;
Expand All @@ -13,7 +11,6 @@ use crate::device::pio::pio_device::PioDevice;
use crate::device::vm_exit::DeviceVmExitHandler;

pub struct DeviceManager {
irq_chip: OnceCell<Arc<dyn InterruptController>>,
pio_manager: PioAddressSpaceManager,
mmio_manager: MmioAddressSpaceManager,
}
Expand Down Expand Up @@ -99,22 +96,11 @@ impl DeviceVmExitHandler for DeviceManager {
impl DeviceManager {
pub fn new(mmio_layout: MmioLayout) -> Self {
DeviceManager {
irq_chip: Default::default(),
pio_manager: PioAddressSpaceManager::default(),
mmio_manager: MmioAddressSpaceManager::new(mmio_layout),
}
}

pub fn register_irq_chip(&mut self, irq_chip: Arc<dyn InterruptController>) -> Result<()> {
self.irq_chip
.set(irq_chip)
.map_err(|_| Error::IrqChipAlreadyExists)
}

pub fn get_irq_chip(&self) -> Option<Arc<dyn InterruptController>> {
self.irq_chip.get().cloned()
}

pub fn register_pio_device(&mut self, device: Box<dyn PioDevice>) -> Result<()> {
self.pio_manager.register(device)
}
Expand Down
2 changes: 1 addition & 1 deletion crates/vm-core/src/device/mmio/mmio_device.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use vm_fdt::FdtWriter;
use crate::device::Device;
use crate::device::mmio::MmioRange;

pub trait MmioHandler: Send {
pub trait MmioHandler: Send + Sync {
fn mmio_range(&self) -> MmioRange;

fn mmio_read(&self, offset: u64, len: usize, data: &mut [u8]);
Expand Down
2 changes: 1 addition & 1 deletion crates/vm-core/src/device/vm_exit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::sync::Arc;
use crate::device::Result;
use crate::device::mmio::MmioLayout;

pub trait DeviceVmExitHandler: Send {
pub trait DeviceVmExitHandler: Send + Sync {
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<()>;
Expand Down
3 changes: 1 addition & 2 deletions crates/vm-core/src/virt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use std::sync::Arc;
use std::sync::Mutex;

use vm_mm::allocator::MemoryContainer;
use vm_mm::manager::MemoryAddressSpace;
Expand Down Expand Up @@ -38,5 +37,5 @@ pub trait Virt: Sized {
fn get_vcpus(&self) -> Result<&Vec<Self::Vcpu>>;
fn get_vcpus_mut(&mut self) -> Result<&mut Vec<Self::Vcpu>>;

fn run(&mut self, device_manager: Arc<Mutex<dyn DeviceVmExitHandler>>) -> Result<()>;
fn run(&mut self, device_manager: Arc<dyn DeviceVmExitHandler>) -> Result<()>;
}
4 changes: 2 additions & 2 deletions crates/vm-core/src/virt/hvp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,8 +322,8 @@ impl Virt for Hvp {
.ok_or_else(|| Error::Internal("vcpu is not initialized".to_string()))
}

fn run(&mut self, device_manager: Arc<Mutex<dyn DeviceVmExitHandler>>) -> Result<()> {
let mmio_layout = device_manager.lock().unwrap().mmio_layout();
fn run(&mut self, device_manager: Arc<dyn DeviceVmExitHandler>) -> Result<()> {
let mmio_layout = device_manager.mmio_layout();

thread::scope(|s| {
let cpu_on_receiver = self.cpu_on_receiver.take().unwrap();
Expand Down
5 changes: 2 additions & 3 deletions crates/vm-core/src/virt/kvm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use std::cell::OnceCell;
use std::marker::PhantomData;
use std::sync::Arc;
use std::sync::Mutex;

use kvm_ioctls::*;
use memmap2::MmapMut;
Expand Down Expand Up @@ -102,15 +101,15 @@ where
.ok_or_else(|| Error::Internal("vcpus is not init".to_string()))
}

fn run(&mut self, device: Arc<Mutex<dyn DeviceVmExitHandler>>) -> Result<()> {
fn run(&mut self, device: Arc<dyn DeviceVmExitHandler>) -> Result<()> {
let vcpus = self
.vcpus
.get_mut()
.ok_or_else(|| Error::Internal("vcpus is not init".to_string()))?;

assert_eq!(vcpus.len(), 1);

let mmio_layout = device.lock().unwrap().mmio_layout();
let mmio_layout = device.mmio_layout();

vcpus.get_mut(0).unwrap().run(mmio_layout.as_ref())?;

Expand Down
23 changes: 3 additions & 20 deletions crates/vm-machine/src/device.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use vm_core::arch::irq::InterruptController;
use vm_core::device::device_manager::DeviceManager;
use vm_core::device::mmio::MmioRange;
use vm_device::device::Device;
use vm_device::device::gic_v3::GicV3;
use vm_device::device::virtio::virtio_blk::VirtIoBlkDevice;
use vm_device::device::virtio::virtio_blk::VirtIoMmioBlkDevice;
use vm_mm::allocator::MemoryContainer;
Expand All @@ -19,7 +18,7 @@ pub trait InitDevice {
&mut self,
mm: Arc<MemoryAddressSpace<C>>,
devices: Vec<Device>,
irq_chip: Option<Arc<dyn InterruptController>>,
irq_chip: Arc<dyn InterruptController>,
) -> Result<(), Error>
where
C: MemoryContainer;
Expand All @@ -29,28 +28,12 @@ impl InitDevice for DeviceManager {
fn init_devices<C>(
&mut self,
mm: Arc<MemoryAddressSpace<C>>,
devices: Vec<Device>,
irq_chip: Option<Arc<dyn InterruptController>>,
_devices: Vec<Device>,
irq_chip: Arc<dyn InterruptController>,
) -> Result<(), Error>
Comment on lines +31 to 33
Copy link

@coderabbitai coderabbitai bot Mar 1, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

init_devices currently drops the caller-provided device list.

At Line 31, devices is changed to _devices, and none of the supplied Device entries are consumed. This means VM-configured devices are silently ignored and never registered.

Please either (1) consume and register devices here, or (2) fail fast when non-empty input is provided to avoid silent misconfiguration.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@crates/vm-machine/src/device.rs` around lines 31 - 33, The init_devices
function currently ignores the caller-provided device list because the parameter
was renamed to _devices; revert to accepting devices: Vec<Device> (or remove the
underscore) and either iterate and consume each Device to register them (e.g.,
call the module's device registration routine such as
self.register_device(device, irq_chip.clone()) or push into the VM/device
registry while wiring interrupts via irq_chip: Arc<dyn InterruptController>), or
if registration support isn't available, add a fast-fail check that returns an
Err when devices.is_empty() is false to avoid silently dropping caller devices;
reference the init_devices signature, the Device type, and the irq_chip /
InterruptController symbols when making the change.

where
C: MemoryContainer,
{
let irq_chip = match irq_chip {
Some(irq_chip) => irq_chip,
None => {
let irq_chip = devices
.iter()
.find(|dev| dev.is_irq_chip())
.ok_or(Error::NoIrqChipSpecified)?;

match irq_chip {
Device::GicV3 => Arc::new(GicV3::default()),
}
}
};

self.register_irq_chip(irq_chip.clone())?;

{
let pci_rc = PciRootComplexMmio::new(
MmioRange {
Expand Down
24 changes: 9 additions & 15 deletions crates/vm-machine/src/vm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::sync::Arc;
use std::sync::Mutex;

use vm_bootloader::boot_loader::BootLoader;
use vm_core::arch::irq::InterruptController;
use vm_core::debug::gdbstub::GdbStub;
use vm_core::device::device_manager::DeviceManager;
use vm_core::virt::Virt;
Expand All @@ -13,7 +13,8 @@ use crate::error::Result;
pub struct Vm<V: Virt> {
pub(crate) memory: Arc<MemoryAddressSpace<V::Memory>>,
pub(crate) virt: V,
pub(crate) device_manager: Arc<Mutex<DeviceManager>>,
pub(crate) irq_chip: Arc<dyn InterruptController>,
pub(crate) device_manager: Arc<DeviceManager>,
pub(crate) gdb_stub: Option<GdbStub>,
}

Expand All @@ -22,19 +23,12 @@ where
V: Virt,
{
pub fn run(&mut self, boot_loader: &dyn BootLoader<V>) -> Result<()> {
{
let device_manager = self.device_manager.lock().unwrap();

boot_loader.load(
&mut self.virt,
&self.memory,
device_manager
.get_irq_chip()
.ok_or_else(|| Error::InitIrqchip("irq_chip is not exists".to_string()))?
.as_ref(),
device_manager.mmio_devices(),
)?;
}
boot_loader.load(
&mut self.virt,
&self.memory,
self.irq_chip.as_ref(),
self.device_manager.mmio_devices(),
)?;

if let Some(gdb_stub) = &self.gdb_stub {
gdb_stub
Expand Down
10 changes: 5 additions & 5 deletions crates/vm-machine/src/vm_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use std::sync::Arc;
use std::sync::Mutex;

use vm_core::arch::layout::MemoryLayout;
use vm_core::debug::gdbstub::GdbStub;
Expand Down Expand Up @@ -49,18 +48,19 @@ impl VmBuilder {
let mmio_layout = MmioLayout::new(layout.get_mmio_start(), layout.get_mmio_len());

let irq_chip = if !self.devices.iter().any(Device::is_irq_chip) {
Some(virt.init_irq()?)
virt.init_irq()?
} else {
None
todo!()
};
Comment on lines 50 to 54
Copy link

@coderabbitai coderabbitai bot Mar 1, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

todo!() in IRQ-chip path will panic for valid input.

At Line 53, build() aborts when an IRQ-chip device is present in self.devices. This should be handled explicitly (construct/use the provided IRQ chip) or return a structured error instead of panicking.

If helpful, I can draft the concrete error-handling + IRQ-chip extraction flow for this branch.

🤖 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 50 - 54, The build()
function currently panics via todo!() when an IRQ-chip device exists; replace
that panic by explicitly handling the provided IRQ chip or returning a
structured error: inspect self.devices to find the device where
Device::is_irq_chip is true, extract/construct the irq_chip from that device (or
call its provided accessor/constructor) and assign it to irq_chip, and if
extraction is not possible return a descriptive Err (not panic) propagated from
build(); keep the existing virt.init_irq() path for the no-IRQ-chip case and
ensure the returned error type matches build()'s signature.


let mut device_manager = DeviceManager::new(mmio_layout);
device_manager.init_devices(memory.clone(), self.devices, irq_chip)?;
device_manager.init_devices(memory.clone(), self.devices, irq_chip.clone())?;

let vm = Vm {
memory,
virt,
device_manager: Arc::new(Mutex::new(device_manager)),
irq_chip,
device_manager: Arc::new(device_manager),
gdb_stub: self.gdb_port.map(GdbStub::new),
};

Expand Down
2 changes: 1 addition & 1 deletion crates/vm-pci/src/device/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::types::configuration_space::ConfigurationSpace;

pub mod type0;

pub trait BarHandler: Send {
pub trait BarHandler: Send + Sync {
fn read(&self, offset: u64, data: &mut [u8]);

fn write(&self, offset: u64, data: &[u8]);
Expand Down
2 changes: 1 addition & 1 deletion crates/vm-pci/src/types/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ pub enum EcamUpdateCallback {
},
}

pub trait PciFunction: PciFunctionArch + Send {
pub trait PciFunction: PciFunctionArch + Send + Sync {
fn ecam_read(&self, offset: u16, buf: &mut [u8]);

fn ecam_write(&self, offset: u16, buf: &[u8]) -> Option<EcamUpdateCallback>;
Expand Down
2 changes: 1 addition & 1 deletion crates/vm-virtio/src/device.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ pub trait VirtQueueHandler {
async fn handler(&self, virt_queue: &mut VirtQueue);
}

pub trait VirtIoDevice: Sized + Send + 'static {
pub trait VirtIoDevice: Sized + Send + Sync + 'static {
const NAME: &str;
const DEVICE_ID: u32;
const VIRT_QUEUES_SIZE_MAX: &[u32];
Expand Down
Loading