Re: [PATCH V2 4/4] OvmfPkg/ResetVector: Update ResetVector to support Tdx


Yao, Jiewen
 

Hi Min, Brijesh, James
I feel very frustrated when I review the existing OVMF reset vector.

A big problem is that this code mixed too many SEV stuff, and we are trying to add more TDX stuff in *one* file, without clear isolation.

Take PageTables64.asm as example, here are the symbols. (* means it is newly added.)
=================
CheckSevFeatures:
GetSevEncBit:
SevEncBitLowHlt:
SevSaveMask:
NoSev:
NoSevEsVcHlt:
NoSevPass:
SevExit:
IsSevEsEnabled:
SevEsDisabled:
SetCr3ForPageTables64:
CheckSev: (*)
SevNotActive:
clearPageTablesMemoryLoop:
pageTableEntriesLoop:
tdClearTdxPageTablesMemoryLoop: (*)
IsSevEs: (*)
pageTableEntries4kLoop:
clearGhcbMemoryLoop:
SetCr3:
SevEsIdtNotCpuid:
SevEsIdtNoCpuidResponse:
SevEsIdtTerminate:
SevEsIdtHlt:
SevEsIdtVmmComm:
NextReg:
VmmDone:
=================

In order to better maintain the ResetVector, may I propose some refinement:
1) The main function only contains the non-TEE function, where TEE == SEV + TDX.
2) The TEE related code is moved to TEE specific standalone file, such *Sev.asm and *Tdx.Asm.

3) We need handle different cases with different pattern.
I hope the patter can be used consistently. As such, the reviewer can easily understand what it is for.

3.1) If TEE function is a hook, then the main function calls TEE function directly. The TEE function need implement a TEE check function (such as IsSev, or IsTdx). For example:
====================
OneTimeCall PreMainFunctionHookSev
OneTimeCall PreMainFunctionHookTdx
MainFunction:
XXXXXX
OneTimeCall PostMainFunctionHookSev
OneTimeCall PostMainFunctionHookTdx
====================
3.2) If TEE function is a replacement for non-TEE function. The main function can call TEE replacement function, then check the return status to decide next step. For example:
====================
OneTimeCall MainFunctionSev
Jz MainFunctionEnd
OneTimeCall MainFunctionTdx
Jz MainFunctionEnd
MainFunction:
XXXXXX
MainFunctionEnd:
====================

4) If we found it is too hard to write code in above patter, we can discuss case by case.

-----Original Message-----
From: Xu, Min M <min.m.xu@intel.com>
Sent: Thursday, July 22, 2021 1:52 PM
To: devel@edk2.groups.io
Cc: Xu, Min M <min.m.xu@intel.com>; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Brijesh Singh <brijesh.singh@amd.com>; Erdem
Aktas <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>;
Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
<thomas.lendacky@amd.com>
Subject: [PATCH V2 4/4] OvmfPkg/ResetVector: Update ResetVector to support
Tdx

RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

In Tdx all CPUs "reset" to run on 32-bit protected mode with flat
descriptor (paging disabled). But in Non-Td guest the initial state of
CPUs is 16-bit real mode. To resolve this conflict, BITS 16/32 is used
in the very beginning of ResetVector. It will check the 32-bit protected
mode or 16-bit real mode, then jump to the corresponding entry point.
This is done in OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm.

ReloadFlat32.asm load the GDT and set the CR0, then jump to Flat-32 mode.

InitTdx.asm is called to record the Tdx signature ('TDXG') and other tdx
information in a TDX_WORK_AREA which can be used by the other routines in
ResetVector.

Init32.asm is 32-bit initialization code in OvmfPkg. It puts above
ReloadFlat32 and InitTdx together to do the initializaiton for Tdx.

After that Tdx jumps to 64-bit long mode by doing following tasks:
1. SetCr3ForPageTables64
For OVMF, some initial page tables is built at:
PcdOvmfSecPageTablesBase - (PcdOvmfSecPageTablesBase + 0x6000)
This page table supports the 4-level page table.
But Tdx support 4-level and 5-level page table based on the CPU GPA width.
48bit is 4-level paging, 52-bit is 5-level paging.
If 5-level page table is supported (GPAW is 52), then a top level
page directory pointers (1 * 256TB entry) is generated in the
TdxPageTable.
2. Set Cr4
Enable PAE.
3. Adjust Cr3
If GPAW is 48, then Cr3 is PT_ADDR (0). If GPAW is 52, then Cr3 is
TDX_PT_ADDR (0).

