openhcl_boot: Serial logging for SEV-SNP#3494
Conversation
SEV-SNP has been the only platform where serial logging isn't supported in the boot shim. Implement the GHCB calls and use that for serial logging via the IOIO exits.
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
There was a problem hiding this comment.
Pull request overview
This PR adds SEV-SNP support for serial logging in the openhcl_boot shim by implementing the GHCB-based IOIO exit path and wiring it into the boot-time logger, bringing SNP in line with other platforms.
Changes:
- Add GHCB page layout definitions (save area, protocol version/usage, Hyper-V hypercall layout) to
x86defs. - Implement GHCB initialization/uninitialization and GHCB-backed port I/O for SNP in
openhcl_bootand route serial writes through it. - Introduce a shared
X64_PAGE_SIZEconstant and add required dependencies.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| vm/x86/x86defs/src/snp.rs | Adds GHCB page/save-area structs and related enums/constants used by the SNP boot shim GHCB implementation. |
| vm/x86/x86defs/src/lib.rs | Introduces X64_PAGE_SIZE constant used for GHCB page sizing and other x64 page math. |
| openhcl/openhcl_boot/src/main.rs | Calls arch init/uninit hooks so SNP can set up/tear down GHCB before/after boot. |
| openhcl/openhcl_boot/src/boot_logger.rs | Adds SNP-specific serial logger variant using GHCB-backed I/O access. |
| openhcl/openhcl_boot/src/arch/x86_64/snp.rs | Implements GHCB mapping/registration, GHCB-backed MSR + IOIO exits, and IoAccess for serial. |
| openhcl/openhcl_boot/src/arch/x86_64/mod.rs | Adds initialize()/uninitialize() dispatch for SNP GHCB lifecycle. |
| openhcl/openhcl_boot/src/arch/x86_64/address_space.rs | Exposes paging constants/types for SNP GHCB mapping logic reuse. |
| openhcl/openhcl_boot/src/arch/aarch64/mod.rs | Adds no-op arch init/uninit to match the new cross-arch interface. |
| openhcl/openhcl_boot/Cargo.toml | Adds bitfield-struct dependency needed by new SNP GHCB/address helpers. |
| Cargo.lock | Locks the new dependency. |
| entries: [u64; PAGE_TABLE_ENTRY_COUNT], | ||
| } | ||
|
|
||
| // Would be great to allocate this pages dynamically as otherwise they go |
| // below to succeed. | ||
| // | ||
| // Soon after this, the GHCB page will be mapped by the kernel at the | ||
| // GPA of its chhosing. The temporarily mapping at GPA 0 poses no |
| // security risk as that page does not contain any sensitive data | ||
| // in the IGVM file. | ||
| // | ||
| // Once support for unpamming the GHCB page from the latest SEV-ES |
| flush_tlb(); | ||
|
|
||
| ghcb_access::clear_page(); | ||
|
|
| pub fn sw_exit_info1() -> u64 { | ||
| ghcb_save_assert_valid_bitmap1!(sw_exit_info1); | ||
| ghcb_save_field_get!(sw_exit_info1, AtomicU64) | ||
| } | ||
|
|
| pub fn set_hypercall_data(data: &[u8], start: usize) { | ||
| // SAFETY: Atomic access to the GHCB page. | ||
| let ghcb_data = unsafe { ghcb_hv_hypercall::<AtomicU8>() }; | ||
| assert!(data.len() <= GHCB_PAGE_HV_HYPERCALL_DATA_SIZE); |
| // SAFETY: Using the `vmgexit` instruction forces an exit to the hypervisor but doesn't | ||
| // directly change program state. | ||
| unsafe { | ||
| asm! {r#" | ||
| rep vmmcall | ||
| "# | ||
| } | ||
| asm!("rep vmmcall", options(nomem, nostack)); | ||
| } |
| } | ||
| } | ||
|
|
||
| pub fn initialize(p: &ShimParams) { |
There was a problem hiding this comment.
these need comments on what the intention of these are for, since i assume for CCA we will need to probably do something similar. We need to document what the requirements for arch:: etc calls are.
| /// to cause memory safety issues. | ||
| fn sev_vmgexit() { | ||
| /// The caller must ensure that the GHCB page is properly mapped | ||
| /// and can be accessed in a memory-safe manner. |
There was a problem hiding this comment.
this is vague, what does it mean? no concurrent accesses from us itself? the host can always concurrently access the data?
| } | ||
| } | ||
|
|
||
| macro_rules! ghcb_field_set { |
There was a problem hiding this comment.
is there some alternatives to all these macros? what do we actually need?
There was a problem hiding this comment.
what do these macros actually do? why do they exist?
| } | ||
| } | ||
|
|
||
| #[allow(dead_code)] |
There was a problem hiding this comment.
this needs to change to expect
| impl Ghcb { | ||
| pub fn initialize() { | ||
| // Make sure page alignment. | ||
| assert!((PAGE_TABLE.get() as u64) & (X64_PAGE_SIZE - 1) == 0); |
|
|
||
| // Flipping the C-bit makes the contents of the GHCB page scrambled, | ||
| // zero it out. | ||
| ghcb_access::clear_page(); |
There was a problem hiding this comment.
should clear_page be called zero_page instead?
| // security risk as that page does not contain any sensitive data | ||
| // in the IGVM file. | ||
| // | ||
| // Once support for unpamming the GHCB page from the latest SEV-ES |
There was a problem hiding this comment.
does the hypervisor support this simplified path yet?
| GHCB_PREVIOUS.replace(unsafe { read_msr(X86X_AMD_MSR_GHCB) }); | ||
| } | ||
|
|
||
| pub fn uninitialize() { |
There was a problem hiding this comment.
i don't know enough about the GHCB spec to confidnetly review this, but my main concern is we must leave the page we used for the GHCB back into the private accepted state. I think that is true, but could we get someone else to review this?
SEV-SNP has been the only platform where serial logging isn't supported in the boot shim.
Implement the GHCB calls and use that for serial logging via the IOIO exits.