-
Notifications
You must be signed in to change notification settings - Fork 6
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
Adding Coconut-SVSM support #19
base: main
Are you sure you want to change the base?
Conversation
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.
There are a number of points needing adjustment. The biggest one is this PR needs to be re-based on upstream. Please adjust and re-request review.
9aad362
to
2332446
Compare
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.
Good fixes! This is what we were looking for per our previous discussions, just a couple more things to adjust.
Please fix the boolean logic for the svsm flag.
Also your verify measurement is adding repetitive code.
You will also need to rebase again because there were some prs merged yesterday, your code is behind.
To ensure that SNP is given priority over SVSM and that any SVSM-specific code changes fall back to the else condition, . @larrydewey , I rebased my fork branch and submitted the commit earlier. Now, I have rebased my fork's main branch and then rebased and submitted the changes. This resolves most of your suggestions. |
Every time you are doing SNP workflows you are checking to see if SVSM is True, which in itself is confusing because it reads like SVSM is "enabled" let met do the SNP only workflows. This doesn't "give priority to SNP". If SVSM is set to true you can do whatever small workflow you need for an svsm and if its false you can default to SNP. It reads better and it will make it easier to consolidate your logic. Or at least that's how I see it. @larrydewey @ryansavino idk what you guys think about that. |
I am thinking of adding SVSM="${SVSM:-false}", or using the function below: |
There's a lot of comments here that need to be addressed. Please try to clean things up, and then I'll a look at the rest of the PR to provide my review. |
54a5f26
to
bb710f8
Compare
3024550
to
47e28f2
Compare
6a34b8a
to
630b85c
Compare
Does this pull request prepare both host and guest kernel for svsm support or only it prepares the host kernel? |
Yes, it prepares both host and guest kernel build's of svsm. |
…t, launching the guest (directly setting up snpguest in the guest VM), and performing attestation & measurement verification (using igvmmeasure vs snpguest)
082727a
to
296b43a
Compare
296b43a
to
5b819ba
Compare
5b819ba
to
c91431e
Compare
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 this addressed my concerns. @ryansavino and @DGonzalezVillal What are your thoughts?
I tried the PR and this what I get, after setting up the host and doing reboot:
Same error I get when setting up the SVSM directly from coconut svsm github repo. |
@pegahnikbakht , Could you please open a new issue so we can discuss and provide suggestions for your setup problems? I don't see the same issue you're experiencing with the PR; it seems more related to your setup and installation |
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.
Only a couple of suggestions and one general question, but overall I'm good with how it looks now!
@@ -116,6 +119,7 @@ usage() { | |||
>&2 echo " stop-guests Stop all SNP guests started by this script" | |||
>&2 echo " where OPTIONS are:" | |||
>&2 echo " -n|--non-upm Build AMDSEV non UPM kernel (sev-snp-devel)" | |||
>&2 echo " -s|--svsm Build coconut-svsm components, launch guest and verify attestation & measurement" |
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.
>&2 echo " -s|--svsm Build coconut-svsm components, launch guest and verify attestation & measurement" | |
>&2 echo " -s|--svsm Build, launch and attest coconut-svsm" |
if $SVSM; then | ||
sudo apt install -y libcunit1 libcunit1-dev build-essential libclang-dev autoconf \ | ||
autoconf-archive pkg-config automake libssl-dev libc6-dev gcc-multilib binutils make musl musl-tools | ||
fi |
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.
Some of these are already being installed (build-essential, libssl-dev, pkg-config)
if "$SVSM"; then | ||
cargo build --release --target x86_64-unknown-linux-musl | ||
scp_guest_command target/x86_64-unknown-linux-musl/release/snpguest "${GUEST_USER}@localhost:/home/${GUEST_USER}" | ||
else | ||
cargo build -r | ||
scp_guest_command target/release/snpguest "${GUEST_USER}@localhost:/home/${GUEST_USER}" | ||
fi |
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.
@larrydewey & @ryansavino Not the biggest deal, but Rama had to add this due to dependency differences in Ubuntu 22.04 and 24.04. Do we want to default to musl builds? If I'm not mistaken this will happen to anyone that has a 24.04 host that tries to use the script to launch a 22.04 guest. It's unrelated to svsm.
Oh also I just thought of this, but maybe update the snp.md documentation with updated instructions to use the SVSM flag. Or do you want that to be a separate PR @ryansavino |
Coconut-SVSM support leverages AMD-ES/AMD-SEV for setting up the host, launching the guest VM (by directly setting up snpguest within the guest VM), and performing attestation and measurement verification (using igvmmeasure versus snpguest).