Tdx MailBox [0x10, 0x800] is reserved for OS. So we initialize piece of this
area ([0x10, 0x20]) to record the Tdx flag ('TDXG') and other Tdx info so that
they can be used in the following flow.

After all above is successfully done, Tdx jump to SecEntry.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 21 ++++++++
OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm | 47 ++++++++++++++++
OvmfPkg/ResetVector/Ia32/Init32.asm | 34 ++++++++++++
OvmfPkg/ResetVector/Ia32/InitTdx.asm | 57 ++++++++++++++++++++
OvmfPkg/ResetVector/Ia32/PageTables64.asm | 41 ++++++++++++++
OvmfPkg/ResetVector/Ia32/ReloadFlat32.asm | 44 +++++++++++++++
OvmfPkg/ResetVector/ResetVector.nasmb | 18 +++++++
7 files changed, 262 insertions(+)
create mode 100644 OvmfPkg/ResetVector/Ia32/Init32.asm
create mode 100644 OvmfPkg/ResetVector/Ia32/InitTdx.asm
create mode 100644 OvmfPkg/ResetVector/Ia32/ReloadFlat32.asm

diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index ac86ce69ebe8..a390ed81d021 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -155,10 +155,31 @@ resetVector:
;
; This is where the processor will begin execution
;
+; In IA32 we follow the standard reset vector flow. While in X64, Td guest
+; may be supported. Td guest requires the startup mode to be 32-bit
+; protected mode but the legacy VM startup mode is 16-bit real mode.
+; To make NASM generate such shared entry code that behaves correctly in
+; both 16-bit and 32-bit mode, more BITS directives are added.
+;
+%ifdef ARCH_IA32
+
nop
nop
jmp EarlyBspInitReal16

+%else
+
+ smsw ax
+ test al, 1
+ jz .Real
+BITS 32
+ jmp Main32
+BITS 16
+.Real:
+ jmp EarlyBspInitReal16
+
+%endif
+
ALIGN 16

fourGigabytes:
diff --git a/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
b/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
index c6d0d898bcd1..2206ca719593 100644
--- a/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
+++ b/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
@@ -17,6 +17,9 @@ Transition32FlatTo64Flat:

OneTimeCall SetCr3ForPageTables64

+ cmp dword[TDX_WORK_AREA], 0x47584454 ; 'TDXG'
+ jz TdxTransition32FlatTo64Flat
+
mov eax, cr4
bts eax, 5 ; enable PAE
mov cr4, eax
@@ -65,10 +68,54 @@ EnablePaging:
bts eax, 31 ; set PG
mov cr0, eax ; enable paging

+ jmp _jumpTo64Bit
+
+;
+; Tdx Transition from 32Flat to 64Flat
+;
+TdxTransition32FlatTo64Flat:
+
+ mov eax, cr4
+ bts eax, 5 ; enable PAE
+
+ ;
+ ; byte[TDX_WORK_AREA_PAGELEVEL5] holds the indicator whether 52bit is
supported.
+ ; if it is the case, need to set LA57 and use 5-level paging
+ ;
+ cmp byte[TDX_WORK_AREA_PAGELEVEL5], 0
+ jz .set_cr4
+ bts eax, 12
+.set_cr4:
+ mov cr4, eax
+ mov ebx, cr3
+
+ ;
+ ; if la57 is not set, we are ok
+ ; if using 5-level paging, adjust top-level page directory
+ ;
+ bt eax, 12
+ jnc .set_cr3
+ mov ebx, TDX_PT_ADDR (0)
+.set_cr3:
+ mov cr3, ebx
+
+ mov eax, cr0
+ bts eax, 31 ; set PG
+ mov cr0, eax ; enable paging
+
+_jumpTo64Bit:
jmp LINEAR_CODE64_SEL:ADDR_OF(jumpTo64BitAndLandHere)
+
BITS 64
jumpTo64BitAndLandHere:

