Repository navigation
openvmm: make the time ABI documentation standalone - #136
Merged
Merged
Conversation
The time ABI's comments cited NVX's time ABI specification, doc/design/time-abi.md in the NVX repository: by path, by its "Backend obligations" table, by its capture and restore step numbers, and as "the specification". NVX has removed that document, and the guide that replaces it numbers the steps differently, so the citations led nowhere or to the wrong step. Make the documentation standalone rather than pointing into another repository: each comment states what the code does, and refers only to items in this repository where a reference helps. - virt: the time_abi module states that a host, backend, or snapshot that cannot meet the contract fails with a stable TimeAbiCode and never falls back. The backend trait, the rate policy, and the code list no longer cite the specification. - virt_kvm, virt_whp: each backend meets the time ABI's backend obligations with the mechanisms that its module documentation lists, instead of filling a column of the specification's table. - cpu_profile: the profile failure codes are the NVX time ABI's stable codes, listed in declaration order. - vmm_core, openvmm_core, openvmm_entry, openvmm_helpers: the capture and restore code describes each check, record, and clock step by what it does and when it runs, instead of by a step number. - virt_mshv: the negative test of unlisted CPUID entries names cpu_profile::check_unlisted_cpuid instead of a verification step. Rename the three tests named after the specification: codes_match_the_specification becomes codes_keep_their_names_and_order, codes_are_distinct_and_spelled_like_the_specification becomes codes_are_distinct_and_well_formed, and identity_leaves_match_the_specification becomes identity_leaves_keep_their_values. There is no behavior change. Refs microsoft/nvx#562 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ppenna
added a commit
to microsoft/nvx
that referenced
this pull request
Oct 15, 2026
#529 replaced doc/design with the architecture guide and removed doc/design/time-abi.md, the time ABI specification. NVX code, tests, and documentation, and OpenVMM's time ABI comments, still cited it: its sections, its snapshot-agent and restore step numbers, and check IDs that no remaining document defines. Retarget every NVX citation to the guide, fold the specification's remaining architecture-level detail into section 3.2, leave its implementation detail to the code that defines it, and pin an OpenVMM revision whose documentation no longer depends on NVX's documents. Folded into 3.2.10 (Guest Obligations): - Which checks capture and restore rerun, instead of "the relevant subset": capture reruns the clocksource, tick-device, and watcher checks; restore reruns them, checks the declared rate in the frequency MSR again, and checks the identity leaves, CPU time bits, and CPU flags of every processor that it brings online. - What the state file reports: the outcome of each phase's checks, the declared rates, the generation, the last downtime and its source, and the wall clock's discontinuities, synchronization, offset, uncertainty, and frequency correction. - Restore repair and the discipline step the clock through the kernel, so a workload observes every step through the kernel's clock-set notification, such as a timerfd armed with TFD_TIMER_CANCEL_ON_SET. The rest of the specification's architecture-level content is already in the guide: the guest's fail-fast statuses (5.6), generations (5.5.6), the rejection of clock tokens in the command line (5.1), and capture rollback (5.3). Its implementation detail stays out of the documentation: the E_* and G_* codes, the check IDs C1 to C12, K1, and X1 to X6, the NVX-TIME-ABI line formats, the state file's path and keys, the watcher's kernel log patterns, report-only mode, and the warp probe's method. guest/common/nvx-time.c now states that it defines its check IDs, guest codes, output lines, and state file format. Renumbered snapshot-agent steps: - guest/common/nvx-time.c, guest/common/nvx-snapshot, and scripts/test_microvm_tests.py number the snapshot agent's steps as the guide does: guest preparation steps 1 to 4 (5.3.2) and guest repair steps 1 to 7 (5.5.7), instead of the removed specification's capture steps 1 to 5 and restore steps 6 to 13. Old steps 6 to 10 are repair steps 1 to 5, old step 12 is repair step 6, and old step 13, with old step 11's checks, is repair step 7. - The comment on repair_restore names repair steps 2 and 3, because cmd_capture reads the status, repair step 1, itself. - Rename step13_cpu_ns, fork_step13, and test_step13_cpu to deferred_restore_cpu_ns, fork_deferred_restore, and test_deferred_restore_cpu, and nvx-time's "step13-cpu" test hook to "deferred-restore-cpu", with its test in scripts/test_guest_time.py. Retargeted documentation: - doc/build.md: the RCU stall timeout stays at its production value because the conformance checks cover the stall-detector settings (3.2.10), rather than because of check C8. - doc/ci.md: describe the exhaustive check's MSRs and the boot check of the debug detectors in words, rather than as checks C3 and C11. Retargeted code comments and test names, with no behavior change: - scripts/nvx_tools/doctor.py: the doctor runs its checks in ID order, as the --checks help text now says, and CHECK_CPU_BUDGET_US holds the CPU-time budgets. - scripts/nvx_tools/microvm_tests.py: the debug-kernel restore cases and the long-downtime case cite doc/ci.md. - scripts/nvx_tools/time_abi.py: the CPU profile catalog cites 2.2, and the warp probe check cites the 1 us skew bound of 3.2. - scripts/test_guest_time.py: the restore packet (5.5.5) and time sample (3.2.9) fixtures follow the guide, and the exhaustive check's lines and the state file fixtures pin the formats that nvx-time.c defines. The exhaustive-report test becomes test_exhaustive_report_format. - scripts/test_time_abi.py: the CPU-time budget comments no longer cite "the spec's" budgets. OpenVMM pin: Pin nanvix/openvmm#136 at c6d97c529986c0b3b1bf1fd7f00406654cdeee67, one commit on 7f225e2c. OpenVMM's time ABI comments cited the removed specification by path, by its "Backend obligations" table, by its step numbers, and as "the specification". They now state what the code does and refer only to OpenVMM's own items, and the three tests named after the specification are renamed. There is no behavior change. Index the new pin in doc/openvmm-upstream-roadmap.md. Fixes #562 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
8 open findings
What changed in this PR
This PR updates OpenVMM’s time ABI documentation to be self-contained by removing references to the deleted NVX doc/design/time-abi.md spec and replacing them with local explanations; it also renames a few tests previously named after the spec.
Changes:
- Replace cross-repo/spec step references with standalone, descriptive comments across time ABI capture/restore paths.
- Update backend module docs (KVM/WHP/MSHV) to describe how each backend meets obligations without referencing NVX’s removed table.
- Rename three tests to reflect invariants (names/order/well-formedness) rather than “match specification”.
| File | Description |
|---|---|
| vmm_core/virt_whp/src/time_abi.rs | Update WHP backend time ABI module docs to be standalone. |
| vmm_core/virt_mshv/src/x86_64/time_abi.rs | Update MSHV test comment to reference a local check rather than spec step. |
| vmm_core/virt_kvm/src/arch/x86_64/time_abi.rs | Update KVM backend time ABI module docs to be standalone. |
| vmm_core/virt/src/time_abi/mod.rs | Remove spec reference, clarify contract behavior, rename a test. |
| vmm_core/virt/src/time_abi/identity.rs | Rename identity CPUID leaves test to avoid spec reference. |
| vmm_core/virt/src/time_abi/backend.rs | Remove spec-step language from backend trait/rate-policy docs. |
| vmm_core/src/partition_unit/vp_set/time_abi.rs | Replace capture/restore step references with descriptive comments. |
| vmm_core/cpu_profile/src/lib.rs | Update crate-level docs to avoid referencing the removed spec. |
| vmm_core/cpu_profile/src/error.rs | Update error-code docs and rename a test to avoid spec wording. |
| openvmm/openvmm_helpers/src/snapshot/time.rs | Replace restore-step references with descriptive preflight wording. |
| openvmm/openvmm_entry/src/microvm/restore.rs | Replace restore-step references with descriptive controller validation wording. |
| openvmm/openvmm_core/src/worker/dispatch/time_abi.rs | Replace step-based docs with descriptive capture/restore explanations. |
| openvmm/openvmm_core/src/worker/dispatch/microvm.rs | Update time ABI capture comments to be step-number free. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Licensed under the MIT License. | ||
|
|
||
| //! CPU profile failures, with the stable codes of the time ABI specification. | ||
| //! CPU profile failures, with the stable codes of the NVX time ABI. |
|
|
||
| /// A stable failure code from the "Failure codes" table of the NVX time ABI | ||
| /// specification. | ||
| /// A stable failure code of the NVX time ABI. |
| ]; | ||
|
|
||
| /// Returns the code as the specification spells it, such as | ||
| /// Returns the code as the NVX time ABI spells it, such as |
| //! | ||
| //! Failures carry the stable codes of the time ABI specification | ||
| //! ([`ProfileError`]). | ||
| //! Failures carry the stable codes of the NVX time ABI ([`ProfileError`]). |
| //! [`ProtoPartitionConfig::time_abi`](virt::ProtoPartitionConfig::time_abi). | ||
| //! It implements the KVM column of "Backend obligations" in NVX | ||
| //! `doc/design/time-abi.md`: | ||
| //! It meets the time ABI's backend obligations with these KVM mechanisms: |
| //! [`ProtoPartitionConfig::time_abi`](virt::ProtoPartitionConfig::time_abi). | ||
| //! It implements the WHP column of "Backend obligations" in NVX | ||
| //! `doc/design/time-abi.md`: | ||
| //! It meets the time ABI's backend obligations with these WHP mechanisms: |
Comment on lines
+624
to
+625
| /// records, about a microsecond of work, and decodes the snapshot's only to | ||
| /// name the differences. |
|
|
||
| // The negative path of verification step 6. A non-zero value at a | ||
| // The negative path of the unlisted-entry check, | ||
| // `cpu_profile::check_unlisted_cpuid`. A non-zero value at a |
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.
Summary
The time ABI's comments cited NVX's time ABI specification,
doc/design/time-abi.mdin the NVX repository: by path, by its "Backend obligations" table, by its capture and restore step numbers, and as "the specification". microsoft/nvx#529 removed that document, and the architecture guide that replaces it numbers the steps differently, so the citations led nowhere or to the wrong step. This makes the documentation standalone rather than pointing into another repository: each comment states what the code does, and refers only to items in this repository where a reference helps.virt: thetime_abimodule states that a host, backend, or snapshot that cannot meet the contract fails with a stableTimeAbiCodeand never falls back; the backend trait, the rate policy, and the code list no longer cite the specificationvirt_kvm,virt_whp: each backend meets the time ABI's backend obligations with the mechanisms that its module documentation lists, instead of filling a column of the specification's tablecpu_profile: the profile failure codes are the NVX time ABI's stable codes, listed in declaration ordervmm_core,openvmm_core,openvmm_entry,openvmm_helpers: the capture and restore code describes each check, record, and clock step by what it does and when it runs, instead of by a step numbervirt_mshv: the negative test of unlisted CPUID entries namescpu_profile::check_unlisted_cpuidinstead of a verification stepcodes_keep_their_names_and_order,codes_are_distinct_and_well_formed, andidentity_leaves_keep_their_valuesOnly comments and test names change: 121 of the 127 changed lines are comments, and the other six rename the three tests. There is no behavior change.
Part of microsoft/nvx#562.
Revision
c6d97c529986c0b3b1bf1fd7f00406654cdeee67, one SSH-signed commit directly onmainat7f225e2cd635bbd1e1839e587e659996433396c8Validation
cargo clippy --all-targetsandcargo doc --no-depsfor every modified package,virt_whpon Windows and the other eight on Linux (WSL): no lints, and no documentation warnings from these packagescargo nextest run --profile agent: on Linux,cpu_profile,virt,virt_kvm,virt_mshv,vmm_core,openvmm_helpers,openvmm_core, andopenvmm_entry, 786 passed and 20 skipped; on Windows,virt_whp, 45 passed and 17 skippedcargo xtask fmt --fix: clean