stage2: add TD Partitioning support #419

peterfang · 2024-07-23T11:16:48Z

This is the second half of the TDX enabling work in stage2. It enables SVSM running on a TDP platform to boot until the last jump in stage2_main(). It also adds support for serial console.

peterfang · 2024-08-05T21:57:52Z

Will update this PR again after #432 is merged since that impacts our stage1 trampoline logic

peterfang · 2024-08-21T12:49:41Z

PR updated. TDP boots stage2 with stage2 moved to 8MB.

Since both the stage2 bootloader and the SVSM kernel dump the CPUID page, there needs to be a page present at the respective GPA on TDP platforms. Insert a normal zero page at the CPUID page location so that dump_cpuid_table() can work as expected. Signed-off-by: Peter Fang <[email protected]>

Since the SVSM kernel touches the secrets page in svsm_start(), there needs to be a page present at the respective GPA on TDP platforms. Insert a normal zero page at the secrets page location so that the SVSM kernel can touch it without issues. Signed-off-by: Peter Fang <[email protected]>

Clean up formatting and crate ordering. Signed-off-by: Peter Fang <[email protected]>

Add the tdx-tdcall crate in preparation for the upcoming TDX enabling changes. Signed-off-by: Peter Fang <[email protected]>

Stage2LaunchInfo has been increased to 48 bytes. Make the according changes in stage1 trampoline. Signed-off-by: Peter Fang <[email protected]>

Setting the initial location counter has no effect since all of the addresses before edata are derived based on the subsequent locations. Signed-off-by: Peter Fang <[email protected]>

Add the TDX implementation of IOPort. This implementation translates all of the in/out operations to TDVMCALL operations as required by the GHCI spec. Signed-off-by: Peter Fang <[email protected]>

Rename struct SVSMIOPort for clarity and distinction from the TDX implementation. Signed-off-by: Peter Fang <[email protected]>

phys_addr_sizes should be the entirety of CPUID leaf 0x80000008.EAX, not just the Physical Address Bits field. Signed-off-by: Peter Fang <[email protected]>

validate_page_range() currently takes GVA due to the design of PVALIDATE. In the case of TDX, GPA is required for page acceptance. Add an additional input, paddr (PhysAddr), to validate_page_range(). An explicit input for GPA is required because virt_to_phys() is not always available. The input region may be out of range for the current FIXED_MAPPING. Signed-off-by: Peter Fang <[email protected]>

On TDP platforms, validate_page_range() means page acceptance. Use the tdx-tdcall crate for this TDCALL operation. Signed-off-by: Peter Fang <[email protected]>

joergroedel

I pointed out two problems which need fixing, other than that it looks good to me. With the log::info! issue fixed it boots for me on SNP and testing passes.

joergroedel · 2024-08-27T15:28:10Z

kernel/src/platform/mod.rs

+    fn validate_page_range(
+        &self,
+        region: MemoryRegion<VirtAddr>,
+        paddr: PhysAddr,
+    ) -> Result<(), SvsmError>;


This interface change puzzles me a bit. I imposes certain constraints on the method caller (like that the virtual region maps physically contiguous memory).
At least the interface needs to be documented after that change. In general I think it is possible to get the physical address from within the method. Stage2 is identity-mapped and for the COCONUT kernel there is virt_to_phys().

virt_to_phys() is limited in stage2 because FIXED_MAPPING is only limited to the first 640KB + the stage2 area, despite the fact that INIT_PGTABLE continues to be updated in map_and_validate(). Do you think we should try to improve virt_to_phys() instead so that it tracks the new page table mappings?

validate_page_range() should really operate on a physical range, because all platforms consider the concept of "validation" as a property of GPAs. The fact that we use virtual addresses anywhere is an artifact of how PVALIDATE works on SNP. The best approach here would be to have valid_page_range() take a physical address range, and then on platforms that require virtual mappings (like SNP), to use phys_to_virt() or a temporary per-CPU virtual mapping to complete the request.

After taking a closer look, I think this is a big hack that really shouldn't be checked in. Yes, validate_page_range() should take a physical address span, but it should not also take a virtual address which might or might not have anything to do with the physical address. I will take a stab at changing the interface to use physical addresses (with per-CPU temporary mappings on SNP to obtain the virtual addresses required by PVALIDATE).

Sounds good. Will rebase this commit after your changes are merged.

joergroedel · 2024-08-27T15:32:42Z

kernel/src/stage2.rs

@@ -440,6 +440,8 @@ pub extern "C" fn stage2_main(launch_info: &Stage2LaunchInfo) {
    // Shut down the GHCB
    shutdown_percpu();

+    log::info!("Starting SVSM kernel...");


This needs to happen before shutdown_percpu(), otherwise it crashes on SNP. The shutdown_percpu() function also shuts down the GHCB on SNP which is required for serial port access, and as a consequence also to print any log messages.

Thanks for catching this! Will fix in my update.

msft-jlange · 2024-09-03T20:30:31Z

kernel/src/svsm_console.rs

@@ -52,6 +55,33 @@ impl IOPort for SVSMIOPort {
    }
 }

+#[derive(Clone, Copy, Debug, Default)]
+pub struct SvsmTdIOPort {}


Can we not just change SVSMIOPort to use IN and OUT instructions natively, and then have the #VC handler issue GHCB calls and the #VE handler issue GHCI calls? This seems like it would be much cleaner because it restricts platform-specific functionality to code paths that are already platform-specific (exception handlers) instead of common code (the console code).

