add igvmfile read/write to vmgstool #2566
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.
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-igvmfilecommand 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 |
SethHollandsworth
force-pushed
the
main
branch
from
86213db to
a684b8d
Compare
vm/vmgs/vmgstool/src/main.rs
Outdated
| GspUnknown, | ||
| #[error("VMGS file is using an unknown encryption algorithm")] | ||
| EncryptionUnknown, | ||
| #[cfg(all(windows, guest_arch = "x86_64"))] |
There was a problem hiding this comment.
What makes any of this x86_64 specific?
There was a problem hiding this comment.
The file this command is being used to parse (vmfirmwarehcl.dll) doesn't exist in ARM.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| /// Tries to read the given resource from a resource dll. If the given data | ||
| /// buffer is not a valid PE file this function returns Ok(None). If it is a PE | ||
| /// file, but the given resource can not be found or loaded this function | ||
| /// returns Err(...). On success the return value contains the starting offset |
There was a problem hiding this comment.
We should probably change the return types to a proper enum with variants like 'NotPeFile, NotFound, Ok(u64, usize)' or something, but that would break most of the ? i think, so feel free to just add a TODO comment instead.
smalis-msft
left a comment
smalis-msft
left a comment
There was a problem hiding this comment.
Non-vmgs portions look good to me, I'll let @tjones60 do the final signoff
| if cfg!(feature = "encryption_ossl") || cfg!(feature = "encryption_win") { | ||
| println!("cargo:rustc-cfg=with_encryption") | ||
| } | ||
| build_rs_guest_arch::emit_guest_arch() |
There was a problem hiding this comment.
I think this is no longer needed now?
There was a problem hiding this comment.
We still use guest_arch so the ARM tests pass while looking for a Windows & x86 specific file: https://github.com/microsoft/openvmm/pull/2566/changes#diff-5a13f8ed20df6cbedce19fb03d00c9d139cf36b9f16de65e6d6691dfa8dbf5a1R1447
There was a problem hiding this comment.
I'm not sure how I feel about a test that's relying on a system file like that... @tjones60 thoughts?
vm/vmgs/vmgstool/src/main.rs
Outdated
| /// Copy the IGVM file from a dll into file ID 8 of the VMGS file. | ||
| /// | ||
| /// The proper key file must be specified to write encrypted data. | ||
| #[cfg(windows)] |
There was a problem hiding this comment.
Again why cfg? One of the benefits of using object as a parser is that it can do so on any platform.
There was a problem hiding this comment.
fixed, we were using Windows-specific paths but I made it use PathBufs instead. Still learning rust 😅
5 participants
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