Skip to content

openvmm_core: Support hotplugging device-backed disks #3190

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

Open
sprt wants to merge 1 commit into
base: main
Choose a base branch
from
+51 −3
Open

openvmm_core: Support hotplugging device-backed disks #3190

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
4 changes: 4 additions & 0 deletions Cargo.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5217,6 +5217,7 @@ dependencies = [
"crc32fast",
"debug_ptr",
"disk_backend",
"disk_blockdevice",
"fdt",
"firmware_pcat",
"firmware_uefi",
Expand Down Expand Up @@ -5250,11 +5251,13 @@ dependencies = [
"openvmm_pcat_locator",
"pal",
"pal_async",
"pal_uring",
"pci_bus",
"pci_core",
"pci_resources",
"pcie",
"range_map_vec",
"scsi_buffers",
"scsi_core",
"scsidisk",
"serial_16550_resources",
Expand All @@ -5265,6 +5268,7 @@ dependencies = [
"tracing",
"uefi_nvram_storage",
"uefi_specs",
"uevent",
"virt",
"virt_whp",
"virtio",
Expand Down
4 changes: 4 additions & 0 deletions openvmm/openvmm_core/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,10 @@ vmswitch.workspace = true

[target.'cfg(target_os = "linux")'.dependencies]
vmgs_broker = { workspace = true, features = ["encryption"] }
disk_blockdevice.workspace = true
pal_uring.workspace = true
scsi_buffers.workspace = true
uevent.workspace = true

[build-dependencies]
build_rs_guest_arch.workspace = true
Expand Down
41 changes: 41 additions & 0 deletions openvmm/openvmm_core/src/worker/dispatch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ use chipset_device_resources::IRQ_LINE_SET;
use debug_ptr::DebugPtr;
use disk_backend::Disk;
use disk_backend::resolve::ResolveDiskParameters;
#[cfg(target_os = "linux")]
use disk_blockdevice::BlockDeviceResolver;
use firmware_uefi::LogLevel;
use firmware_uefi::UefiCommandSet;
use floppy_resources::FloppyDiskConfig;
Expand Down Expand Up @@ -75,10 +77,14 @@ use pal_async::DefaultPool;
use pal_async::local::block_with_io;
use pal_async::task::Spawn;
use pal_async::task::Task;
#[cfg(target_os = "linux")]
use pal_uring::IoUringPool;
use pci_core::PciInterruptPin;
use pcie::root::GenericPcieRootComplex;
use pcie::root::GenericPcieRootPortDefinition;
use pcie::switch::GenericPcieSwitch;
#[cfg(target_os = "linux")]
use scsi_buffers::BounceBufferTracker;
use scsi_core::ResolveScsiDeviceHandleParams;
use scsidisk::SimpleScsiDisk;
use scsidisk::atapi_scsi::AtapiScsiDisk;
Expand All @@ -91,6 +97,8 @@ use std::sync::Arc;
use std::thread;
use std::thread::JoinHandle;
use storvsp::ScsiControllerDisk;
#[cfg(target_os = "linux")]
use uevent::UeventListener;
use virt::ProtoPartition;
use virt::VpIndex;
use virtio::PciInterruptModel;
Expand Down Expand Up @@ -1025,6 +1033,39 @@ impl InitializedVm {

resolver.add_resolver(vmm_core::platform_resolvers::HaltResolver(halt_vps.clone()));

#[cfg(target_os = "linux")]
{
let block_io_uring: Arc<dyn pal_uring::Initiate> = {
let pool = IoUringPool::new("disk_blockdevice", 16)
.context("failed to create block device io_uring pool")?;
let initiator = Arc::new(pool.client().initiator().clone());
thread::Builder::new()
.name("disk-blockdevice-iouring".to_string())
.spawn(move || pool.run())
.context("failed to spawn block device io_uring pool thread")?;
initiator
Comment on lines +1041 to +1046
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

The io_uring pool thread is spawned and the JoinHandle is immediately dropped, so the thread can’t be joined on shutdown and panics inside the pool won’t be observed. Consider storing the handle in VM state (and joining it during teardown) or using an existing managed pool/threading abstraction so repeated VM restarts don’t accumulate detached background threads.

Copilot uses AI. Check for mistakes.
};

let uevent_listener = Arc::new(
UeventListener::new(&driver_source.simple())
.context("failed to start block device uevent listener")?,
);

let bounce_buffer_tracker = Arc::new(BounceBufferTracker::new(
2048,
processor_topology.vp_count() as usize,
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

BounceBufferTracker::new is sized with processor_topology.vp_count(), but disk_blockdevice indexes the tracker with affinity::get_cpu_number() (host CPU id). If the async tasks run on a host CPU index >= vp_count, acquire_bounce_buffers() will panic on .get(thread).unwrap(). Consider sizing the tracker by host CPU count (e.g. pal::unix::affinity::num_procs()) or otherwise ensuring the indexing scheme matches the value passed here.

Suggested change
processor_topology.vp_count() as usize,
pal::unix::affinity::num_procs(),

Copilot uses AI. Check for mistakes.
));

resolver.add_async_resolver::<DiskHandleKind, _, disk_blockdevice::OpenBlockDeviceConfig, _>(
BlockDeviceResolver::new(
block_io_uring,
Some(uevent_listener),
bounce_buffer_tracker,
cfg!(guest_arch = "aarch64"),
),
);
Comment on lines +1038 to +1066
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

Creating the io_uring pool and starting the uevent listener unconditionally during VM initialization means OpenVMM will fail to start on Linux systems where io_uring is unavailable/disabled (e.g. ENOSYS/permissions), even if no disk_handle:block disks are configured. Consider deferring pool/listener creation until a block device disk is actually resolved (lazy init inside the resolver), or gracefully skipping registration when io_uring isn’t supported and only erroring if a block disk is requested.

Suggested change
let block_io_uring: Arc<dyn pal_uring::Initiate> = {
let pool = IoUringPool::new("disk_blockdevice", 16)
.context("failed to create block device io_uring pool")?;
let initiator = Arc::new(pool.client().initiator().clone());
thread::Builder::new()
.name("disk-blockdevice-iouring".to_string())
.spawn(move || pool.run())
.context("failed to spawn block device io_uring pool thread")?;
initiator
};
let uevent_listener = Arc::new(
UeventListener::new(&driver_source.simple())
.context("failed to start block device uevent listener")?,
);
let bounce_buffer_tracker = Arc::new(BounceBufferTracker::new(
2048,
processor_topology.vp_count() as usize,
));
resolver.add_async_resolver::<DiskHandleKind, _, disk_blockdevice::OpenBlockDeviceConfig, _>(
BlockDeviceResolver::new(
block_io_uring,
Some(uevent_listener),
bounce_buffer_tracker,
cfg!(guest_arch = "aarch64"),
),
);
let is_optional_block_backend_error = |err: &anyhow::Error| {
err.chain().any(|cause| {
cause
.downcast_ref::<std::io::Error>()
.is_some_and(|io_err| {
matches!(
io_err.kind(),
std::io::ErrorKind::PermissionDenied
| std::io::ErrorKind::Unsupported
) || matches!(io_err.raw_os_error(), Some(38 | 1 | 13))
})
})
};
let block_backend = (|| -> anyhow::Result<_> {
let block_io_uring: Arc<dyn pal_uring::Initiate> = {
let pool = IoUringPool::new("disk_blockdevice", 16)
.context("failed to create block device io_uring pool")?;
let initiator = Arc::new(pool.client().initiator().clone());
thread::Builder::new()
.name("disk-blockdevice-iouring".to_string())
.spawn(move || pool.run())
.context("failed to spawn block device io_uring pool thread")?;
initiator
};
let uevent_listener = Arc::new(
UeventListener::new(&driver_source.simple())
.context("failed to start block device uevent listener")?,
);
let bounce_buffer_tracker = Arc::new(BounceBufferTracker::new(
2048,
processor_topology.vp_count() as usize,
));
Ok((block_io_uring, uevent_listener, bounce_buffer_tracker))
})();
match block_backend {
Ok((block_io_uring, uevent_listener, bounce_buffer_tracker)) => {
resolver.add_async_resolver::<DiskHandleKind, _, disk_blockdevice::OpenBlockDeviceConfig, _>(
BlockDeviceResolver::new(
block_io_uring,
Some(uevent_listener),
bounce_buffer_tracker,
cfg!(guest_arch = "aarch64"),
),
);
}
Err(err) if is_optional_block_backend_error(&err) => {
tracing::warn!(
error = %err,
"block device backend unavailable on this host; skipping block device resolver registration"
);
}
Err(err) => {
return Err(err)
.context("failed to initialize block device backend");
}
}

Copilot uses AI. Check for mistakes.
}

let generation_id_recv = cfg.generation_id_recv.unwrap_or_else(|| mesh::channel().1);

let logger = Box::new(emuplat::firmware::MeshLogger::new(
Expand Down
5 changes: 2 additions & 3 deletions vm/devices/storage/disk_blockdevice/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ use guestmem::MemoryWrite;
use inspect::Inspect;
use io_uring::opcode;
use io_uring::types;
use io_uring::types::RwFlags;
use mesh::MeshPayload;
use nvme::check_nvme_status;
use nvme_spec::nvm;
Expand Down Expand Up @@ -635,7 +634,7 @@ impl DiskIo for BlockDevice {

// Documented in Linux manual page: https://man7.org/linux/man-pages/man2/readv.2.html
// It's only defined in linux_gnu but not in linux_musl. So we have to define it.
const RWF_DSYNC: RwFlags = 0x00000002;
const RWF_DSYNC: u32 = 0x00000002;

// SAFETY: the buffers for the IO are this stack, and they will be
// kept alive for the duration of the IO since we immediately call
Expand All @@ -648,7 +647,7 @@ impl DiskIo for BlockDevice {
io_vecs.len() as _,
)
.offset((sector * self.sector_size() as u64) as _)
.rw_flags(if fua { RWF_DSYNC } else { 0 })
.rw_flags((if fua { RWF_DSYNC } else { 0 }) as _)
.build()
})
}
Expand Down
Loading