feat(sev): add AMD SEV support #542

zyuiop · 2025-04-24T12:37:13Z

Currently, this library's page table features does not work on an AMD encrypted memory system.

This is because AMD-SME and AMD-SEV use one physical address bit to map the encryption status in the page table entry.
The position of this bit is not known at compile time and must be determined at runtime.
We therefore need to know before using any page table feature where this bit is so that we can remove it from physical addresses when using page table entries.

My approach is as follows:

Modify the PageTableFlags structure to allow any unknown bits ;
Define a static mask for the physical address bits, that can be (unsafely) modified, and use that to determine the physical address in a page table entry ;
Add a amd_sev module with an init function that detects the presence of an memory encryption features, and if so, (unsafely) modifies the physical address mask.

Someone implementing an operating system designed to work under AMD memory encryption features would need to call amd_sev::init before doing any page table operations.

I am not 100% confident my approach, in particular unsafely modifying static variables, so I gladly take feedback on this PR.

Freax13

Thank you for your contribution!

For my own curiosity: What's your intended use case for this? Are you using SEV or SEV-SNP?

src/structures/amd_sev.rs

Freax13 · 2025-04-26T05:06:00Z

Cargo.toml

@@ -26,8 +26,10 @@ volatile = "0.4.4"
 rustversion = "1.0.5"

 [features]
-default = ["nightly", "instructions"]
+default = ["nightly", "instructions", "amd_sev"]


I'm not sure if we should enable this by default. Most users won't be running their code in an SEV VM, and there's likely a non-zero performance impact.

People have also been using this crate with AMD SEV-SNP and Intel TDX for a while, even without explicit support in this crate, and we don't want to accidentally break their code. In practice, on all currently available platforms, the C-bit/S-bit is always an address bit (I think this is even guaranteed on Intel TDX). People have been considering it as such and not a page table bit, so they're just setting the bit in PhysAddr/PhysFrame directly.

You're right - it should not be enabled by default, I will fix.

The reason I'm making this patch is that I tried running an unmodified Hermit OS in an QEMU SEV VM, and it did not work, because of the c-bit.
In practice, I could retract the patch and do what you suggest with physical address bits in Hermit directly.

Hermit OS will need to be modified anyway; SEV/SEV-ES/SEV-SNP/TDX all require kernel modifications.

FYI: At Hermit we are also working on SEV support, but as it indeed requires numerous kernel modifications, this is not (yet) ready for publication. We also made our own quick-and-dirty changes to this crate - not too different from what is proposed here, but also not yet ready and generic. So a clean and upstream version of SEV support in this crate would be much appreciated 👍

That's great to hear. Out of curiosity, what generation are you targeting, SEV, SEV-ES, or SEV-SNP?

FYI: At Hermit we are also working on SEV support, but as it indeed requires numerous kernel modifications, this is not (yet) ready for publication. We also made our own quick-and-dirty changes to this crate - not too different from what is proposed here, but also not yet ready and generic. So a clean and upstream version of SEV support in this crate would be much appreciated 👍

Oh, is there any way we could get in touch? I should probably have started with that, but better late than never.

That's great to hear. Out of curiosity, what generation are you targeting, SEV, SEV-ES, or SEV-SNP?

Well, SEV without SNP doesn't make much sense anymore, right? 😁

Oh, is there any way we could get in touch? I should probably have started with that, but better late than never.

Sure, just drop us a mail 😉

src/structures/amd_sev.rs

src/structures/paging/page_table.rs

zyuiop · 2025-04-28T08:59:21Z

Thanks a lot for your review, I will submit a modified patch as soon as possible.

I was trying to run Hermit-OS in an AMD SEV environment, and was quickly blocked by an error as addresses were not canonical because of the C bit. I found this way to solve this, although you're right that it may be simpler to leave that to the OS.

I am indeed targeting SEV-ES and SEV-SNP, and I have written a VC handler as well as structures for the GHCB protocol, but I am not sure it would be a good idea to put them in this library - I feel like a separate library would be better as most x86_64 users are not interested by this feature.

x86_64/sev: remove SEV detection, replace with a simpler c-bit defining routine

