(RFC) Write a diagnostic marker when provisioning the VMGS #2552
Conversation
stunes-ms
requested review from
mingweishih and
tjones60
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
This RFC introduces a diagnostic provisioning marker to VMGS files that records metadata about how the file was initially provisioned. The marker captures information such as the provisioner entity (OpenHCL, HCL, etc.), vTPM version and configuration details, and the HCL commit hash.
Key changes:
- New
ProvisioningMarkerstruct andVmgsProvisionerenum invmgs_formatto define the diagnostic marker format - vTPM-related constants moved to
tpm_protocolfor reuse across components - Logic in OpenHCL to write the provisioning marker when a VMGS file is provisioned
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| vm/vmgs/vmgs_format/src/lib.rs | Defines the ProvisioningMarker struct, VmgsProvisioner enum, and adds new file ID for the marker |
| vm/vmgs/vmgs_format/Cargo.toml | Adds tpm_protocol dependency for TpmVersion type |
| vm/vmgs/vmgs/src/vmgs_impl.rs | Tracks whether VMGS was formatted due to failure, adds accessor method |
| vm/vmgs/vmgs/Cargo.toml | Reorders dependency (cosmetic change) |
| vm/devices/tpm/tpm_protocol/src/lib.rs | Defines TpmVersion enum and default TPM configuration constants |
| vm/devices/tpm/tpm_protocol/Cargo.toml | Adds dependencies needed for the new enum (open_enum, zerocopy) |
| openhcl/underhill_core/src/worker.rs | Writes provisioning marker to VMGS when file is provisioned |
| openhcl/underhill_core/Cargo.toml | Adds dependencies for tpm_protocol and vmgs_format |
| pub vtpm_nvram_size: u32, | ||
| pub vtpm_akcert_size: u32, | ||
| pub vtpm_akcert_attrs: u32, | ||
| pub hcl_version: [u8; 40], // OpenHCL: string representation of commit hash |
There was a problem hiding this comment.
The comment 'OpenHCL: string representation of commit hash' is ambiguous about what happens when the provisioner is not OpenHCL. Consider clarifying whether this field is used by all provisioners or documenting the expected value for non-OpenHCL provisioners.
| pub hcl_version: [u8; 40], // OpenHCL: string representation of commit hash | |
| /// For provisioner == OPENHCL, this contains the string representation of the OpenHCL commit hash. | |
| /// For all other provisioners, this field is zero-filled and should be ignored. | |
| pub hcl_version: [u8; 40], |
|
|
||
| if let Some((_, ref mut vmgs)) = vmgs { | ||
| if vmgs.was_provisioned_this_boot() { | ||
| let mut rev: [u8; 40] = [0; 40]; |
There was a problem hiding this comment.
The magic number 40 is duplicated here and in the ProvisioningMarker struct definition. Consider extracting it as a named constant (e.g., HCL_VERSION_MAX_LEN) in vmgs_format to ensure consistency and make future changes easier.
There was a problem hiding this comment.
this sounds like a good suggestion
| let marker = ProvisioningMarker { | ||
| marker_version: vmgs_format::PROVISIONING_MARKER_CURRENT_VERSION, | ||
| provisioner: VmgsProvisioner::OPENHCL, | ||
| reset_by_gsl_flag: (matches!( | ||
| dps.general.guest_state_lifetime, | ||
| GuestStateLifetime::Reprovision | ||
| ) || (matches!( | ||
| dps.general.guest_state_lifetime, | ||
| GuestStateLifetime::ReprovisionOnFailure | ||
| ) && vmgs.was_formatted_on_failure())) as u8, |
There was a problem hiding this comment.
This complex boolean expression spanning multiple lines is difficult to read. Consider extracting it into a well-named helper function or variable (e.g., was_reset_by_gsl) to improve clarity.
| let marker = ProvisioningMarker { | |
| marker_version: vmgs_format::PROVISIONING_MARKER_CURRENT_VERSION, | |
| provisioner: VmgsProvisioner::OPENHCL, | |
| reset_by_gsl_flag: (matches!( | |
| dps.general.guest_state_lifetime, | |
| GuestStateLifetime::Reprovision | |
| ) || (matches!( | |
| dps.general.guest_state_lifetime, | |
| GuestStateLifetime::ReprovisionOnFailure | |
| ) && vmgs.was_formatted_on_failure())) as u8, | |
| let was_reset_by_gsl = matches!( | |
| dps.general.guest_state_lifetime, | |
| GuestStateLifetime::Reprovision | |
| ) || (matches!( | |
| dps.general.guest_state_lifetime, | |
| GuestStateLifetime::ReprovisionOnFailure | |
| ) && vmgs.was_formatted_on_failure()); | |
| let marker = ProvisioningMarker { | |
| marker_version: vmgs_format::PROVISIONING_MARKER_CURRENT_VERSION, | |
| provisioner: VmgsProvisioner::OPENHCL, | |
| reset_by_gsl_flag: was_reset_by_gsl as u8, |
3 participants
We want to leave a marker in a VMGS file that indicates that it was provisioned originally by HCL, with some diagnostic details (vTPM version, etc.) This is intended to be information-only.