Repository navigation
extdeps.ampere.nvparam: NVParamDef.h key table; decode the boot's NVPARAM dump - #13059
Conversation
…ARAM dump
extdeps.ampere.nvparam transcribes the 513 NV_SI_* keys of edk2-platforms
Silicon/Ampere/AmpereAltraPkg/Include/NVParamDef.h (BSD-2-Clause-Patent,
@2170c80c) mechanically: key = region base + 8 * index, four regions (pre-boot,
manufacturing, user, board). A key the header does not define is
NvParamKeyUndefined { key }.
gunbc.machine_intake_ampere_nvparam_console_observation reads the dump
DebugInfoPei.c PrintNVRAM prints ("<Region> Configuration Setting:" then
" %04X: 0x%X (%d)") into named settings. Rows count only inside a section;
hex/decimal disagreement or a key outside the section's region is malformed,
kept as text. Value semantics are not decoded.
The witness reads run A's committed capture by path and recovers
NV_SI_WDT_BIOS_EXP_MINS = 5, NV_SI_SEC_WDT_BIOS_EXP_MINS = 300.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
briansrls
left a comment
There was a problem hiding this comment.
The source boundary is clean, but the console grammar currently admits rows that the cited printer cannot emit.
DebugInfoPei.c iterates each section in NVPARAM_SIZE steps, and NVParamDef.h defines NVPARAM/the printed Val as UINT32. nvparam_row_of instead accepts any in-region key and an unbounded hex Int. In particular, 0x100000000 (0) is accepted because d + 2^32 == v, even though it is a 33-bit value, and an unaligned key such as 8021 is accepted as an undefined key despite the printer advancing by 8-byte parameter slots.
Required change:
- Admit a value only when
0 <= v <= 0xffffffffand the decimal rendering is in the signed 32-bit domain the%drendering can produce. Then require eitherd == vforv <= 0x7ffffffford + 2^32 == vfor the high half. - Require
(key - region_base) % nvparam_bytes_per_parameter == 0before accepting a setting. Keep aligned but unlisted keys asNvParamKeyUndefined; that preserves forward compatibility without accepting an impossible printer address. - Add REDs for a 33-bit hex value that currently wraps to decimal zero and for an unaligned in-region key.
The source-provenance review passes. The key names, bases, slot size, maxima and PrintNVRAM grammar all come from the pinned public edk2-platforms tree. I found no facts sourced from firmware disassembly, the confidential SCP user manual, or CHANGELOG.txt, and the PR deliberately does not decode value semantics. The committed console capture is an observation, not a reverse-engineered codebook.
Re-run exact-head CI after these grammar controls land; the current exact-head workflow is not green.
…rovider) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…-32 %d rendering, 8-byte key stride) Review request: values past 0xFFFFFFFF, an unsigned decimal for a value >= 2^31, and an off-stride key are malformed; an on-stride key the header does not name stays an explicit unknown. REDs for 0x100000000 (0), 0xFFFFFF01 (4294967041) and key 0x8021. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Addressed in a0d073b:
— sent from cool-ant-760 |
chars yields code points, not one-character strings, so json_hex_nibble over it was a type error (floor: expected String, got Int). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
briansrls
left a comment
There was a problem hiding this comment.
Re-reviewed exact head b705aa6. The previous parser-admission blockers are resolved.
The parser now bounds the hexadecimal value to UINT32, compares the decimal against the exact signed-32 %d rendering, and requires an 8-byte NVPARAM_SIZE stride from the section base. The witnesses cover the 33-bit wrap case, unsigned rendering of a high-half UINT32, an off-stride key, and the positive forward-compatible case where an aligned but unlisted key stays NvParamKeyUndefined. The committed run-A capture still inhabits the real parser/key-table route.
The source boundary remains clean: the key names, region bases, stride, SPI-NOR layout statement, and PrintNVRAM grammar all come from pinned public edk2-platforms. Value semantics remain intentionally undecoded; I found no disassembly-, SCP-UM-, or CHANGELOG-derived fact in this PR.
Merge after the exact-head required workflow is green.
…nsumes the decoded dump; one keyed parse Review 74486: - extdeps.ampere.nvparam declares nvparam_region_start / nvparam_region_max under the header's *_PARAM_START / *_PARAM_MAX; the intake bounds keys by them (the START..MAX range PrintNVRAM walks) instead of re-typing bases and a 16 KB extent. - Consumer: gunbc.machine_intake_mtcollins1_boot_diagnostic_bundle ConsoleRetained carries the decoded dump (nvparam) and console_text renders every setting by name. Floor enrolment margin (run_a claim 617 ms, 1.01M eval steps vs 302 ms): keys resolve through a Map built once per parse (nvparam_key_index / nvparam_key_in) instead of a 513-row scan per row, and the two real-capture claims are one parse. Measured on BuildBuddy: 92 ms, 82,745 eval steps. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Addressed review 74486:
Also fixed the floor's enrolment-margin refusal. The real-capture claim was 617 ms, 1.01M eval steps; it is now 92 ms, 82,745 steps, because keys resolve through a map built once per parse and the two real-capture claims share a single parse. — sent from cool-ant-760 |
…SIZE name; cheaper real-capture claim Review 74528: - deletes nvparam_key_of; the witness resolves keys through nvparam_key_in(index: nvparam_key_index()), the route production uses - nvparam_setting_named had no production caller; it now lives in the witness - the fold conses each row and reverses once (it appended per row before) - nvparam_bytes_per_parameter renamed nvparam_size after NVParamLib.h NVPARAM_SIZE Floor new-witness step budget (84,865 vs 72,300): lines outside a section are passed over unless they can be a header, and the real-capture claim reads the first 50 lines of the dump (48 real rows, every asserted key). 13/13 PASS on BuildBuddy before the slice (83,871 steps). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Addressed review 74528:
Same head, for the floor's new-witness step budget (84,865 vs 72,300):
— sent from cool-ant-760 |
Review 74566: the stride is a byte quantity; the intake converts it once where it checks the key stride. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Review 74566: I've left the keys and the region start/max as
Typing them as a byte-quantity carrier would describe them as amounts. They are names that the header happens to spell as byte offsets. The one quantity in the module, the stride, now carries its unit. — sent from cool-ant-760 |
briansrls
left a comment
There was a problem hiding this comment.
Re-reviewed exact head b45cc3df21. Approval renewed.
The two post-approval changes are sound:
- Region admission now uses the public header's exact
*_PARAM_START..*_PARAM_MAXbounds, matchingPrintNVRAM's inclusive loops, rather than a re-authored 16 KiB extent. The values are on the publicNVPARAM_SIZEstride, and the prior UINT32/signed-%d/alignment refusals remain intact. - The boot diagnostic bundle now actually consumes the decoded observation:
ConsoleRetainedcarriesNvParamConsoleObservation, the real console reader derives it from the retained lines, and the bundle renderer emits named settings and malformed rows. - Key lookup has one production authority (
nvparam_key_index/nvparam_key_in); the old scan route is deleted. The per-setting append was replaced with cons plus one reversal, preserving order with linear accumulation. The named-key helper is correctly test-only. - The cheaper live-tree claim deliberately slices enough real capture lines to inhabit the real file-read -> grammar -> key-index -> renderer route; the producer-grammar and region-boundary REDs remain separate and complete.
The public/private boundary remains clean: these changes rely only on pinned public edk2-platforms NVParamDef.h, NVParamLib.h, and DebugInfoPei.c; no SCP-UM, CHANGELOG, or disassembly-derived semantics were added, and values remain undecoded.
Exact-head required CI is still queued, so merge after it is green. Non-blocking metadata: the PR body still says the tests had not yet run and does not mention the new boot-bundle consumer.
briansrls
left a comment
There was a problem hiding this comment.
Re-reviewed exact head cb836e0. The delta from the previously approved b45cc3d head is one clean type correction: public NVPARAM_SIZE is now carried as std.measure.ByteSize, and the parser converts it once with byte_size_count at the arithmetic boundary. The key-stride admission, UINT32/signed-decimal controls, exact header-derived region bounds, single key-resolution route, linear fold, and boot-bundle consumer are otherwise unchanged. Exact-head witnesses completed successfully. Approved.
…nvparam # Conflicts: # dag/gunbc/extdeps_scope_frontier.dag
briansrls
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 50eade0a549cdfa63eeda434e8a99e3c90d3a3fa after the auto-heal merge from main. Approval renewed.
The merge commit reports only one actual conflict, in extdeps_scope_frontier; its resolution is additive and correct: it retains main's newly landed Ampere SMpro scope carriers and adds dag/extdeps/ampere/nvparam.dag beside them.
Against current main, the PR is seven files. The two newly visible witness deltas are mechanical exhaustiveness updates for the approved ConsoleRetained extension:
- the module-decompression witness adds
nvparam: _while continuing to inspect the samemodules: mrefusal record; - the boot-diagnostic-bundle witness adds
nvparam: _while preserving its firmware/refusal and reference-comparison assertions.
Neither changes the claims those witnesses make.
The previously approved NVPARAM semantics remain intact: header-derived inclusive region bounds, NVPARAM_SIZE as ByteSize, UINT32 plus exact signed-%d admission, stride refusal with aligned unknown-key preservation, one indexed key route, linear accumulation, real-capture parsing, and production consumption/rendering in the boot diagnostic bundle. The source boundary remains public edk2-platforms only, with value semantics intentionally undecoded.
Exact-head CI is still running; merge after witnesses is green.
Names the NVPARAM keys a Mt. Collins boot prints. Public sources only: edk2-platforms @2170c80c (BSD-2-Clause-Patent).
extdeps.ampere.nvparam(new): all 513NV_SI_*keys fromSilicon/Ampere/AmpereAltraPkg/Include/NVParamDef.h.((n * 8) + <REGION>_START)defines, keeping its names.NvParamKeyUndefined { key }.gunbc.machine_intake_ampere_nvparam_console_observation(new): reads the dumpDebugInfoPei.cPrintNVRAMprints into named settings." %04X: 0x%X (%d)".%dof a large UINT32 counts as agreement.extdeps_scope_frontier: enrolls the new module as a scope carrier.Witness
ampere_nvparam_console_observation_witness_testcovers:artifacts/bmc/mtcollins1-run-a-sparse-reference-j1-j17-2026-09-13.txt), read by path, yields NV_SI_WDT_BIOS_EXP_MINS = 5, NV_SI_SEC_WDT_BIOS_EXP_MINS = 300 and NV_SI_RO_BOARD_VENDOR = 52538.That archive renders each CR as a literal
^M; it contains no 0x0d byte. The witness removes the rendering per line, asmtcollins1_sixteen_module_restore_witness_testalready does.Evidence: not yet run locally; CI is the first execution.
🤖 Generated with Claude Code