+ ;
+ ; For Td guest we are done and jump to the end
+ ;
+ mov eax, TDX_WORK_AREA
+ cmp dword [eax], 0x47584454 ; 'TDXG'
+ jz GoodCompare
+
;
; Check if the second step of the SEV-ES mitigation is to be performed.
;
diff --git a/OvmfPkg/ResetVector/Ia32/Init32.asm
b/OvmfPkg/ResetVector/Ia32/Init32.asm
new file mode 100644
index 000000000000..772adc51e531
--- /dev/null
+++ b/OvmfPkg/ResetVector/Ia32/Init32.asm
@@ -0,0 +1,34 @@
+;------------------------------------------------------------------------------
+; @file
+; 32-bit initialization for Tdx
+;
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+BITS 32
+
+;
+; Modified: EBP
+;
+; @param[in] EBX [6:0] CPU supported GPA width
+; [7:7] 5 level page table support
+; @param[in] ECX [31:0] TDINITVP - Untrusted Configuration
+; @param[in] EDX [31:0] VCPUID
+; @param[in] ESI [31:0] VCPU_Index
+;
+Init32:
+ ;
+ ; Save EBX in EBP because EBX will be changed in ReloadFlat32
+ ;
+ mov ebp, ebx
+
+ OneTimeCall ReloadFlat32
+
+ ;
+ ; Init Tdx
+ ;
+ OneTimeCall InitTdx
+
+ OneTimeCallRet Init32
diff --git a/OvmfPkg/ResetVector/Ia32/InitTdx.asm
b/OvmfPkg/ResetVector/Ia32/InitTdx.asm
new file mode 100644
index 000000000000..de8273da6a0c
--- /dev/null
+++ b/OvmfPkg/ResetVector/Ia32/InitTdx.asm
@@ -0,0 +1,57 @@
+;------------------------------------------------------------------------------
+; @file
+; Initialize TDX_WORK_AREA to record the Tdx flag ('TDXG') and other Tdx info
+; so that the following codes can use these information.
+;
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+BITS 32
+
+;
+; Modified: EBP
+;
+InitTdx:
+ ;
+ ; In Td guest, BSP/AP shares the same entry point
+ ; BSP builds up the page table, while APs shouldn't do the same task.
+ ; Instead, APs just leverage the page table which is built by BSP.
+ ; APs will wait until the page table is ready.
+ ; In Td guest, vCPU 0 is treated as the BSP, the others are APs.
+ ; ESI indicates the vCPU ID.
+ ;
+ cmp esi, 0
+ je tdBspEntry
+
+apWait:
+ cmp byte[TDX_WORK_AREA_PGTBL_READY], 0
+ je apWait
+ jmp doneTdxInit
+
+tdBspEntry:
+ ;
+ ; It is of Tdx Guest
+ ; Save the Tdx info in TDX_WORK_AREA so that the following code can use
+ ; these information.
+ ;
+ mov dword [TDX_WORK_AREA], 0x47584454 ; 'TDXG'
+
+ ;
+ ; EBP[6:0] CPU supported GPA width
+ ;
+ and ebp, 0x3f
+ cmp ebp, 52
+ jl NotPageLevel5
+ mov byte[TDX_WORK_AREA_PAGELEVEL5], 1
+
+NotPageLevel5:
+ ;
+ ; ECX[31:0] TDINITVP - Untrusted Configuration
+ ;
+ mov DWORD[TDX_WORK_AREA_INITVP], ecx
+ mov DWORD[TDX_WORK_AREA_INFO], ebp
+
+doneTdxInit:
+ OneTimeCallRet InitTdx
diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index 5fae8986d9da..508df6cf5967 100644
--- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
+++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
@@ -218,6 +218,24 @@ SevEsDisabled:
;
SetCr3ForPageTables64:

+ ;
+ ; Check Td guest
+ ;
+ cmp dword[TDX_WORK_AREA], 0x47584454 ; 'TDXG'
+ jnz CheckSev
+
+ xor edx, edx
+
+ ;
+ ; In Td guest, BSP builds the page table and set the flag of
+ ; TDX_WORK_AREA_PGTBL_READY. APs check this flag and then set
+ ; cr3 directly.
+ ;
+ cmp byte[TDX_WORK_AREA_PGTBL_READY], 1
+ jz SetCr3
+ jmp SevNotActive
+
+CheckSev:
OneTimeCall CheckSevFeatures
xor edx, edx
test eax, eax
@@ -277,6 +295,29 @@ pageTableEntriesLoop:
mov [(ecx * 8 + PT_ADDR (0x2000 - 8)) + 4], edx
loop pageTableEntriesLoop