Freax13 · 2025-04-29T19:14:17Z

[...] as well as structures for the GHCB protocol, but I am not sure it would be a good idea to put them in this library - I feel like a separate library would be better as most x86_64 users are not interested by this feature.

Yeah, I agree with that, the GHCB spec isn't really an architectural thing by rather a standard for compliant guests and hypervisors to communicate (like a hypercall interface). There's nothing in the CPU that enforces (or even directly interacts) with any of it.

That said, if you wanted to contribute some of the more architectural stuff (e.g. the sev status MSR, the GHCB MSR, pvalidate, rmpadjust wrappers), we'd be very welcome to that. No pressure, though.

Freax13

I like the direction in which this is heading.

src/structures/mem_encrypt.rs

Freax13 · 2025-04-29T18:55:25Z

src/structures/paging/page_table.rs

+/// The mask used to remove flags from a page table entry to obtain the physical address
+#[cfg(feature = "dynamic_flags")]
+pub(crate) static PHYSICAL_ADDRESS_MASK: AtomicU64 = AtomicU64::new(0x000f_ffff_ffff_f000u64);


Given that this is currently only used for an encryption bit, let's not over-engineer things for now and just have the memory_encryption feature and remove the dynamic_flags feature.

I'm also not sure if the dynamic_flags feature is the way to go. It has the risk of feature-explosion, but without too much knowledge about the x86_64 codebase, I feel like defining the bit via a cargo feature would be the way to go. I mean, the flag does not change during runtime, and the global state is a little error-prone.

The bit doesn't change at runtime, but it also can't be known ahead of time. Different CPU generations use different positions for the C-bit (e.g. bit 47 for EPYC Milan and bit 51 for EPYC Genoa and Turin). With Intel TDX, the S-bit position is configurable, it's either bit 47 or 51.

Hard-coding the bit position would certainly make things easier and possibly improve performance, but it comes at the cost of compatibility.

I completely see that point, and I'm not 100% sure what is the correct approach here. My argument would be, that if you use one of these memory encryption technologies, you'll have to customize the code to that specific architecture anyway. Ok, not necessarily for the encryption of the page, but basically for everything else.

Maybe a combination of both approaches could be a solution: Have a dynamic_encryption_bit feature, an encryption_bit_51 feature and an encryption_bit_47 feature (mutually exclusive)?

The AMD programmers manual recommends determining the position of the bit at runtime, I suspect that's the same for Intel TDX?

I completely see that point, and I'm not 100% sure what is the correct approach here. My argument would be, that if you use one of these memory encryption technologies, you'll have to customize the code to that specific architecture anyway. Ok, not necessarily for the encryption of the page, but basically for everything else.

I wouldn't call Milan vs Genoa/Turin different architectures. If one wants to run some code on Milan, I'd reasonably expect the same code to run on Genoa/Turin. I don't think one usually has to change a lot of code (if any) when porting stuff from Milan to Genoa/Turin. Most kernels (including Linux) I'm aware of support more than one bit position.

Maybe a combination of both approaches could be a solution: Have a dynamic_encryption_bit feature, an encryption_bit_51 feature and an encryption_bit_47 feature (mutually exclusive)?

Yeah, that's an interesting approach. We'd need separate flags for SEV and TDX though, so we'd need four different flags to cover everything, and that seems a bit excessive.

If there's demand for hard-coding the bit position, I'd certainly consider this, but I'm not sure that there is a lot of demand.

The AMD programmers manual recommends determining the position of the bit at runtime, I suspect that's the same for Intel TDX?

I don't think they make any value judgments on what users should do, but the architecture is setup so that that information is available to the user.

I was mostly referring to Vol.2, §7.10.1, (and similarly in §15.34.1) in AMD's programmer manual:

the physical address size of the processor may be reduced when memory encryption features are enabled, for example from 48 to 43 bits. [...]
When memory encryption is supported in an implementation, CPUID Fn8000_001F[EBX]
reports any physical address size reduction present.

There is indeed no judgement but it read as the recommended approach to me.

Cargo.toml

