Temporary diagnotics for SNP: Add import regions hash and cache diagnostics on initrd CRC hash mismatch by jennagoddard · Pull Request #4202 · microsoft/openvmm

Jenna Goddard (jennagoddard) · 2026-08-10T18:42:06Z

Temporary diagnostics to collect data on imported regions hash mismatch failure.

github-actions · 2026-08-10T18:42:33Z

⚠️ Unsafe Code Detected

This PR modifies files containing unsafe Rust code. Extra scrutiny is required during review.

For more on why we check whole files, instead of just diffs, check out the Rustonomicon

Steven Malis (smalis-msft) · 2026-08-10T18:43:35Z

Do we need to merge this, or could we just rerun CI on this PR a bunch of times until we get a hit?

Copilot

Pull request overview

This PR adds additional diagnostics to help debug initrd CRC32 mismatches during openhcl_boot startup (notably for SNP scenarios), aiming to distinguish “data is wrong” from “reads are inconsistent” and to localize divergences within the initrd.

Changes:

Added build_initrd_crc_diagnostic to capture CRC metadata, a second-read CRC, head/tail fingerprints, and per-slice (“eighths”) CRCs.
When confidential_debug is enabled and an initrd CRC mismatch is detected, logs/panics with the expanded diagnostic string to aid root-cause analysis.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+    if computed_crc != p.initrd_crc && is_confidential_debug {
+        let diag = build_initrd_crc_diagnostic(&p, computed_crc);
+        log::error!("{}", diag.as_str());
+        panic!("{}", diag.as_str());
+    }


+    // Split the initrd into (up to) 8 roughly-equal slices and CRC each.
+    // Bytes past the aligned slices go into the last chunk.
+    let mut eighths = [0u32; 8];


