Skip to content

openvmm_core: Support hotplugging device-backed disks#3190

Open
sprt wants to merge 1 commit intomicrosoft:mainfrom
sprt:blockdevice-resolver
Open

openvmm_core: Support hotplugging device-backed disks#3190
sprt wants to merge 1 commit intomicrosoft:mainfrom
sprt:blockdevice-resolver

Conversation

@sprt
Copy link
Copy Markdown
Contributor

@sprt sprt commented Apr 3, 2026

This adds a resolver for disk_handle:block based on prior art in Underhill. File-backed disks are already supported.

We also retype RWF_DSYNC in a backward-compatible way as, in future io-uring versions, RwFlags is deprecated and rw_flags() takes i32.

This adds a resolver for disk_handle:block based on prior art in Underhill.
File-backed disks are already supported.

We also retype RWF_DSYNC in a backward-compatible way as, in future io-uring
versions, RwFlags is deprecated and rw_flags() takes i32.
Copilot AI review requested due to automatic review settings April 3, 2026 18:44
@github-actions github-actions bot added the unsafe Related to unsafe code label Apr 3, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 3, 2026

⚠️ Unsafe Code Detected

This PR modifies files containing unsafe Rust code. Extra scrutiny is required during review.

For more on why we check whole files, instead of just diffs, check out the Rustonomicon

@sprt sprt marked this pull request as ready for review April 3, 2026 18:46
@sprt sprt requested review from a team as code owners April 3, 2026 18:46
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds Linux block-device disk support to openvmm_core by wiring in the disk_blockdevice resolver (io_uring + uevent resize support) and updates disk_blockdevice’s RWF_DSYNC handling to be compatible with evolving io_uring flag types.

Changes:

  • Register a Linux-only disk_handle:block async resolver in openvmm_core, backed by a dedicated io_uring pool thread and uevent listener.
  • Add Linux-only dependencies to openvmm_core for block-device disk support (disk_blockdevice, pal_uring, scsi_buffers, uevent).
  • Retype RWF_DSYNC in disk_blockdevice to avoid relying on io_uring::types::RwFlags.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.

File Description
vm/devices/storage/disk_blockdevice/src/lib.rs Adjusts RWF_DSYNC typing/casting to remain compatible with io_uring API changes.
openvmm/openvmm_core/src/worker/dispatch.rs Adds Linux-only initialization and registration for the block-device disk resolver (io_uring pool, uevent listener, bounce buffers).
openvmm/openvmm_core/Cargo.toml Adds Linux-only crate dependencies required for block-device disk hotplug/IO.
Cargo.lock Locks in the new openvmm_core dependency edges.

Comment on lines +1038 to +1066
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"),
),
);
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.
Comment on lines +1041 to +1046
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
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 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

unsafe Related to unsafe code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants