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

["Min Xu" <min.m.xu@intel.com>]

Agree. I will update the patch set based upon your suggestions.

Thanks!
Xu, Min
> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Sunday, July 25, 2021 2:01 PM
> To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Brijesh Singh
> <brijesh.singh@amd.com>; Erdem Aktas <erdemaktas@google.com>; James
> Bottomley <jejb@linux.ibm.com>; Tom Lendacky
> <thomas.lendacky@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.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