+fn build_initrd_crc_diagnostic(p: &ShimParams, first_computed_crc: u32) -> ArrayString<384> {
+    let initrd_bytes = p.initrd();
+
+    // A second read from the same VA. If this differs from the first read,
+    // the initrd memory is not being read consistently, which typically
+    // indicates stale/mismatched cache lines rather than actual data
+    // corruption.
+    let second_computed_crc = crc32fast::hash(initrd_bytes);


github-actions · 2026-08-10T20:07:39Z

34 Petri tests failed (4 unstable)

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (5)

openhcl/openhcl_boot/src/arch/x86_64/memory.rs:627

Phase-C verification re-reads and hashes the just-accepted memory, and can also emit large hex diffs. This is valuable for debugging but is expensive and can produce substantial logs; it should be compiled out (or explicitly feature-gated) for non-debug builds.

                // DIAG: re-hash the chunk from the freshly written private
                // page (Phase C) and compare against the Phase-A bytes still
                // sitting in `ram_buffer`. A mismatch here means the accept/
                // copy-back path corrupted this chunk; hex diffs of the first
                // few differing cache lines are logged.

openhcl/openhcl_boot/src/arch/x86_64/memory.rs:683

On imported-regions hash mismatch this path can dump bytes from accepted/private memory (diag_dump_head), which can be sensitive and is also a lot of log volume. Gate the detailed diagnostics behind cfg(debug_assertions) (or a dedicated, explicitly-enabled flag), and keep the unconditional behavior to just panic.

    if final_hash.as_slice() != expected {
        log::error!(
            "DIAG_COMBINED_PHASE_D combined_phase_d={} expected={}",
            HexBytes(&final_hash),
            HexBytes(expected),
        );
        diag_report_phase_d(expected);
        panic!("Imported regions hash mismatch");

openhcl/openhcl_boot/src/main.rs:639

write! into an ArrayString can fail (capacity overflow), but the result is currently ignored, which would silently truncate the diagnostic and make the panic/log misleading. Capture the fmt::Result and emit a clear "diagnostic truncated" log when formatting overflows (or increase capacity).

    let mut buf = ArrayString::<384>::new();
    let _ = write!(
        &mut buf,
        "initrd crc mismatch: iso={:?} base={:#x} size={:#x} \
         exp={:#x} got={:#x} got2={:#x} head={:02x?} tail={:02x?} \

openhcl/openhcl_boot/src/arch/x86_64/memory.rs:585

This SHA-384 instrumentation runs in the hot accept/copy path for every 2MB chunk, which is a significant boot-time cost in non-debug builds. If this is intended as temporary diagnostics, gate it behind cfg(debug_assertions) (or a dedicated feature) so production builds don't pay the hashing overhead.

This issue also appears in the following locations of the same file:

line 623
line 676

                // DIAG: record the SHA-384 of this chunk while it's still the
                // shared/host-loaded content, and feed it into a running
                // combined Phase-A hash for later comparison against the
                // measured expected hash.
                diag_record_phase_a(range.start(), &ram_buffer[..]);

openhcl/openhcl_boot/src/arch/x86_64/memory.rs:159

Repository guideline prefers assert!/assert_eq! over debug_assert! for internal invariants so violations are caught in release too. Since this assumes equal lengths after the earlier check, use assert_eq! here.

fn diag_dump_first_diffs(tag: &str, gpa: u64, pre: &[u8], post: &[u8]) {
    const CACHE_LINE: usize = 64;
    const MAX_DIFF_LINES: usize = 8;

    debug_assert_eq!(pre.len(), post.len());

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (5)

openhcl/openhcl_boot/src/arch/x86_64/memory.rs:588

This SHA-384 hashing runs for every accepted chunk on both SNP and TDX builds, even in release builds, which can add noticeable boot-time overhead. If the intent is temporary/diagnostic, gate it (e.g., cfg!(debug_assertions) and/or IsolationType::Snp) so production builds don’t pay for it.

                // DIAG: record the SHA-384 of this chunk while it's still the
                // shared/host-loaded content, and feed it into a running
                // combined Phase-A hash for later comparison against the
                // measured expected hash.
                diag_record_phase_a(range.start(), &ram_buffer[..]);

openhcl/openhcl_boot/src/arch/x86_64/memory.rs:640

Phase-C verification currently executes on every accept iteration (and for TDX as well), including creating an identity-mapped slice and doing a full pre == post compare. If this is intended only for debugging rare SNP issues, gate it similarly to the Phase-A hashing to avoid extra work in normal boots.

                // DIAG: re-hash the chunk from the freshly written private
                // page (Phase C) and compare against the Phase-A bytes still
                // sitting in `ram_buffer`. A mismatch here means the accept/
                // copy-back path corrupted this chunk; hex diffs of the first
                // few differing cache lines are logged.
                {

openhcl/openhcl_boot/src/main.rs:639

write! into an ArrayString<384> can fail (capacity overflow) and the result is currently ignored, which can lead to an empty/truncated diagnostic string right when it’s most needed. Consider falling back to a shorter message if formatting overflows so the panic/log always contains useful context.

    let mut buf = ArrayString::<384>::new();
    let _ = write!(
        &mut buf,
        "initrd crc mismatch: iso={:?} base={:#x} size={:#x} \
         exp={:#x} got={:#x} got2={:#x} head={:02x?} tail={:02x?} \

openhcl/openhcl_boot/src/arch/x86_64/memory.rs:158

diag_dump_first_diffs uses debug_assert_eq! for a non-negotiable invariant (callers already rely on equal lengths). In this repo, internal invariants are generally asserted in all builds; consider using assert_eq! (or removing the assertion entirely since the function is only called after a length check).

fn diag_dump_first_diffs(tag: &str, gpa: u64, pre: &[u8], post: &[u8]) {
    const CACHE_LINE: usize = 64;
    const MAX_DIFF_LINES: usize = 8;

    debug_assert_eq!(pre.len(), post.len());

    let mut total_diff_bytes: usize = 0;

openhcl/openhcl_boot/src/arch/x86_64/memory.rs:52

This file adds extensive diagnostics for "Imported regions hash mismatch" (including always-on hashing of accepted memory), but the PR title only mentions initrd CRC mismatch. Please update the PR title/description to reflect the additional scope, or split this into a separate PR so reviewers can evaluate the operational/perf impact independently.

// This is a temporary debugging aid intended for local investigation of a rare
// mismatch seen on SNP boots. It computes SHA-384 of every 2 MB accept chunk
// at three points to isolate which phase corruption occurs in:
//   Phase A: bytes as read from the shared (host-visible) page, before the
//            shared -> private transition.
//   Phase C: bytes as read from the same GPA immediately after the transition
//            (via the C=1 identity map), i.e. after accept + copy-back.
//   Phase D: bytes as read from the same GPA at final verify time.
// A running combined Phase-A hash is also accumulated to compare against
// `imported_regions_hash()` to distinguish "host loaded bad data" from
// "shim mangled it".
//
// Not intended for check-in.

Steven Malis (smalis-msft)

This will be reverted once the issue is understood and fixed.

github-actions · 2026-08-14T15:30:32Z

2 Petri tests failed (0 unstable)

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

vm/loader/igvmfilegen/src/file_loader.rs:628

The overflow error message reports the number of per-page hashes as “pages”, but count is the number of hash entries (one per shared 4KB page). This is confusing when diagnosing real failures.

                    anyhow::bail!(
                        "expected-page-hashes region overflow: {} pages > {} max \
                         (increase PARAVISOR_MEASURED_VTL2_CONFIG_PAGE_HASHES_SIZE_PAGES)",
                        count,
                        EXPECTED_PAGE_HASH_MAX_COUNT,

openhcl/openhcl_boot/src/arch/x86_64/memory.rs:790

This diagnostic hashing runs on every 2MB accept chunk (Phase A capture + Phase B verification), which adds significant SHA-384 work and extra memory traffic on every SNP/TDX boot. Since the comment explicitly says “Not intended for check-in”, this should be gated (e.g., behind a dedicated Cargo feature and/or debug_assertions) so release builds don’t pay the cost or carry the extra code size.

                // DIAG: record the SHA-384 of this chunk while it's still the
                // shared/host-loaded content, and feed it into a running
                // combined Phase-A hash for later comparison against the
                // measured expected hash.
                diag_record_phase_a(range.start(), &ram_buffer[..]);

vm/loader/loader_defs/src/paravisor.rs:92

PARAVISOR_MEASURED_VTL2_CONFIG_PAGE_HASHES_SIZE_PAGES = 256 increases PARAVISOR_VTL2_CONFIG_REGION_PAGE_COUNT_MAX (and thus the reserved parameter region) by 1MB for all images. If this region is intended only for debug diagnostics, consider making the size conditional (feature-flagged) or deriving it from the number of shared pages to avoid permanently inflating measured memory and build/boot costs.

pub const PARAVISOR_MEASURED_VTL2_CONFIG_PAGE_HASHES_SIZE_PAGES: u64 = 256;

vm/loader/igvmfilegen/src/file_loader.rs:614

The new expected-page-hashes region emission path isn’t covered by the existing unit tests in this file (tests never set set_expected_page_hashes_config_page/set_imported_regions_config_page). Adding a unit test that enables the config pages and asserts the region header (magic/version/count) and per-page hash ordering would help prevent regressions in the diagnostic format.

            if let Some(hashes_page_base) = self.expected_page_hashes_config_page {

+        use loader_defs::paravisor::{
+            EXPECTED_PAGE_HASH_MAX_COUNT, EXPECTED_PAGE_HASHES_MAGIC, EXPECTED_PAGE_HASHES_VERSION,
+            ExpectedPageHash, ExpectedPageHashesHeader,
+            PARAVISOR_MEASURED_VTL2_CONFIG_PAGE_HASHES_PAGE_INDEX,
+        };
+
+        let header_start = self.parameter_region_start
+            + (PARAVISOR_MEASURED_VTL2_CONFIG_PAGE_HASHES_PAGE_INDEX * hvdef::HV_PAGE_SIZE);
+


Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

vm/loader/igvmfilegen/src/file_loader.rs:603

The overflow error message is reporting count as "pages", but count is the number of per-page hash entries. This is misleading when diagnosing sizing issues (it can cause someone to interpret the value as 4KB pages rather than hashes).

                    anyhow::bail!(
                        "expected-page-hashes region overflow: {} pages > {} max \
                         (increase PARAVISOR_MEASURED_VTL2_CONFIG_PAGE_HASHES_SIZE_PAGES)",
                        count,
                        EXPECTED_PAGE_HASH_MAX_COUNT,

openhcl/openhcl_boot/src/arch/x86_64/memory.rs:63

This introduces a large amount of always-on diagnostic instrumentation into a hot path (accept_pending_vtl2_memory / verify_imported_regions_hash), and the comment explicitly says "Not intended for check-in." If this needs to live in-tree, it should be gated (e.g., behind a Cargo feature or cfg(debug_assertions) / confidential-debug) so production boots don’t pay the hashing/logging cost or risk COM3 log spam.

// zeroed page, a substituted page, etc., and we don't want to spam COM3 with
// 64 lines per mismatched chunk.
//
// Not intended for check-in.

openhcl/openhcl_boot/src/main.rs:638

The write! into a fixed-capacity ArrayString ignores the fmt::Result. If the buffer is too small, the diagnostic will be silently truncated (or empty), which defeats the purpose when you immediately log/panic with it.

    let mut buf = ArrayString::<384>::new();
    let _ = write!(
        &mut buf,

openhcl/openhcl_boot/src/arch/x86_64/memory.rs:70

This file already uses static_assertions::const_assert! later; using const _: () = assert!(...) here is inconsistent and may be less clear. Prefer const_assert! for compile-time checks.

const DIAG_MAX_PAGES_PER_CHUNK: usize = X64_LARGE_PAGE_SIZE as usize / DIAG_PAGE_SIZE;
const _: () = assert!(DIAG_MAX_PAGES_PER_CHUNK <= 512);

+        let header_start = self.parameter_region_start
+            + (PARAVISOR_MEASURED_VTL2_CONFIG_PAGE_HASHES_PAGE_INDEX * hvdef::HV_PAGE_SIZE);
+
+        // SAFETY: header_start is a measured address inside the parameter
+        // region reserved for this purpose at IGVM build time.
+        let header = unsafe { &*(header_start as *const ExpectedPageHashesHeader) };
+        if header.magic != EXPECTED_PAGE_HASHES_MAGIC
+            || header.version != EXPECTED_PAGE_HASHES_VERSION
+        {
+            return &[];
+        }
+        let count = (header.page_hash_count as usize).min(EXPECTED_PAGE_HASH_MAX_COUNT);


Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (5)

vm/loader/igvmfilegen/src/file_loader.rs:603

The overflow error message says "{} pages" but count here is the number of per-page hash entries, not a page count. This makes sizing/debugging misleading when the region is too small.

                    anyhow::bail!(
                        "expected-page-hashes region overflow: {} pages > {} max \
                         (increase PARAVISOR_MEASURED_VTL2_CONFIG_PAGE_HASHES_SIZE_PAGES)",
                        count,
                        EXPECTED_PAGE_HASH_MAX_COUNT,

openhcl/openhcl_boot/src/arch/x86_64/memory.rs:790

This diagnostic hashing runs unconditionally for every accepted chunk, which is expensive and (for TDX) likely unnecessary. Consider gating it to SNP + debug builds so normal boots don't pay the SHA-384 cost.

                // DIAG: record the SHA-384 of this chunk while it's still the
                // shared/host-loaded content, and feed it into a running
                // combined Phase-A hash for later comparison against the
                // measured expected hash.
                diag_record_phase_a(range.start(), &ram_buffer[..]);

openhcl/openhcl_boot/src/arch/x86_64/memory.rs:844

This post-copy verification re-reads and hashes the entire chunk on every iteration. If this is meant as temporary diagnostics, it should be gated to SNP + debug builds to avoid added overhead and extra unsafe reads in normal boots.

                {
                    // SAFETY: Same memory just written above; identity mapped.
                    let post = unsafe {
                        core::slice::from_raw_parts(
                            range.start() as *const u8,

openhcl/openhcl_boot/src/arch/x86_64/memory.rs:63

The comment "Not intended for check-in" is confusing in committed code. If this is temporary instrumentation, reword it to reflect the intended gating/removal plan.

// Not intended for check-in.

openhcl/openhcl_boot/src/main.rs:639

The diagnostic formatting ignores the write! result, so if the fixed-size ArrayString fills up the message will be silently truncated (reducing its usefulness when debugging CRC mismatches). Consider failing loudly if the buffer is too small.

    let _ = write!(
        &mut buf,
        "initrd crc mismatch: iso={:?} base={:#x} size={:#x} \
         exp={:#x} got={:#x} got2={:#x} head={:02x?} tail={:02x?} \
         eighths=[{:#x},{:#x},{:#x},{:#x},{:#x},{:#x},{:#x},{:#x}]",

Chris Oo (chris-oo)

Lets make it clear that this is temporary and to be reverted (in the description as well please)

github-actions · 2026-08-14T23:22:33Z

2 Petri tests failed (0 unstable)

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

openhcl/openhcl_boot/src/arch/x86_64/memory.rs:1027

diag_record_phase_a() hashes every accepted chunk (and may also do per-page SHA-384 work) on every isolated boot, including TDX. This is likely too expensive for normal boots given this is intended as temporary SNP instrumentation. Consider gating the call behind debug builds and/or IsolationType::Snp.

                diag_record_phase_a(range.start(), &ram_buffer[..]);

openhcl/openhcl_boot/src/arch/x86_64/memory.rs:1076

diag_verify_phase_b() re-hashes and compares the entire chunk (plus per-page hashing on mismatch) on every accept iteration. Since this is temporary instrumentation, consider gating this block behind debug builds / IsolationType::Snp to avoid impacting boot time on normal runs.

                {
                    // SAFETY: Same memory just written above; identity mapped.
                    let post = unsafe {
                        core::slice::from_raw_parts(
                            range.start() as *const u8,

openhcl/openhcl_boot/src/arch/x86_64/memory.rs:751

The imported-regions hash mismatch diagnostics are currently enabled for all isolated boots (including VBS) via an unconditional diag_init_expected_hashes(shim_params) call. This adds extra work (and may touch a region that some loaders/isolations may not reserve/populate) even when the SNP-only diagnostics aren’t relevant. Consider gating this behind debug builds and/or IsolationType::Snp so production/non-SNP paths don’t pay for it.

This issue also appears in the following locations of the same file:

line 1027
line 1072

    // DIAG: cache the loader-emitted per-page expected hashes (from the
    // measured expected-page-hashes region) so that as Phase A captures
    // each 4 KB shared page we can compare it against the loader's
    // baseline. Silently no-ops on older IGVMs without the region.
    diag_init_expected_hashes(shim_params);

+        let header_start = self.parameter_region_start
+            + (PARAVISOR_MEASURED_VTL2_CONFIG_PAGE_HASHES_PAGE_INDEX * hvdef::HV_PAGE_SIZE);
+
+        // SAFETY: header_start is a measured address inside the parameter
+        // region reserved for this purpose at IGVM build time.
+        let header = unsafe { &*(header_start as *const ExpectedPageHashesHeader) };


Jenna Goddard (jennagoddard) added 2 commits August 10, 2026 10:25

add logging

eb83715

debug only

d91a8de

Copilot AI lite review requested due to automatic review settings August 10, 2026 18:42

Jenna Goddard (jennagoddard) requested a review from a team as a code owner August 10, 2026 18:42

github-actions Bot added the unsafe Related to unsafe code label Aug 10, 2026

Copilot started reviewing on behalf of Jenna Goddard (jennagoddard) August 10, 2026 18:42 View session

Copilot AI reviewed Aug 10, 2026

View reviewed changes

hash diagnostics

254001a

Copilot AI review requested due to automatic review settings August 13, 2026 07:21

Copilot started reviewing on behalf of Jenna Goddard (jennagoddard) August 13, 2026 07:21 View session

Copilot AI reviewed Aug 13, 2026

View reviewed changes

format

b91baae

Copilot AI review requested due to automatic review settings August 13, 2026 15:08

Copilot started reviewing on behalf of Jenna Goddard (jennagoddard) August 13, 2026 15:08 View session

Copilot AI reviewed Aug 13, 2026

View reviewed changes

Steven Malis (smalis-msft) approved these changes Aug 13, 2026

View reviewed changes

add hashing for each page

32c632f

Copilot AI review requested due to automatic review settings August 14, 2026 17:48

Copilot started reviewing on behalf of Jenna Goddard (jennagoddard) August 14, 2026 17:49 View session

Copilot AI reviewed Aug 14, 2026

View reviewed changes

fix memory issue

2e2344f

Copilot AI review requested due to automatic review settings August 14, 2026 19:06

Copilot started reviewing on behalf of Jenna Goddard (jennagoddard) August 14, 2026 19:07 View session

Copilot AI reviewed Aug 14, 2026

View reviewed changes

nit

f912005

Copilot AI review requested due to automatic review settings August 14, 2026 22:13

Copilot started reviewing on behalf of Jenna Goddard (jennagoddard) August 14, 2026 22:13 View session

Jenna Goddard (jennagoddard) changed the title ~~SNP: Add cache diagnostics on initrd CRC hash mismatch~~ SNP: Add import regions hash and cache diagnostics on initrd CRC hash mismatch Aug 14, 2026

Copilot AI reviewed Aug 14, 2026

View reviewed changes

Jenna Goddard (jennagoddard) changed the title ~~SNP: Add import regions hash and cache diagnostics on initrd CRC hash mismatch~~ Temporary diagnotics for SNP: Add import regions hash and cache diagnostics on initrd CRC hash mismatch Aug 14, 2026

Chris Oo (chris-oo) approved these changes Aug 14, 2026

View reviewed changes

phase a page comparison

1a743e1

Copilot AI review requested due to automatic review settings August 15, 2026 00:07

Copilot started reviewing on behalf of Jenna Goddard (jennagoddard) August 15, 2026 00:08 View session

Copilot AI reviewed Aug 15, 2026

View reviewed changes

Ben Hillis (benhillis) approved these changes Aug 15, 2026

View reviewed changes

Chris Oo (chris-oo) approved these changes Aug 15, 2026

View reviewed changes

Jenna Goddard (jennagoddard) merged commit 61fd38f into microsoft:main Aug 15, 2026
71 checks passed

Uh oh!

Conversation

Jenna Goddard (jennagoddard) commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 10, 2026

Uh oh!

Steven Malis (smalis-msft) commented Aug 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

github-actions Bot commented Aug 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Steven Malis (smalis-msft) left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Chris Oo (chris-oo) left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Jenna Goddard (jennagoddard) commented Aug 10, 2026 •

edited

Loading