Repository navigation
[DO NOT REVIEW] vmotherboard: gate a device's input without closing it to the vCPUs - #5005
Merged
Pedro Henrique Penna (ppenna) merged 2 commits intoNov 10, 2026
Conversation
Restore hung, before it validated anything, when a FIFO without a writer stood in place of a snapshot's manifest.bin, state.bin, memory.bin, or paired scratch.img. Restore opens each artifact without following a final link and then checks the type of the file that it opened, but a read-only open of a FIFO waits until a writer opens the FIFO, so the open never returned to the check: --restore-snapshot, and a management-RPC CreateVm with a restore_path, waited in the open forever (microsoft/nvx#1154). The path-based opener, which the claimed scratch link and write_snapshot's memory backing file use, waited the same way. On Unix, both openers now add O_NONBLOCK, so that the open of a FIFO returns at once and the existing check refuses it like any other file that is not a regular file: "<artifact> is not a regular file". The check then clears O_NONBLOCK on the descriptor of a regular file, so the descriptors that restore keeps and hands to the VM worker, such as the one that the worker maps as RAM, have the flags of a blocking open, as before. Windows needs no change: NTFS holds no FIFOs, and the AF_UNIX sockets and WSL FIFOs that it can hold are reparse points, which the relative open opens as themselves, without waiting, and refuses. Tests replace each artifact of a paired-scratch resume snapshot with a FIFO without a writer and open the snapshot, and its paired scratch, and open a FIFO with each artifact opener, on another thread: each open must fail with the type error within 10 s, and the handle of a regular file must not be nonblocking. Without the change, the opens wait until the tests give the FIFOs writers, and the tests fail. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A snapshot boundary gates host input before it stops the vCPUs, and resumes a gate that failed before any of them stops, so the vCPUs run through both. The chipset device unit nevertheless changed each gate with the device's mutex closed, from the call to the end of the change's wait, as it changes the device's other states, which run only while the vCPUs are stopped. An access that a running vCPU made in that window met the closed mutex: the chipset answered no port or MMIO access, so a read returned all ones and a write was lost, an interrupt from another device was dropped, and an EOI or a PCI configuration access panicked the VM worker with "lock should not be closed" (microsoft/nvx#1138). The window lasted the whole wait of a virtio transport's gate for its device task, and of a remote device's gate for its worker, and even a gate that completes at once, as most devices' gates do, closed the device for an instant, which a vCPU that signals EOIs in a loop met within one to five gates. A gate change now has two parts. ChangeDeviceState::quiesce_input and resume_input make the synchronous part and return what remains as an InputGateWait, a future that does not borrow the device; a device that overrides them returns one. The chipset device unit makes the call under the device's ordinary lock, as a vCPU's access does, releases the lock, and then awaits the wait, so a vCPU's access comes before the synchronous part or after it and is served either way. The virtio transports and the remote device proxy send their gate's request in the synchronous part, on the channel that carries the commands of the guest's accesses, so that their device takes the gate in the order of those accesses, and wait for its answer without the device. The erased chipset devices forward both parts. The other state changes still close the device, since they run only while the vCPUs are stopped. Tests: a vCPU's port write, EOI, and interrupt made while a device's gate change waits, for a quiesce and for a resume, reach the device after the change, without a panic; a vCPU that writes a port and signals EOIs in a loop while the gate is taken and resumed 1000 times, as the guest's snapshot requests make it, finds every access served; on both virtio transports and through the remote device proxy, a gate change reaches the device in the order of the guest's accesses around it, and a remote gate's error is reported. Without the fix, the first test fails with the port write dropped, the EOI panicking, and the interrupt dropped, and the second fails in 10 of 10 runs with an EOI panicking. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
Pedro Henrique Penna (ppenna)
added a commit
to microsoft/nvx
that referenced
this pull request
Nov 9, 2026
A guest's snapshot request could crash a multi-vCPU microVM, or lose the device accesses of its other vCPUs (#1138). The snapshot boundary gates host input before it stops the vCPUs, so they run while each device's state unit changes the device's gate, and the unit held the device's mutex closed for the change, until the end of its wait. A port or MMIO access that another vCPU made then was dropped, a read returning all ones; an interrupt from another device was lost; and an EOI or a PCI configuration access panicked the VM worker with "lock should not be closed". Pin 46bfe02ed65697e858f66ce55eb60831b3ac3461, the head of microsoft/openvmm#5005, one commit on cba21d109, the head of microsoft/openvmm#5004, on which it stacks, which follows ac3dc27a7, the head of microsoft/openvmm#5003, and b4028c1aa, the head of feature/microvm/dev: - 46bfe02ed vmotherboard: gate a device's input without closing it to the vCPUs The revision splits a device's gate change into a synchronous part, which the unit makes under the lock that the vCPUs' accesses take, and a wait that does not borrow the device, which the unit awaits without it. The virtio transports and the remote device proxy send their gate's request in the synchronous part, in the order of the guest's accesses. Evidence: a microVM guest with 2 and 4 vCPUs requested 2000 snapshots, each of which gated host input and then failed its preflight, since its destination existed, while its other vCPUs read the magic value of the virtio-fs device's MMIO window, and with 4 vCPUs the console's too, in a loop. With OpenVMM at d6addf8c1, 145 to 2358 of a run's reads returned all ones, and OpenVMM logged that the device did not respond, in each of 3 runs per count on nvx-kvm-test (KVM), nvx-mshv-test (MSHV), and nvx-whp-test (WHP), all virtual machines; with a91d4c7a4, the same patch on d6addf8c1, no read did. With a91d4c7a4, the default microVM suite passed on all three, with 1, 2, and 4 vCPUs. At the pinned revision, the OpenVMM tests of the changed packages and the virtio devices pass, 545 on Windows and 550 on Linux, as they did at a91d4c7a4. NVX needs no coordinated change: the control-session vectors, the source excerpts, the contract versions, and the CPU profiles that nvx-rs copies from OpenVMM match the revision's. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Pedro Henrique Penna (ppenna)
added a commit
to microsoft/nvx
that referenced
this pull request
Nov 9, 2026
Maps NVX's new pin, 46bfe02ed, the head of microsoft/openvmm#5005, at position 238, after position 237, cba21d109, the head of microsoft/openvmm#5004, on which it stacks, which follows positions 236, ac3dc27a7, the head of microsoft/openvmm#5003, and 235, b4028c1aa, the head of feature/microvm/dev. The pin is 107 commits past the mapped tip, and positions 132-238 stay unassigned. Position 238 changes the input gate that PR09's position 018 adds: the chipset device unit, which changed a device's gate with the device's mutex closed, now makes the gate's synchronous part under the device's ordinary lock and awaits the rest without the device, since the vCPUs run through a gate change. It changes the gate's methods of ChangeDeviceState, the erased chipset devices, and the remote device proxy, which position 018 adds, and the virtio transports' forwarding of the gate, which PR13's position 040 adds, and adds tests of the unit, both virtio transports, and the proxy. Replaying it on upstream main conflicts in all eight of the files that it changes, since upstream main lacks the input gate of position 018 and its forwarding by position 040, so it follows PR09 and PR13 upstream. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Nov 9, 2026
Pedro Henrique Penna (ppenna)
changed the base branch from
copilot/nvx-issue-1154-fifo-artifact
to
feature/microvm/dev
November 10, 2026 02:29
Pedro Henrique Penna (ppenna)
marked this pull request as ready for review
November 10, 2026 02:29
Pedro Henrique Penna (ppenna)
requested review from
a team and
Anatol Belski (weltling)
as code owners
November 10, 2026 02:29
Pedro Henrique Penna (ppenna)
merged commit Nov 10, 2026
46bfe02
into
feature/microvm/dev
73 of 74 checks passed
syeedali
pushed a commit
to syeedali/nvx
that referenced
this pull request
Nov 11, 2026
Maps NVX's new pin, 4c2697aef, the head of microsoft/openvmm#5010, at position 240, after position 239, b5d20bf84, its other commit, which follows position 238, 46bfe02ed, the head of feature/microvm/dev, which microsoft/openvmm#5005 merged after microsoft/openvmm#5004 and microsoft/openvmm#5003. The pin is 109 commits past the mapped tip, and positions 132-240 stay unassigned. Positions 239 and 240 change the snapshot boundary of PR17. Position 239 lets a VMM test be the snapshot controller: it gives the boundary's channels to petri's handle of the snapshot-request port, which PR17's position 076 adds without one, and to petri's launch of the VM worker, whose boundary parameters position 072 adds, keeps them in the petri microVM support of PR15's position 065, and adds a test beside position 065's cold-boot test. Position 240 has the worker of position 072 refuse to release the boundary of a quiesced VM and reject a quiesce outside a boundary, extends the snapshot guide's paragraph that position 076 adds, and adds tests beside position 239's. Replaying them on upstream main conflicts in every file but petri's start of the VM, since upstream main lacks the petri microVM support of position 065 and the boundary of positions 072 and 076, so they follow PR15 and PR17 upstream. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
Makes the snapshot boundary's input gating leave each device open to the vCPUs that still run, for microsoft/nvx#1138. A guest's snapshot request could crash a multi-vCPU microVM, or lose the device accesses of its other vCPUs: the chipset device unit held a device's mutex closed while it changed the device's input gate, so a port or MMIO access that another vCPU made then went unanswered, a read returning all ones and a write lost, an interrupt from another device was dropped, and an EOI or a PCI configuration access panicked the VM worker with "lock should not be closed".
One commit on
cba21d109, the head of #5004, which it stacks on. microsoft/nvx#1425 pins this head. It is a draft, at the top of a stack of three; see Stack.Gate a device's input without closing it to the vCPUs (
46bfe02ed)ChangeDeviceState::quiesce_inputandresume_inputmake the synchronous part and return what remains as anInputGateWait, an owned, boxed future that does not borrow the device. The chipset device unit makes the call under the device's ordinary lock, as a vCPU's access does, releases the lock, and then awaits the wait, so a vCPU's access comes before the synchronous part or after it and is served either way. The virtio transports (MMIO and PCI) and the remote device proxy send their gate's request in the synchronous part, on the channel that carries the commands of the guest's accesses, so that their device takes the gate in the order of those accesses, and wait for its answer without the device. The erased chipset devices ofchipset_device_resourcesforward both parts. The remote device worker is unchanged.vp_accesses_during_an_input_gate_change_reach_the_devicemakes a vCPU's port write, EOI, and interrupt while a device's gate change waits, for a quiesce and for a resume, and requires them to reach the device after the change, without a panic.immediate_input_gates_serve_concurrent_vp_accessestakes and resumes the gate 1000 times while a vCPU writes a port and signals EOIs in a loop, and requires every access to be served.gate_changes_keep_the_order_of_guest_accesses_mmioand_pci, andremote_input_gate_changes_keep_the_order_of_requests, require a gate change to reach the device in the order of the guest's accesses around it, on both virtio transports and through the remote device proxy, and a remote gate's error to be reported.A pre-existing, related defect that review found, a guest's virtio reset clearing a device's input gate while a boundary holds host input, is microsoft/nvx#1416, outside this change.
Validation
At this head,
46bfe02ed, with Rust 1.97.1. It is the commit that NVX's phase-1 validation tested,a91d4c7a4ond6addf8c1, rebased ontocba21d109;git range-diffshows the patch identical, and the commits fromd6addf8c1tocba21d109change none of its files.cargo clippy --all-targetsforvmcore,chipset_device_resources,virtio,vmotherboard, andchipset_device_worker, and for the virtio devicesvirtiofs,virtio_net, andvirtio_console, andcargo doc --no-depsfor the eight, without warnings;cargo nextest run --profile agentfor the eight andstate_unit, 545 passed and 2 skipped.cargo xtask fmt, clean.At
a91d4c7a4:Without the fix, the first test failed, with the port write dropped ("device didn't respond"), the EOI panicking ("lock should not be closed"), and the interrupt lost, and the stress test failed in 10 of 10 runs; with it, 20 of 20 runs passed. Phase 1 also ran mutation checks of the virtio and proxy gates. The same package checks passed, 545 tests on Windows and 550 on Linux.
End to end, a microVM guest requested 2000 snapshots, each of which gated host input and then failed its preflight, while its other vCPUs read a virtio-mmio register in a loop, 3 runs per cell. Reads that returned all ones:
OpenVMM logged "device didn't respond" before, and nothing after. NVX's snapshot and SMP scenarios and its full default microVM suite, with 1, 2, and 4 vCPUs, passed with OpenVMM built from it on KVM, MSHV, and WHP.
Stack
Three drafts, one for each of three microsoft/nvx issues, each based on the branch of the one before it:
feature/microvm/dev.NVX pins each one's head in its own draft, stacked the same way: microsoft/nvx#1423, microsoft/nvx#1424, and microsoft/nvx#1425, in this order, with microsoft/nvx#1427, part 2 of microsoft/nvx#544, which moves no pin, on top.
This one lands last, after #5004: once #5004 has landed, retarget this one to
feature/microvm/dev(gh pr edit 5005 --repo microsoft/openvmm --base feature/microvm/dev) and fast-forwardfeature/microvm/devto this head,46bfe02ed65697e858f66ce55eb60831b3ac3461, since this repository's merge button squashes and would leave NVX's pin off the branch.