Add support for TDG.PAGE.RELEASE when TDXConnect is in use.#3748
Open
Raz-Aloni wants to merge 16 commits into
Open
Add support for TDG.PAGE.RELEASE when TDXConnect is in use.#3748Raz-Aloni wants to merge 16 commits into
Raz-Aloni wants to merge 16 commits into
Conversation
Per Intel guidance, TDG.VM.RD on the TD-scope CONFIG_FLAGS field is sufficient to detect TDX Connect / page-release support, so the TDG.SYS.RD path (and the global-scope TDX_FEATURES0 metadata) is no longer needed. Remove the SYS.RD leaf, the tdcall_sys_rd helper, TdFeatures0, TdgSysRdResult, TDX_FIELD_CODE_TDX_FEATURES0, and all associated callers in openhcl_boot, hcl, and underhill_mem. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
|
@Raz-Aloni please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for TDX Connect–related behavior by introducing TD configuration flag retrieval (TDG.VM.RD) and implementing page release (TDG.MEM.PAGE.RELEASE) plumbing, then wiring page release into TDX page-visibility transitions.
Changes:
- Add new TDX definitions for CONFIG_FLAGS metadata and TDG.MEM.PAGE.RELEASE inputs/outputs.
- Implement TDG.MEM.PAGE.RELEASE + range helper and TDG.VM.RD (CONFIG_FLAGS parsing) in
tdcall. - Use CONFIG_FLAGS to conditionally require page release before making pages host-visible.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| vm/x86/x86defs/src/tdx.rs | Adds leaf/metadata constants and bitfield structs for config flags + page release. |
| vm/x86/tdcall/src/lib.rs | Implements TDG.MEM.PAGE.RELEASE, range release helper, and TDG.VM.RD for CONFIG_FLAGS. |
| openhcl/underhill_mem/src/lib.rs | Conditionally calls TDX page release when unaccepting pages under TDX Connect. |
| openhcl/openhcl_boot/src/arch/x86_64/tdx.rs | Calls page release before host-visible mapping when TDX Connect is enabled and caches CONFIG_FLAGS. |
| openhcl/hcl/src/ioctl/tdx.rs | Exposes VTL helpers for config flag read and page release TDCalls. |
| pub level: TdgMemPageLevel, | ||
| #[bits(9)] | ||
| pub reserved: u64, | ||
| /// The page number for this accept call. |
| pub level: TdgMemPageLevel, | ||
| #[bits(9)] | ||
| pub reserved: u64, | ||
| /// The page number for this accept call. |
Comment on lines
+198
to
+199
| #[bits(1)] | ||
| pub reserved3: u8, |
Comment on lines
+357
to
+358
| #[error("page size mismatch. Expected size {1:?}: {0:?}")] | ||
| PageSizeMismatch(TdCallResultCode, TdgMemPageLevel), |
Comment on lines
+243
to
+248
| let config_flags = mshv_vtl.tdx_get_config_flags(); | ||
| if config_flags.tdx_connect() { | ||
| assert!(config_flags.page_release()); | ||
|
|
||
| flags.tdx_page_release_required = true; | ||
| } |
Comment on lines
+312
to
+316
| if self.flags.tdx_page_release_required { | ||
| self.mshv_vtl | ||
| .tdx_release_pages(range) | ||
| .expect("Failed to release pages"); | ||
| } |
Member
There was a problem hiding this comment.
no this should be expect. but you should put the range of the page that failed to release (or what page it was) as part of the expect failure
Comment on lines
+136
to
+138
| // If TDX Connect is present, then TDG.MEM.PAGE.RELEASE must be called before making pages host-visible. | ||
| if host_visible && get_td_config_flags().tdx_connect() { | ||
| assert!(get_td_config_flags().page_release()); |
smalis-msft
reviewed
Jun 15, 2026
| && range.len() >= x86defs::X64_LARGE_PAGE_SIZE | ||
| { | ||
| if let Err(e) = tdcall_release_page(call, range.start_4k_gpn(), true) { | ||
| match e { |
Contributor
There was a problem hiding this comment.
Since we're only checking for one variant I feel like this would read cleaner as an if let instead of a match.
smalis-msft
reviewed
Jun 15, 2026
| pub fn tdcall_vm_rd( | ||
| call: &mut impl Tdcall, | ||
| field_id: TdxExtendedFieldCode, | ||
| ) -> Result<TdgVmRdResult, TdCallResult> { |
Contributor
There was a problem hiding this comment.
Would it be cleaner to just return r8 and let the caller construct the enum? That way all callers of this method wouldn't have to worry about the case of getting the wrong output type somehow. Maybe we could even have each caller pass in their expected field_id, and this method could turn a mismatch into an error/panic.
Member
There was a problem hiding this comment.
some kind of enum or generic function? I do agree that the fact that callers have to do this unreachable! dance means this API is wrong.
chris-oo
reviewed
Jun 18, 2026
chris-oo
left a comment
chris-oo
left a comment
Member
There was a problem hiding this comment.
see feedback. what is the timeline for us to have this path tested in ci?
| } | ||
|
|
||
| #[bitfield(u64)] | ||
| pub struct TdConfigFlags { |
Member
There was a problem hiding this comment.
can we link to all the defns here to the corresponding defn in intel's specifications?
| pub fn tdcall_vm_rd( | ||
| call: &mut impl Tdcall, | ||
| field_id: TdxExtendedFieldCode, | ||
| ) -> Result<TdgVmRdResult, TdCallResult> { |
Member
There was a problem hiding this comment.
some kind of enum or generic function? I do agree that the fact that callers have to do this unreachable! dance means this API is wrong.
|
|
||
| let mut range = range; | ||
| while !range.is_empty() { | ||
| // Attempt to release in large page chunks, if possible. |
Member
There was a problem hiding this comment.
don't we do this pattern somewhere else in the file? should we have a common fn op for this?
Comment on lines
+312
to
+316
| if self.flags.tdx_page_release_required { | ||
| self.mshv_vtl | ||
| .tdx_release_pages(range) | ||
| .expect("Failed to release pages"); | ||
| } |
Member
There was a problem hiding this comment.
no this should be expect. but you should put the range of the page that failed to release (or what page it was) as part of the expect failure
| let mut flags = MemoryAcceptorFlags::default(); | ||
|
|
||
| if isolation == IsolationType::Tdx { | ||
| // Check if TDX Connect is enabled on this TD. If so, page release is required when |
Member
There was a problem hiding this comment.
why do we do this, instead of just checking for page release?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In TDX, on TDs where TDX Connect is enabled, private pages must be explicitly released via TDG.MEM.PAGE.RELEASE before they can be made shared/host-visible, otherwise VMM calls to TDH.MEM.RANGE.BLOCK will fail.
This change modifies both openhcl and openhcl_boot to add a query for TDX Connect enablement on the current TD, and if enabled, TDG.MEM.PAGE.RELEASE calls will be made. TDs without TDXConnect have unchanged behavior.