Agree. I will update the patch set based upon your suggestions. Thanks! Xu, Min > -----Original Message----- > From: Yao, Jiewen <jiewen....@intel.com> > Sent: Sunday, July 25, 2021 2:01 PM > To: Xu, Min M <min.m...@intel.com>; devel@edk2.groups.io > Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>; Brijesh Singh > <brijesh.si...@amd.com>; Erdem Aktas <erdemak...@google.com>; James > Bottomley <j...@linux.ibm.com>; Tom Lendacky > <thomas.lenda...@amd.com> > Subject: RE: [PATCH V2 4/4] OvmfPkg/ResetVector: Update ResetVector to > support Tdx > > 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 (#78156): https://edk2.groups.io/g/devel/message/78156 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] -=-=-=-=-=-=-=-=-=-=-=-