src/structures/mem_encrypt.rs

src/structures/paging/page_table.rs

src/structures/amd_sev.rs

Freax13 · 2025-04-29T19:08:51Z

Cargo.toml

@@ -26,8 +26,10 @@ volatile = "0.4.4"
 rustversion = "1.0.5"

 [features]
-default = ["nightly", "instructions"]
+default = ["nightly", "instructions", "amd_sev"]


Hermit OS will need to be modified anyway; SEV/SEV-ES/SEV-SNP/TDX all require kernel modifications.

Freax13

We should also update the PhysAddr/PhysFrame types to reject the encryption bit being set.

src/structures/mem_encrypt.rs

zyuiop · 2025-04-30T08:41:40Z

We should also update the PhysAddr/PhysFrame types to reject the encryption bit being set.

I tried something in 1560047, not sure it's the best approach

.github/workflows/build.yml

src/structures/mem_encrypt.rs

src/addr.rs

src/structures/mem_encrypt.rs

src/structures/paging/page_table.rs

zyuiop · 2025-05-01T13:43:07Z

I've applied your suggestions, except for the const which I'm not sure how I should do.
Should I also bump the version number in Cargo.toml, considering the semver test failure?

Freax13 · 2025-05-01T13:45:05Z

Should I also bump the version number in Cargo.toml, considering the semver test failure?

No, please don't do that. There shouldn't be any breaking changes in this PR; the semver checks CI job should succeed.

zyuiop · 2025-05-01T14:25:00Z

Should I also bump the version number in Cargo.toml, considering the semver test failure?

No, please don't do that. There shouldn't be any breaking changes in this PR; the semver checks CI job should succeed.

Okay.
I have tested locally with the const_fn crate, and it seems I'm still getting the semver errors - even though I should not

Edit: I have found a solution. It requires changing the CI configuration though, and I'd strongly suggest reverting that change when it's time for the next release... Happy to take any other suggestion.

Edit2: Due to the existing const_fn feature, I had to make the dependency optional and remove the legacy feature. Issue is, any package that disables default features will possibly break. Open to suggestions to fix that...

Edit3: Had forgotten about dependencies renaming. A bit ugly, but at least it really does not break semver compatibility this time

…` is not enabled

…r semver

Freax13 · 2025-05-01T15:15:41Z

@zyuiop Please let me know when this is ready for review.

zyuiop · 2025-05-01T15:16:28Z

@zyuiop Please let me know when this is ready for review.

I think it now is. Sorry for the multiple attempts and formatting fails.

Freax13

This is looking great so far, I think this will be ready soon.

@jounathaen Given that you also expressed interest in SME/SEV support, do you have time for a review of this PR to make sure it satisfies your needs?

.github/workflows/build.yml

src/addr.rs

jounathaen

I think this looks pretty good! 👍

feat(sev): add AMD SEV support

19a03a9

Freax13 requested changes Apr 26, 2025

View reviewed changes

zyuiop added 3 commits April 29, 2025 16:50

x86_64/sev: move SEV structures to codebase

1b26f6c

x86_64/sev: remove SEV detection, replace with a simpler c-bit defining routine

x86_64/memory_encryption: don't enable ME by default

0a8a281

x86_64/memory_encryption: fix review and clippy issues

073c196

Freax13 requested changes Apr 29, 2025

View reviewed changes

src/structures/mem_encrypt.rs Outdated Show resolved Hide resolved

zyuiop added 2 commits April 30, 2025 10:22

memory_encryption: refactor for Intel TDX support

6558dd8

memory_encryption: enable memory_encryption flag in CI

684aaa7

zyuiop force-pushed the feat/sev-upstream branch 3 times, most recently from 5cb8438 to 99dad6d Compare April 30, 2025 08:29

zyuiop force-pushed the feat/sev-upstream branch 2 times, most recently from ac566d2 to 2063040 Compare April 30, 2025 09:05

zyuiop added 4 commits April 30, 2025 11:15

memory_encryption: reduce function duplication in page_table.rs

8a3abd1

memory_encryption: remove feature dynamic_flags

96f7233

memory_encryption: prevent setting the encryption bit in PhysAddr

1560047

memory_encryption: missing documentation

8519ada

zyuiop force-pushed the feat/sev-upstream branch from 2063040 to 8519ada Compare April 30, 2025 09:15

Freax13 requested changes May 1, 2025

View reviewed changes

zyuiop added 2 commits May 1, 2025 15:29

ci: test memory_encryption separately

3e05fa7

memory_encryption: address PR feedback

43e3db9

zyuiop added 2 commits May 1, 2025 16:29

memory_encryption: keep const functions const if `memory_encryption…

