vmservice: support SNP IGVM boot over TTRPC - #4399
Nikola Bojanic (nbojanic) wants to merge 7 commits into
Conversation
|
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.
🟡 Changes recommended
The MSHV SNP launch state machine now silently treats “launch already finished” as success, which can mask incorrect call sequencing and diverges from other backends’ semantics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends OpenVMM’s VM service (TTRPC) and MSHV SNP backend to support booting SEV-SNP guests from an IGVM file, including preserving IGVM launch measurement ordering and propagating IGVM-provided SNP launch parameters (policy + optional ID block/auth data). This builds on the IGVM SNP launch plumbing added in PR #4361 and wires it through the VM service API surface.
Changes:
- Add IGVM boot + SNP isolation selection to the TTRPC VM service API and map it into
openvmm_entryconfiguration. - Teach the MSHV SNP launch path to consume IGVM-provided VMSA backing, preserve import order for measurement, and submit IGVM policy/ID block data at launch finish.
- Extend IGVM file generation and manifests to support restricted interrupt injection variants and document usage.
File summaries
| File | Description |
|---|---|
| vmm_core/virt/src/x86/snp.rs | Add reusable helpers to build SNP PSP ID block and ID_AUTH structures plus unit tests. |
| vmm_core/virt_mshv/src/x86_64/snp.rs | Add IGVM-prepared SNP config, preserve measurement import order, and submit policy/ID block during launch finish. |
| vmm_core/virt_mshv/src/x86_64/mod.rs | Plumb IGVM SNP config into proto-partition creation/build flow and enforce VMSA mapping constraints. |
| vmm_core/virt_mshv/src/lib.rs | Store proto isolation state (incl. prepared SNP config) in MshvProtoPartition instead of only a cpuid-offload flag. |
| vm/x86/x86defs/src/snp.rs | Define zerocopy-compatible layouts for SNP PSP ID_AUTH structures (4096B page). |
| vm/loader/manifests/snp-linux-direct-restricted.json | Add a restricted-injection manifest profile for MSHV bring-up. |
| vm/loader/manifests/README.md | Document normal vs restricted injection artifacts and clarify generation command example. |
| vm/loader/igvmfilegen/src/snp_linux_direct.rs | Allow selecting normal vs restricted injection when generating the IGVM VMSA. |
| vm/loader/igvmfilegen/src/main.rs | Remove restriction that forced normal injection for snp_linux_direct and pass injection type through. |
| vm/loader/igvmfilegen_config/src/lib.rs | Add config parsing test coverage for the new restricted-injection manifest. |
| openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto | Add IGVM boot config and isolation config to the VM service API. |
| openvmm/openvmm_entry/src/ttrpc/mod.rs | Implement IGVM boot + SNP isolation handling for VM service requests and plumb igvm_path. |
| Guide/src/reference/openvmm/management/cli.md | Update CLI docs to include SNP IGVM boot support and an example invocation. |
Review details
Suppressed comments (1)
vmm_core/virt_mshv/src/x86_64/snp.rs:700
- Returning
Ok(())forSnpLaunchState::Finishedmakes SNP launch idempotent for MSHV only and can hide accidental double-launches; returning an explicit error here keeps backend semantics consistent and makes misuse visible to callers.
match *state {
SnpLaunchState::NotStarted => *state = SnpLaunchState::Started,
SnpLaunchState::Started => return Err(SnpError::LaunchInProgress.into()),
SnpLaunchState::Finished => return Ok(()),
SnpLaunchState::Failed => return Err(SnpError::LaunchFailed.into()),
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
6c9f27c to
5ac8ff9
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It touches security-sensitive SNP launch plumbing (measurement ordering, VMSA handling, launch-finish ID block submission) and introduces a new external TTRPC configuration surface, so it merits final human review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
5ac8ff9 to
cc2e7cf
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
MSHV SNP launch currently treats a repeated “accept initial pages” call as a silent success, which can mask incorrect call patterns and drop later imports without error.
Review details
Suppressed comments (2)
vmm_core/virt_mshv/src/x86_64/snp.rs:19
- MSHV SNP launch state machine now treats a second launch attempt as success (via
Finished => Ok(())), but theSnpErrorenum no longer has a dedicated error for this condition. It’s safer to keep a distinctLaunchAlreadyFinishederror (matching the KVM backend) so repeat calls don’t silently become no-ops with potentially differentpagesinputs.
#[error("SNP launch is already in progress")]
LaunchInProgress,
#[error("SNP launch previously failed")]
LaunchFailed,
vmm_core/virt_mshv/src/x86_64/snp.rs:700
- Returning
Ok(())when the SNP launch state is alreadyFinishedmakesaccept_initial_pagessilently idempotent. If this method is accidentally called again with a differentpagesset, the second call would be ignored without an error, which can mask real loader/worker bugs. Consider failing fast with aLaunchAlreadyFinishederror (as KVM does).
match *state {
SnpLaunchState::NotStarted => *state = SnpLaunchState::Started,
SnpLaunchState::Started => return Err(SnpError::LaunchInProgress.into()),
SnpLaunchState::Finished => return Ok(()),
SnpLaunchState::Failed => return Err(SnpError::LaunchFailed.into()),
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
cc2e7cf to
2bd5e14
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new IGVM boot path can silently ignore requested SMBIOS overrides (and accepts empty igvm_path with a poor error), which needs input validation aligned with existing IGVM constraints.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The current TTRPC IGVM path can panic on certain user-provided configs (VMBus vs HVSocket), and there are API/documentation and validation-consistency issues that should be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
openvmm/openvmm_core/src/worker/dispatch.rs:1281
- This change removes the worker-level rejection of
cfg.hypervisor.with_hvfor KVM SNP guest_memfd, but the CLI path still rejects the same configuration inopenvmm_entry::validate_snp_config(openvmm/openvmm_entry/src/lib.rs:2184-2188). This leaves inconsistent behavior across entry points (CLI vs TTRPC vs any other caller) and makes it unclear whether Hyper-V enlightenments are actually supported for SNP.
openvmm/openvmm_entry/src/ttrpc/mod.rs:936
- When IGVM boot returns
vmbus = None, a request that includeshvsocket_configwill later hitconfig.vmbus.as_mut().unwrap()and panic. Since this is user-controlled via the TTRPC request, it can crash the service (DoS). Add an explicit validation that rejects HVSocket configuration when VMBus is disabled (or make the HVSocket wiring avoidunwrap()).
};
let mut chipset_builder =
VmManifestBuilder::new(base_chipset_type, arch).with_serial(ports);
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
| base_chipset_type, | ||
| None, | ||
| Some(igvm_path), | ||
| None, |
There was a problem hiding this comment.
I don't think it's great to tie the vmbus state to the boot config in this way.
There was a problem hiding this comment.
Agree, added an explicit no_vmbus to match the CLI interface.
Chris Oo (chris-oo)
left a comment
There was a problem hiding this comment.
i don't think we want to tie IGVM to implicitly not offer vmbus, so maybe we need another ttrpc argument for if vmbus should be enabled or disabled?
| vm_manifest_builder::BaseChipsetType::HypervGen2Uefi, | ||
| Some((base_template, uefi.secure_boot_enabled)), | ||
| None, | ||
| Some(VmbusConfig::default()), |
There was a problem hiding this comment.
hm... shouldn't this be based on --no-vmbus or the equivalent? we don't want to enable vmbus unless it was set in the config
There was a problem hiding this comment.
oh, this is the uefi branch
There was a problem hiding this comment.
agree, added no_vmbus to match cli interface
There was a problem hiding this comment.
🟡 Changes recommended
SNP TTRPC launches must derive Hyper-V configuration from the isolation mode to avoid KVM rejection.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
openvmm/openvmm_entry/src/ttrpc/mod.rs:1040
- When
isolationis SNP, the worker rejects anycfg.vmbus.is_some()(openvmm_core/src/worker/dispatch.rs:1288-1289), but this still creates VMBus unless the caller happens to set the newno_vmbusflag. Thus the minimal IGVM+SNP request fails with the proto defaultno_vmbus = false; either infer no-VMBus for SNP (including the chipset layout above) or validate this required combination before launching the worker.
vmbus: (!req_config.no_vmbus).then(VmbusConfig::default),
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Critical configuration issues currently block reliable SNP IGVM boot on KVM and leave VMBus enabled by default.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
Add an IGVM boot option and an explicit isolation configuration to the VM-service API. Map SNP IGVM requests to the existing IGVM loader.
Add an IGVM personality field, defaulting to Linux-direct. Reject UEFI personality for now. Reject SMBIOS overrides for IGVM boot rather than silently ignoring them.
Add no_vmbus independently of boot, reject HVSocket without VMBus. Clarify that IGVM boot does not support SMBIOS overrides.
Reject unsupported enum values instead of treating them as no isolation.
4727d00 to
b8af788
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate issues remain in SNP validation, COM3 serial wiring, and no_vmbus device-tree handling.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
openvmm/openvmm_entry/src/ttrpc/mod.rs:1042
no_vmbusremoves the VMBus server and MMIO reservation, but the IGVM loader still unconditionally emitsmicrosoft,vmbusVTL0/VTL2 nodes in its generated DeviceTree. Thus an IGVM request withno_vmbus=truestill advertises a VMBus device to the guest even though no server exists, so this flag cannot provide a working no-VMBus SNP guest (and is especially problematic for the KVM SNP workaround). Propagate the disabled state into the IGVM DeviceTree generation or reject this combination explicitly.
vmbus: (!req_config.no_vmbus).then(VmbusConfig::default),
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
COM3 discovery and integration coverage remain unresolved.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
openvmm/openvmm_entry/src/ttrpc/mod.rs:831
- This new IGVM/SNP CreateVm path has no integration coverage: the existing TTRPC suite exercises Linux-direct and UEFI requests, but not the isolation/personality mapping,
no_vmbuswiring, or actual IGVM launch. A regression here can compile while failing only on the MSHV/SNP host; add a host-gated TTRPC test that submits this configuration and verifies the VM reaches the expected lifecycle state.
openvmm/openvmm_entry/src/ttrpc/mod.rs:854
- The TTRPC path installs serial backends in the chipset, but
com_serial: Nonemeans the IGVM device-tree loader emits nons16550node, so a configured COM3 socket is not discoverable by the Linux guest. The CLI passes COM3'sSerialInformationfor the same IGVM path; derive it here from the TTRPC port configuration as well.
com_serial: None,
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
|
||
| optional string crash_dump_path = 14; | ||
|
|
||
| bool no_vmbus = 16; |
There was a problem hiding this comment.
Needs a comment here and on other new fields.
There was a problem hiding this comment.
Added comments on new fields.
|
|
||
| optional string crash_dump_path = 14; | ||
|
|
||
| bool no_vmbus = 16; |
There was a problem hiding this comment.
Can we call this disable_vmbus?
There was a problem hiding this comment.
Ok, renamed to disable_vmbus.
There was a problem hiding this comment.
🔵 Needs a closer look
Address SNP validation gaps and add the requested test coverage and documentation.
Review details
Suppressed comments (3)
openvmm/openvmm_entry/src/ttrpc/mod.rs:1032
- When this maps
isolationto SNP, the TTRPC path still bypasses the existing SNP validation. The API can therefore combine SNP withNumaConfigexplicit hugepage/large-page memory or non-virtio PCIe endpoints (NVMe/VFIO), although SNP explicitly supports neither; the worker's KVM checks do not reject these cases. Validate the fully built TTRPC config with the shared SNP rules (including the memory checks) before launching the worker.
with_isolation: isolation,
openvmm/openvmm_entry/src/ttrpc/mod.rs:832
- The new IGVM/SNP CreateVM branch has no automated coverage: the existing TTRPC integration tests exercise DirectBoot and UEFI, but never send
IgvmBootorIsolationConfig. Add an integration or focused validation test for a valid SNP IGVM request and the unsupported personality/isolation combinations so this new API path is checked in CI.
vmservice::vm_config::BootConfig::Igvm(boot) => {
if smbios_requested {
bail!("VM-service IGVM boot does not support SMBIOS overrides");
}
if isolation != Some(IsolationType::Snp) {
bail!("VM-service IGVM boot currently supports only SNP isolation");
openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto:80
- This adds a new externally visible boot path, but the Guide has no corresponding description of the ttrpc fields or its SNP constraints. The firmware reference already points readers to an equivalent ttrpc configuration, while the CLI page documents the required VTL0/no-relocation/no-VMBus limitations; please update the management documentation with the ttrpc request shape and restrictions, or explicitly track that follow-up.
// Boot from an IGVM file.
message IgvmBoot {
enum Personality {
// Linux direct boot with Hyper-V enlightenments.
LINUX_DIRECT = 0;
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Support booting an IGVM + SEV SNP vm over the TTRPC API. This is useful for kata-containers.
Builds on #4361, which should be merged first.