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.

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

platform/tdp: replace tdx-tdcall crate with local implementation #515

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

msft-jlange
Copy link
Contributor

The tdx-tdcall crate is built for different security and design assumptions than what is required for COCONUT-SVSM. The TDCALL/TDVMCALL functionality required can be implemented locally, providing better integration with the COCONUT-SVSM environment.

@msft-jlange
Copy link
Contributor Author

@peterfang it would great to get your review on this

kernel/src/tdx/tdcall.rs Outdated Show resolved Hide resolved
kernel/src/tdx/tdcall.rs Outdated Show resolved Hide resolved
kernel/src/tdx/tdcall.rs Outdated Show resolved Hide resolved
kernel/src/tdx/tdcall.rs Outdated Show resolved Hide resolved
kernel/src/tdx/tdcall.rs Outdated Show resolved Hide resolved
kernel/src/tdx/tdcall.rs Outdated Show resolved Hide resolved
kernel/src/tdx/tdcall.rs Outdated Show resolved Hide resolved
kernel/src/tdx/tdcall.rs Outdated Show resolved Hide resolved
kernel/src/platform/tdp.rs Outdated Show resolved Hide resolved
kernel/src/tdx/tdcall.rs Show resolved Hide resolved
kernel/src/tdx/tdcall.rs Outdated Show resolved Hide resolved
kernel/src/tdx/tdcall.rs Outdated Show resolved Hide resolved
kernel/src/tdx/tdcall.rs Show resolved Hide resolved
The `tdx-tdcall` crate is built for different security and design
assumptions than what is required for COCONUT-SVSM.  The TDCALL/TDVMCALL
functionality required can be implemented locally, providing better
integration with the COCONUT-SVSM environment.

Signed-off-by: Jon Lange <[email protected]>
@peterfang
Copy link
Contributor

@peterfang it would great to get your review on this

Sure, I'll take a look

@joergroedel joergroedel added the in-review PR is under active review and not yet approved label Nov 15, 2024
if addr.is_aligned(PAGE_SIZE_2M) && addr + PAGE_SIZE_2M <= end {
let ret = tdx_result(tdg_mem_page_accept(PageFrame::Size2M(addr)));
match ret {
Err(TdxError::PageAlreadyAccepted) => {

Choose a reason for hiding this comment

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

What is the usecase to allow such behavior, i.e. why we don't treat PageAlreadyAccepted as a bug condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PageAlreadyAccepted does not necessarily indicate an error. Suppose a TD has a private page at GPA X, and calls GHCI.MapGPA to convert it to a shared page. The host may choose to insert a different HPA at GPA X+Shared without ever calling TDH.MEM.PAGE.REMOVE on GPA X. This is acceptable to the TD. Then, the TD may call GHCI.MapGPA to convert back to a pivate page. The host may choose simply to remove GPA X+Shared, still without ever calling TDH.MEM.PAGE.REMOVE on GPA X. There's nothing inappropriate about this flow. The TD is expecting to accept a page of zeroes, so if it sees that the page was already accepted in this case, it should just zero the page and move on.

Similarly, not observing PageAlreadyAccepted does not mean that acceptance is safe. A TD may call TDG.MEM.PAGE.ACCEPT on GPA X, and then may call TDG.MEM.PAGE.ACCEPT on GPA X a second time (perhaps intentionally, perhaps inadvertently). If the host calls TDH.MEM.PAGE.REMOVE/TDH.MEM.PAGE.AUG in between the two calls, the second TDG.MEM.PAGE.ACCEPT call will succeed and zero the page, even if the TD expected that the page had already been accepted.

Since PageAlreadyAccepted is not a reliable way to detect an error condition, it's better just to make page acceptance be consistent, and to say that every call will result in a zeroed page regardless of what was there before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-review PR is under active review and not yet approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants