Skip to content

DNM: Copilot messing around with SNP#3912

Closed
smalis-msft wants to merge 4 commits into
microsoft:mainfrom
smalis-msft:snp-testing
Closed

DNM: Copilot messing around with SNP#3912
smalis-msft wants to merge 4 commits into
microsoft:mainfrom
smalis-msft:snp-testing

Conversation

@smalis-msft

Copy link
Copy Markdown
Contributor

Don't bother reviewing

@smalis-msft smalis-msft requested a review from a team as a code owner July 9, 2026 21:44
Copilot AI review requested due to automatic review settings July 9, 2026 21:44
@github-actions github-actions Bot added the unsafe Related to unsafe code label Jul 9, 2026
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown

⚠️ 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

@smalis-msft smalis-msft added the release-ci-required Add to a PR to trigger PR gates in release mode label Jul 9, 2026

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.

Pull request overview

This PR introduces support for the AMD SNP COHERENCY_SFW_NO CPUID bit and uses it to conditionally apply (or skip) the SNP cache coherency fixup in the OpenHCL bootshim, while also extending the CVM CPUID masking/tests to recognize the new bit.

Changes:

  • Extend ExtendedSevFeaturesEbx with the coherency_sfw_no bit and allow it through SNP CPUID masking.
  • Add a CVM CPUID unit test validating that coherency_sfw_no is preserved through the SNP CPUID filtering path.
  • Use COHERENCY_SFW_NO in OpenHCL boot to decide whether to perform the cache-line fixup during page acceptance, and log the decision during SNP memory setup.

Reviewed changes

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

Show a summary per file
File Description
vm/x86/x86defs/src/cpuid.rs Adds the coherency_sfw_no bit to the ExtendedSevFeaturesEbx CPUID bitfield definition.
openhcl/virt_mshv_vtl/src/cvm_cpuid/tests/mod.rs Adds a unit test ensuring coherency_sfw_no survives SNP CPUID leaf filtering/masking.
openhcl/virt_mshv_vtl/src/cvm_cpuid/snp.rs Extends the SNP CPUID EBX mask to include the coherency_sfw_no bit.
openhcl/openhcl_boot/src/arch/x86_64/snp.rs Introduces cache_fixup_required() and conditions cache fixup during page acceptance on COHERENCY_SFW_NO.
openhcl/openhcl_boot/src/arch/x86_64/memory.rs Logs whether the SNP cache coherency fixup is required during VTL2 memory setup.

Comment on lines 874 to +880
/// Accepts or unaccepts a specific gpa range. On SNP systems, this corresponds to issuing a
/// pvalidate over the GPA range with the desired value of the validate bit.
///
/// When accepting (`validate` is true), the cache lines of each freshly
/// validated page are fixed up before the page is accessed under its new
/// (private) assignment. No fixup is performed when unvalidating.
/// validated page are fixed up, if required by the processor, before the page
/// is accessed under its new (private) assignment. No fixup is performed when
/// unvalidating.
Copilot AI review requested due to automatic review settings July 9, 2026 21:48

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.

Pull request overview

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

Comments suppressed due to low confidence (1)

openhcl/openhcl_boot/src/arch/x86_64/snp.rs:580

  • Ghcb::initialize() still transitions the GHCB page through SNP page-state changes (unaccept + PAGE_STATE_CHANGE + mapping change). Removing the cache-line fixup here means CPUs that do require the coherency workaround can hit the same stale-cache risk described in cache_lines_fixup_page, potentially leading to memory corruption during boot logging initialization. Reintroduce the fixup, gated by cache_fixup_required(), before the page is accessed under its new (shared) assignment.
        // Map the page as non-confidential by updating the PTE.
        page_table[PT_INDEX] = pte_for_pfn(page_number, false);
        flush_tlb();

        // Flipping the C-bit makes the contents of the GHCB page scrambled,
        // zero it out.
        ghcb_access::zero_page();
        ghcb_access::set_protocol_version(GhcbProtocolVersion::V2);

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

Copilot AI review requested due to automatic review settings July 10, 2026 14:48

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.

Pull request overview

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

Comments suppressed due to low confidence (1)

openhcl/openhcl_boot/src/arch/x86_64/snp.rs:597

  • The GHCB shared/unshared transition here still changes page assignment state (pvalidate + RMP state change), and on CPUs where the SNP cache coherency workaround is required this needs a cache-line fixup before the page is accessed in its new state. The previous cache_lines_fixup_page call was removed, but this path doesn’t use set_page_acceptance(..., fixup_cache=...), so the workaround would never run for the GHCB page.
        // Map the page as non-confidential by updating the PTE.
        page_table[PT_INDEX] = pte_for_pfn(page_number, false);
        flush_tlb();

        // Flipping the C-bit makes the contents of the GHCB page scrambled,
        // zero it out.
        ghcb_access::zero_page();

@github-actions

Copy link
Copy Markdown

Copilot AI review requested due to automatic review settings July 10, 2026 16:20

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.

Pull request overview

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

Comment on lines +763 to +764
assert!(range.start().is_multiple_of(PAGE_SIZE as u64));
assert!(range.end().is_multiple_of(PAGE_SIZE as u64));
@github-actions

Copy link
Copy Markdown

@smalis-msft smalis-msft deleted the snp-testing branch July 13, 2026 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-ci-required Add to a PR to trigger PR gates in release mode unsafe Related to unsafe code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants