Repository navigation
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## ilstam-hotplug-2 #6228 +/- ##
====================================================
+ Coverage 83.01% 83.17% +0.16%
====================================================
Files 279 279
Lines 31829 32166 +337
====================================================
+ Hits 26423 26755 +332
- Misses 5406 5411 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ilstam
added this pull request to stack #6230
September 17, 2026 17:16
ilstam
marked this pull request as ready for review
September 17, 2026 17:16
ilstam
force-pushed
the
ilstam-hotplug-3
branch
from
September 17, 2026 18:11
907c364 to
e174e38
Compare
ilstam
force-pushed
the
ilstam-hotplug-3
branch
from
September 18, 2026 17:22
e174e38 to
4794850
Compare
ilstam
added
the
Status: Awaiting review
Manciukic
reviewed
Oct 2, 2026
Contributor
There was a problem hiding this comment.
Thanks Ilias, this is a nice piece of work. Comments inline.
The happy paths are well covered, including on restored VMs via uvm_any, but nothing snapshots in the middle of a hotplug operation, which is where most of the bugs above live. Test gaps I couldn't anchor inline:
- snapshot/restore while a graceful unplug is pending (DELETE, then snapshot inside pciehp's 5 s window): the restored guest should still complete the removal and the device should leave
GET /vm/config - a unit test for an ack that hasn't been processed yet when the state is saved (
acked_busesset,complete_hotplug_removals()not run): the restored VM should still drop the device. The window is too narrow to hit reliably from Python, see the comment onapi_server_adapter.rs - hotplug, snapshot, restore, then unplug (graceful and forced).
test_hotplug_preserved_after_snapshotstops after I/O, anduvm_restoredsnapshots before anything is plugged, so the restored slot state (presence, power indicator, guest-programmed MSI-X) is never exercised by an unplug - a
removableboot device across snapshot/restore, then unplugged (graceful and forced). The topology tests usemicrovm_factorydirectly, so they only run booted - DELETE twice on the same device (would catch the attention-button cancel)
- hotplug/unplug requests issued while the VM is paused, then resume
- negative tests for
removable: with PCI off, more removable devices than ports, and the root device markedremovabletogether with another drive - a unit test for the BAR allocation failure path from 512a314
btw test_root_ports_start_empty and test_no_root_ports could use uvm_any.
Nits in the commit messages: "mnual" in 43cffe7, and "could have been still be using it" in 7f31dc3.
Comment on lines
125
to
128
| # Verify the device is gone | ||
| time.sleep(UNPLUG_SLEEP) | ||
| _, lspci_after_unplug, _ = vm.ssh.check_output("lspci -n") | ||
| assert lspci_after_unplug == lspci_before |
Contributor
There was a problem hiding this comment.
The fixed sleeps (0.4s plug, 6s unplug, 10s after the 29 unplugs, 1s after force) are tight next to pciehp's own 5s wait on a loaded CI host, and they cost a lot of time per run (test_hotplug_max_devices alone sleeps ~33s). Could we poll instead? tenacity is already used in framework/utils.py:
Suggested change
| # Verify the device is gone | |
| time.sleep(UNPLUG_SLEEP) | |
| _, lspci_after_unplug, _ = vm.ssh.check_output("lspci -n") | |
| assert lspci_after_unplug == lspci_before | |
| # Verify the device is gone | |
| for attempt in Retrying( | |
| stop=stop_after_delay(30), wait=wait_fixed(0.2), reraise=True | |
| ): | |
| with attempt: | |
| _, lspci_after_unplug, _ = vm.ssh.check_output("lspci -n") | |
| assert lspci_after_unplug == lspci_before |
(needs from tenacity import Retrying, stop_after_delay, wait_fixed). Same for the other PLUG_SLEEP / UNPLUG_SLEEP call sites.
| description: | ||
| Path to the socket of vhost-user-block backend. | ||
| This field is required for vhost-user-block config should be omitted for virtio-block configuration. | ||
| removable: |
Contributor
There was a problem hiding this comment.
honestly, I feel like we're adding a lot of complexity for this use case with little benefits. What's the difference with requiring the device to be hotplugged right after boot for it to be removable?
Contributor
Author
There was a problem hiding this comment.
The difference is ... you have to hotplug it first, so potentially extra latency? But as discussed over lunch, I'm happy to drop this removable idea altogether and reconsider it if anyone asks for it and presents a valid use case.
| // We can't call complete_hotplug_removals() here because we have no | ||
| // reference to the EventManager. Simply drain the eventfd so it stops | ||
| // firing. complete_hotplug_removals() is called in the VMM event loop. | ||
| if let Some(evt) = self.device_manager.hotplug_completion_evt() |
Contributor
There was a problem hiding this comment.
I'm wondering if this could be simpler by just using the RemoteEndpoint feature of event-manager.
Contributor
Author
There was a problem hiding this comment.
Haven't seen this before, but does it help here where the problem is that we have no reference to the EventManager? The doc says "The optional remote_endpoint feature enables interactions with the EventManager from different threads without the need of more intrusive synchronization.", in this case the hotplug completion event is delivered in the main thread, which is the one owning the EventManager already.
This reverts commit 93391d4. That commit was add when Firecracker didn't support BAR relocation. Today, not only it supports BAR relocation, but it will soon rely on it when PCIe root ports are present. Firecracker doesn't pre-program the memory windows of root ports so we rely on Linux to program them and then move the device BARs inside them. Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
BAR allocation unwraps its result so a device hotplug could theoretically take the VMM down. Propagate the failure up instead. Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Add a 'hint' parameter to allocate_bars() to let the caller attempt to place a BAR at a specific address. This will be useful for PCI devices behind PCI root ports. Each root port has a non-prefetchable window behind which the BARs of the device behind the root port must live in. If the BAR is allocated outside that window then Linux will either have to move the BAR itself or (in case the the root port window is not large enough) it will have to resize and move the root port window. When hot-plugging devices, the guest has typically already programmed the root port windows, so use them as hints for where the device BAR should go. In case that fails for whatever reason, allocate an address from the bottom of the pool as we did until now. Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Some devices are about to be placed behind PCIe root ports, and Linux's default bridge memory window is 1 MiB, so two devices whose BARs share a megabyte cannot sit behind different ports. BARs are allocated CAPABILITY_BAR_SIZE apart, which is half of that, so a guest that found two such devices would have to move one of them out of the other's window if they are to be placed behind root ports. Allocate on a bridge window boundary instead so that no two devices ever fall within the same bridge window. Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Introduce a new PciPlacement enum describing whether a device must be plugged to the primary bus or to a root port / secondary bus. Place devices added after boot in a free root port slot rather than directly on the root bus, and signal the insertion so the guest can discover them. On removal signal the guest that the port slot is empty. This eliminate the need for manually re-scaning the PCI bus from inside the guest after hot-plugging a device. Note that attach_common() calls PciRootPort::plug() with hotplug=false, because that function is called on snapshot restore and we should not inject spurious notifications to the guest. In case of a real hotplug, hotplug_device() also calls PciRootPort::plug() with hotplug=true to actually inject the interrupt. Also note that the hot-detach is still not "graceful", i.e. the guest is not given the chance to react. Subsequent commits will introduce separate graceful/forced detach options. Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Add a new 'removable' boolean option to the config of devices that be hot-plugged. Setting this means that the device will be placed behind a PCI root port so that it's possible for it to be hot-unplugged later. Otherwise the device will sit on the primary bus and won't be possible to unplug it later. Each removable device consumes one of the ports set aside by 'pcie_hotplug_ports'. The option is ignored for MMIO devices. Note that unfortunately a HashSet storing removable devices is needed in VmResources. That is needed because for most devices we don't store the their JSON config, we rather store the device objects. At the same time GET /vm/config needs to return correct information. An alternative is storing 'removable' in the backend objects, but this is unnecessary adds redundant information to the snapshot. The topology information (which bus the device sits on) is enough to answer whether the device is removable or not, and we don't want to have to add extra checks on snapshot restore to ensure the different snapshots states agree. However, the topology information is not known pre-boot, so to make GET /vm/config work at that point store the removable devices in a HashSet. Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Place removable devices behind root ports rather on the primary so that they can be hot-unplugged later. Since we want non-removable devices to occupy the first slots in the primary bus followed by the root ports, in build_microvm_for_boot() we need to do two passes for some devices (block, net, pmem) where we attach the non-removable ones in the first pass and the removable ones in the second pass. Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Removal has so far been allowed for any non-root device. However, for devices not behind a root port there's no way to notify the guest of an upcoming removal. Reject the unplug request in that case. Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Make device DELETE take an optional body with a single 'force' flag. When not set, the detach is meant to be graceful; the guest is notified and Firecracker waits for a guest acknowledgement before removing the device. When force is set, then the device is removed immediately. In either case the DELETE request is non-blocking. The DELETE endpoints had no Swagger description at all, so document them along with the new body. Only the parsing and plumbing are done in this commit, graceful detach is not implemented yet, all unplugs are still in 'force' mode. Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Removing a device has so far been a surprise removal. The device simply went away with unpredictable consequences for the guest which could have been still be using it. A previous commit added a 'force' option to unplug requests. Make 'graceful detach' be the default behaviour. When force is not set the guest is notified and Firecracker waits for a guest acknowledgement before removing the device. When force is set, then the device is removed immediately. In either case the DELETE request is non-blocking. Since the guest ack is delivered to a vCPU thread and the device removal needs to happen in the main thread, a new HotplugCompletion struct with an eventfd is introduced. A root port can use the eventfd to notify the main thread of device removal acknowledgements and the VMM thread processes the removals in its event loop. Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Implement save/restore for root ports and add a unit test to test it. PciConfiguration::type0_from_state() doesn't seem to be specific to type 0 devices, so rename it to PciConfiguration::from_state(). Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Without native PCIe hotplugging support all integration tests relied on mnual bus rescanning from the guest. Now that PCIe auto-discovery works adjust all tests accordingly. Note that Linux's pciehp driver sleeps after a hot-plug and hot-unplug so the integration tests take that into account. Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
Add new hotplugging and topology tests cases that cover removable devices and forced unplug. Signed-off-by: Ilias Stamatis <ilstam@amazon.com>
ilstam
force-pushed
the
ilstam-hotplug-3
branch
from
October 6, 2026 13:07
4794850 to
80cfe3f
Compare
Contributor
Author
The code has been changed as per your suggestion so this is not a possibility anymore. |
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
2 participants
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.
This PR implements the routing of devices behind root ports when necessary and adds support for graceful and forceful unplug options.