Repository navigation
openvmm: reject a saved PIT control word with RW = 0 - #1558
Open
Pedro Henrique Penna (ppenna) wants to merge 2 commits into
Open
Pedro Henrique Penna (ppenna) wants to merge 2 commits into
Pedro Henrique Penna (ppenna) wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The OpenVMM pin remains on an unmerged PR, so a maintainer must confirm exact-pin CI and runtime results and fast-forward landing on feature/microvm/dev before integration.
No open issues 🎉
What changed in this PR
This PR updates NVX’s OpenVMM pin to reject saved PIT control words with RW = 0 before changing device state, preventing malformed snapshots from causing a panic on subsequent counter access. It addresses #1066 and #1147.
Changes:
- Records OpenVMM revision
607407fa9as roadmap position 265. - Updates chain counts and documents the fix’s tests, dependencies, and upstream replay considerations.
| File | Description |
|---|---|
doc/openvmm-upstream-roadmap.md |
Updates the pinned revision and maps the PIT restore validation fix into the upstream roadmap. |
Review effort: Balanced
OpenVMM's emulated 8254 PIT restored a saved control word with RW = 0, bits 5:4, which encodes the counter latch command: it latches a counter's count without programming the counter, so no counter holds it. The PIT's control register handles it as a latch and never stores it, and its counter reads and writes handle only RW = 1, 2, and 3. A modified snapshot whose PIT state held RW = 0 thus restored, and the guest's next read or write of that counter's port reached unreachable!() and panicked OpenVMM's VM worker (#1066, #1147). Pin 6f57a6d61160486a662530d528ca184430a76c7b, the head of microsoft/openvmm#5067, two commits on 8eede133c, the head of feature/microvm/dev: - 607407fa9 chipset: reject a saved PIT control word with RW = 0 - 6f57a6d61 chipset: keep the PIT restore tests within Linux's open file limit PitDevice::restore now checks every timer's control word before it changes any state, and fails with RestoreError::InvalidSavedState when one has RW = 0, so a rejected restore leaves the PIT as it was. Every state that the PIT saved still restores, and no saved-state field, snapshot format, device state schema, or time ABI version changes. The second commit changes only the first's tests, which created PITs for each case and so exhausted Linux's default limit of 1024 open files per process. Evidence: on Windows, the revision's chipset tests reproduce both issues and fail without the fix: restoring a fresh PIT's saved state, with a counter's control word changed to one with RW = 0, succeeds, after which the next write of port 0x40 panics in Timer::write, and the next read of port 0x40, 0x41, or 0x42 in Timer::read. At the pin, the restore fails with "timer N control word 0x00 has no read/write mode" and leaves the PIT unchanged, and the tests pass with the other 110 tests of chipset, on Windows and on Linux. NVX needs no coordinated change: the control-session vectors, the source excerpts, the contract versions, the activation bound, and the CPU profiles that nvx-rs copies from OpenVMM match the revision's. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The NVX pin moves to 6f57a6d61, position 266, 135 commits past the mapped tip. Positions 265 and 266, which microsoft/openvmm#5067 proposes for feature/microvm/dev on its head, 8eede133c, are unassigned, and position 266 folds into position 265. The Scope also says that microsoft/openvmm#5065 merged position 264. Position 265 makes PitDevice::restore reject a saved control word with RW = 0, the counter latch command, before it changes any state, in code that upstream main has, and adds two tests on position 263's test helper, in a file that PR10's position 022 and positions 139, 263, and 264 also change. It changes none of the code of positions 022 and 139, and nothing that a mapped PR adds. Replaying it on upstream main conflicts in that file where it adds its error, its tests' import, and its tests next to code of positions 139, 263, and 264, which upstream main lacks; its check applies there. It follows position 264 upstream. Position 266 changes only position 265's tests, which created PITs for each case and so exhausted Linux's default limit of open files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Pedro Henrique Penna (ppenna)
force-pushed
the
agents/issue-1066-pit-restore-rw0
branch
from
November 12, 2026 23:48
10b3d52 to
cef6ac0
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The PR description, landing target, and validation references still identify an earlier revision rather than the current two-commit head, leaving the landing instructions inconsistent with the actual pin.
1 open issue
Review effort: Balanced
This branch has not been deployed
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.

Fixes #1066
Fixes #1147
What and why
A modified snapshot whose PIT state held a counter control word with RW = 0 restored, and the guest's next read or write of that counter's port,
0x40,0x41, or0x42, panicked OpenVMM's VM worker atunreachable!()inTimer::readorTimer::write. #1147 reports the same defect as #1066, so this fixes both.Root cause, in OpenVMM: RW = 0, bits 5:4 of an 8254 control word, encodes the counter latch command, which latches a counter's count without programming the counter, so no counter holds it.
Timer::set_controllatches the count and never stores such a control word, andTimer::readandTimer::writehandle only RW = 1, 2, and 3.PitDevice::restoreinstalled each saved control word withControlWord::from, which takes any byte, without that check.The pin, microsoft/openvmm#5067 at
6f57a6d61160486a662530d528ca184430a76c7b, adds two commits tofeature/microvm/dev,8eede133c, #1065's pin:607407fa9chipset: reject a saved PIT control word with RW = 0.restorechecks every timer's saved control word, after the fraction of the last tick and before it changes any state, and fails withRestoreError::InvalidSavedState, "timer N control word 0xNN has no read/write mode", when one has RW = 0, so a rejected restore leaves the PIT as it was. Two unit tests come with it.6f57a6d61chipset: keep the PIT restore tests within Linux's open file limit. It changes only the first commit's tests, which created aGuestPitfor each case: a droppedGuestPitleaves some of its files open on Linux, so they exhausted the default limit of 1024 open files per process. They now reset and reuse five.The NVX commits:
4e50d08e885openvmm: pin the PIT restore's control word check. It changes only the gitlink, from8eede133cto6f57a6d61.cef6ac02417doc: map the PIT restore's control word check in the OpenVMM roadmap. Positions 265 and 266, unassigned; 266 folds into 265.Decisions:
unreachable!()on the next access #1066 expects a rejected restore to leave the device usable.Timer::set_controlhas never stored RW = 0, and a reset counter holds RW = 1, so every state that the PIT saved still restores.Not in this change, found while auditing what restored state the PIT accepts, neither of which panics:
read_highset while the counter's control word has RW = 1 or 2, which no guest programming produces, keepsTimer::readfrom releasing a latched count, so the counter's latched reads repeat one count until the guest writes a control word for the counter.InvalidLastTickandE_PIT_ACTIVEchecks run afterrestoreinstalls the saved timers,last, andlast_frac_ns, so a restore that fails them leaves that state installed. A failed restore ends the VM before any processor runs.Validation
Local checks at this head,
cef6ac02417(Windows):python scripts/openvmm_pin.py check-branch --topic: passed (2 signed commits, no coordinated files).python scripts/openvmm_pin.py check-roadmap --worktree: passed (266 positions, 275 references).python scripts/openvmm_pin.py source 6f57a6d61160486a662530d528ca184430a76c7b --pr 5067: good signature, and the revision is the pull request's head.python scripts/nvx.py verify: passed.python scripts/local_gates.py --base origin/dev --only test-changes markdown-links test-layout openvmm-vectors openvmm-cpu-profiles --run: passed. That ischeck_test_changes.py(0 changes to existing tests),check_markdown_links.py,check_test_layout.py, and the ignoredopenvmm_vectors(4) andopenvmm_cpu_profiles(1) tests ofnvx-rs.nvx-cligate,python -m unittest discoveroverscripts/: not run locally. This change does not touchscripts/, and the gate takes over two hours on this shared machine. It is left to CI'sNVX CLI / LinuxandNVX CLI / Windowsjobs.OpenVMM package checks, at
6f57a6d61, forchipset: on Windows,cargo check,cargo clippy --all-targets(clean apart from the environment's LNK4199 linker notices),cargo doc --no-deps, andcargo nextest run --profile agent(112 passed), thencargo xtask fmt --fix(no changes); on Linux, on nvx-kvm-test, at the default limit of 1024 open files,cargo nextest run --profile agentandcargo test --lib(112 passed each). At607407fa9, the first commit's tests passed on Windows, butrestore_rejects_rw0_control_word_without_changing_statefailed on that Linux host with "Too many open files (os error 24)" and passed there with a limit of 16384, which the second commit fixes.Reproduction, with
cargo test -p chipseton Windows x64:0x10changed to0x00in the encoded saved state), then write and read port0x40restore_rejects_rw0_control_word_without_changing_statec2c21ddb2, and8eede133cwith the check disabledTimer::writeTimer::read0x00restores6f57a6d61With the check in the loop that installs the timers, as in #1147's patch, the test fails for counter 1, whose rejection leaves counter 0's saved state installed. A scratch test that restored 300,000 random saved states, with random timers,
lastup to the current time, andlast_frac_ns, and then accessed every port, in a debug build, panicked at once without the check and passed with it.Remote runs, in a temporary worktree of this head on each test VM, with OpenVMM built from
6f57a6d61(clean) bynvx.py build-openvmm --backend BACKENDand the guest artifacts ofv0.1.0-dev.c5fdc8b850c6, three first-parent commits earlier, whichnvx-rs downloadinstalled. The scenarios areguest-boot,interrupt-map,snapshot-core,restore-processors,smp-snapshot,restore-downtime,time-abi-conformance, andtime-host-correction, with--processors 1 2:cargo nextest run --profile agent -p chipsetinopenvmm/, on Linux, at the default limit of 1024 open filesf73e4587ebee1b04(musl)80116a90The same scenarios also passed on the KVM and MSHV test VMs with OpenVMM built from
607407fa9, whose code outside the tests is the pin's. NVX's guests disable the PIT at boot and never program it again, so these runs check that guests still boot, capture, and restore, with downtimes up to the simulated year, through the changed restore on each backend; the crate-level tests above are the reproduction, since a restore that the check rejects needs a modified saved state. Bare-metal hosts and the remaining scenarios are left to CI.CI at this head,
cef6ac02417: in progress, run 43331990118. Itsopenvmm-cijob waits for OpenVMM's CI of6f57a6d61, run 43331611028.No existing test changes, so no
tests-modifiedlabel.6f57a6d61160486a662530d528ca184430a76c7b.8eede133c, the head offeature/microvm/devafter [DO NOT REVIEW] chipset: keep the sub-100 ns part of the PIT's last tick time openvmm#5065 (PITevaluatedrops sub-100ns time, so the PIT runs fast and same-instant reads differ #1065), which follows [DO NOT REVIEW] chipset: start each PIT low/high count with its low byte openvmm#5061 (chipset: PIT LSB/MSB access mode keeps the buffered LSB after the MSB write #1063). There are no other prerequisites.cababce14.feature/microvm/devfrom8eede133cto exactly6f57a6d61160486a662530d528ca184430a76c7b. OpenVMM's merge button would squash the commits and leave the pin off the branch. This pull request then merges through the queue.