add igvmfile read/write to vmgstool #2566

hgarvison · 2025-12-15T20:24:26Z

Add tool to VmgsTool to read the IGVMfile from a DLL (passed in as a data file) and write it to VMGS FileId 8 (GUEST_FIRMWARE). To do this pass one of three resource codes (nonconfidential, snp, tdx) into the cmdline tool:

vmgstool.exe copy-igvmfile --filepath --keypath --datapath --resource-code

github-actions · 2025-12-15T20:24:40Z

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

Copilot

Pull request overview

This PR adds functionality to vmgstool for reading IGVM firmware files from Windows DLLs and writing them to VMGS file ID 8 (GUEST_FIRMWARE). The implementation uses Windows API calls to extract resources from DLLs and supports multiple resource codes (nonconfidential, snp, snp_no_hcl, tdx, tdx_no_hcl) for different VM configurations.

Key changes:

New copy-igvmfile command that extracts IGVM files from DLLs using Windows resource APIs
Support for encrypted and unencrypted VMGS files when writing IGVM data
Platform-specific implementation for Windows x86_64 only

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.

File	Description
vm/vmgs/vmgstool/src/main.rs	Implements the copy-igvmfile command with Windows API resource loading, adds error handling, command-line parsing, and test cases
vm/vmgs/vmgstool/build.rs	Adds rustc-check-cfg directive for guest_arch configuration
vm/vmgs/vmgstool/Cargo.toml	Adds winapi dependency with required Windows API features
Guide/src/dev_guide/dev_tools/vmgstool.md	Documents the new copy-igvmfile command usage

vm/vmgs/vmgstool/src/main.rs

Guide/src/dev_guide/dev_tools/vmgstool.md

github-actions · 2025-12-16T19:01:59Z

At least one Petri test failed.

github-actions · 2025-12-16T20:14:31Z

At least one Petri test failed.

vm/vmgs/vmgstool/src/main.rs

openvmm/openvmm_pcat_locator/src/lib.rs

tjones60 · 2026-01-09T19:04:57Z

vm/vmgs/vmgstool/src/main.rs

    GspUnknown,
    #[error("VMGS file is using an unknown encryption algorithm")]
    EncryptionUnknown,
+    #[cfg(all(windows, guest_arch = "x86_64"))]


What makes any of this x86_64 specific?

The file this command is being used to parse (vmfirmwarehcl.dll) doesn't exist in ARM.

If the code is capable of running on ARM we should let it, even if it'll never be used. cfgs like this should only be used when the code can't run on a given platform due to actual hardware differences.

sounds good, thanks for the feedback. I'll take out the cfgs and gate the couple of tests that try to access vmfirmwarehcl.dll, so they don't fail in the ARM tests

If you do what I suggested below and use include_bytes, the test should work fine on arm.

vm/vmgs/vmgstool/src/main.rs

openvmm/openvmm_pcat_locator/src/lib.rs

support/resource_dll_parser/src/lib.rs

smalis-msft

Non-vmgs portions look good to me, I'll let @tjones60 do the final signoff

github-actions · 2026-01-15T21:32:51Z

At least one Petri test failed.

tjones60 · 2026-01-22T21:18:48Z

vm/vmgs/vmgstool/src/main.rs

