Fix: Update Config structures to accurately reflect TCBs#314
Merged
Conversation
DGonzalezVillal
requested review from
larrydewey and
tylerfanelli
as code owners
DGonzalezVillal
force-pushed
the
commit-fix
branch
4 times, most recently
from
81ffbc1 to
d6c7866
Compare
tylerfanelli
requested changes
Jul 18, 2025
DGonzalezVillal
force-pushed
the
commit-fix
branch
3 times, most recently
from
f4afa2f to
ab943c0
Compare
Member
Author
|
@tylerfanelli Returned it to original api functionality, let me know how it looks |
tylerfanelli
requested changes
Jul 22, 2025
DGonzalezVillal
changed the title
DGonzalezVillal
changed the title
DGonzalezVillal
force-pushed
the
commit-fix
branch
from
ab943c0 to
ad66ab1
Compare
When we updated the TCB structure, we overlooked modifying the commit-related structures to account for differences in TCB layout across CPU generations. This commit ensures that raw TCB bytes are correctly constructed for the FFI layer, and that conversions from the user-level structure to the FFI format use the appropriate TCB structure based on the processor generation. The API snp_set_config function remains the same, it will tell the CPU generation using a CPUID call. Signed-off-by: DGonzalezVillal <Diego.GonzalezVillalobos@amd.com>
DGonzalezVillal
force-pushed
the
commit-fix
branch
from
ad66ab1 to
2d1b3fc
Compare
|
|
||
| #[cfg(feature = "snp")] | ||
| /// TryFrom to FFI Config when manually passing in the CPU generation | ||
| impl TryFrom<(Config, Generation)> for FFI::types::SnpSetConfig { |
Member
Author
There was a problem hiding this comment.
I created both implementations in the previous version of the commit: one where we don’t retrieve the CPU generation automatically, and the user provides it manually. I kept this implementation in case we ever want to change the API or if a user wants to create configurations for hosts that are not their current machine.
I'm not sure if these are reasonable use cases. This implementation is currently unused in the library—if you think it’s unnecessary, I’m happy to remove it.
Member
There was a problem hiding this comment.
Ok, I guess it doesn't hurt to include.
|
|
||
| #[cfg(feature = "snp")] | ||
| /// TryFrom from FFI Config type when CPU Generation is manually passed in | ||
| impl TryFrom<(FFI::types::SnpSetConfig, Generation)> for Config { |
Member
There was a problem hiding this comment.
This as well. In what context is this used?
tylerfanelli
approved these changes
Jul 28, 2025
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
3 participants
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.
When we updated the TCB structure, we overlooked modifying the config-related structures to account for differences in TCB layout across CPU generations.
This commit ensures that raw TCB bytes are correctly constructed for the FFI layer, and that conversions from the user-level structure to the FFI format use the appropriate TCB structure based on the processor generation.