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...@intel.com> > Sent: Thursday, July 22, 2021 1:52 PM > To: devel@edk2.groups.io > Cc: Xu, Min M <min.m...@intel.com>; Ard Biesheuvel > <ardb+tianoc...@kernel.org>; Brijesh Singh <brijesh.si...@amd.com>; Erdem > Aktas <erdemak...@google.com>; James Bottomley <j...@linux.ibm.com>; > Yao, Jiewen <jiewen....@intel.com>; Tom Lendacky > <thomas.lenda...@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+tianoc...@kernel.org> > Cc: Brijesh Singh <brijesh.si...@amd.com> > Cc: Erdem Aktas <erdemak...@google.com> > Cc: James Bottomley <j...@linux.ibm.com> > Cc: Jiewen Yao <jiewen....@intel.com> > Cc: Tom Lendacky <thomas.lenda...@amd.com> > Signed-off-by: Min Xu <min.m...@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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#78151): https://edk2.groups.io/g/devel/message/78151 Mute This Topic: https://groups.io/mt/84373830/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-