+    Ok(())
+}
+
+async fn write_igvmfile(


I don't think this function is necessary, just put the calls to read_igvmfile and vmgs_write in vmgs_file_copy_igvmfile.

tjones60 · 2026-01-22T21:19:08Z

vm/vmgs/vmgstool/src/main.rs

+
+    let encrypt = key_path.is_some();
+    let mut vmgs = vmgs_file_open(file_path, key_path, OpenMode::ReadWrite).await?;
+    eprintln!("Reading IGVMfile from DLL");


Also, as written we log "Reading IGVMfile..." three times; we should do that only once.

tjones60 · 2026-01-22T21:20:12Z

vm/vmgs/vmgstool/src/main.rs

+    allow_overwrite: bool,
+    resource_code: ResourceCode,
+) -> Result<(), Error> {
+    eprintln!("Writing IGVMfile to VMGS");


I think this is redundant, vmgs_write logs the file ID

tjones60 · 2026-01-22T21:20:31Z

vm/vmgs/vmgstool/src/main.rs

+    use std::io::{Read, Seek, SeekFrom};
+
+    eprintln!("Reading IGVMfile from DLL");
+    // Convert the wide string back to a PathBuf


Is this comment stale?

tjones60 · 2026-01-22T21:23:58Z

vm/vmgs/vmgstool/src/main.rs

    #[error("VMGS file is using an unknown encryption algorithm")]
    EncryptionUnknown,
+    #[error("Unable to read IGVM file with Error: {0}")]
+    UnableToReadIgvmFile(String),


This should probably just be

#[error("Unable to read IGVM file")] IgvmFile(#[source] std::io::Error)

That way you don't have to manually format the string every time. Also, we could probably just use Error::DataFile for this, since for this operation it is obvious that the data file is the igvm file.

Actually, since the error from try_find_resource_from_dll is anyhow, how about we use this for the anyhow errors and use DataFile for the io Errors

#[error("Unable to parse IGVM file")] IgvmFile(#[source] anyhow::Error)

tjones60 · 2026-01-22T21:39:54Z

vm/vmgs/vmgstool/src/main.rs

+async fn read_igvmfile(dll_path: PathBuf, resource_code: ResourceCode) -> Result<Vec<u8>, Error> {
+    use std::io::{Read, Seek, SeekFrom};
+
+    eprintln!("Reading IGVMfile from DLL");


nit: Is this the correct stylization? Seems like "IGVM file" would look better than "IGVMfile" here and elsewhere.

tjones60 · 2026-01-22T21:41:53Z

vm/vmgs/vmgstool/src/main.rs

+    file.read_exact(&mut bytes)
+        .map_err(|e| Error::UnableToReadIgvmFile(format!("Failed to read resource data: {}", e)))?;
+
+    eprintln!("Successfully loaded IGVMfile from DLL");


nit: this seems redundant

tjones60 · 2026-01-22T21:44:29Z

vm/vmgs/vmgstool/src/main.rs

    /// `keypath` and `encryptionalgorithm` must both be specified if encrypted
    /// guest state is required.
    Create {
+        /// VMGS file path


Why was this added? FilePathArg should contain the appropriate doc comment used in the help message.

tjones60 · 2026-01-22T21:45:32Z

Guide/src/dev_guide/dev_tools/vmgstool.md

+### Read DLL File to Write IGVMfile to VMGS
+
+Additionally, the VmgsTool contains a tool to read the IGVMfile from a DLL (passed in as a data file)
+and write it to VMGS FileId 8 (GUEST_FIRMWARE). To do this pass one of three resource codes


...five resource codes... ?

tjones60 · 2026-01-22T21:48:14Z

vm/vmgs/vmgstool/src/main.rs


+fn parse_resource_code(resource_code: &str) -> Result<ResourceCode, &'static str> {
+    match resource_code {
+        "nonconfidential" => Ok(ResourceCode::NonConfidential),


nit: All the other values use all caps, would it be ok to be consistent here and use all caps for the resource codes as well? We should probably just make it case insensitive, but I'd rather be consistent for now.

Actually, since this isn't an OpenEnum used elsewhere, we can probably just have clap auto generate the help message and parsing function (#[derive(clap::ValueEnum)]). It wouldn't be consistent with the other ones (which are used in data structures that rely on their format), but would prevent us from having extra boilerplate.

tjones60 · 2026-01-22T21:55:24Z

vm/vmgs/vmgstool/src/main.rs

 }

+#[derive(Debug, Clone, Copy)]
+#[repr(i32)]


DllResourceDescriptor::new accepts a u32, but here you are saying it is an i32. Shouldn't they be the same?

tjones60 · 2026-01-22T21:57:14Z

vm/vmgs/vmgstool/src/main.rs

+) -> Result<(), Error> {
+    eprintln!("Writing IGVMfile to VMGS");
+
+    let encrypt = key_path.is_some();


Why would we ever want to encrypt the IGVM file? Would it ever contain user data?

That was a holdover, I changed it in the Host Agent version, but I guess Heather didn't get a chance to change it here. I'll update

hgarvison requested a review from a team as a code owner December 15, 2025 20:24

Copilot AI review requested due to automatic review settings December 15, 2025 20:24

hgarvison requested a review from a team as a code owner December 15, 2025 20:24

github-actions bot added Guide unsafe Related to unsafe code labels Dec 15, 2025

Copilot AI reviewed Dec 15, 2025

View reviewed changes

smalis-msft reviewed Dec 17, 2025

View reviewed changes

vm/vmgs/vmgstool/src/main.rs Outdated Show resolved Hide resolved

add igvmfile read/write to vmgstool

a684b8d

SethHollandsworth force-pushed the main branch from 86213db to a684b8d Compare January 9, 2026 14:20

smalis-msft reviewed Jan 9, 2026

View reviewed changes

openvmm/openvmm_pcat_locator/src/lib.rs Outdated Show resolved Hide resolved

abstracting out the dll parser into its own package to be shared

302cd47

SethHollandsworth mentioned this pull request Jan 9, 2026

abstracting out the dll parser into its own package to be shared hgarvison/openvmm#5

Merged

updating pcat_locator

2f976d6

tjones60 reviewed Jan 9, 2026

View reviewed changes

vm/vmgs/vmgstool/src/main.rs Outdated Show resolved Hide resolved

SethHollandsworth and others added 6 commits January 9, 2026 14:14

fixing styling

2d93b3a

fixing module docs

1fcef7c

fixing styling again

73c6a02

telling rust_doc not to run example

3bc56b8

Merge branch 'microsoft:main' into main

4d3a403

Merge branch 'microsoft:main' into main

93aef2d

smalis-msft reviewed Jan 13, 2026

View reviewed changes

openvmm/openvmm_pcat_locator/src/lib.rs Outdated Show resolved Hide resolved

smalis-msft reviewed Jan 13, 2026

View reviewed changes

support/resource_dll_parser/src/lib.rs Show resolved Hide resolved

smalis-msft reviewed Jan 13, 2026

View reviewed changes

SethHollandsworth and others added 3 commits January 13, 2026 10:56

converting type to use

18c44f6

getting rid of cfgs on new vmgstool command

e98f7a2

Merge branch 'microsoft:main' into main

43f8daf

SethHollandsworth and others added 4 commits January 14, 2026 10:01

making functions OS agnostic

56dcfd7

styling fixes

1dbb5b0

Merge branch 'microsoft:main' into main

8934788

Merge branch 'microsoft:main' into main

947c2cd

hgarvison added 5 commits January 16, 2026 16:50

Merge branch 'microsoft:main' into main

482ea1e

Merge branch 'microsoft:main' into main

f11bcbd

Merge branch 'microsoft:main' into main

40ea28c

Merge branch 'microsoft:main' into main

7508bc2

Merge branch 'microsoft:main' into main

dea7df5

tjones60 reviewed Jan 22, 2026

View reviewed changes

SethHollandsworth mentioned this pull request Jan 22, 2026

refactoring for comments from upstream hgarvison/openvmm#7

Merged

tjones60 reviewed Jan 22, 2026

View reviewed changes

SethHollandsworth added 7 commits January 26, 2026 11:19

refactoring for comments

40d3972

changing test to create test vmgs file dynamically

9c68463

formatting

0e50bec

changing error formatting and resource code casing

0697bbc

updating to get rid of encryption for igvm file

084dcbf

formatting change

e5f35c0

using clap ValueEnum instead of boilerplate

a858069

add igvmfile read/write to vmgstool #2566

Are you sure you want to change the base?

add igvmfile read/write to vmgstool #2566

Uh oh!

Conversation

hgarvison commented Dec 15, 2025

Uh oh!

github-actions bot commented Dec 15, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 16, 2025

Uh oh!

github-actions bot commented Dec 16, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smalis-msft left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone