-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SEV generation string handling #133
Conversation
bc5c3da
to
a6ddccc
Compare
src/firmware/linux/host/types/snp.rs
Outdated
@@ -108,7 +108,7 @@ impl CertTableEntry { | |||
/// ``` | |||
/// | |||
#[cfg(target_os = "linux")] | |||
pub fn uapi_to_vec_bytes(table: &Vec<UAPI::CertTableEntry>) -> Result<Vec<u8>, CertError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clippy
suggested this change. Of course we want to follow its suggestions mostly, but this would break API... drop it for now? It provides no functional change and isn't exactly necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the change should be fine, but if it is breaking API, maybe make it part of a different PR?
src/firmware/linux/host/types/snp.rs
Outdated
@@ -108,7 +108,7 @@ impl CertTableEntry { | |||
/// ``` | |||
/// | |||
#[cfg(target_os = "linux")] | |||
pub fn uapi_to_vec_bytes(table: &Vec<UAPI::CertTableEntry>) -> Result<Vec<u8>, CertError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the change should be fine, but if it is breaking API, maybe make it part of a different PR?
Everything else looks good to me |
94fc335
to
ce999c7
Compare
@DGonzalezVillal I removed the commit as I would like this merged without needing a major release. We can make the API breaking change on the next major release. |
@tylerfanelli Where we will be doing a major release anyway because of the kernel changes, should we look at just adding this in? @DGonzalezVillal is working on a PR to remove the unnecessary methods/functions which are no longer needed. |
@larrydewey By "this change", you're talking about this correct? |
I was referring more to your PR. |
@tylerfanelli Can you rebase and add support for Bergamo and Siena? |
Sure, will do. |
At times (most notably in attestation scenarios), the SEV generation in which to attest with is specified via strings. Add a helper method to parse the SEV generation from a given string. Signed-off-by: Tyler Fanelli <[email protected]>
At times (notably during attestation), it is required to fetch certificates from AMD's KDS. To do this, a title-cased string of the current SEV generation needs to be specified. Create a helper method to marshal this string. Signed-off-by: Tyler Fanelli <[email protected]>
Signed-off-by: Tyler Fanelli <[email protected]>
Signed-off-by: Tyler Fanelli <[email protected]>
Signed-off-by: Tyler Fanelli <[email protected]>
ce999c7
to
c695448
Compare
@larrydewey Added Bergamo + Siena support. |
@DGonzalezVillal can you review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
No description provided.