Skip to content
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.

Sign up for GitHub

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

Jump to bottom

vmsa: Updating for kernel debug_swap #32

Merged
merged 1 commit into from
Feb 2, 2024
Merged

vmsa: Updating for kernel debug_swap #32

merged 1 commit into from
Feb 2, 2024

Conversation

larrydewey
Copy link
Collaborator

The upstream host kernel patches have a new
debug_swap feature which modifies the initial
state each vCPU's VMSA page, which may affect
the generated measurement. This change should
address the requirements introduced by the
upstream kernel change.

Resolves: #195
Resolves: #30

@larrydewey larrydewey added the bug Something isn't working label Nov 13, 2023
@larrydewey larrydewey self-assigned this Nov 13, 2023
@auto-assign auto-assign bot requested a review from slp November 13, 2023 19:49
@dubek
Copy link
Member

dubek commented Nov 14, 2023

Thanks @larrydewey . The fix itself looks fine, but raises a few questions:

  1. Can you please link to the kernel commit that modified this value?
  2. Unit-tests are now (obviously) broken.
  3. Do we want to mandate use of kernel >= X.Y.Z (whichever version that added debug_swap upstream) when sev-snp-measure is used? If we want, this should be documented. @tylerfanelli @fitzthum What's you opinion here?

@larrydewey larrydewey changed the title vmsa: Updating for kernel debug_swap WIP: vmsa: Updating for kernel debug_swap Nov 14, 2023
@larrydewey
Copy link
Collaborator Author

Thanks @larrydewey . The fix itself looks fine, but raises a few questions:

  1. Can you please link to the kernel commit that modified this value?

It looks like it was introduced with commit d1f85fbe836e6 by Alexey Kardashevskiy.

  1. Unit-tests are now (obviously) broken.

Excellent point! I forgot to add a WIP to the title of this PR. The tests definitely need to be updated. I need to figure out exactly how we can test for this change, though.

  1. Do we want to mandate use of kernel >= X.Y.Z (whichever version that added debug_swap upstream) when sev-snp-measure is used? If we want, this should be documented. @tylerfanelli @fitzthum What's you opinion here?

I think this would be a good approach. An alternative may be introducing logic to cover both cases? Other opinions are definitely welcome.

@larrydewey
Copy link
Collaborator Author

larrydewey commented Nov 14, 2023 •
edited
Loading

Upon further review, it sounds like this functionality will vary based upon whether or not this feature has been enabled. I think it would make the most sense here to add a new command-line argument to compensate for debug_swap being enabled or not.

@dubek
Copy link
Member

dubek commented Nov 14, 2023

From reading the kernel commit d1f85fbe836e6 , I understand that the new debug_swap bit is enabled by default (in kernel >= 6.6, and if the hardware supports it) but can be disabled with kernel command-line kvm_amd.sev_es_debug_swap_enabled=0(but I'm not sure, maybe I mis-read the code).

@dubek dubek mentioned this pull request Jan 24, 2024
Closed
@larrydewey larrydewey force-pushed the debugswap branch 5 times, most recently from cea96c4 to b2eb21c Compare January 29, 2024 19:29
@larrydewey
Copy link
Collaborator Author

larrydewey commented Jan 29, 2024 •
edited
Loading

@dubek I need to update the test cases on this, but do you have any thoughts/feelings on the logic? I just re-based on the latest changes, so I think this should be good to go after the tests are updated.

@dubek I re-based off the latest upstream code, and added in the tests. Do you have any concerns with the following code?

@larrydewey larrydewey force-pushed the debugswap branch 2 times, most recently from d1eb1cf to e687a65 Compare January 29, 2024 20:45
@larrydewey larrydewey changed the title WIP: vmsa: Updating for kernel debug_swap vmsa: Updating for kernel debug_swap Jan 29, 2024
@larrydewey larrydewey force-pushed the debugswap branch 6 times, most recently from 8e540a9 to c9674e2 Compare January 29, 2024 22:57
tylerfanelli
tylerfanelli approved these changes Jan 30, 2024
View reviewed changes
Copy link
Member

@tylerfanelli tylerfanelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for updating the tests.

@tylerfanelli
Copy link
Member

  1. Do we want to mandate use of kernel >= X.Y.Z (whichever version that added debug_swap upstream) when sev-snp-measure is used? If we want, this should be documented. @tylerfanelli @fitzthum What's you opinion here?

Agreed, especially if the feature can be turned off (which also should be documented if so).

dubek
dubek reviewed Jan 30, 2024
View reviewed changes
Copy link
Member

@dubek dubek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @larrydewey and @tylerfanelli . See my comments below.

sevsnpmeasure/vmsa.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
sevsnpmeasure/cli.py Outdated Show resolved Hide resolved
@larrydewey
vmsa: Updating for kernel debug_swap
84dff98 
The upstream host kernel patches have a new
`debug_swap` feature which modifies the initial
state each vCPU's VMSA page, which may affect
the generated measurement. This change should
address the requirements introduced by the
upstream kernel change.

*EDIT*:
- Added flake8 required changes
- Updated test cases.
- Addressing PR requests

Signed-off-by: Larry Dewey <[email protected]>
@larrydewey
Copy link
Collaborator Author

@dubek @tylerfanelli would you mind performing another review?

dubek
dubek approved these changes Jan 31, 2024
View reviewed changes
Copy link
Member

@dubek dubek left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @larrydewey , this looks good to me.

I think I'll release a new 0.0.8 today before merging this, and then release 0.0.9 with this change. This way, if users want the previous behaviour (and don't want to add --guest-features=0x1) they can pip install sev-snp-measure==0.0.8