+ ;
+ ; If it is Td guest, TdxExtraPageTable should be initialized as well
+ ;
+ cmp dword[TDX_WORK_AREA], 0x47584454 ; 'TDXG'
+ jnz IsSevEs
+
+ xor eax, eax
+ mov ecx, 0x400
+tdClearTdxPageTablesMemoryLoop:
+ mov dword [ecx * 4 + TDX_PT_ADDR (0) - 4], eax
+ loop tdClearTdxPageTablesMemoryLoop
+
+ xor edx, edx
+ ;
+ ; Top level Page Directory Pointers (1 * 256TB entry)
+ ;
+ mov dword[TDX_PT_ADDR (0)], PT_ADDR (0) + PAGE_PDP_ATTR
+ mov dword[TDX_PT_ADDR (4)], edx
+
+ mov byte[TDX_WORK_AREA_PGTBL_READY], 1
+ jmp SetCr3
+
+IsSevEs:
OneTimeCall IsSevEsEnabled
test eax, eax
jz SetCr3
diff --git a/OvmfPkg/ResetVector/Ia32/ReloadFlat32.asm
b/OvmfPkg/ResetVector/Ia32/ReloadFlat32.asm
new file mode 100644
index 000000000000..06d44142625a
--- /dev/null
+++ b/OvmfPkg/ResetVector/Ia32/ReloadFlat32.asm
@@ -0,0 +1,44 @@
+;------------------------------------------------------------------------------
+; @file
+; Load the GDT and set the CR0/CR4, then jump to Flat 32 protected mode.
+;
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+%define SEC_DEFAULT_CR0 0x00000023
+%define SEC_DEFAULT_CR4 0x640
+
+BITS 32
+
+;
+; Modified: EAX, EBX, CR0, CR4, DS, ES, FS, GS, SS
+;
+ReloadFlat32:
+
+ cli
+ mov ebx, ADDR_OF(gdtr)
+ lgdt [ebx]
+
+ mov eax, SEC_DEFAULT_CR0
+ mov cr0, eax
+
+ jmp LINEAR_CODE_SEL:dword ADDR_OF(jumpToFlat32BitAndLandHere)
+BITS 32
+jumpToFlat32BitAndLandHere:
+
+ mov eax, SEC_DEFAULT_CR4
+ mov cr4, eax
+
+ debugShowPostCode POSTCODE_32BIT_MODE
+
+ mov ax, LINEAR_SEL
+ mov ds, ax
+ mov es, ax
+ mov fs, ax
+ mov gs, ax
+ mov ss, ax
+
+ OneTimeCallRet ReloadFlat32
+
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb
b/OvmfPkg/ResetVector/ResetVector.nasmb
index b653fe87abd6..3ec163613477 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -106,6 +106,21 @@
%define TDX_EXTRA_PAGE_TABLE_BASE FixedPcdGet32
(PcdOvmfSecGhcbPageTableBase)
%define TDX_EXTRA_PAGE_TABLE_SIZE FixedPcdGet32
(PcdOvmfSecGhcbPageTableSize)

+ ;
+ ; TdMailboxBase [0x10, 0x800] is reserved for OS.
+ ; Td guest initialize piece of this area (TdMailboxBase [0x10,0x20]) to
+ ; record the Td guest info so that this information can be used in the
+ ; following ResetVector flow.
+ ;
+ %define TD_MAILBOX_WORKAREA_OFFSET 0x10
+ %define TDX_WORK_AREA (TDX_MAILBOX_MEMORY_BASE +
TD_MAILBOX_WORKAREA_OFFSET)
+ %define TDX_WORK_AREA_PAGELEVEL5 (TDX_WORK_AREA + 4)
+ %define TDX_WORK_AREA_PGTBL_READY (TDX_WORK_AREA + 5)
+ %define TDX_WORK_AREA_INITVP (TDX_WORK_AREA + 8)
+ %define TDX_WORK_AREA_INFO (TDX_WORK_AREA + 8 + 4)
+
+ %define TDX_PT_ADDR(Offset) (TDX_EXTRA_PAGE_TABLE_BASE + (Offset))
+
%define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) +
(Offset))

%define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
@@ -117,6 +132,9 @@
%define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32
(PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))

%include "X64/TdxMetadata.asm"
+ %include "Ia32/Init32.asm"
+ %include "Ia32/InitTdx.asm"
+ %include "Ia32/ReloadFlat32.asm"

%include "Ia32/Flat32ToFlat64.asm"
%include "Ia32/PageTables64.asm"
--
2.29.2.windows.2

Join devel@edk2.groups.io to automatically receive all group messages.