search

coconut-svsm / svsm

COCONUT-SVSM
MIT License
122 stars 43 forks source link

igvm: initial support for TD Partitioning #356

Closed peterfang closed 5 months ago

peterfang commented 6 months ago

This is the first set of patches to begin supporting TD Partitioning in the IGVM framework. Patches for TDX enabling will follow. No changes to the IGVM spec are proposed yet. The goal is to allow TDP enabling to hit the ground running first. The main work in this patchset is to separate TDP from SEV platforms.

To generate an IGVM file for a TDP platform as part of the build process, run:

$ make PLATFORM=tdp FW_FILE=/path/to/OVMF.fd

msft-jlange commented 6 months ago

This PR seems to require a specific choice of platform: either SNP or TDP. The IGVM format is designed to be multi-platform (this is how the NATIVE support in the IGVM builder is currently implemented). It would be much, much better if platform selection could be a combination instead of a choice. I would argue that the default make igvm should build an IGVM file that contains both TDP and SNP support in a single file, and should build all of the components needed by both platforms, and that command-line arguments to make should only be used if there is a desire to suppress one platform or another.

peterfang commented 6 months ago

This PR seems to require a specific choice of platform: either SNP or TDP. The IGVM format is designed to be multi-platform (this is how the NATIVE support in the IGVM builder is currently implemented). It would be much, much better if platform selection could be a combination instead of a choice. I would argue that the default make igvm should build an IGVM file that contains both TDP and SNP support in a single file, and should build all of the components needed by both platforms, and that command-line arguments to make should only be used if there is a desire to suppress one platform or another.

Thanks for sharing how you envision IGVM multi-platform support in the future. This is very helpful. I'll do some reworking and update this PR soon.

peterfang commented 6 months ago

Hi @msft-jlange the PR has been updated. Please take a look. Thanks!

msft-jlange commented 6 months ago

Something is wrong with the way stage1 is included, at least in the Hyper-V version of the IGVM file. The bytes at the reset vector are e9 0d fe 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00. But because TDX starts in 32-bit protected mode, that first instruction decodes as jmp 0x2E66FE02 which is clearly not correct.

peterfang commented 6 months ago

Something is wrong with the way stage1 is included, at least in the Hyper-V version of the IGVM file. The bytes at the reset vector are e9 0d fe 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00. But because TDX starts in 32-bit protected mode, that first instruction decodes as jmp 0x2E66FE02 which is clearly not correct.

This actually doesn't work in this patchset because stage1 has not been changed yet. In my next PR I'll start adding TDX support in stage1 & stage2 and stage1 will then support 32-bit protected mode at the reset vector.

peterfang commented 5 months ago

Hi @msft-jlange I just updated the PR. Please take a look. Thanks.

msft-jlange commented 5 months ago

Hi @msft-jlange I just updated the PR. Please take a look. Thanks.

This is in great shape now! My only outstanding comment is the one about the stage 1 file argument name.

peterfang commented 5 months ago

Hi @msft-jlange I just updated the PR. Please take a look. Thanks.

This is in great shape now! My only outstanding comment is the one about the stage 1 file argument name.

@msft-jlange the comment has been addressed in the last two commits and the others remain the same as the previous version.

msft-jlange commented 5 months ago

@msft-jlange the comment has been addressed in the last two commits and the others remain the same as the previous version.

@peterfang, I've submitted a PR review approving the changes. You will have to mark all of the conversations as Resolved before it will be possible to merge.

joergroedel commented 5 months ago

Hey @peterfang,

Can you please rebase this to the latest upstream branch? There is a conflict and a few missing pieces required by the recent APIC support changes.

Thanks!

peterfang commented 5 months ago

Hey @peterfang,

Can you please rebase this to the latest upstream branch? There is a conflict and a few missing pieces required by the recent APIC support changes.

Thanks!

Thanks @joergroedel ! Will rebase as well as incorporate the latest feedback.

peterfang commented 5 months ago

Hey @peterfang, Can you please rebase this to the latest upstream branch? There is a conflict and a few missing pieces required by the recent APIC support changes. Thanks!

Thanks @joergroedel ! Will rebase as well as incorporate the latest feedback.

This PR has been updated based on the latest upstream branch.