@larrydewey @tylerfanelli Does that sound fine to you?

@tylerfanelli
Copy link
Member

@dubek Sounds good to me. I won't merge right now, as to give you the opportunity to release 0.0.8. Thanks!

@dubek dubek merged commit e4d7586 into virtee:main Feb 2, 2024
8 checks passed
@dubek
Copy link
Member

dubek commented Feb 2, 2024

Merged. Thanks @larrydewey . I'll add a notice to the changelog that this is a breaking change (for kernel < 6.6).

@larrydewey larrydewey deleted the debugswap branch February 2, 2024 04:07
derpsteb added a commit to edgelesssys/sev-snp-measure-go that referenced this pull request Feb 6, 2024
@derpsteb
Add guestFeatures as arg for public functions
311dc1d 
This implements the changes from upstream:
virtee/sev-snp-measure#32
derpsteb added a commit to edgelesssys/sev-snp-measure-go that referenced this pull request Feb 6, 2024
@derpsteb
Add guestFeatures as arg for public functions
634576f 
This implements the changes from upstream:
virtee/sev-snp-measure#32
derpsteb added a commit to edgelesssys/sev-snp-measure-go that referenced this pull request Feb 6, 2024
@derpsteb
Add guestFeatures as arg for public functions
938edb9 
This implements the changes from upstream:
virtee/sev-snp-measure#32
derpsteb added a commit to edgelesssys/sev-snp-measure-go that referenced this pull request Feb 6, 2024
@derpsteb
Add guestFeatures as arg for public functions
b0e59fb 
This implements the changes from upstream:
virtee/sev-snp-measure#32
derpsteb added a commit to virtee/sev-snp-measure-go that referenced this pull request Feb 6, 2024
@derpsteb
Add guestFeatures as arg for public functions
1d37c07 
This implements the changes from upstream:
virtee/sev-snp-measure#32
@derpsteb derpsteb mentioned this pull request Feb 6, 2024
Merged
derpsteb added a commit to virtee/sev-snp-measure-go that referenced this pull request Feb 6, 2024
@derpsteb
Add guestFeatures as arg for public functions
1021d87 
This implements the changes from upstream:
virtee/sev-snp-measure#32

Signed-off-by: Otto Bittner <[email protected]>
derpsteb added a commit to virtee/sev-snp-measure-go that referenced this pull request Feb 6, 2024
@derpsteb
Add guestFeatures as arg for public functions
355bacc 
This implements the changes from upstream:
virtee/sev-snp-measure#32

Signed-off-by: Otto Bittner <[email protected]>
derpsteb added a commit to virtee/sev-snp-measure-go that referenced this pull request Feb 7, 2024
@derpsteb
Add guestFeatures as arg for public functions
2b213b4 
This implements the changes from upstream:
virtee/sev-snp-measure#32

Signed-off-by: Otto Bittner <[email protected]>
@derpsteb derpsteb mentioned this pull request Feb 8, 2024
Merged
derpsteb added a commit to edgelesssys/sev-snp-measure-go that referenced this pull request Feb 8, 2024
@derpsteb
Add guestFeatures as arg for public functions
560a60b 
This implements the changes from upstream:
virtee/sev-snp-measure#32
@dubek dubek mentioned this pull request Mar 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dubek dubek dubek approved these changes

@tylerfanelli tylerfanelli tylerfanelli approved these changes

@fitzthum fitzthum Awaiting requested review from fitzthum

@DGonzalezVillal DGonzalezVillal Awaiting requested review from DGonzalezVillal

@slp slp Awaiting requested review from slp

Assignees

@larrydewey larrydewey

@tylerfanelli tylerfanelli

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
4 participants
@larrydewey @dubek @tylerfanelli @ryansavino