vpci: hot-add/removal for PCI devices over VPCI - #4377
vpci: hot-add/removal for PCI devices over VPCI#4377Daman Mulye (damanm24) wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
The change set touches core hotplug/eject behavior across the control plane, worker dispatch, and VPCI device protocol handling, so it warrants final human validation of edge cases and operational safety.
Pull request overview
This PR adds runtime hot-add/hot-remove support for PCI devices over Hyper-V VPCI, exposing it through the ttrpc control plane and implementing the necessary dynamic device + bus state-unit plumbing to safely offer/revoke VPCI channels while a guest is running (including a bounded grace-period for guest-acknowledged ejection).
Changes:
- Add new ttrpc RPCs
AddVpciDevice/RemoveVpciDevice, plus worker-side dispatch to dynamically build and tear down VPCI-backed PCI devices. - Introduce deferred VPCI channel offering and an ejection control path so hot-added devices can be started before the guest can open the channel, and removed even with an uncooperative guest.
- Extend tests and docs to cover the new RPCs and validate VPCI virtio-fs hotplug behavior on supported hosts.
File summaries
| File | Description |
|---|---|
| vmm_tests/vmm_tests/tests/tests/ttrpc.rs | Expands ttrpc integration tests to exercise VPCI add/remove (including virtio-fs over VPCI). |
| vmm_core/src/vmbus_unit.rs | Updates offer_simple_device_unit call site for the new offer API parameter. |
| vmm_core/src/device_builder.rs | Adds DynamicVpciDevice and builder flow to create a dynamic PCI device + VPCI bus with deferred channel offer. |
| vm/devices/vmbus/vmbus_channel/src/simple.rs | Extends offer_simple_device to optionally start a device before offering the channel. |
| vm/devices/pci/vpci/src/device.rs | Adds an eject control mechanism and protocol handling for EJECT_COMPLETE, integrating with the VPCI worker loop. |
| vm/devices/pci/vpci/src/bus.rs | Refactors VPCI bus to support “unoffered/offered/revoked” channel state, deferred offer, and channel revocation + eject handle. |
| openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto | Defines the new AddVpciDevice/RemoveVpciDevice RPCs and related request/response messages; adds built-in virtio-fs config. |
| openvmm/openvmm_entry/src/ttrpc/mod.rs | Wires new RPCs into the entry service and implements request translation into worker RPCs/resources. |
| openvmm/openvmm_defs/src/rpc.rs | Adds VmRpc::AddVpciDevice / VmRpc::RemoveVpciDevice worker RPC variants. |
| openvmm/openvmm_core/src/worker/dispatch.rs | Implements add/remove handling, enforces the 64-device limit, and adds a timed ejection grace period before forced removal. |
| Guide/src/reference/openvmm/management/grpc.md | Documents the new management RPCs and VPCI semantics/constraints. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a new hotplug control-plane surface and significant runtime device-lifecycle/concurrency behavior changes that warrant final human review despite only minor nits found.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
Will Wright (will-j-wright)
left a comment
Will Wright (will-j-wright)
left a comment
There was a problem hiding this comment.
the vmbus side looks fine but someone should look from the vpci pov
There was a problem hiding this comment.
🟡 Changes recommended
offer_simple_device_unit should select InitialDeviceState::Running when state units are already running to avoid a guest-open panic for dynamically offered simple VMBus devices.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The VPCI eject path in vpci/src/device.rs can be blocked by completion-ring backpressure, preventing timely processing of eject requests and undermining the intended graceful-removal behavior.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
vm/devices/pci/vpci/src/device.rs:781
wait_for_completion_space()is awaited before the loop can observeeject_recv, but VPCI eject requests are sent asInBandNoCompletionpackets. If the completion ring is full (e.g., a stuck guest), the worker can block here and never process the eject request, causing the remove path to unnecessarily hit the grace-period timeout and force-remove the device.
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The added VPCI test helper/probe has brittleness and shell-quoting issues that should be fixed to avoid flaky or unsafe test behavior across environments.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
vmm_tests/vmm_tests/tests/tests/ttrpc.rs:1117
- mount_virtio_fs builds a shell script that interpolates {tag}/{target} into the command text without quoting/parameterization. While current callers pass simple constants, this helper will break on spaces/shell metacharacters and is unnecessarily risky; pass values as positional parameters to
sh -cinstead.
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
| Err(err) => assert!(err.message.contains("does not support VPCI devices")), | ||
| } |
To use OpenVMM for WSL, we need the ability to add/remove devices after a VM has been created. This PR adds the following:
AddVpciDeviceandRemoveVpciDeviceto the RPC control planeThere were a couple of special considerations that made this implementation more complicated than I had initially intended.