c13dd06

…` is not enabled

memory_encryption: exclude memory_encryption feature when checking fo…

29e9466

…r semver

zyuiop force-pushed the feat/sev-upstream branch from 77a7ee7 to 023cd44 Compare May 1, 2025 14:46

memory_encryption: fix const_fn name already taken

65bdd52

zyuiop force-pushed the feat/sev-upstream branch from 595d6b3 to 65bdd52 Compare May 1, 2025 15:06

memory_encryption: fix formatting

6fb583a

Freax13 requested changes May 1, 2025

View reviewed changes

.github/workflows/build.yml Outdated Show resolved Hide resolved

src/addr.rs Outdated Show resolved Hide resolved

src/addr.rs Outdated Show resolved Hide resolved

zyuiop added 4 commits May 1, 2025 17:38

memory_encryption: review fixes

648ae66

memory_encryption: review fixes + fix imports changes

a872185

memory_encryption: fix import style

bd7a9ef

ci: only run semver checks on default features

5370c17

jounathaen approved these changes May 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sev): add AMD SEV support #542

feat(sev): add AMD SEV support #542

zyuiop commented Apr 24, 2025 •

edited

Loading

Freax13 left a comment

Freax13 Apr 26, 2025

zyuiop Apr 28, 2025

Freax13 Apr 29, 2025

jounathaen Apr 29, 2025

Freax13 Apr 29, 2025

zyuiop Apr 29, 2025

jounathaen Apr 29, 2025

zyuiop commented Apr 28, 2025

Freax13 commented Apr 29, 2025

Freax13 left a comment

Freax13 Apr 29, 2025

jounathaen Apr 29, 2025

Freax13 Apr 29, 2025

jounathaen Apr 29, 2025 •

edited

Loading

zyuiop Apr 29, 2025

Freax13 Apr 30, 2025

Freax13 Apr 30, 2025

zyuiop Apr 30, 2025

Freax13 Apr 29, 2025

Freax13 left a comment

zyuiop commented Apr 30, 2025 •

edited

Loading

zyuiop commented May 1, 2025 •

edited

Loading

Freax13 commented May 1, 2025

zyuiop commented May 1, 2025 •

edited

Loading

Freax13 commented May 1, 2025

zyuiop commented May 1, 2025 •

edited

Loading

Freax13 left a comment

jounathaen left a comment

feat(sev): add AMD SEV support #542

Are you sure you want to change the base?

feat(sev): add AMD SEV support #542

Conversation

zyuiop commented Apr 24, 2025 • edited Loading

Freax13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyuiop commented Apr 28, 2025

Freax13 commented Apr 29, 2025

Freax13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jounathaen Apr 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Freax13 left a comment

Choose a reason for hiding this comment

zyuiop commented Apr 30, 2025 • edited Loading

zyuiop commented May 1, 2025 • edited Loading

Freax13 commented May 1, 2025

zyuiop commented May 1, 2025 • edited Loading

Freax13 commented May 1, 2025

zyuiop commented May 1, 2025 • edited Loading

Freax13 left a comment

Choose a reason for hiding this comment

jounathaen left a comment

Choose a reason for hiding this comment

zyuiop commented Apr 24, 2025 •

edited

Loading

jounathaen Apr 29, 2025 •

edited

Loading

zyuiop commented Apr 30, 2025 •

edited

Loading

zyuiop commented May 1, 2025 •

edited

Loading

zyuiop commented May 1, 2025 •

edited

Loading

zyuiop commented May 1, 2025 •

edited

Loading