Sounds good to me. I can make that change if it's fine by @joergroedel .

msft-jlange · 2024-09-03T20:31:15Z

kernel/src/cpu/idt/common.rs

@@ -215,7 +215,7 @@ impl WriteLockGuard<'static, IDT> {
    /// Load an IDT. Its lifetime must be static so that its entries are
    /// always available to the CPU.
    pub fn load(&self) {
-        let desc: IdtDesc = IdtDesc {
+        let desc = IdtDesc {


What's the reason for this change? Is this related to TDP?

I think this bled into the patchset when I created this PR. I can just create a clean-up patch for this.

Personally, I always prefer when the set of changes in a patch is limited to work described in the PR summary - i.e., that each patch be minimal and specific. Others may feel differently.

msft-jlange · 2024-09-03T20:32:19Z

kernel/src/debug/gdbstub.rs

@@ -173,7 +173,7 @@ pub mod svsm_gdbstub {

    static GDB_INITIALISED: AtomicBool = AtomicBool::new(false);
    static GDB_STATE: SpinLock<Option<SvsmGdbStub<'_>>> = SpinLock::new(None);
-    static GDB_IO: SVSMIOPort = SVSMIOPort::new();
+    static GDB_IO: SvsmSevIOPort = SvsmSevIOPort::new();


Isn't this going to be problematic when we finally make GDB work on TDP? Wouldn't it be better to define a platform abstraction now since you are already forced to touch this code?

I agree. I wasn't planning on tackling this in this PR yet but this becomes easier with a common SVSMIOPort.

Maybe add a TODO comment to point out that this is a known future work item.

peterfang force-pushed the dev-tdp-stage2 branch 2 times, most recently from 285a867 to a323d9f Compare July 31, 2024 05:48

peterfang force-pushed the dev-tdp-stage2 branch from a323d9f to 01a984d Compare August 21, 2024 12:47

peterfang added 11 commits August 22, 2024 20:53

Cargo.toml: Clean up

6ee8487

Clean up formatting and crate ordering. Signed-off-by: Peter Fang <[email protected]>

Cargo.toml: Add tdx-tdcall crate

ed379fe

Add the tdx-tdcall crate in preparation for the upcoming TDX enabling changes. Signed-off-by: Peter Fang <[email protected]>

stage1: Fix stage2 launch info size

a1e7391

Stage2LaunchInfo has been increased to 48 bytes. Make the according changes in stage1 trampoline. Signed-off-by: Peter Fang <[email protected]>

stage1: Revert lds change

bea3392

Setting the initial location counter has no effect since all of the addresses before edata are derived based on the subsequent locations. Signed-off-by: Peter Fang <[email protected]>

svsm_console: Add SvsmTdIOPort

60076fa

Add the TDX implementation of IOPort. This implementation translates all of the in/out operations to TDVMCALL operations as required by the GHCI spec. Signed-off-by: Peter Fang <[email protected]>

svsm_console: Rename SVSMIOPort to SvsmSevIOPort

a50cd4f

Rename struct SVSMIOPort for clarity and distinction from the TDX implementation. Signed-off-by: Peter Fang <[email protected]>

platform: tdp: Fix phys_addr_sizes

b03a387

phys_addr_sizes should be the entirety of CPUID leaf 0x80000008.EAX, not just the Physical Address Bits field. Signed-off-by: Peter Fang <[email protected]>

platform: tdp: Implement validate_page_range()

c70ec34

On TDP platforms, validate_page_range() means page acceptance. Use the tdx-tdcall crate for this TDCALL operation. Signed-off-by: Peter Fang <[email protected]>

peterfang force-pushed the dev-tdp-stage2 branch from 01a984d to c70ec34 Compare August 23, 2024 04:01

joergroedel added the wait-for-review PR needs for approval by reviewers label Aug 27, 2024

joergroedel requested changes Aug 27, 2024

View reviewed changes

joergroedel added wait-for-update PR is waiting to be updated to address review comments and removed wait-for-review PR needs for approval by reviewers labels Aug 27, 2024

msft-jlange reviewed Sep 3, 2024

View reviewed changes

peterfang mentioned this pull request Sep 23, 2024

platform: move I/O port driver into platform abstraction #463

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stage2: add TD Partitioning support #419

stage2: add TD Partitioning support #419

peterfang commented Jul 23, 2024

peterfang commented Aug 5, 2024 •

edited

Loading

peterfang commented Aug 21, 2024

joergroedel left a comment

joergroedel Aug 27, 2024

peterfang Aug 28, 2024

msft-jlange Aug 30, 2024

msft-jlange Sep 3, 2024

peterfang Sep 9, 2024

joergroedel Aug 27, 2024

peterfang Aug 28, 2024

msft-jlange Sep 3, 2024

peterfang Sep 4, 2024

msft-jlange Sep 3, 2024

peterfang Sep 4, 2024 •

edited

Loading

msft-jlange Sep 4, 2024

msft-jlange Sep 3, 2024

peterfang Sep 4, 2024

msft-jlange Sep 4, 2024

stage2: add TD Partitioning support #419

Are you sure you want to change the base?

stage2: add TD Partitioning support #419

Conversation

peterfang commented Jul 23, 2024

peterfang commented Aug 5, 2024 • edited Loading

peterfang commented Aug 21, 2024

joergroedel 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterfang Sep 4, 2024 • 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

peterfang commented Aug 5, 2024 •

edited

Loading

peterfang Sep 4, 2024 •

edited

Loading