Skip to content

openvmm: start each PIT low/high count with its low byte - #1551

Merged
Pedro Henrique Penna (ppenna) merged 2 commits into
devfrom
agents/issue-1063-pit-low-high-reload
Nov 12, 2026
Merged

Pedro Henrique Penna (ppenna) merged 2 commits into
devfrom
agents/issue-1063-pit-low-high-reload

Conversation

@ppenna

@ppenna Pedro Henrique Penna (ppenna) commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1063
Fixes #1146

What and why

In its low-then-high access mode (control word RW = 3), the 8254 PIT takes a count as two bytes, the low byte first, through a write flip-flop that a control word for the counter resets to the low byte, so a guest can write one count after another without a new control word, as Linux's PIT one-shot clock event device does for each event after its control word 0x38. OpenVMM's emulated PIT kept the low byte that it had buffered once the high byte completed a count, so after the first count it took every byte as a high byte with that stale low byte: control word 0x30, then the counts 0 and 16, left counter 0 counting 65536 rather than 16 (#1063), and control word 0x34, then the counts 0x1234 and 0x5678, left the count register at 0x5634 (#1146). Such a guest programmed a wrong period or one-shot length, and IRQ0 at the wrong rate. #1146 duplicates #1063, so this closes both.

Root cause, in OpenVMM: the RwMode::LOW_HIGH arm of Timer::write (vm/devices/chipset/src/pit/mod.rs) read the buffered low byte with if let Some(low) = self.state.cr_low, which left it set; only set_control cleared it.

The pin, microsoft/openvmm#5061 at 5d656d854a8daca0f23833666b0b71229fe2ab73, adds one commit to c2c21ddb2, the head of feature/microvm/dev (#1375's pin):

The NVX commits:

  • ce76a5fadf9 openvmm: pin the PIT low/high count fix. It changes only the gitlink, from c2c21ddb2 to 5d656d854.
  • 86c885dd8a2 doc: map the PIT low/high count fix in the OpenVMM roadmap. Position 263, unassigned.

Decisions:

  • The issues' suggested fix, take(), and nothing else: the low-byte-only and high-byte-only access modes, the reset by a control word, the counter latch and read-back commands, and the read flip-flop (read_high) are unchanged.
  • No saved-state, format, or version change: the saved state already holds the buffered byte (cr_low). A state that OpenVMM saved without this change after such a count holds the stale byte, which a restore keeps, since it cannot tell it from a low byte that the guest wrote; that guest's next counts then go wrong until its next control word for the counter, as they did before. NVX's guests disable the PIT at boot and never program it again (doc/design/3.2-clock-and-time.md, section 3.2.5), so they never wrote the second count that the defect corrupts, and nothing changes for them.
  • Not in this change: in mode 0, the 8254 also stops counting and drives OUT low when the first byte of a two-byte count arrives. Timer::write acts only on a complete count, so the previous count keeps running until the high byte arrives. That predates this change, which does not alter what a first byte does, and is a separate behavior that no issue reports.

Validation

Before and after the fix, on Windows, with Rust 1.97.1:

Check Without the fix (c2c21ddb2, with the new tests) With the fix (5d656d854)
low_high_access_takes_each_count_low_byte_first failed: count register 0x5634, expected 0x5678 (mode 0, the first checked) passed
low_high_access_reloads_a_count_without_a_control_word, #1063's port sequence failed: IRQ0 still low after 17 ticks, latched count 0xfff0 of a count of 65536 passed
low_high_access_reprograms_a_running_rate_generator, #1146's port sequence failed: latched count 0x5634, expected 0x5678 passed
The test attached to #1063, run in place as cargo test -p chipset --test pit_lsb_msb_reprogram and not committed failed: "mode 0 with count 16 did not reach terminal count after 100 ticks (latched count 0xff9d)" passed

OpenVMM package checks:

  • At 5d656d854, on Windows, for chipset: cargo check, cargo clippy --all-targets (clean apart from the environment's LNK4199 linker notices), cargo doc --no-deps, and cargo nextest run --profile agent (104 passed), then cargo xtask fmt --fix (clean).
  • On upstream main ba105dd7f with this commit, the merged tree that openvmm_pin.py roadmap reports, in a temporary worktree: cargo check -p chipset --all-targets passed, and the 14 PIT tests passed.

NVX checks at this head, rebased onto cc12bae1b84 for #1537's test fixes (#1554):

  • python scripts/openvmm_pin.py source 5d656d854a8daca0f23833666b0b71229fe2ab73 --pr 5061: good signature, and the revision is the pull request's head.
  • python scripts/openvmm_pin.py check-branch --topic: passed (2 signed commits, no coordinated files).
  • python scripts/openvmm_pin.py check-roadmap --worktree: passed (263 positions, 272 references).
  • 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 is check_test_changes.py (0 changes to existing tests), check_markdown_links.py, check_test_layout.py, and the ignored openvmm_vectors (4) and openvmm_cpu_profiles (1) tests of nvx-rs.
  • Not run locally: the nvx-cli gate, python -m unittest discover over scripts/, which takes over two hours on this shared machine and whose code this change does not touch, is left to CI's NVX CLI / Linux and NVX CLI / Windows jobs; and the openvmm-ci gate, which waits for OpenVMM's CI of the pin.

Hosts: none. The PIT is OpenVMM's own on every backend, so the defect and the fix are backend-independent, and the crate-level tests reproduce them deterministically. No host or microVM run would show the changed behavior: NVX's guests never write a PIT count after boot, and a sandbox's unprivileged workload cannot reach the PIT's ports. Left to CI: the microVM tests on the KVM, MSHV, and WHP runners, and OpenVMM's unit tests on Linux and Windows and VMM tests on KVM, MSHV, and WHP hosts, which openvmm-ci requires for the pin.

CI at this head, 86c885dd8a2, in run 43283378192: passed, on its second attempt.

  • Attempt 1: NVX microVM tests / Linux / MSHV / Shard 1 of 2 failed in the long-downtime restore scenario on azure-mshv-2. Across a simulated one-year downtime, the guest's frequency-corrected clocks, monotonic and boottime, gained about 186 s (5.9 ppm) more over realtime than the scenario expects from the guest's frequency correction, while monotonic_raw gained what it expects; that runner's TSC runs 9.81 ppm off the backend's declared rate. The same scenario passed on azure-mshv-1 with this same change in run 43269651949, and the PIT plays no part in it, as NVX's guests disable the PIT at boot, so I reran the failed job once: attempt 2 passed, on azure-mshv-2 again.
  • OpenVMM CI of the pinned revision: passed. OpenVMM's required jobs passed for 5d656d854 in its run 43267707236, with no retried tests: unit tests [x64-linux], unit tests [x64-windows], and run vmm-tests on x64-linux-amd-kvm (106 passed), x64-linux-intel-mshv (102), x64-windows-intel (275), and x64-windows-amd (274). Of the jobs that NVX does not require, run vmm-tests [x64-windows-amd-snp] failed; NVX does not use SEV-SNP. OpenVMM's results bot counts 29 failed Petri tests in the run, 28 of them unstable, none of them in the required jobs.
  • NVX CLI / Linux, NVX CLI / Windows, nvx-rs / ubuntu-latest, and nvx-rs / windows-latest: passed.
  • The microVM tests on the KVM, MSHV, and WHP runners, including the MSHV debug kernel and the WHP runtime adapter, the platform benchmarks on KVM, MSHV, and WHP, and the performance regression gate: passed.
  • Before the rebase, at a159c4a0b0a, run 43269651949 passed every job but nvx-rs / ubuntu-latest and NVX CLI / Linux, which failed in CI: two tests on dev assume GNU coreutils, which ubuntu-latest (now 26.04) no longer has #1537's tests on the new ubuntu-26.04 image, which tests: keep the Linux tests true with Ubuntu 26.04's coreutils #1554 fixed on dev.

No existing test changes, so no tests-modified label.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues remain for the documentation update to the OpenVMM pin and PIT fix mapping.

No open issues 🎉

What changed in this PR

This PR documents NVX’s OpenVMM pin for the PIT low/high-byte write fix addressing #1063 and #1146. The roadmap records the fix’s place in the commit chain and its suitability for independent upstream submission.

Changes:

  • Updates the documented pin to 5d656d854, position 263, and adjusts chain counts and ranges.
  • Adds the PIT fix’s commit mapping and notes its conflict-free replay and passing checks on upstream main.
File Description
doc/​openvmm-upstream-roadmap.md Updates the pin, chain accounting, and upstream mapping for the PIT count-write fix.

Review effort: Balanced

In its low-then-high access mode (RW = 3), OpenVMM's emulated 8254 PIT
kept the low byte that it had buffered once the high byte completed a
count, so after the first count it took every byte as a high byte with
that stale low byte. A guest that wrote a count without a new control
word so programmed a wrong one: control word 0x30, then the counts 0
and 16, left counter 0 counting 65536 (#1063), and control word 0x34,
then the counts 0x1234 and 0x5678, left the count register at 0x5634
(#1146).

Pin 5d656d854a8daca0f23833666b0b71229fe2ab73, the head of
microsoft/openvmm#5061, one commit on c2c21ddb2, the head of
feature/microvm/dev:

- 5d656d854 chipset: start each PIT low/high count with its low byte

Timer::write now takes the buffered low byte with the high byte, which
returns the write flip-flop to the low byte of the next count. The
saved state, which already holds that byte, keeps its format.

Evidence: on Windows, the revision's three new chipset tests, two of
which program counter 0 through the I/O ports as the issues do, fail
without the fix, with a count register of 0x5634, with IRQ0 still low
after 17 ticks of a count of 65536, and with a latched count of 0x5634,
and pass at the pin, with the 101 other tests of chipset. The test
attached to #1063 fails at c2c21ddb2 with the latched count 0xff9d of a
count of 65536, and passes at the pin. NVX's guests disable the PIT at
boot and never program it again, so they never wrote the second count
that the defect corrupted.

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>
The NVX pin moves to 5d656d854, position 263, 132 commits past the
mapped tip. Position 263, which microsoft/openvmm#5061 proposes for
feature/microvm/dev on its head, c2c21ddb2, is unassigned. The Scope
also says that microsoft/openvmm#5056 merged position 262.

It makes the PIT's count writes in the low-then-high access mode take
each count's low byte first, in Timer::write, code that upstream main
has, and adds three tests, in a file that PR10's position 022 and
position 139 also change, neither of them in the code that it changes.
It changes nothing that a mapped PR adds. Replaying it on upstream main
applies without conflicts, and chipset type-checks there with its
tests, which pass, so it can go upstream on its own.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ppenna
Pedro Henrique Penna (ppenna) force-pushed the agents/issue-1063-pit-low-high-reload branch from a159c4a to 86c885d Compare November 12, 2026 18:46
Copilot AI balanced review requested due to automatic review settings November 12, 2026 18:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues remain for the roadmap update documenting the OpenVMM PIT fix and updated pin.

No open issues 🎉

Review effort: Balanced

@ppenna
Pedro Henrique Penna (ppenna) added this pull request to the merge queue Nov 12, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Nov 12, 2026
@ppenna
Pedro Henrique Penna (ppenna) added this pull request to the merge queue Nov 12, 2026
Merged via the queue into dev with commit c5fdc8b Nov 12, 2026
90 of 92 checks passed
@ppenna
Pedro Henrique Penna (ppenna) deleted the agents/issue-1063-pit-low-high-reload branch November 12, 2026 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants