[PATCH V7 0/1] Add Intel TDX support in OvmfPkg/ResetVector

[PATCH V7 0/1] Add Intel TDX support in OvmfPkg/ResetVector

[Min Xu]

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

Intel's Trust Domain Extensions (Intel TDX) refers to an Intel technology
that extends Virtual Machines Extensions (VMX) and Multi-Key Total Memory
Encryption (MKTME) with a new kind of virutal machines guest called a 
Trust Domain (TD). A TD is desinged to run in a CPU mode that protects the
confidentiality of TD memory contents and the TD's CPU state from other
software, including the hosting Virtual-Machine Monitor (VMM), unless
explicitly shared by the TD itself.

The patch-sets to support Intel TDX in OvmfPkg is split into several
waves. This is wave-1 which adds Intel TDX support in OvmfPkg/ResetVector.
Note: TDX only works in X64.

[TDX]: https://software.intel.com/content/dam/develop/external/us/en/
documents/tdx-whitepaper-final9-17.pdf

[TDVF]: https://software.intel.com/content/dam/develop/external/us/en/
documents/tdx-virtual-firmware-design-guide-rev-1.pdf

Code is at https://github.com/mxu9/edk2/tree/tdvf_wave1.v7

v7 changes:
 - Refine the offset of TdxMetadata and remove the definition of
   PcdOvmfImageSizeInKB
 - Use MOV CR* instead of smsw in ResetVector
 - Remove the new field (SubType) in
   CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER.

v6 changes:
 - Remove the 5-level paging support. 5-level paging enabling is *NOT*
   super critical for TDX enabling at this moment. It will be enabled
   later in a separate patch.
 - Add a new field (SubType) in CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER
   to record the VM Guest SubType.
 - In Main16 entry point, after TransitionFromReal16To32BitFlat,
   WORK_AREA_GUEST_TYPE is cleared to 0. WORK_AREA_GUEST_TYPE was
   previously cleared in SetCr3ForPageTables64 (see commit ab77b60).
   This doesn't work after TDX is introduced in Ovmf. It is because all
   TDX CPUs (BSP and APs) start to run from 0xfffffff0. In previous code
   WORK_AREA_GUEST_TYPE will be cleared multi-times in TDX guest. So for
   SEV and Legacy guest it is moved to Main16 entry point (after
   TransitionFromReal16To32BitFlat). For TDX guest WORK_AREA_GUEST_TYPE
   is cleared and set in InitTdxWorkarea.
 - Make the return result of IsTdx be consistent with IsTdxEnabled.
 - Fix some typo in the code comments.

v5 changes:
 - Remove the changes of OVMF_WORK_AREA because Commit ab77b60 covers
   those changes.
 - Refine the TDX related changes in PageTables64.asm and
   Flat32ToFlat64.asm.
 - Add CheckTdxFeaturesBeforeBuildPagetables to check Non-Tdx, Tdx-BSP or
   Tdx-APs. This routine is called before building page tables.

v4 changes:
 - Refine the PageTables64.asm and Flat32ToFlat64.asm to enable TDX.
 - Refine SEV_ES_WORK_AREA so that SEV/TDX/Legach guest all can use this
   memory region. https://edk2.groups.io/g/devel/message/78345 is the
   discussion.
 - AmdSev.asm is removed because Brijesh Singh has done it in
   https://edk2.groups.io/g/devel/message/78241.

v3 changes:
 - Refine PageTables64.asm and Flat32ToFlat64.asm based on the review
   comments in [ReviewComment-1] and [ReviewComment-2].
 - SEV codes are in AmdSev.asm
 - TDX codes are in IntelTdx.asm
 - Main.asm is created in OvmfPkg/ResetVector. The one in
   UefiCpuPkg/ResetVector/Vtf0 is not used.
 - Init32.asm/ReloadFlat32.asm in UefiCpuPkg/ResetVector/Vtf0/Ia32 are
   deleted. They're moved to OvmfPkg/ResetVector/Ia32.
 - InitTdx.asm is renamed to InteTdx.asm

v2 changes:
 - Move InitTdx.asm and ReloadFlat32.asm from UefiCpuPkg/ResetVector/Vtf0
   to OvmfPkg/ResetVector. Init32.asm is created which is a null stub of
   32-bit initialization. In Main32 just simply call Init32. It makes
   the Main.asm in UefiCpuPkg/ResetVector clean and clear.
 - Init32.asm/InitTdx.asm/ReloadFlat32.asm are created under
   OvmfPkg/ResetVector/Ia32.
 - Update some descriptions of the patch-sets.
 - Update the REF link in cover letter.
 - Add Ard Biesheuvel in Cc list.

v1: https://edk2.groups.io/g/devel/message/77675

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
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>


Min Xu (1):
  OvmfPkg: Enable TDX in ResetVector

 OvmfPkg/OvmfPkg.dec                          |   9 +
 OvmfPkg/OvmfPkgDefines.fdf.inc               |   9 +
 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm |  39 +++
 OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm  |  11 +
 OvmfPkg/ResetVector/Ia32/IntelTdx.asm        | 235 +++++++++++++++++++
 OvmfPkg/ResetVector/Ia32/PageTables64.asm    |  21 +-
 OvmfPkg/ResetVector/Main.asm                 | 119 ++++++++++
 OvmfPkg/ResetVector/ResetVector.inf          |   9 +
 OvmfPkg/ResetVector/ResetVector.nasmb        |  40 +++-
 OvmfPkg/ResetVector/X64/IntelTdxMetadata.asm | 102 ++++++++
 10 files changed, 587 insertions(+), 7 deletions(-)
 create mode 100644 OvmfPkg/ResetVector/Ia32/IntelTdx.asm
 create mode 100644 OvmfPkg/ResetVector/Main.asm
 create mode 100644 OvmfPkg/ResetVector/X64/IntelTdxMetadata.asm

-- 
2.29.2.windows.2

[PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Min Xu]

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

Intel's Trust Domain Extensions (Intel TDX) refers to an Intel technology
that extends Virtual Machines Extensions (VMX) and Multi-Key Total Memory
Encryption (MKTME) with a new kind of virutal machines guest called a
Trust Domain (TD). A TD is desinged to run in a CPU mode that protects the
confidentiality of TD memory contents and the TD's CPU state from other
software, including the hosting Virtual-Machine Monitor (VMM), unless
explicitly shared by the TD itself.

Note: Intel TDX is only available on X64, so the Tdx related changes are
in X64 path. In IA32 path, there may be null stub to make the build
success.

This patch includes below major changes.

1. Definition of BFV & CFV
Tdx Virtual Firmware (TDVF) includes one Firmware Volume (FV) known
as the Boot Firmware Volume (BFV). The FV format is defined in the
UEFI Platform Initialization (PI) spec. BFV includes all TDVF components
required during boot.

TDVF also include a configuration firmware volume (CFV) that is separated
from the BFV. The reason is because the CFV is measured in RTMR, while
the BFV is measured in MRTD.

In practice BFV is the code part of Ovmf image (OVMF_CODE.fd). CFV is the
vars part of Ovmf image (OVMF_VARS.fd).

2. X64/IntelTdxMetadata.asm
IntelTdxMetadata describes the information about the image for VMM use.
For example, the base address and length of the TdHob, TdMailbox, etc.
Its offset is put in a GUID-ed structure which is appended in the GUID-ed
chain from a fixed GPA (0xffffffd0). Below are the items in TdxMetadata:
 _Bfv:
    Boot Firmware Volume
 _Cfv:
    Configuration Firmware Volume
 _Stack:
    Initial stack
 _Heap:
    Initial heap
 _MailBox:
    TDVF reserves the memory region so each AP can receive the message
    sent by the guest OS.
 _OvmfWorkarea:
    Compute Confidential work area which is consumed by CC technologies,
    such as SEV, TDX.
 _TdHob:
    VMM pass the resource information in TdHob to TDVF.
 _OvmfPageTable:
    Initial page table for standard Ovmf.

TDVF indicates above chunks of temporary initialized memory region
(_Stack/_Heap/_MailBox/_OvmfWorkarea/_TdHob/_TdxPageTables/OvmfPageTable)
to support TDVF code finishing the memory initialization. Because the
other unaccepted memory cannot be accessed until they're accepted.

Since AMD SEV has already defined some SEV specific memory region in
MEMFD. TDX re-use the memory regions defined by SEV.
 - MailBox      : PcdOvmfSecGhcbBackupBase|PcdOvmfSecGhcbBackupSize
 - TdHob        : PcdOvmfSecGhcbBase|PcdOvmfSecGhcbSize

3. Ia32/IntelTdx.asm
IntelTdx.asm includes below routines used in ResetVector
 - IsTdx
   Check if the running system is Tdx guest.

 - InitTdxWorkarea
   It initialize the TDX_WORK_AREA. Because it is called by both BSP and
   APs and to avoid the race condition, only BSP can initialize the
   WORK_AREA. AP will wait until the field of TDX_WORK_AREA_PGTBL_READY
   is set.

 - ReloadFlat32
   After reset all CPUs in TDX are initialized to 32-bit protected mode.
   But GDT register is not set. So this routine loads the GDT and set the
   CR0, then jump to Flat 32 protected mode. After that CR4 and other
   registers are set.

 - InitTdx
   This routine wrap above 3 routines together to do Tdx initialization
   in ResetVector phase.

 - IsTdxEnabled
   It is a OneTimeCall to probe if TDX is enabled by checking the
   CC_WORK_AREA.

 - CheckTdxFeaturesBeforeBuildPagetables
   This routine is called to check if it is Non-TDX guest, TDX-Bsp or
   TDX-APs. Because in TDX guest all the initialization is done by BSP
   (including the page tables). APs should not build the tables.

 - TdxPostBuildPageTables
   It is called after Page Tables are built by BSP.
   byte[TDX_WORK_AREA_PGTBL_READY] is set by BSP to indicate APs can
   leave spin and go.

4. Main.asm
Previously OvmfPkg/ResetVector use the Main.asm in UefiCpuPkg. There is
only Main16 entry point. Main32 entry point is needed in Main.asm because
of Intel TDX. To reduce the complexity of Main.asm in UefiCpuPkg, OvmfPkg
create its own Main.asm to meet the requirement of Intel TDX. There are
below changes in this Main.asm:
 - A new entry point (Main32) is added. TDX guest will jump to Main32
   from ResetVecotr. In Main32, InitTdx is called to initialize TDX
   specific information.
 - In Main16 entry point, after TransitionFromReal16To32BitFlat,
   WORK_AREA_GUEST_TYPE is cleared to 0. WORK_AREA_GUEST_TYPE was
   previously cleared in SetCr3ForPageTables64 (see commit ab77b60).
   This doesn't work after TDX is introduced in Ovmf. It is because all
   TDX CPUs (BSP and APs) start to run from 0xfffffff0. In previous code
   WORK_AREA_GUEST_TYPE will be cleared multi-times in TDX guest. So for
   SEV and Legacy guest it is moved to Main16 entry point (after
   TransitionFromReal16To32BitFlat). For TDX guest WORK_AREA_GUEST_TYPE
   is cleared and set in InitTdxWorkarea.

5. Ia32/PageTables64.asm
As described in 6) only the TDX BSP build the page tables. So
PageTables64.asm is updated to make sure only TDX BSP build the
PageTables. TDX APs will skip the page table building and set Cr3
directly.

6. Ia16/ResetVectorVtf0.asm
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 ResetVectorVtf0.asm. It checks the 32-bit protected mode or 16-bit
real mode, then jump to the corresponding entry point.

7. ResetVector.nasmb
TDX related macros and files are added in ResetVecotr.nasmb.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
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/OvmfPkg.dec                          |   9 +
 OvmfPkg/OvmfPkgDefines.fdf.inc               |   9 +
 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm |  39 +++
 OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm  |  11 +
 OvmfPkg/ResetVector/Ia32/IntelTdx.asm        | 235 +++++++++++++++++++
 OvmfPkg/ResetVector/Ia32/PageTables64.asm    |  21 +-
 OvmfPkg/ResetVector/Main.asm                 | 119 ++++++++++
 OvmfPkg/ResetVector/ResetVector.inf          |   9 +
 OvmfPkg/ResetVector/ResetVector.nasmb        |  40 +++-
 OvmfPkg/ResetVector/X64/IntelTdxMetadata.asm | 102 ++++++++
 10 files changed, 587 insertions(+), 7 deletions(-)
 create mode 100644 OvmfPkg/ResetVector/Ia32/IntelTdx.asm
 create mode 100644 OvmfPkg/ResetVector/Main.asm
 create mode 100644 OvmfPkg/ResetVector/X64/IntelTdxMetadata.asm

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index c37dafad49bb..1e3342af6fac 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -340,6 +340,15 @@
   # header definition.
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader|0|UINT32|0x51
 
+  ## The base address and size of the TDX Cfv base and size.
+  gUefiOvmfPkgTokenSpaceGuid.PcdCfvBase|0|UINT32|0x52
+  gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataOffset|0|UINT32|0x53
+  gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataSize|0|UINT32|0x54
+
+  ## The base address and size of the TDX Bfv base and size.
+  gUefiOvmfPkgTokenSpaceGuid.PcdBfvBase|0|UINT32|0x55
+  gUefiOvmfPkgTokenSpaceGuid.PcdBfvRawDataOffset|0|UINT32|0x56
+  gUefiOvmfPkgTokenSpaceGuid.PcdBfvRawDataSize|0|UINT32|0x57
 
 [PcdsDynamic, PcdsDynamicEx]
   gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
diff --git a/OvmfPkg/OvmfPkgDefines.fdf.inc b/OvmfPkg/OvmfPkgDefines.fdf.inc
index 3b5e45253916..6170c5993ce5 100644
--- a/OvmfPkg/OvmfPkgDefines.fdf.inc
+++ b/OvmfPkg/OvmfPkgDefines.fdf.inc
@@ -9,6 +9,7 @@
 ##
 
 DEFINE BLOCK_SIZE        = 0x1000
+DEFINE VARS_OFFSET       = 0
 
 #
 # A firmware binary built with FD_SIZE_IN_KB=1024, and a firmware binary built
@@ -88,6 +89,14 @@ SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize = $(VARS_SPARE_
 # Computing Work Area header defined in the Include/WorkArea.h
 SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader  = 4
 
+SET gUefiOvmfPkgTokenSpaceGuid.PcdCfvBase           = $(FW_BASE_ADDRESS)
+SET gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataOffset  = $(VARS_OFFSET)
+SET gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataSize    = $(VARS_SIZE)
+
+SET gUefiOvmfPkgTokenSpaceGuid.PcdBfvBase           = $(CODE_BASE_ADDRESS)
+SET gUefiOvmfPkgTokenSpaceGuid.PcdBfvRawDataOffset  = $(VARS_SIZE)
+SET gUefiOvmfPkgTokenSpaceGuid.PcdBfvRawDataSize    = $(CODE_SIZE)
+
 !if $(SMM_REQUIRE) == TRUE
 SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase
 SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase
diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index 7ec3c6e980c3..76bc3aa00735 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -47,6 +47,25 @@ TIMES (15 - ((guidedStructureEnd - guidedStructureStart + 15) % 16)) DB 0
 ;
 guidedStructureStart:
 
+%ifdef ARCH_X64
+;
+; TDX Metadata offset block
+;
+; TdxMetadata.asm is included in ARCH_X64 because Inte TDX is only
+; available in ARCH_X64. Below block describes the offset of
+; TdxMetadata block in Ovmf image
+;
+; GUID : e47a6535-984a-4798-865e-4685a7bf8ec2
+;
+tdxMetadataOffsetStart:
+    DD      tdxMetadataOffsetStart - TdxMetadataGuid - 16
+    DW      tdxMetadataOffsetEnd - tdxMetadataOffsetStart
+    DB      0x35, 0x65, 0x7a, 0xe4, 0x4a, 0x98, 0x98, 0x47
+    DB      0x86, 0x5e, 0x46, 0x85, 0xa7, 0xbf, 0x8e, 0xc2
+tdxMetadataOffsetEnd:
+
+%endif
+
 ; SEV Hash Table Block
 ;
 ; This describes the guest ram area where the hypervisor should
@@ -158,10 +177,30 @@ 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
+
+    mov     eax, cr0
+    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..eb3546668ef8 100644
--- a/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
+++ b/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
@@ -21,6 +21,17 @@ Transition32FlatTo64Flat:
     bts     eax, 5                      ; enable PAE
     mov     cr4, eax
 
+    ;
+    ; In TDX LME has already been set. So we're done and jump to enable
+    ; paging directly if Tdx is enabled.
+    ; EBX is cleared because in the later it will be used to check if
+    ; the second step of the SEV-ES mitigation is to be performed.
+    ;
+    xor     ebx, ebx
+    OneTimeCall IsTdxEnabled
+    test    eax, eax
+    jnz     EnablePaging
+
     mov     ecx, 0xc0000080
     rdmsr
     bts     eax, 8                      ; set LME
diff --git a/OvmfPkg/ResetVector/Ia32/IntelTdx.asm b/OvmfPkg/ResetVector/Ia32/IntelTdx.asm
new file mode 100644
index 000000000000..122f6860a6ec
--- /dev/null
+++ b/OvmfPkg/ResetVector/Ia32/IntelTdx.asm
@@ -0,0 +1,235 @@
+;------------------------------------------------------------------------------
+; @file
+;   Intel TDX routines
+;
+; 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
+%define VM_GUEST_TDX     2
+
+BITS 32
+
+;
+; Check if it is Intel Tdx
+;
+; Modified: EAX, EBX, ECX, EDX
+;
+; If it is Intel Tdx, EAX is 1
+; If it is not Intel Tdx, EAX is 0
+;
+IsTdx:
+    ;
+    ; CPUID (0)
+    ;
+    mov     eax, 0
+    cpuid
+    cmp     ebx, 0x756e6547  ; "Genu"
+    jne     IsNotTdx
+    cmp     edx, 0x49656e69  ; "ineI"
+    jne     IsNotTdx
+    cmp     ecx, 0x6c65746e  ; "ntel"
+    jne     IsNotTdx
+
+    ;
+    ; CPUID (1)
+    ;
+    mov     eax, 1
+    cpuid
+    test    ecx, 0x80000000
+    jz      IsNotTdx
+
+    ;
+    ; CPUID[0].EAX >= 0x21?
+    ;
+    mov     eax, 0
+    cpuid
+    cmp     eax, 0x21
+    jl      IsNotTdx
+
+    ;
+    ; CPUID (0x21,0)
+    ;
+    mov     eax, 0x21
+    mov     ecx, 0
+    cpuid
+
+    cmp     ebx, 0x65746E49   ; "Inte"
+    jne     IsNotTdx
+    cmp     edx, 0x5844546C   ; "lTDX"
+    jne     IsNotTdx
+    cmp     ecx, 0x20202020   ; "    "
+    jne     IsNotTdx
+
+    mov     eax, 1
+    jmp     ExitIsTdx
+
+IsNotTdx:
+    xor     eax, eax
+
+ExitIsTdx:
+
+  OneTimeCallRet IsTdx
+
+;
+; Initialize work area if it is Tdx guest. Detailed definition is in
+; OvmfPkg/Include/WorkArea.h.
+; BSP and APs all go here. Only BSP initialize this work area.
+;
+; Param[in] EBP[5:0]    CPU Supported GPAW (48 or 52)
+; Param[in] ESI[31:0]   vCPU ID (BSP is 0, others are AP)
+;
+; Modified:  EBP
+;
+InitTdxWorkarea:
+
+    ;
+    ; First check if it is Tdx
+    ;
+    OneTimeCall IsTdx
+
+    test    eax, eax
+    jz      ExitInitTdxWorkarea
+
+    cmp     esi, 0
+    je      TdxBspEntry
+
+    ;
+    ; 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.
+    ;
+TdxApWait:
+    cmp     byte[TDX_WORK_AREA_PGTBL_READY], 0
+    je      TdxApWait
+    jmp     ExitInitTdxWorkarea
+
+TdxBspEntry:
+    ;
+    ; Set Type of WORK_AREA_GUEST_TYPE so that the following code can use
+    ; these information.
+    ;
+    mov     byte[WORK_AREA_GUEST_TYPE], VM_GUEST_TDX
+
+    ;
+    ; EBP[5:0] CPU supported GPA width
+    ;
+    and     ebp, 0x3f
+    mov     DWORD[TDX_WORK_AREA_GPAW], ebp
+
+ExitInitTdxWorkarea:
+    OneTimeCallRet InitTdxWorkarea
+
+;
+; Load the GDT and set the CR0, then jump to Flat 32 protected mode.
+;
+; 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)
+
+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
+
+;
+; Tdx initialization after entering into ResetVector
+;
+; Modified:  EAX, EBX, ECX, EDX, EBP, EDI, ESP
+;
+InitTdx:
+    ;
+    ; Save EBX in EBP because EBX will be changed in ReloadFlat32
+    ;
+    mov     ebp, ebx
+
+    ;
+    ; First load the GDT and jump to Flat32 mode
+    ;
+    OneTimeCall ReloadFlat32
+
+    ;
+    ; Initialization of Tdx work area
+    ;
+    OneTimeCall  InitTdxWorkarea
+
+    OneTimeCallRet InitTdx
+
+;
+; Check TDX features, TDX or TDX-BSP or TDX-APs?
+;
+; By design TDX BSP is reponsible for initializing the PageTables.
+; After PageTables are ready, byte[TDX_WORK_AREA_PGTBL_READY] is set to 1.
+; APs will spin when byte[TDX_WORK_AREA_PGTBL_READY] is 0 until it is set to 1.
+;
+; When this routine is run on TDX BSP, byte[TDX_WORK_AREA_PGTBL_READY] should be 0.
+; When this routine is run on TDX APs, byte[TDX_WORK_AREA_PGTBL_READY] should be 1.
+;
+;
+; Modified:  EAX, EDX
+;
+; 0-NonTdx, 1-TdxBsp, 2-TdxAps
+;
+CheckTdxFeaturesBeforeBuildPagetables:
+    xor     eax, eax
+    cmp     byte[WORK_AREA_GUEST_TYPE], VM_GUEST_TDX
+    jne     NotTdx
+
+    xor     edx, edx
+    mov     al, byte[TDX_WORK_AREA_PGTBL_READY]
+    inc     eax
+
+NotTdx:
+    OneTimeCallRet CheckTdxFeaturesBeforeBuildPagetables
+
+;
+; Set byte[TDX_WORK_AREA_PGTBL_READY] to 1
+;
+TdxPostBuildPageTables:
+    cmp     byte[WORK_AREA_GUEST_TYPE], VM_GUEST_TDX
+    jne     ExitTdxPostBuildPageTables
+    mov     byte[TDX_WORK_AREA_PGTBL_READY], 1
+
+ExitTdxPostBuildPageTables:
+    OneTimeCallRet TdxPostBuildPageTables
+
+;
+; Check if TDX is enabled
+;
+; Modified:  EAX
+;
+; If TDX is enabled then EAX will be 1
+; If TDX is disabled then EAX will be 0.
+;
+IsTdxEnabled:
+    xor     eax, eax
+    cmp     byte[WORK_AREA_GUEST_TYPE], VM_GUEST_TDX
+    jne     TdxNotEnabled
+    mov     eax, 1
+
+TdxNotEnabled:
+    OneTimeCallRet IsTdxEnabled
diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index 07b6ca070909..dc640dd2bf58 100644
--- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
+++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
@@ -37,14 +37,23 @@ BITS    32
                        PAGE_READ_WRITE + \
                        PAGE_PRESENT)
 
+%define TDX_BSP         1
+%define TDX_AP          2
+
 ;
 ; Modified:  EAX, EBX, ECX, EDX
 ;
 SetCr3ForPageTables64:
-
-    ; Clear the WorkArea header. The SEV probe routines will populate the
-    ; work area when detected.
-    mov     byte[WORK_AREA_GUEST_TYPE], 0
+    ; Check the TDX features.
+    ; If it is TDX APs, then jump to SetCr3 directly.
+    ; In TD guest the initialization is done by BSP, including building
+    ; the page tables. APs will spin on until byte[TDX_WORK_AREA_PGTBL_READY]
+    ; is set.
+    OneTimeCall   CheckTdxFeaturesBeforeBuildPagetables
+    cmp       eax, TDX_BSP
+    je        ClearOvmfPageTables
+    cmp       eax, TDX_AP
+    je        SetCr3
 
     ; Check whether the SEV is active and populate the SevEsWorkArea
     OneTimeCall   CheckSevFeatures
@@ -54,6 +63,7 @@ SetCr3ForPageTables64:
     ; the page table build below.
     OneTimeCall   GetSevCBitMaskAbove31
 
+ClearOvmfPageTables:
     ;
     ; For OVMF, build some initial page tables at
     ; PcdOvmfSecPageTablesBase - (PcdOvmfSecPageTablesBase + 0x6000).
@@ -105,6 +115,9 @@ pageTableEntriesLoop:
     ; Clear the C-bit from the GHCB page if the SEV-ES is enabled.
     OneTimeCall   SevClearPageEncMaskForGhcbPage
 
+    ; Set byte[TDX_WORK_AREA_PGTBL_READY] if TDX is enabled.
+    OneTimeCall   TdxPostBuildPageTables
+
 SetCr3:
     ;
     ; Set CR3 now that the paging structures are available
diff --git a/OvmfPkg/ResetVector/Main.asm b/OvmfPkg/ResetVector/Main.asm
new file mode 100644
index 000000000000..2a7efbc48a2a
--- /dev/null
+++ b/OvmfPkg/ResetVector/Main.asm
@@ -0,0 +1,119 @@
+;------------------------------------------------------------------------------
+; @file
+; Main routine of the pre-SEC code up through the jump into SEC
+;
+; Copyright (c) 2008 - 2009, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+
+BITS    16
+
+;
+; Modified:  EBX, ECX, EDX, EBP
+;
+; @param[in,out]  RAX/EAX  Initial value of the EAX register
+;                          (BIST: Built-in Self Test)
+; @param[in,out]  DI       'BP': boot-strap processor, or
+;                          'AP': application processor
+; @param[out]     RBP/EBP  Address of Boot Firmware Volume (BFV)
+; @param[out]     DS       Selector allowing flat access to all addresses
+; @param[out]     ES       Selector allowing flat access to all addresses
+; @param[out]     FS       Selector allowing flat access to all addresses
+; @param[out]     GS       Selector allowing flat access to all addresses
+; @param[out]     SS       Selector allowing flat access to all addresses
+;
+; @return         None  This routine jumps to SEC and does not return
+;
+Main16:
+    OneTimeCall EarlyInit16
+
+    ;
+    ; Transition the processor from 16-bit real mode to 32-bit flat mode
+    ;
+    OneTimeCall TransitionFromReal16To32BitFlat
+
+BITS    32
+%ifdef ARCH_X64
+
+    ; Clear the WorkArea header. The SEV probe routines will populate the
+    ; work area when detected.
+    mov     byte[WORK_AREA_GUEST_TYPE], 0
+
+    jmp SearchBfv
+
+;
+; Entry point of Main32
+;
+Main32:
+    OneTimeCall InitTdx
+
+SearchBfv:
+
+%endif
+    ;
+    ; Search for the Boot Firmware Volume (BFV)
+    ;
+    OneTimeCall Flat32SearchForBfvBase
+
+    ;
+    ; EBP - Start of BFV
+    ;
+
+    ;
+    ; Search for the SEC entry point
+    ;
+    OneTimeCall Flat32SearchForSecEntryPoint
+
+    ;
+    ; ESI - SEC Core entry point
+    ; EBP - Start of BFV
+    ;
+
+%ifdef ARCH_IA32
+
+    ;
+    ; Restore initial EAX value into the EAX register
+    ;
+    mov     eax, esp
+
+    ;
+    ; Jump to the 32-bit SEC entry point
+    ;
+    jmp     esi
+
+%else
+
+    ;
+    ; Transition the processor from 32-bit flat mode to 64-bit flat mode
+    ;
+    OneTimeCall Transition32FlatTo64Flat
+
+BITS    64
+
+    ;
+    ; Some values were calculated in 32-bit mode.  Make sure the upper
+    ; 32-bits of 64-bit registers are zero for these values.
+    ;
+    mov     rax, 0x00000000ffffffff
+    and     rsi, rax
+    and     rbp, rax
+    and     rsp, rax
+
+    ;
+    ; RSI - SEC Core entry point
+    ; RBP - Start of BFV
+    ;
+
+    ;
+    ; Restore initial EAX value into the RAX register
+    ;
+    mov     rax, rsp
+
+    ;
+    ; Jump to the 64-bit SEC entry point
+    ;
+    jmp     rsi
+
+%endif
diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
index a2520dde5508..320e5f2c6527 100644
--- a/OvmfPkg/ResetVector/ResetVector.inf
+++ b/OvmfPkg/ResetVector/ResetVector.inf
@@ -44,6 +44,15 @@
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
+  gUefiOvmfPkgTokenSpaceGuid.PcdCfvBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataOffset
+  gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataSize
+  gUefiOvmfPkgTokenSpaceGuid.PcdBfvBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdBfvRawDataOffset
+  gUefiOvmfPkgTokenSpaceGuid.PcdBfvRawDataSize
 
 [FixedPcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
index d1d800c56745..5f30d099a7f1 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -67,8 +67,39 @@
     %error "This implementation inherently depends on PcdOvmfSecGhcbBase not straddling a 2MB boundary"
   %endif
 
+  %define TDX_BFV_RAW_DATA_OFFSET   FixedPcdGet32 (PcdBfvRawDataOffset)
+  %define TDX_BFV_RAW_DATA_SIZE     FixedPcdGet32 (PcdBfvRawDataSize)
+  %define TDX_BFV_MEMORY_BASE       FixedPcdGet32 (PcdBfvBase)
+  %define TDX_BFV_MEMORY_SIZE       FixedPcdGet32 (PcdBfvRawDataSize)
+
+  %define TDX_CFV_RAW_DATA_OFFSET   FixedPcdGet32 (PcdCfvRawDataOffset)
+  %define TDX_CFV_RAW_DATA_SIZE     FixedPcdGet32 (PcdCfvRawDataSize)
+  %define TDX_CFV_MEMORY_BASE       FixedPcdGet32 (PcdCfvBase),
+  %define TDX_CFV_MEMORY_SIZE       FixedPcdGet32 (PcdCfvRawDataSize),
+
+  %define TDX_HEAP_MEMORY_BASE      FixedPcdGet32 (PcdOvmfSecPeiTempRamBase)
+  %define TDX_HEAP_MEMORY_SIZE      FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) / 2
+
+  %define TDX_STACK_MEMORY_BASE     (TDX_HEAP_MEMORY_BASE + TDX_HEAP_MEMORY_SIZE)
+  %define TDX_STACK_MEMORY_SIZE     FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) / 2
+
+  %define TDX_HOB_MEMORY_BASE       FixedPcdGet32 (PcdOvmfSecGhcbBase)
+  %define TDX_HOB_MEMORY_SIZE       FixedPcdGet32 (PcdOvmfSecGhcbSize)
+
+  %define TDX_MAILBOX_MEMORY_BASE   FixedPcdGet32 (PcdOvmfSecGhcbBackupBase)
+  %define TDX_MAILBOX_MEMORY_SIZE   FixedPcdGet32 (PcdOvmfSecGhcbBackupSize)
+
+  %define OVMF_PAGE_TABLE_BASE      FixedPcdGet32 (PcdOvmfSecPageTablesBase)
+  %define OVMF_PAGE_TABLE_SIZE      FixedPcdGet32 (PcdOvmfSecPageTablesSize)
+
+  %define TDX_WORK_AREA_PGTBL_READY (FixedPcdGet32 (PcdOvmfWorkAreaBase) + 4)
+  %define TDX_WORK_AREA_GPAW        (FixedPcdGet32 (PcdOvmfWorkAreaBase) + 8)
+
   %define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + (Offset))
 
+  %define OVMF_WORK_AREA_BASE (FixedPcdGet32 (PcdOvmfWorkAreaBase))
+  %define OVMF_WORK_AREA_SIZE (FixedPcdGet32 (PcdOvmfWorkAreaSize))
+
   %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
   %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
   %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
@@ -77,9 +108,12 @@
   %define SEV_ES_WORK_AREA_RDRAND (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 8)
   %define SEV_ES_WORK_AREA_ENC_MASK (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 16)
   %define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
-%include "Ia32/Flat32ToFlat64.asm"
-%include "Ia32/AmdSev.asm"
-%include "Ia32/PageTables64.asm"
+
+  %include "X64/IntelTdxMetadata.asm"
+  %include "Ia32/Flat32ToFlat64.asm"
+  %include "Ia32/AmdSev.asm"
+  %include "Ia32/PageTables64.asm"
+  %include "Ia32/IntelTdx.asm"
 %endif
 
 %include "Ia16/Real16ToFlat32.asm"
diff --git a/OvmfPkg/ResetVector/X64/IntelTdxMetadata.asm b/OvmfPkg/ResetVector/X64/IntelTdxMetadata.asm
new file mode 100644
index 000000000000..18e10931bbc2
--- /dev/null
+++ b/OvmfPkg/ResetVector/X64/IntelTdxMetadata.asm
@@ -0,0 +1,102 @@
+;------------------------------------------------------------------------------
+; @file
+; Tdx Virtual Firmware metadata
+;
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+BITS    64
+
+%define TDX_METADATA_SECTION_TYPE_BFV       0
+%define TDX_METADATA_SECTION_TYPE_CFV       1
+%define TDX_METADATA_SECTION_TYPE_TD_HOB    2
+%define TDX_METADATA_SECTION_TYPE_TEMP_MEM  3
+%define TDX_METADATA_VERSION                1
+%define TDX_METADATA_ATTRIBUTES_EXTENDMR    0x00000001
+
+ALIGN   16
+TIMES (15 - ((TdxGuidedStructureEnd - TdxGuidedStructureStart + 15) % 16)) DB 0
+
+TdxGuidedStructureStart:
+
+;
+; TDVF meta data
+;
+TdxMetadataGuid:
+  DB  0xf3, 0xf9, 0xea, 0xe9, 0x8e, 0x16, 0xd5, 0x44
+  DB  0xa8, 0xeb, 0x7f, 0x4d, 0x87, 0x38, 0xf6, 0xae
+
+_Descriptor:
+  DB 'T','D','V','F'                                  ; Signature
+  DD TdxGuidedStructureEnd - _Descriptor              ; Length
+  DD TDX_METADATA_VERSION                             ; Version
+  DD (TdxGuidedStructureEnd - _Descriptor - 16)/32    ; Number of sections
+
+_Bfv:
+  DD TDX_BFV_RAW_DATA_OFFSET
+  DD TDX_BFV_RAW_DATA_SIZE
+  DQ TDX_BFV_MEMORY_BASE
+  DQ TDX_BFV_MEMORY_SIZE
+  DD TDX_METADATA_SECTION_TYPE_BFV
+  DD TDX_METADATA_ATTRIBUTES_EXTENDMR
+
+_Cfv:
+  DD TDX_CFV_RAW_DATA_OFFSET
+  DD TDX_CFV_RAW_DATA_SIZE
+  DQ TDX_CFV_MEMORY_BASE
+  DQ TDX_CFV_MEMORY_SIZE
+  DD TDX_METADATA_SECTION_TYPE_CFV
+  DD 0
+
+_Stack:
+  DD 0
+  DD 0
+  DQ TDX_STACK_MEMORY_BASE
+  DQ TDX_STACK_MEMORY_SIZE
+  DD TDX_METADATA_SECTION_TYPE_TEMP_MEM
+  DD 0
+
+_Heap:
+  DD 0
+  DD 0
+  DQ TDX_HEAP_MEMORY_BASE
+  DQ TDX_HEAP_MEMORY_SIZE
+  DD TDX_METADATA_SECTION_TYPE_TEMP_MEM
+  DD 0
+
+_MailBox:
+  DD 0
+  DD 0
+  DQ TDX_MAILBOX_MEMORY_BASE
+  DQ TDX_MAILBOX_MEMORY_SIZE
+  DD TDX_METADATA_SECTION_TYPE_TEMP_MEM
+  DD 0
+
+_OvmfWorkarea:
+  DD 0
+  DD 0
+  DQ OVMF_WORK_AREA_BASE
+  DQ OVMF_WORK_AREA_SIZE
+  DD TDX_METADATA_SECTION_TYPE_TEMP_MEM
+  DD 0
+
+_TdHob:
+  DD 0
+  DD 0
+  DQ TDX_HOB_MEMORY_BASE
+  DQ TDX_HOB_MEMORY_SIZE
+  DD TDX_METADATA_SECTION_TYPE_TD_HOB
+  DD 0
+
+_OvmfPageTable:
+  DD 0
+  DD 0
+  DQ OVMF_PAGE_TABLE_BASE
+  DQ OVMF_PAGE_TABLE_SIZE
+  DD TDX_METADATA_SECTION_TYPE_TEMP_MEM
+  DD 0
+
+TdxGuidedStructureEnd:
+ALIGN   16
-- 
2.29.2.windows.2

Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Gerd Hoffmann]

  Hi,

> +%ifdef ARCH_X64
> +;
> +; TDX Metadata offset block
> +;
> +; TdxMetadata.asm is included in ARCH_X64 because Inte TDX is only
> +; available in ARCH_X64. Below block describes the offset of
> +; TdxMetadata block in Ovmf image
> +;
> +; GUID : e47a6535-984a-4798-865e-4685a7bf8ec2
> +;
> +tdxMetadataOffsetStart:
> +    DD      tdxMetadataOffsetStart - TdxMetadataGuid - 16
> +    DW      tdxMetadataOffsetEnd - tdxMetadataOffsetStart
> +    DB      0x35, 0x65, 0x7a, 0xe4, 0x4a, 0x98, 0x98, 0x47
> +    DB      0x86, 0x5e, 0x46, 0x85, 0xa7, 0xbf, 0x8e, 0xc2
> +tdxMetadataOffsetEnd:
> +
> +%endif

This should be switched to common ovmf metadata (see patches 4-7 of the
SEV-SNP series).

Min: please have a look at these patches.

Brijesh: It might be useful to post the metadata patches as separate
series.

> +; Load the GDT and set the CR0, then jump to Flat 32 protected mode.

That comment isn't correct, you are already in 32-bit mode.

> +; Modified:  EAX, EBX, CR0, CR4, DS, ES, FS, GS, SS

CS too ...

> +    jmp     LINEAR_CODE_SEL:dword ADDR_OF(jumpToFlat32BitAndLandHere)
> +jumpToFlat32BitAndLandHere:

... right here.

> --- /dev/null
> +++ b/OvmfPkg/ResetVector/Main.asm

Can you add a separate patch for "copy Main.asm from UefiCpuPkg
unmodified" please?  Having the changes for TDX separately is helpful
for review.

take care,
  Gerd

Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Min Xu]

On September 22, 2021 3:49 PM, Gerd Hoffmann wrote:
>   Hi,
> 
> > +%ifdef ARCH_X64
> > +;
> > +; TDX Metadata offset block
> > +;
> > +; TdxMetadata.asm is included in ARCH_X64 because Inte TDX is only ;
> > +available in ARCH_X64. Below block describes the offset of ;
> > +TdxMetadata block in Ovmf image ; ; GUID :
> > +e47a6535-984a-4798-865e-4685a7bf8ec2
> > +;
> > +tdxMetadataOffsetStart:
> > +    DD      tdxMetadataOffsetStart - TdxMetadataGuid - 16
> > +    DW      tdxMetadataOffsetEnd - tdxMetadataOffsetStart
> > +    DB      0x35, 0x65, 0x7a, 0xe4, 0x4a, 0x98, 0x98, 0x47
> > +    DB      0x86, 0x5e, 0x46, 0x85, 0xa7, 0xbf, 0x8e, 0xc2
> > +tdxMetadataOffsetEnd:
> > +
> > +%endif
> 
> This should be switched to common ovmf metadata (see patches 4-7 of the
> SEV-SNP series).
> 
> Min: please have a look at these patches.
>
Hi, Gerd
I checked the patches 4-7 of the SEV-SNP series. The common OvmfMetadata is designed for both SEV and TDX, right? 
If so, then it means the SEV and TDX metadata will be mixed in this OvmfMetadata. I am thinking there will always be different fields for SEV and TDX. For example, SEV has PcdOvmfSecGhcbPageTable but TDX doesn't need that page. If the common OvmfMetadata is consumed by TDX-QEMU, then PcdOvmfSecGhcbPageTableBase will be initialized too. That doesn't make sense.
I am thinking that SEV and TDX can keep their own Metadata (in separate files, SevMetadata.asm and TdxMetadata.asm) which are pointed by the SEV or TDX offsets in the GUID-ed chain in ResetVector. In this case, SEV and TDX can design their own metadata flexibly, for example, the attribute, the item structure, add/remove/update the items, etc. And it will be more friendly to the reviewer for the Metadata, at least from the name of the items.
> 
> Brijesh: It might be useful to post the metadata patches as separate series.
> 
> > +; Load the GDT and set the CR0, then jump to Flat 32 protected mode.
> 
> That comment isn't correct, you are already in 32-bit mode.
Thanks. It will be updated in the next version.
> 
> > +; Modified:  EAX, EBX, CR0, CR4, DS, ES, FS, GS, SS
> 
> CS too ...
It will be fixed in the next version.
> 
> > +    jmp     LINEAR_CODE_SEL:dword
> ADDR_OF(jumpToFlat32BitAndLandHere)
> > +jumpToFlat32BitAndLandHere:
> 
> ... right here.
> 
> > --- /dev/null
> > +++ b/OvmfPkg/ResetVector/Main.asm
> 
> Can you add a separate patch for "copy Main.asm from UefiCpuPkg
> unmodified" please?  Having the changes for TDX separately is helpful for
> review.
Sure. It will be separated in the next version.
> 

Thanks!
Min

Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Gerd Hoffmann]

On Thu, Sep 23, 2021 at 12:38:24AM +0000, Xu, Min M wrote:
> On September 22, 2021 3:49 PM, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > +%ifdef ARCH_X64
> > > +;
> > > +; TDX Metadata offset block
> > > +;
> > > +; TdxMetadata.asm is included in ARCH_X64 because Inte TDX is only ;
> > > +available in ARCH_X64. Below block describes the offset of ;
> > > +TdxMetadata block in Ovmf image ; ; GUID :
> > > +e47a6535-984a-4798-865e-4685a7bf8ec2
> > > +;
> > > +tdxMetadataOffsetStart:
> > > +    DD      tdxMetadataOffsetStart - TdxMetadataGuid - 16
> > > +    DW      tdxMetadataOffsetEnd - tdxMetadataOffsetStart
> > > +    DB      0x35, 0x65, 0x7a, 0xe4, 0x4a, 0x98, 0x98, 0x47
> > > +    DB      0x86, 0x5e, 0x46, 0x85, 0xa7, 0xbf, 0x8e, 0xc2
> > > +tdxMetadataOffsetEnd:
> > > +
> > > +%endif
> > 
> > This should be switched to common ovmf metadata (see patches 4-7 of the
> > SEV-SNP series).
> > 
> > Min: please have a look at these patches.
> >

> Hi, Gerd
> I checked the patches 4-7 of the SEV-SNP series. The common
> OvmfMetadata is designed for both SEV and TDX, right? 

That is the idea, yes.

> If so, then it means the SEV and TDX metadata will be mixed in this
> OvmfMetadata.

Yes.

> I am thinking there will always be different fields for
> SEV and TDX. For example, SEV has PcdOvmfSecGhcbPageTable but TDX
> doesn't need that page. If the common OvmfMetadata is consumed by
> TDX-QEMU, then PcdOvmfSecGhcbPageTableBase will be initialized too.
> That doesn't make sense.

We have different range types.  OVMF_* are the common areas.  SEV_* will
be used by sev only, TDX_* will be used by tdx only.  TDX and SEV
entries are allowed to overlap, i.e. PcdOvmfSecGhcbPageTableBase should
have some SEV_* type for sev (I think this needs fixing in the series),
and tdx can use the page for something else by adding an TDX_* entry for
the same range.

> I am thinking that SEV and TDX can keep their own Metadata (in
> separate files, SevMetadata.asm and TdxMetadata.asm) which are pointed
> by the SEV or TDX offsets in the GUID-ed chain in ResetVector.

I'd very much prefer to have a single table to avoid duplication for the
common memory areas and keep the reset vector small.

Having separate SevMetadata.asm + TdxMetadata.asm files (then have
OvmfMetadata.asm include these two) is an option.  I think this isn't
needed, we can also just group the entries in OvmfMetadata.asm.

> In this case, SEV and TDX can design their own metadata flexibly, for
> example, the attribute, the item structure, add/remove/update the
> items, etc.

Why have two ways to do the same thing?

take care,
  Gerd

Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Yao, Jiewen]

I strongly recommend to separate SEV and TDX in all context, if it is something SEV or TDX specific.
Then each file has clear ownership.
If it is something generic for both SEV and TDX, it can in one file. 

For example, SecPeiTempRam/SecPageTable can be in common file.
But SevSnpSecrets/GhcbBookkeeping should be in SEV file.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Thursday, September 23, 2021 4:48 PM
> To: Xu, Min M <min.m.xu@intel.com>
> Cc: devel@edk2.groups.io; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen,
> Jordan L <jordan.l.justen@intel.com>; 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: Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
> On Thu, Sep 23, 2021 at 12:38:24AM +0000, Xu, Min M wrote:
> > On September 22, 2021 3:49 PM, Gerd Hoffmann wrote:
> > >   Hi,
> > >
> > > > +%ifdef ARCH_X64
> > > > +;
> > > > +; TDX Metadata offset block
> > > > +;
> > > > +; TdxMetadata.asm is included in ARCH_X64 because Inte TDX is only ;
> > > > +available in ARCH_X64. Below block describes the offset of ;
> > > > +TdxMetadata block in Ovmf image ; ; GUID :
> > > > +e47a6535-984a-4798-865e-4685a7bf8ec2
> > > > +;
> > > > +tdxMetadataOffsetStart:
> > > > +    DD      tdxMetadataOffsetStart - TdxMetadataGuid - 16
> > > > +    DW      tdxMetadataOffsetEnd - tdxMetadataOffsetStart
> > > > +    DB      0x35, 0x65, 0x7a, 0xe4, 0x4a, 0x98, 0x98, 0x47
> > > > +    DB      0x86, 0x5e, 0x46, 0x85, 0xa7, 0xbf, 0x8e, 0xc2
> > > > +tdxMetadataOffsetEnd:
> > > > +
> > > > +%endif
> > >
> > > This should be switched to common ovmf metadata (see patches 4-7 of the
> > > SEV-SNP series).
> > >
> > > Min: please have a look at these patches.
> > >
> 
> > Hi, Gerd
> > I checked the patches 4-7 of the SEV-SNP series. The common
> > OvmfMetadata is designed for both SEV and TDX, right?
> 
> That is the idea, yes.
> 
> > If so, then it means the SEV and TDX metadata will be mixed in this
> > OvmfMetadata.
> 
> Yes.
> 
> > I am thinking there will always be different fields for
> > SEV and TDX. For example, SEV has PcdOvmfSecGhcbPageTable but TDX
> > doesn't need that page. If the common OvmfMetadata is consumed by
> > TDX-QEMU, then PcdOvmfSecGhcbPageTableBase will be initialized too.
> > That doesn't make sense.
> 
> We have different range types.  OVMF_* are the common areas.  SEV_* will
> be used by sev only, TDX_* will be used by tdx only.  TDX and SEV
> entries are allowed to overlap, i.e. PcdOvmfSecGhcbPageTableBase should
> have some SEV_* type for sev (I think this needs fixing in the series),
> and tdx can use the page for something else by adding an TDX_* entry for
> the same range.
> 
> > I am thinking that SEV and TDX can keep their own Metadata (in
> > separate files, SevMetadata.asm and TdxMetadata.asm) which are pointed
> > by the SEV or TDX offsets in the GUID-ed chain in ResetVector.
> 
> I'd very much prefer to have a single table to avoid duplication for the
> common memory areas and keep the reset vector small.
> 
> Having separate SevMetadata.asm + TdxMetadata.asm files (then have
> OvmfMetadata.asm include these two) is an option.  I think this isn't
> needed, we can also just group the entries in OvmfMetadata.asm.
> 
> > In this case, SEV and TDX can design their own metadata flexibly, for
> > example, the attribute, the item structure, add/remove/update the
> > items, etc.
> 
> Why have two ways to do the same thing?
> 
> take care,
>   Gerd

Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Brijesh Singh]

Like Gerd I would prefer to have one metadata table in the reset GUID.
The metadata table will contain multiple entries; lot of entries are
common between SNP and TDX. Some entries will have specific meaning for
the platform. Those special entries should be marked using the
OVMF_SECTION_TYPE_{TDX,SNP}_XXXX. It is perfectly fine to have a more
than one entry for the same region with different type, e.g

GhcbBookkeepingSnp:

  GHCB_BOOKKEPING_BASE_ADDRESS

  GHCB_BOOKKEEPING_SIZE

  OVMF_SECTION_TYPE_SNP_MEM

TdxMailBoxExt:

  GHCB_BOOKKEPING_BASE_ADDRESS

  GHCB_BOOKKEEPING_SIZE

  OVMF_SECTION_TYPE_TDX_MAILBOX

If we want all the OVMF_SECTION_TYPE_SNP_xxx should be defined in a
separate file then that is also doable. I put everything in one place
because I was trying to keep entry order similar to what is present in
MEMFD.

thanks

On 9/23/21 6:39 AM, Yao, Jiewen wrote:
> I strongly recommend to separate SEV and TDX in all context, if it is something SEV or TDX specific.
> Then each file has clear ownership.
> If it is something generic for both SEV and TDX, it can in one file. 
>
> For example, SecPeiTempRam/SecPageTable can be in common file.
> But SevSnpSecrets/GhcbBookkeeping should be in SEV file.
>
> Thank you
> Yao Jiewen
>
>> -----Original Message-----
>> From: Gerd Hoffmann <kraxel@redhat.com>
>> Sent: Thursday, September 23, 2021 4:48 PM
>> To: Xu, Min M <min.m.xu@intel.com>
>> Cc: devel@edk2.groups.io; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen,
>> Jordan L <jordan.l.justen@intel.com>; 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: Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
>>
>> On Thu, Sep 23, 2021 at 12:38:24AM +0000, Xu, Min M wrote:
>>> On September 22, 2021 3:49 PM, Gerd Hoffmann wrote:
>>>>   Hi,
>>>>
>>>>> +%ifdef ARCH_X64
>>>>> +;
>>>>> +; TDX Metadata offset block
>>>>> +;
>>>>> +; TdxMetadata.asm is included in ARCH_X64 because Inte TDX is only ;
>>>>> +available in ARCH_X64. Below block describes the offset of ;
>>>>> +TdxMetadata block in Ovmf image ; ; GUID :
>>>>> +e47a6535-984a-4798-865e-4685a7bf8ec2
>>>>> +;
>>>>> +tdxMetadataOffsetStart:
>>>>> +    DD      tdxMetadataOffsetStart - TdxMetadataGuid - 16
>>>>> +    DW      tdxMetadataOffsetEnd - tdxMetadataOffsetStart
>>>>> +    DB      0x35, 0x65, 0x7a, 0xe4, 0x4a, 0x98, 0x98, 0x47
>>>>> +    DB      0x86, 0x5e, 0x46, 0x85, 0xa7, 0xbf, 0x8e, 0xc2
>>>>> +tdxMetadataOffsetEnd:
>>>>> +
>>>>> +%endif
>>>> This should be switched to common ovmf metadata (see patches 4-7 of the
>>>> SEV-SNP series).
>>>>
>>>> Min: please have a look at these patches.
>>>>
>>> Hi, Gerd
>>> I checked the patches 4-7 of the SEV-SNP series. The common
>>> OvmfMetadata is designed for both SEV and TDX, right?
>> That is the idea, yes.
>>
>>> If so, then it means the SEV and TDX metadata will be mixed in this
>>> OvmfMetadata.
>> Yes.
>>
>>> I am thinking there will always be different fields for
>>> SEV and TDX. For example, SEV has PcdOvmfSecGhcbPageTable but TDX
>>> doesn't need that page. If the common OvmfMetadata is consumed by
>>> TDX-QEMU, then PcdOvmfSecGhcbPageTableBase will be initialized too.
>>> That doesn't make sense.
>> We have different range types.  OVMF_* are the common areas.  SEV_* will
>> be used by sev only, TDX_* will be used by tdx only.  TDX and SEV
>> entries are allowed to overlap, i.e. PcdOvmfSecGhcbPageTableBase should
>> have some SEV_* type for sev (I think this needs fixing in the series),
>> and tdx can use the page for something else by adding an TDX_* entry for
>> the same range.
>>
>>> I am thinking that SEV and TDX can keep their own Metadata (in
>>> separate files, SevMetadata.asm and TdxMetadata.asm) which are pointed
>>> by the SEV or TDX offsets in the GUID-ed chain in ResetVector.
>> I'd very much prefer to have a single table to avoid duplication for the
>> common memory areas and keep the reset vector small.
>>
>> Having separate SevMetadata.asm + TdxMetadata.asm files (then have
>> OvmfMetadata.asm include these two) is an option.  I think this isn't
>> needed, we can also just group the entries in OvmfMetadata.asm.
>>
>>> In this case, SEV and TDX can design their own metadata flexibly, for
>>> example, the attribute, the item structure, add/remove/update the
>>> items, etc.
>> Why have two ways to do the same thing?
>>
>> take care,
>>   Gerd

Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Yao, Jiewen]

The metadata table definition for TDX is at https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-virtual-firmware-design-guide-rev-1.pdf, Chapter 11.2 TDVF descriptor. And we will add more entry there.
May I get a proposed SEV or OVMF meta-table definition somewhere?
Before we get a clear definition for SEV meta-table, I feel it is too early to say one-table.

Another thing I would like to claim *TDVF metadata design principle*: We treat metadata is the *interface* between TDVF and VMM.
The metadata table should contain and only contain the information that is consumed by VMM.
If it is something VMM does not need to know, then it should NOT be in metadata table.



For https://github.com/AMDESE/ovmf/blob/snp-v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm, I am not sure why we put below structure there.
ExtractHandlerTable:
  DD  GUID_EXTRACT_HANDLER_TABLE_BASE
  DD  GUID_EXTRACT_HANDLER_TABLE_SIZE
  DD  OVMF_SECTION_TYPE_SEC_MEM

Does VMM need to know the Extraction Handler Table???

Thank you
Yao Jiewen

> -----Original Message-----
> From: Brijesh Singh <brijesh.singh@amd.com>
> Sent: Thursday, September 23, 2021 8:55 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Xu, Min M <min.m.xu@intel.com>
> Cc: devel@edk2.groups.io; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Erdem Aktas <erdemaktas@google.com>;
> James Bottomley <jejb@linux.ibm.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> Subject: Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
> Like Gerd I would prefer to have one metadata table in the reset GUID.
> The metadata table will contain multiple entries; lot of entries are
> common between SNP and TDX. Some entries will have specific meaning for
> the platform. Those special entries should be marked using the
> OVMF_SECTION_TYPE_{TDX,SNP}_XXXX. It is perfectly fine to have a more
> than one entry for the same region with different type, e.g
> 
> GhcbBookkeepingSnp:
> 
>   GHCB_BOOKKEPING_BASE_ADDRESS
> 
>   GHCB_BOOKKEEPING_SIZE
> 
>   OVMF_SECTION_TYPE_SNP_MEM
> 
> TdxMailBoxExt:
> 
>   GHCB_BOOKKEPING_BASE_ADDRESS
> 
>   GHCB_BOOKKEEPING_SIZE
> 
>   OVMF_SECTION_TYPE_TDX_MAILBOX
> 
> If we want all the OVMF_SECTION_TYPE_SNP_xxx should be defined in a
> separate file then that is also doable. I put everything in one place
> because I was trying to keep entry order similar to what is present in
> MEMFD.
> 
> thanks
> 
> On 9/23/21 6:39 AM, Yao, Jiewen wrote:
> > I strongly recommend to separate SEV and TDX in all context, if it is something
> SEV or TDX specific.
> > Then each file has clear ownership.
> > If it is something generic for both SEV and TDX, it can in one file.
> >
> > For example, SecPeiTempRam/SecPageTable can be in common file.
> > But SevSnpSecrets/GhcbBookkeeping should be in SEV file.
> >
> > Thank you
> > Yao Jiewen
> >
> >> -----Original Message-----
> >> From: Gerd Hoffmann <kraxel@redhat.com>
> >> Sent: Thursday, September 23, 2021 4:48 PM
> >> To: Xu, Min M <min.m.xu@intel.com>
> >> Cc: devel@edk2.groups.io; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Justen,
> >> Jordan L <jordan.l.justen@intel.com>; 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: Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> >>
> >> On Thu, Sep 23, 2021 at 12:38:24AM +0000, Xu, Min M wrote:
> >>> On September 22, 2021 3:49 PM, Gerd Hoffmann wrote:
> >>>>   Hi,
> >>>>
> >>>>> +%ifdef ARCH_X64
> >>>>> +;
> >>>>> +; TDX Metadata offset block
> >>>>> +;
> >>>>> +; TdxMetadata.asm is included in ARCH_X64 because Inte TDX is only ;
> >>>>> +available in ARCH_X64. Below block describes the offset of ;
> >>>>> +TdxMetadata block in Ovmf image ; ; GUID :
> >>>>> +e47a6535-984a-4798-865e-4685a7bf8ec2
> >>>>> +;
> >>>>> +tdxMetadataOffsetStart:
> >>>>> +    DD      tdxMetadataOffsetStart - TdxMetadataGuid - 16
> >>>>> +    DW      tdxMetadataOffsetEnd - tdxMetadataOffsetStart
> >>>>> +    DB      0x35, 0x65, 0x7a, 0xe4, 0x4a, 0x98, 0x98, 0x47
> >>>>> +    DB      0x86, 0x5e, 0x46, 0x85, 0xa7, 0xbf, 0x8e, 0xc2
> >>>>> +tdxMetadataOffsetEnd:
> >>>>> +
> >>>>> +%endif
> >>>> This should be switched to common ovmf metadata (see patches 4-7 of
> the
> >>>> SEV-SNP series).
> >>>>
> >>>> Min: please have a look at these patches.
> >>>>
> >>> Hi, Gerd
> >>> I checked the patches 4-7 of the SEV-SNP series. The common
> >>> OvmfMetadata is designed for both SEV and TDX, right?
> >> That is the idea, yes.
> >>
> >>> If so, then it means the SEV and TDX metadata will be mixed in this
> >>> OvmfMetadata.
> >> Yes.
> >>
> >>> I am thinking there will always be different fields for
> >>> SEV and TDX. For example, SEV has PcdOvmfSecGhcbPageTable but TDX
> >>> doesn't need that page. If the common OvmfMetadata is consumed by
> >>> TDX-QEMU, then PcdOvmfSecGhcbPageTableBase will be initialized too.
> >>> That doesn't make sense.
> >> We have different range types.  OVMF_* are the common areas.  SEV_* will
> >> be used by sev only, TDX_* will be used by tdx only.  TDX and SEV
> >> entries are allowed to overlap, i.e. PcdOvmfSecGhcbPageTableBase should
> >> have some SEV_* type for sev (I think this needs fixing in the series),
> >> and tdx can use the page for something else by adding an TDX_* entry for
> >> the same range.
> >>
> >>> I am thinking that SEV and TDX can keep their own Metadata (in
> >>> separate files, SevMetadata.asm and TdxMetadata.asm) which are pointed
> >>> by the SEV or TDX offsets in the GUID-ed chain in ResetVector.
> >> I'd very much prefer to have a single table to avoid duplication for the
> >> common memory areas and keep the reset vector small.
> >>
> >> Having separate SevMetadata.asm + TdxMetadata.asm files (then have
> >> OvmfMetadata.asm include these two) is an option.  I think this isn't
> >> needed, we can also just group the entries in OvmfMetadata.asm.
> >>
> >>> In this case, SEV and TDX can design their own metadata flexibly, for
> >>> example, the attribute, the item structure, add/remove/update the
> >>> items, etc.
> >> Why have two ways to do the same thing?
> >>
> >> take care,
> >>   Gerd

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Min Xu]

I suggest SEV and TDX keep their own metadata in separate files. This is because SEV and TDX has different item structure.

From the OvmfMetadata definition in SEV (https://github.com/AMDESE/ovmf/blob/snp-v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm) there are 3 fields in the item. (Base/Size/Type). 

But for TDX, there are 6 fields (DataOffset/RawDataSize/MemoryAddress/MemorySize/Type/Attribute) in one item. 
That is because TDX-QEMU not only initialize the memory region, but also does more tasks (measurement) if the Attribute indicates.
DataOffset/RawDataSize is used by the TDX-QEMU to do the measurement if the Attribute field is MR.EXTEND. 
MemoryAddress/MemorySize indicates the TDX-QEMU how to initialize the memory region.

We can add more fields in the item to make it workable for both SEV and TDX, (for example, add DataOffset/RawDataSize/Attribute), but it also restrict the changes in the future if more fields is needed (TDX's change will impact the existing SEV-QEMU).

On September 23, 2021 8:55 PM, Brijesh Singh wrote:
> 
> Like Gerd I would prefer to have one metadata table in the reset GUID.
> The metadata table will contain multiple entries; lot of entries are common
> between SNP and TDX. Some entries will have specific meaning for the platform.
> Those special entries should be marked using the
> OVMF_SECTION_TYPE_{TDX,SNP}_XXXX. It is perfectly fine to have a more than
> one entry for the same region with different type, e.g
> 
> GhcbBookkeepingSnp:
> 
>   GHCB_BOOKKEPING_BASE_ADDRESS
> 
>   GHCB_BOOKKEEPING_SIZE
> 
>   OVMF_SECTION_TYPE_SNP_MEM
> 
> TdxMailBoxExt:
> 
>   GHCB_BOOKKEPING_BASE_ADDRESS
> 
>   GHCB_BOOKKEEPING_SIZE
> 
>   OVMF_SECTION_TYPE_TDX_MAILBOX
> 
> If we want all the OVMF_SECTION_TYPE_SNP_xxx should be defined in a
> separate file then that is also doable. I put everything in one place because I was
> trying to keep entry order similar to what is present in MEMFD.
> 
> thanks
> 
> On 9/23/21 6:39 AM, Yao, Jiewen wrote:
> > I strongly recommend to separate SEV and TDX in all context, if it is something
> SEV or TDX specific.
> > Then each file has clear ownership.
> > If it is something generic for both SEV and TDX, it can in one file.
> >
> > For example, SecPeiTempRam/SecPageTable can be in common file.
> > But SevSnpSecrets/GhcbBookkeeping should be in SEV file.
> >
> > Thank you
> > Yao Jiewen
> >
> >> -----Original Message-----
> >> From: Gerd Hoffmann <kraxel@redhat.com>
> >> Sent: Thursday, September 23, 2021 4:48 PM
> >> To: Xu, Min M <min.m.xu@intel.com>
> >> Cc: devel@edk2.groups.io; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> >> Justen, Jordan L <jordan.l.justen@intel.com>; 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: Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> >>
> >> On Thu, Sep 23, 2021 at 12:38:24AM +0000, Xu, Min M wrote:
> >>> On September 22, 2021 3:49 PM, Gerd Hoffmann wrote:
> >>>>   Hi,
> >>>>
> >>>>> +%ifdef ARCH_X64
> >>>>> +;
> >>>>> +; TDX Metadata offset block
> >>>>> +;
> >>>>> +; TdxMetadata.asm is included in ARCH_X64 because Inte TDX is
> >>>>> +only ; available in ARCH_X64. Below block describes the offset of
> >>>>> +; TdxMetadata block in Ovmf image ; ; GUID :
> >>>>> +e47a6535-984a-4798-865e-4685a7bf8ec2
> >>>>> +;
> >>>>> +tdxMetadataOffsetStart:
> >>>>> +    DD      tdxMetadataOffsetStart - TdxMetadataGuid - 16
> >>>>> +    DW      tdxMetadataOffsetEnd - tdxMetadataOffsetStart
> >>>>> +    DB      0x35, 0x65, 0x7a, 0xe4, 0x4a, 0x98, 0x98, 0x47
> >>>>> +    DB      0x86, 0x5e, 0x46, 0x85, 0xa7, 0xbf, 0x8e, 0xc2
> >>>>> +tdxMetadataOffsetEnd:
> >>>>> +
> >>>>> +%endif
> >>>> This should be switched to common ovmf metadata (see patches 4-7 of
> >>>> the SEV-SNP series).
> >>>>
> >>>> Min: please have a look at these patches.
> >>>>
> >>> Hi, Gerd
> >>> I checked the patches 4-7 of the SEV-SNP series. The common
> >>> OvmfMetadata is designed for both SEV and TDX, right?
> >> That is the idea, yes.
> >>
> >>> If so, then it means the SEV and TDX metadata will be mixed in this
> >>> OvmfMetadata.
> >> Yes.
> >>
> >>> I am thinking there will always be different fields for SEV and TDX.
> >>> For example, SEV has PcdOvmfSecGhcbPageTable but TDX doesn't need
> >>> that page. If the common OvmfMetadata is consumed by TDX-QEMU, then
> >>> PcdOvmfSecGhcbPageTableBase will be initialized too.
> >>> That doesn't make sense.
> >> We have different range types.  OVMF_* are the common areas.  SEV_*
> >> will be used by sev only, TDX_* will be used by tdx only.  TDX and
> >> SEV entries are allowed to overlap, i.e. PcdOvmfSecGhcbPageTableBase
> >> should have some SEV_* type for sev (I think this needs fixing in the
> >> series), and tdx can use the page for something else by adding an
> >> TDX_* entry for the same range.
> >>
> >>> I am thinking that SEV and TDX can keep their own Metadata (in
> >>> separate files, SevMetadata.asm and TdxMetadata.asm) which are
> >>> pointed by the SEV or TDX offsets in the GUID-ed chain in ResetVector.
> >> I'd very much prefer to have a single table to avoid duplication for
> >> the common memory areas and keep the reset vector small.
> >>
> >> Having separate SevMetadata.asm + TdxMetadata.asm files (then have
> >> OvmfMetadata.asm include these two) is an option.  I think this isn't
> >> needed, we can also just group the entries in OvmfMetadata.asm.
> >>
> >>> In this case, SEV and TDX can design their own metadata flexibly,
> >>> for example, the attribute, the item structure, add/remove/update
> >>> the items, etc.
> >> Why have two ways to do the same thing?
> >>
> >> take care,
> >>   Gerd
> 
> 
> 
> 

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Yao, Jiewen]

Good point, Min.

If https://github.com/AMDESE/ovmf/blob/snp-v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm is the proposal, then I have more comment:

Type: OVMF_SECTION_TYPE_CODE, OVMF_SECTION_TYPE_VARS are NOT used for SEV. I am not sure why they are there.

Type: OVMF_SECTION_TYPE_CPUID should be SEV specific. TDX does not need CPUID page.

Type: OVMF_SECTION_TYPE_SEC_MEM also seems for SEV. TDX does not need this special memory, such as Page table. It is already covered by code.

Type: OVMF_SECTION_TYPE_SNP_SECRETS / OVMF_SECTION_TYPE_SNP_SEC_MEM is SEV specific.

The SEV table is totally different with TDX metadata table. I really cannot see the benefit to merge into one table.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Thursday, September 23, 2021 9:20 PM
> To: devel@edk2.groups.io; brijesh.singh@amd.com; Yao, Jiewen
> <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Erdem Aktas <erdemaktas@google.com>; James
> Bottomley <jejb@linux.ibm.com>; Tom Lendacky <thomas.lendacky@amd.com>
> Subject: RE: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
> I suggest SEV and TDX keep their own metadata in separate files. This is because
> SEV and TDX has different item structure.
> 
> From the OvmfMetadata definition in SEV
> (https://github.com/AMDESE/ovmf/blob/snp-
> v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm) there are 3 fields in the
> item. (Base/Size/Type).
> 
> But for TDX, there are 6 fields
> (DataOffset/RawDataSize/MemoryAddress/MemorySize/Type/Attribute) in one
> item.
> That is because TDX-QEMU not only initialize the memory region, but also does
> more tasks (measurement) if the Attribute indicates.
> DataOffset/RawDataSize is used by the TDX-QEMU to do the measurement if
> the Attribute field is MR.EXTEND.
> MemoryAddress/MemorySize indicates the TDX-QEMU how to initialize the
> memory region.
> 
> We can add more fields in the item to make it workable for both SEV and TDX,
> (for example, add DataOffset/RawDataSize/Attribute), but it also restrict the
> changes in the future if more fields is needed (TDX's change will impact the
> existing SEV-QEMU).
> 
> On September 23, 2021 8:55 PM, Brijesh Singh wrote:
> >
> > Like Gerd I would prefer to have one metadata table in the reset GUID.
> > The metadata table will contain multiple entries; lot of entries are common
> > between SNP and TDX. Some entries will have specific meaning for the
> platform.
> > Those special entries should be marked using the
> > OVMF_SECTION_TYPE_{TDX,SNP}_XXXX. It is perfectly fine to have a more
> than
> > one entry for the same region with different type, e.g
> >
> > GhcbBookkeepingSnp:
> >
> >   GHCB_BOOKKEPING_BASE_ADDRESS
> >
> >   GHCB_BOOKKEEPING_SIZE
> >
> >   OVMF_SECTION_TYPE_SNP_MEM
> >
> > TdxMailBoxExt:
> >
> >   GHCB_BOOKKEPING_BASE_ADDRESS
> >
> >   GHCB_BOOKKEEPING_SIZE
> >
> >   OVMF_SECTION_TYPE_TDX_MAILBOX
> >
> > If we want all the OVMF_SECTION_TYPE_SNP_xxx should be defined in a
> > separate file then that is also doable. I put everything in one place because I
> was
> > trying to keep entry order similar to what is present in MEMFD.
> >
> > thanks
> >
> > On 9/23/21 6:39 AM, Yao, Jiewen wrote:
> > > I strongly recommend to separate SEV and TDX in all context, if it is
> something
> > SEV or TDX specific.
> > > Then each file has clear ownership.
> > > If it is something generic for both SEV and TDX, it can in one file.
> > >
> > > For example, SecPeiTempRam/SecPageTable can be in common file.
> > > But SevSnpSecrets/GhcbBookkeeping should be in SEV file.
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > >> -----Original Message-----
> > >> From: Gerd Hoffmann <kraxel@redhat.com>
> > >> Sent: Thursday, September 23, 2021 4:48 PM
> > >> To: Xu, Min M <min.m.xu@intel.com>
> > >> Cc: devel@edk2.groups.io; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> > >> Justen, Jordan L <jordan.l.justen@intel.com>; 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: Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> > >>
> > >> On Thu, Sep 23, 2021 at 12:38:24AM +0000, Xu, Min M wrote:
> > >>> On September 22, 2021 3:49 PM, Gerd Hoffmann wrote:
> > >>>>   Hi,
> > >>>>
> > >>>>> +%ifdef ARCH_X64
> > >>>>> +;
> > >>>>> +; TDX Metadata offset block
> > >>>>> +;
> > >>>>> +; TdxMetadata.asm is included in ARCH_X64 because Inte TDX is
> > >>>>> +only ; available in ARCH_X64. Below block describes the offset of
> > >>>>> +; TdxMetadata block in Ovmf image ; ; GUID :
> > >>>>> +e47a6535-984a-4798-865e-4685a7bf8ec2
> > >>>>> +;
> > >>>>> +tdxMetadataOffsetStart:
> > >>>>> +    DD      tdxMetadataOffsetStart - TdxMetadataGuid - 16
> > >>>>> +    DW      tdxMetadataOffsetEnd - tdxMetadataOffsetStart
> > >>>>> +    DB      0x35, 0x65, 0x7a, 0xe4, 0x4a, 0x98, 0x98, 0x47
> > >>>>> +    DB      0x86, 0x5e, 0x46, 0x85, 0xa7, 0xbf, 0x8e, 0xc2
> > >>>>> +tdxMetadataOffsetEnd:
> > >>>>> +
> > >>>>> +%endif
> > >>>> This should be switched to common ovmf metadata (see patches 4-7 of
> > >>>> the SEV-SNP series).
> > >>>>
> > >>>> Min: please have a look at these patches.
> > >>>>
> > >>> Hi, Gerd
> > >>> I checked the patches 4-7 of the SEV-SNP series. The common
> > >>> OvmfMetadata is designed for both SEV and TDX, right?
> > >> That is the idea, yes.
> > >>
> > >>> If so, then it means the SEV and TDX metadata will be mixed in this
> > >>> OvmfMetadata.
> > >> Yes.
> > >>
> > >>> I am thinking there will always be different fields for SEV and TDX.
> > >>> For example, SEV has PcdOvmfSecGhcbPageTable but TDX doesn't need
> > >>> that page. If the common OvmfMetadata is consumed by TDX-QEMU,
> then
> > >>> PcdOvmfSecGhcbPageTableBase will be initialized too.
> > >>> That doesn't make sense.
> > >> We have different range types.  OVMF_* are the common areas.  SEV_*
> > >> will be used by sev only, TDX_* will be used by tdx only.  TDX and
> > >> SEV entries are allowed to overlap, i.e. PcdOvmfSecGhcbPageTableBase
> > >> should have some SEV_* type for sev (I think this needs fixing in the
> > >> series), and tdx can use the page for something else by adding an
> > >> TDX_* entry for the same range.
> > >>
> > >>> I am thinking that SEV and TDX can keep their own Metadata (in
> > >>> separate files, SevMetadata.asm and TdxMetadata.asm) which are
> > >>> pointed by the SEV or TDX offsets in the GUID-ed chain in ResetVector.
> > >> I'd very much prefer to have a single table to avoid duplication for
> > >> the common memory areas and keep the reset vector small.
> > >>
> > >> Having separate SevMetadata.asm + TdxMetadata.asm files (then have
> > >> OvmfMetadata.asm include these two) is an option.  I think this isn't
> > >> needed, we can also just group the entries in OvmfMetadata.asm.
> > >>
> > >>> In this case, SEV and TDX can design their own metadata flexibly,
> > >>> for example, the attribute, the item structure, add/remove/update
> > >>> the items, etc.
> > >> Why have two ways to do the same thing?
> > >>
> > >> take care,
> > >>   Gerd
> >
> >
> > 
> >

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Brijesh Singh]

SEV hardware does not have a concept of the metadata. To boot SEV guest 
we need to pass some information to VMM and in past those information 
were passed through SNP_BOOT_BLOCK (GUIDed structure) but Gerd 
recommended that it will be good idea if both SEV and TDX uses a common 
metadata approach to pass these information. I personally think it was a 
good suggestion. So, in SNP series I went ahead and created a generic 
metadata structure and  hope that TDX will build on it. The user of the 
metadata structure is VMM (qemu, etc); while launching the guest the VMM 
knows whether its creating the SEV or TDX guest and will process the 
entries accordingly.

As per the number of fields in the metadata is concerns, I felt 3 fields 
(start, size and type) should be good enough for all the cases. There 
was a question from Gerd to Min asking why do you need the 
dataoffset/rawdatasize etc and I don't remember seeing the answer for 
it. As I said in the start, SNP hardware does not enforce metadata 
layout so I am flexible to add more field or remove or keep it separate.

thanks

On 9/23/21 8:38 AM, Yao, Jiewen wrote:
> Good point, Min.
> 
> If https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Fovmf%2Fblob%2Fsnp-v8%2FOvmfPkg%2FResetVector%2FX64%2FOvmfMetadata.asm&data=04%7C01%7Cbrijesh.singh%40amd.com%7C52f6327efa33480bf4a308d97e977ff0%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637680011416117826%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=zDfikRYhxd8ERY%2Fw6kJLhJKRNWbYTl4D6PpqK%2BVNsus%3D&reserved=0 is the proposal, then I have more comment:
> 
> Type: OVMF_SECTION_TYPE_CODE, OVMF_SECTION_TYPE_VARS are NOT used for SEV. I am not sure why they are there.
> 
> Type: OVMF_SECTION_TYPE_CPUID should be SEV specific. TDX does not need CPUID page.
> 
> Type: OVMF_SECTION_TYPE_SEC_MEM also seems for SEV. TDX does not need this special memory, such as Page table. It is already covered by code.
> 
> Type: OVMF_SECTION_TYPE_SNP_SECRETS / OVMF_SECTION_TYPE_SNP_SEC_MEM is SEV specific.
> 
> The SEV table is totally different with TDX metadata table. I really cannot see the benefit to merge into one table.
> 
> Thank you
> Yao Jiewen
> 
>> -----Original Message-----
>> From: Xu, Min M <min.m.xu@intel.com>
>> Sent: Thursday, September 23, 2021 9:20 PM
>> To: devel@edk2.groups.io; brijesh.singh@amd.com; Yao, Jiewen
>> <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; Erdem Aktas <erdemaktas@google.com>; James
>> Bottomley <jejb@linux.ibm.com>; Tom Lendacky <thomas.lendacky@amd.com>
>> Subject: RE: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
>>
>> I suggest SEV and TDX keep their own metadata in separate files. This is because
>> SEV and TDX has different item structure.
>>
>>  From the OvmfMetadata definition in SEV
>> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Fovmf%2Fblob%2Fsnp-&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C52f6327efa33480bf4a308d97e977ff0%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637680011416117826%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=at43H9sgmlaD773wsU5%2BoPPSjImo0UiCxQ0nmwdV9ds%3D&amp;reserved=0
>> v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm) there are 3 fields in the
>> item. (Base/Size/Type).
>>
>> But for TDX, there are 6 fields
>> (DataOffset/RawDataSize/MemoryAddress/MemorySize/Type/Attribute) in one
>> item.
>> That is because TDX-QEMU not only initialize the memory region, but also does
>> more tasks (measurement) if the Attribute indicates.
>> DataOffset/RawDataSize is used by the TDX-QEMU to do the measurement if
>> the Attribute field is MR.EXTEND.
>> MemoryAddress/MemorySize indicates the TDX-QEMU how to initialize the
>> memory region.
>>
>> We can add more fields in the item to make it workable for both SEV and TDX,
>> (for example, add DataOffset/RawDataSize/Attribute), but it also restrict the
>> changes in the future if more fields is needed (TDX's change will impact the
>> existing SEV-QEMU).
>>
>> On September 23, 2021 8:55 PM, Brijesh Singh wrote:
>>>
>>> Like Gerd I would prefer to have one metadata table in the reset GUID.
>>> The metadata table will contain multiple entries; lot of entries are common
>>> between SNP and TDX. Some entries will have specific meaning for the
>> platform.
>>> Those special entries should be marked using the
>>> OVMF_SECTION_TYPE_{TDX,SNP}_XXXX. It is perfectly fine to have a more
>> than
>>> one entry for the same region with different type, e.g
>>>
>>> GhcbBookkeepingSnp:
>>>
>>>    GHCB_BOOKKEPING_BASE_ADDRESS
>>>
>>>    GHCB_BOOKKEEPING_SIZE
>>>
>>>    OVMF_SECTION_TYPE_SNP_MEM
>>>
>>> TdxMailBoxExt:
>>>
>>>    GHCB_BOOKKEPING_BASE_ADDRESS
>>>
>>>    GHCB_BOOKKEEPING_SIZE
>>>
>>>    OVMF_SECTION_TYPE_TDX_MAILBOX
>>>
>>> If we want all the OVMF_SECTION_TYPE_SNP_xxx should be defined in a
>>> separate file then that is also doable. I put everything in one place because I
>> was
>>> trying to keep entry order similar to what is present in MEMFD.
>>>
>>> thanks
>>>
>>> On 9/23/21 6:39 AM, Yao, Jiewen wrote:
>>>> I strongly recommend to separate SEV and TDX in all context, if it is
>> something
>>> SEV or TDX specific.
>>>> Then each file has clear ownership.
>>>> If it is something generic for both SEV and TDX, it can in one file.
>>>>
>>>> For example, SecPeiTempRam/SecPageTable can be in common file.
>>>> But SevSnpSecrets/GhcbBookkeeping should be in SEV file.
>>>>
>>>> Thank you
>>>> Yao Jiewen
>>>>
>>>>> -----Original Message-----
>>>>> From: Gerd Hoffmann <kraxel@redhat.com>
>>>>> Sent: Thursday, September 23, 2021 4:48 PM
>>>>> To: Xu, Min M <min.m.xu@intel.com>
>>>>> Cc: devel@edk2.groups.io; Ard Biesheuvel <ardb+tianocore@kernel.org>;
>>>>> Justen, Jordan L <jordan.l.justen@intel.com>; 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: Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
>>>>>
>>>>> On Thu, Sep 23, 2021 at 12:38:24AM +0000, Xu, Min M wrote:
>>>>>> On September 22, 2021 3:49 PM, Gerd Hoffmann wrote:
>>>>>>>    Hi,
>>>>>>>
>>>>>>>> +%ifdef ARCH_X64
>>>>>>>> +;
>>>>>>>> +; TDX Metadata offset block
>>>>>>>> +;
>>>>>>>> +; TdxMetadata.asm is included in ARCH_X64 because Inte TDX is
>>>>>>>> +only ; available in ARCH_X64. Below block describes the offset of
>>>>>>>> +; TdxMetadata block in Ovmf image ; ; GUID :
>>>>>>>> +e47a6535-984a-4798-865e-4685a7bf8ec2
>>>>>>>> +;
>>>>>>>> +tdxMetadataOffsetStart:
>>>>>>>> +    DD      tdxMetadataOffsetStart - TdxMetadataGuid - 16
>>>>>>>> +    DW      tdxMetadataOffsetEnd - tdxMetadataOffsetStart
>>>>>>>> +    DB      0x35, 0x65, 0x7a, 0xe4, 0x4a, 0x98, 0x98, 0x47
>>>>>>>> +    DB      0x86, 0x5e, 0x46, 0x85, 0xa7, 0xbf, 0x8e, 0xc2
>>>>>>>> +tdxMetadataOffsetEnd:
>>>>>>>> +
>>>>>>>> +%endif
>>>>>>> This should be switched to common ovmf metadata (see patches 4-7 of
>>>>>>> the SEV-SNP series).
>>>>>>>
>>>>>>> Min: please have a look at these patches.
>>>>>>>
>>>>>> Hi, Gerd
>>>>>> I checked the patches 4-7 of the SEV-SNP series. The common
>>>>>> OvmfMetadata is designed for both SEV and TDX, right?
>>>>> That is the idea, yes.
>>>>>
>>>>>> If so, then it means the SEV and TDX metadata will be mixed in this
>>>>>> OvmfMetadata.
>>>>> Yes.
>>>>>
>>>>>> I am thinking there will always be different fields for SEV and TDX.
>>>>>> For example, SEV has PcdOvmfSecGhcbPageTable but TDX doesn't need
>>>>>> that page. If the common OvmfMetadata is consumed by TDX-QEMU,
>> then
>>>>>> PcdOvmfSecGhcbPageTableBase will be initialized too.
>>>>>> That doesn't make sense.
>>>>> We have different range types.  OVMF_* are the common areas.  SEV_*
>>>>> will be used by sev only, TDX_* will be used by tdx only.  TDX and
>>>>> SEV entries are allowed to overlap, i.e. PcdOvmfSecGhcbPageTableBase
>>>>> should have some SEV_* type for sev (I think this needs fixing in the
>>>>> series), and tdx can use the page for something else by adding an
>>>>> TDX_* entry for the same range.
>>>>>
>>>>>> I am thinking that SEV and TDX can keep their own Metadata (in
>>>>>> separate files, SevMetadata.asm and TdxMetadata.asm) which are
>>>>>> pointed by the SEV or TDX offsets in the GUID-ed chain in ResetVector.
>>>>> I'd very much prefer to have a single table to avoid duplication for
>>>>> the common memory areas and keep the reset vector small.
>>>>>
>>>>> Having separate SevMetadata.asm + TdxMetadata.asm files (then have
>>>>> OvmfMetadata.asm include these two) is an option.  I think this isn't
>>>>> needed, we can also just group the entries in OvmfMetadata.asm.
>>>>>
>>>>>> In this case, SEV and TDX can design their own metadata flexibly,
>>>>>> for example, the attribute, the item structure, add/remove/update
>>>>>> the items, etc.
>>>>> Why have two ways to do the same thing?
>>>>>
>>>>> take care,
>>>>>    Gerd
>>>
>>>
>>> 
>>>
> 

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Min Xu]

On September 23, 2021 10:04 PM, Brijesh Singh wrote:
> 
> SEV hardware does not have a concept of the metadata. To boot SEV guest we
> need to pass some information to VMM and in past those information were
> passed through SNP_BOOT_BLOCK (GUIDed structure) but Gerd recommended
> that it will be good idea if both SEV and TDX uses a common metadata approach
> to pass these information. I personally think it was a good suggestion. So, in SNP
> series I went ahead and created a generic metadata structure and  hope that
> TDX will build on it. The user of the metadata structure is VMM (qemu, etc);
> while launching the guest the VMM knows whether its creating the SEV or TDX
> guest and will process the entries accordingly.
> 
> As per the number of fields in the metadata is concerns, I felt 3 fields (start, size
> and type) should be good enough for all the cases. There was a question from
> Gerd to Min asking why do you need the dataoffset/rawdatasize etc and I don't
> remember seeing the answer for it.
>
The discussion is in this link. https://edk2.groups.io/g/devel/message/80289
>
>As I said in the start, SNP hardware does not
> enforce metadata layout so I am flexible to add more field or remove or keep it
> separate.
> 
> thanks
> 
> On 9/23/21 8:38 AM, Yao, Jiewen wrote:
> > Good point, Min.
> >
> > If
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2FAMDESE%2Fovmf%2Fblob%2Fsnp-
> v8%2FOvmfPkg%2FResetVector%2FX64%2FOvmfMetadata.asm&data=0
> 4%7C01%7Cbrijesh.singh%40amd.com%7C52f6327efa33480bf4a308d97e977
> ff0%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637680011416
> 117826%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
> uMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=zDfikRYhxd8E
> RY%2Fw6kJLhJKRNWbYTl4D6PpqK%2BVNsus%3D&reserved=0 is the
> proposal, then I have more comment:
> >
> > Type: OVMF_SECTION_TYPE_CODE, OVMF_SECTION_TYPE_VARS are NOT
> used for SEV. I am not sure why they are there.
> >
> > Type: OVMF_SECTION_TYPE_CPUID should be SEV specific. TDX does not
> need CPUID page.
> >
> > Type: OVMF_SECTION_TYPE_SEC_MEM also seems for SEV. TDX does not
> need this special memory, such as Page table. It is already covered by code.
> >
> > Type: OVMF_SECTION_TYPE_SNP_SECRETS /
> OVMF_SECTION_TYPE_SNP_SEC_MEM is SEV specific.
> >
> > The SEV table is totally different with TDX metadata table. I really cannot see
> the benefit to merge into one table.
> >
> > Thank you
> > Yao Jiewen
> >
> >> -----Original Message-----
> >> From: Xu, Min M <min.m.xu@intel.com>
> >> Sent: Thursday, September 23, 2021 9:20 PM
> >> To: devel@edk2.groups.io; brijesh.singh@amd.com; Yao, Jiewen
> >> <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> >> <jordan.l.justen@intel.com>; Erdem Aktas <erdemaktas@google.com>;
> >> James Bottomley <jejb@linux.ibm.com>; Tom Lendacky
> >> <thomas.lendacky@amd.com>
> >> Subject: RE: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in
> >> ResetVector
> >>
> >> I suggest SEV and TDX keep their own metadata in separate files. This
> >> is because SEV and TDX has different item structure.
> >>
> >>  From the OvmfMetadata definition in SEV
> >> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> >> thub.com%2FAMDESE%2Fovmf%2Fblob%2Fsnp-
> &amp;data=04%7C01%7Cbrijesh.sin
> >>
> gh%40amd.com%7C52f6327efa33480bf4a308d97e977ff0%7C3dd8961fe488
> 4e608e1
> >>
> 1a82d994e183d%7C0%7C0%7C637680011416117826%7CUnknown%7CTWF
> pbGZsb3d8ey
> >>
> JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> 7C10
> >>
> 00&amp;sdata=at43H9sgmlaD773wsU5%2BoPPSjImo0UiCxQ0nmwdV9ds%3D
> &amp;res
> >> erved=0
> >> v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm) there are 3 fields in
> >> the item. (Base/Size/Type).
> >>
> >> But for TDX, there are 6 fields
> >> (DataOffset/RawDataSize/MemoryAddress/MemorySize/Type/Attribute) in
> >> one item.
> >> That is because TDX-QEMU not only initialize the memory region, but
> >> also does more tasks (measurement) if the Attribute indicates.
> >> DataOffset/RawDataSize is used by the TDX-QEMU to do the measurement
> >> if the Attribute field is MR.EXTEND.
> >> MemoryAddress/MemorySize indicates the TDX-QEMU how to initialize the
> >> memory region.
> >>
> >> We can add more fields in the item to make it workable for both SEV
> >> and TDX, (for example, add DataOffset/RawDataSize/Attribute), but it
> >> also restrict the changes in the future if more fields is needed
> >> (TDX's change will impact the existing SEV-QEMU).
> >>
> >> On September 23, 2021 8:55 PM, Brijesh Singh wrote:
> >>>
> >>> Like Gerd I would prefer to have one metadata table in the reset GUID.
> >>> The metadata table will contain multiple entries; lot of entries are
> >>> common between SNP and TDX. Some entries will have specific meaning
> >>> for the
> >> platform.
> >>> Those special entries should be marked using the
> >>> OVMF_SECTION_TYPE_{TDX,SNP}_XXXX. It is perfectly fine to have a
> >>> more
> >> than
> >>> one entry for the same region with different type, e.g
> >>>
> >>> GhcbBookkeepingSnp:
> >>>
> >>>    GHCB_BOOKKEPING_BASE_ADDRESS
> >>>
> >>>    GHCB_BOOKKEEPING_SIZE
> >>>
> >>>    OVMF_SECTION_TYPE_SNP_MEM
> >>>
> >>> TdxMailBoxExt:
> >>>
> >>>    GHCB_BOOKKEPING_BASE_ADDRESS
> >>>
> >>>    GHCB_BOOKKEEPING_SIZE
> >>>
> >>>    OVMF_SECTION_TYPE_TDX_MAILBOX
> >>>
> >>> If we want all the OVMF_SECTION_TYPE_SNP_xxx should be defined in a
> >>> separate file then that is also doable. I put everything in one
> >>> place because I
> >> was
> >>> trying to keep entry order similar to what is present in MEMFD.
> >>>
> >>> thanks
> >>>
> >>> On 9/23/21 6:39 AM, Yao, Jiewen wrote:
> >>>> I strongly recommend to separate SEV and TDX in all context, if it
> >>>> is
> >> something
> >>> SEV or TDX specific.
> >>>> Then each file has clear ownership.
> >>>> If it is something generic for both SEV and TDX, it can in one file.
> >>>>
> >>>> For example, SecPeiTempRam/SecPageTable can be in common file.
> >>>> But SevSnpSecrets/GhcbBookkeeping should be in SEV file.
> >>>>
> >>>> Thank you
> >>>> Yao Jiewen
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Gerd Hoffmann <kraxel@redhat.com>
> >>>>> Sent: Thursday, September 23, 2021 4:48 PM
> >>>>> To: Xu, Min M <min.m.xu@intel.com>
> >>>>> Cc: devel@edk2.groups.io; Ard Biesheuvel
> >>>>> <ardb+tianocore@kernel.org>; Justen, Jordan L
> >>>>> <jordan.l.justen@intel.com>; 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: Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> >>>>>
> >>>>> On Thu, Sep 23, 2021 at 12:38:24AM +0000, Xu, Min M wrote:
> >>>>>> On September 22, 2021 3:49 PM, Gerd Hoffmann wrote:
> >>>>>>>    Hi,
> >>>>>>>
> >>>>>>>> +%ifdef ARCH_X64
> >>>>>>>> +;
> >>>>>>>> +; TDX Metadata offset block
> >>>>>>>> +;
> >>>>>>>> +; TdxMetadata.asm is included in ARCH_X64 because Inte TDX is
> >>>>>>>> +only ; available in ARCH_X64. Below block describes the offset
> >>>>>>>> +of ; TdxMetadata block in Ovmf image ; ; GUID :
> >>>>>>>> +e47a6535-984a-4798-865e-4685a7bf8ec2
> >>>>>>>> +;
> >>>>>>>> +tdxMetadataOffsetStart:
> >>>>>>>> +    DD      tdxMetadataOffsetStart - TdxMetadataGuid - 16
> >>>>>>>> +    DW      tdxMetadataOffsetEnd - tdxMetadataOffsetStart
> >>>>>>>> +    DB      0x35, 0x65, 0x7a, 0xe4, 0x4a, 0x98, 0x98, 0x47
> >>>>>>>> +    DB      0x86, 0x5e, 0x46, 0x85, 0xa7, 0xbf, 0x8e, 0xc2
> >>>>>>>> +tdxMetadataOffsetEnd:
> >>>>>>>> +
> >>>>>>>> +%endif
> >>>>>>> This should be switched to common ovmf metadata (see patches 4-7
> >>>>>>> of the SEV-SNP series).
> >>>>>>>
> >>>>>>> Min: please have a look at these patches.
> >>>>>>>
> >>>>>> Hi, Gerd
> >>>>>> I checked the patches 4-7 of the SEV-SNP series. The common
> >>>>>> OvmfMetadata is designed for both SEV and TDX, right?
> >>>>> That is the idea, yes.
> >>>>>
> >>>>>> If so, then it means the SEV and TDX metadata will be mixed in
> >>>>>> this OvmfMetadata.
> >>>>> Yes.
> >>>>>
> >>>>>> I am thinking there will always be different fields for SEV and TDX.
> >>>>>> For example, SEV has PcdOvmfSecGhcbPageTable but TDX doesn't need
> >>>>>> that page. If the common OvmfMetadata is consumed by TDX-QEMU,
> >> then
> >>>>>> PcdOvmfSecGhcbPageTableBase will be initialized too.
> >>>>>> That doesn't make sense.
> >>>>> We have different range types.  OVMF_* are the common areas.
> >>>>> SEV_* will be used by sev only, TDX_* will be used by tdx only.
> >>>>> TDX and SEV entries are allowed to overlap, i.e.
> >>>>> PcdOvmfSecGhcbPageTableBase should have some SEV_* type for sev (I
> >>>>> think this needs fixing in the series), and tdx can use the page
> >>>>> for something else by adding an
> >>>>> TDX_* entry for the same range.
> >>>>>
> >>>>>> I am thinking that SEV and TDX can keep their own Metadata (in
> >>>>>> separate files, SevMetadata.asm and TdxMetadata.asm) which are
> >>>>>> pointed by the SEV or TDX offsets in the GUID-ed chain in ResetVector.
> >>>>> I'd very much prefer to have a single table to avoid duplication
> >>>>> for the common memory areas and keep the reset vector small.
> >>>>>
> >>>>> Having separate SevMetadata.asm + TdxMetadata.asm files (then have
> >>>>> OvmfMetadata.asm include these two) is an option.  I think this
> >>>>> isn't needed, we can also just group the entries in OvmfMetadata.asm.
> >>>>>
> >>>>>> In this case, SEV and TDX can design their own metadata flexibly,
> >>>>>> for example, the attribute, the item structure, add/remove/update
> >>>>>> the items, etc.
> >>>>> Why have two ways to do the same thing?
> >>>>>
> >>>>> take care,
> >>>>>    Gerd
> >>>
> >>>
> >>> 
> >>>
> >

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Yao, Jiewen]

HI Brijesh
Thanks for the explanation.

All fields in TDX metadata are required. So the current SEV proposal (3 fields) does not work for TDX. The extra fields are used to guide VMM on how to copy the binary, allocate memory, and how to measure the component. And we are adding more extension to them to support more use cases.

Based upon the factor that TDX and SEV are using different architecture to boot, I am not sure why we have to combine them into one table. (TDX need assist from TDX module - run in host CPU, and SEV need assist from PSP - a coprocessor)

I strongly recommend to use two tables. TDX only defines TDX required info. SEV only defines SEV required info. Each one can add its own extension separately. 
Otherwise, there will be unnecessary information in the one metadata, which will increase the image size.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Thursday, September 23, 2021 10:15 PM
> To: Brijesh Singh <brijesh.singh@amd.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; devel@edk2.groups.io; Gerd Hoffmann
> <kraxel@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Erdem Aktas <erdemaktas@google.com>; James
> Bottomley <jejb@linux.ibm.com>; Tom Lendacky <thomas.lendacky@amd.com>
> Subject: RE: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
> On September 23, 2021 10:04 PM, Brijesh Singh wrote:
> >
> > SEV hardware does not have a concept of the metadata. To boot SEV guest we
> > need to pass some information to VMM and in past those information were
> > passed through SNP_BOOT_BLOCK (GUIDed structure) but Gerd recommended
> > that it will be good idea if both SEV and TDX uses a common metadata
> approach
> > to pass these information. I personally think it was a good suggestion. So, in
> SNP
> > series I went ahead and created a generic metadata structure and  hope that
> > TDX will build on it. The user of the metadata structure is VMM (qemu, etc);
> > while launching the guest the VMM knows whether its creating the SEV or TDX
> > guest and will process the entries accordingly.
> >
> > As per the number of fields in the metadata is concerns, I felt 3 fields (start,
> size
> > and type) should be good enough for all the cases. There was a question from
> > Gerd to Min asking why do you need the dataoffset/rawdatasize etc and I
> don't
> > remember seeing the answer for it.
> >
> The discussion is in this link. https://edk2.groups.io/g/devel/message/80289
> >
> >As I said in the start, SNP hardware does not
> > enforce metadata layout so I am flexible to add more field or remove or keep
> it
> > separate.
> >
> > thanks
> >
> > On 9/23/21 8:38 AM, Yao, Jiewen wrote:
> > > Good point, Min.
> > >
> > > If
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> > com%2FAMDESE%2Fovmf%2Fblob%2Fsnp-
> > v8%2FOvmfPkg%2FResetVector%2FX64%2FOvmfMetadata.asm&amp;data=0
> > 4%7C01%7Cbrijesh.singh%40amd.com%7C52f6327efa33480bf4a308d97e977
> > ff0%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637680011416
> > 117826%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
> > uMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=zDfikRYhxd8E
> > RY%2Fw6kJLhJKRNWbYTl4D6PpqK%2BVNsus%3D&amp;reserved=0 is the
> > proposal, then I have more comment:
> > >
> > > Type: OVMF_SECTION_TYPE_CODE, OVMF_SECTION_TYPE_VARS are NOT
> > used for SEV. I am not sure why they are there.
> > >
> > > Type: OVMF_SECTION_TYPE_CPUID should be SEV specific. TDX does not
> > need CPUID page.
> > >
> > > Type: OVMF_SECTION_TYPE_SEC_MEM also seems for SEV. TDX does not
> > need this special memory, such as Page table. It is already covered by code.
> > >
> > > Type: OVMF_SECTION_TYPE_SNP_SECRETS /
> > OVMF_SECTION_TYPE_SNP_SEC_MEM is SEV specific.
> > >
> > > The SEV table is totally different with TDX metadata table. I really cannot see
> > the benefit to merge into one table.
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > >> -----Original Message-----
> > >> From: Xu, Min M <min.m.xu@intel.com>
> > >> Sent: Thursday, September 23, 2021 9:20 PM
> > >> To: devel@edk2.groups.io; brijesh.singh@amd.com; Yao, Jiewen
> > >> <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> > >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> > >> <jordan.l.justen@intel.com>; Erdem Aktas <erdemaktas@google.com>;
> > >> James Bottomley <jejb@linux.ibm.com>; Tom Lendacky
> > >> <thomas.lendacky@amd.com>
> > >> Subject: RE: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in
> > >> ResetVector
> > >>
> > >> I suggest SEV and TDX keep their own metadata in separate files. This
> > >> is because SEV and TDX has different item structure.
> > >>
> > >>  From the OvmfMetadata definition in SEV
> > >> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> > >> thub.com%2FAMDESE%2Fovmf%2Fblob%2Fsnp-
> > &amp;data=04%7C01%7Cbrijesh.sin
> > >>
> > gh%40amd.com%7C52f6327efa33480bf4a308d97e977ff0%7C3dd8961fe488
> > 4e608e1
> > >>
> > 1a82d994e183d%7C0%7C0%7C637680011416117826%7CUnknown%7CTWF
> > pbGZsb3d8ey
> > >>
> > JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> > 7C10
> > >>
> > 00&amp;sdata=at43H9sgmlaD773wsU5%2BoPPSjImo0UiCxQ0nmwdV9ds%3D
> > &amp;res
> > >> erved=0
> > >> v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm) there are 3 fields in
> > >> the item. (Base/Size/Type).
> > >>
> > >> But for TDX, there are 6 fields
> > >> (DataOffset/RawDataSize/MemoryAddress/MemorySize/Type/Attribute) in
> > >> one item.
> > >> That is because TDX-QEMU not only initialize the memory region, but
> > >> also does more tasks (measurement) if the Attribute indicates.
> > >> DataOffset/RawDataSize is used by the TDX-QEMU to do the measurement
> > >> if the Attribute field is MR.EXTEND.
> > >> MemoryAddress/MemorySize indicates the TDX-QEMU how to initialize the
> > >> memory region.
> > >>
> > >> We can add more fields in the item to make it workable for both SEV
> > >> and TDX, (for example, add DataOffset/RawDataSize/Attribute), but it
> > >> also restrict the changes in the future if more fields is needed
> > >> (TDX's change will impact the existing SEV-QEMU).
> > >>
> > >> On September 23, 2021 8:55 PM, Brijesh Singh wrote:
> > >>>
> > >>> Like Gerd I would prefer to have one metadata table in the reset GUID.
> > >>> The metadata table will contain multiple entries; lot of entries are
> > >>> common between SNP and TDX. Some entries will have specific meaning
> > >>> for the
> > >> platform.
> > >>> Those special entries should be marked using the
> > >>> OVMF_SECTION_TYPE_{TDX,SNP}_XXXX. It is perfectly fine to have a
> > >>> more
> > >> than
> > >>> one entry for the same region with different type, e.g
> > >>>
> > >>> GhcbBookkeepingSnp:
> > >>>
> > >>>    GHCB_BOOKKEPING_BASE_ADDRESS
> > >>>
> > >>>    GHCB_BOOKKEEPING_SIZE
> > >>>
> > >>>    OVMF_SECTION_TYPE_SNP_MEM
> > >>>
> > >>> TdxMailBoxExt:
> > >>>
> > >>>    GHCB_BOOKKEPING_BASE_ADDRESS
> > >>>
> > >>>    GHCB_BOOKKEEPING_SIZE
> > >>>
> > >>>    OVMF_SECTION_TYPE_TDX_MAILBOX
> > >>>
> > >>> If we want all the OVMF_SECTION_TYPE_SNP_xxx should be defined in a
> > >>> separate file then that is also doable. I put everything in one
> > >>> place because I
> > >> was
> > >>> trying to keep entry order similar to what is present in MEMFD.
> > >>>
> > >>> thanks
> > >>>
> > >>> On 9/23/21 6:39 AM, Yao, Jiewen wrote:
> > >>>> I strongly recommend to separate SEV and TDX in all context, if it
> > >>>> is
> > >> something
> > >>> SEV or TDX specific.
> > >>>> Then each file has clear ownership.
> > >>>> If it is something generic for both SEV and TDX, it can in one file.
> > >>>>
> > >>>> For example, SecPeiTempRam/SecPageTable can be in common file.
> > >>>> But SevSnpSecrets/GhcbBookkeeping should be in SEV file.
> > >>>>
> > >>>> Thank you
> > >>>> Yao Jiewen
> > >>>>
> > >>>>> -----Original Message-----
> > >>>>> From: Gerd Hoffmann <kraxel@redhat.com>
> > >>>>> Sent: Thursday, September 23, 2021 4:48 PM
> > >>>>> To: Xu, Min M <min.m.xu@intel.com>
> > >>>>> Cc: devel@edk2.groups.io; Ard Biesheuvel
> > >>>>> <ardb+tianocore@kernel.org>; Justen, Jordan L
> > >>>>> <jordan.l.justen@intel.com>; 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: Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> > >>>>>
> > >>>>> On Thu, Sep 23, 2021 at 12:38:24AM +0000, Xu, Min M wrote:
> > >>>>>> On September 22, 2021 3:49 PM, Gerd Hoffmann wrote:
> > >>>>>>>    Hi,
> > >>>>>>>
> > >>>>>>>> +%ifdef ARCH_X64
> > >>>>>>>> +;
> > >>>>>>>> +; TDX Metadata offset block
> > >>>>>>>> +;
> > >>>>>>>> +; TdxMetadata.asm is included in ARCH_X64 because Inte TDX is
> > >>>>>>>> +only ; available in ARCH_X64. Below block describes the offset
> > >>>>>>>> +of ; TdxMetadata block in Ovmf image ; ; GUID :
> > >>>>>>>> +e47a6535-984a-4798-865e-4685a7bf8ec2
> > >>>>>>>> +;
> > >>>>>>>> +tdxMetadataOffsetStart:
> > >>>>>>>> +    DD      tdxMetadataOffsetStart - TdxMetadataGuid - 16
> > >>>>>>>> +    DW      tdxMetadataOffsetEnd - tdxMetadataOffsetStart
> > >>>>>>>> +    DB      0x35, 0x65, 0x7a, 0xe4, 0x4a, 0x98, 0x98, 0x47
> > >>>>>>>> +    DB      0x86, 0x5e, 0x46, 0x85, 0xa7, 0xbf, 0x8e, 0xc2
> > >>>>>>>> +tdxMetadataOffsetEnd:
> > >>>>>>>> +
> > >>>>>>>> +%endif
> > >>>>>>> This should be switched to common ovmf metadata (see patches 4-7
> > >>>>>>> of the SEV-SNP series).
> > >>>>>>>
> > >>>>>>> Min: please have a look at these patches.
> > >>>>>>>
> > >>>>>> Hi, Gerd
> > >>>>>> I checked the patches 4-7 of the SEV-SNP series. The common
> > >>>>>> OvmfMetadata is designed for both SEV and TDX, right?
> > >>>>> That is the idea, yes.
> > >>>>>
> > >>>>>> If so, then it means the SEV and TDX metadata will be mixed in
> > >>>>>> this OvmfMetadata.
> > >>>>> Yes.
> > >>>>>
> > >>>>>> I am thinking there will always be different fields for SEV and TDX.
> > >>>>>> For example, SEV has PcdOvmfSecGhcbPageTable but TDX doesn't
> need
> > >>>>>> that page. If the common OvmfMetadata is consumed by TDX-QEMU,
> > >> then
> > >>>>>> PcdOvmfSecGhcbPageTableBase will be initialized too.
> > >>>>>> That doesn't make sense.
> > >>>>> We have different range types.  OVMF_* are the common areas.
> > >>>>> SEV_* will be used by sev only, TDX_* will be used by tdx only.
> > >>>>> TDX and SEV entries are allowed to overlap, i.e.
> > >>>>> PcdOvmfSecGhcbPageTableBase should have some SEV_* type for sev
> (I
> > >>>>> think this needs fixing in the series), and tdx can use the page
> > >>>>> for something else by adding an
> > >>>>> TDX_* entry for the same range.
> > >>>>>
> > >>>>>> I am thinking that SEV and TDX can keep their own Metadata (in
> > >>>>>> separate files, SevMetadata.asm and TdxMetadata.asm) which are
> > >>>>>> pointed by the SEV or TDX offsets in the GUID-ed chain in ResetVector.
> > >>>>> I'd very much prefer to have a single table to avoid duplication
> > >>>>> for the common memory areas and keep the reset vector small.
> > >>>>>
> > >>>>> Having separate SevMetadata.asm + TdxMetadata.asm files (then have
> > >>>>> OvmfMetadata.asm include these two) is an option.  I think this
> > >>>>> isn't needed, we can also just group the entries in OvmfMetadata.asm.
> > >>>>>
> > >>>>>> In this case, SEV and TDX can design their own metadata flexibly,
> > >>>>>> for example, the attribute, the item structure, add/remove/update
> > >>>>>> the items, etc.
> > >>>>> Why have two ways to do the same thing?
> > >>>>>
> > >>>>> take care,
> > >>>>>    Gerd
> > >>>
> > >>>
> > >>> 
> > >>>
> > >

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Gerd Hoffmann]

On Thu, Sep 23, 2021 at 01:38:52PM +0000, Yao, Jiewen wrote:
> Good point, Min.
> 
> If https://github.com/AMDESE/ovmf/blob/snp-v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm is the proposal, then I have more comment:
> 
> Type: OVMF_SECTION_TYPE_CODE, OVMF_SECTION_TYPE_VARS are NOT used for SEV. I am not sure why they are there.

tdx needs them (for measurement).  It's not a tdx-specific concept,
possibly sev-snp wants use that too in the future.

> Type: OVMF_SECTION_TYPE_CPUID should be SEV specific. TDX does not need CPUID page.

A cpuid page can be used without sev too.

> Type: OVMF_SECTION_TYPE_SEC_MEM also seems for SEV. TDX does not need this special memory, such as Page table. It is already covered by code.

These are "needs pre-validation / pre-acceptance" regions.
TDX surely needs that too.

> Type: OVMF_SECTION_TYPE_SNP_SECRETS / OVMF_SECTION_TYPE_SNP_SEC_MEM is SEV specific.

Yes.

> The SEV table is totally different with TDX metadata table.

I can't see a fundamental difference.  In both cases the VMM needs
to know the firmware memory layout for (a) attestation, and (b)
pre-validating/pre-acceptance of memory, and (c) some
hardware-specific ranges such as snp secrets page.

> I really cannot see the benefit to merge into one table.

Keep reset vector small?
Have common parser structs and code?

take care,
  Gerd

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Gerd Hoffmann]

  Hi,

> > SEV hardware does not have a concept of the metadata. To boot SEV guest we
> > need to pass some information to VMM and in past those information were
> > passed through SNP_BOOT_BLOCK (GUIDed structure) but Gerd recommended
> > that it will be good idea if both SEV and TDX uses a common metadata approach
> > to pass these information. I personally think it was a good suggestion. So, in SNP
> > series I went ahead and created a generic metadata structure and  hope that
> > TDX will build on it. The user of the metadata structure is VMM (qemu, etc);
> > while launching the guest the VMM knows whether its creating the SEV or TDX
> > guest and will process the entries accordingly.
> > 
> > As per the number of fields in the metadata is concerns, I felt 3 fields (start, size
> > and type) should be good enough for all the cases. There was a question from
> > Gerd to Min asking why do you need the dataoffset/rawdatasize etc and I don't
> > remember seeing the answer for it.
> 
> The discussion is in this link. https://edk2.groups.io/g/devel/message/80289

The question why TDX_BFV_RAW_DATA_OFFSET and TDX_BFV_RAW_DATA_SIZE are
needed and why TDX_BFV_MEMORY_BASE + TDX_BFV_MEMORY_SIZE can't be used
is still open.

While being at it: The question why "config-b" with a completely
different initialization code path is needed is still open too.  The
tdvf design guide is not helpful here.  Although explains what is
different in "config-a" vs. "config-b" it does not explain the
background, i.e. why some features are supported by "config-b"
only.

And I guess these two questions are related.  With "config-a" there is a
fixed offset between TDX_BFV_RAW_DATA_OFFSET + TDX_BFV_MEMORY_BASE, so
if you know one of them you can easily calculate the other.  With
"config-b" this is possibly not the case.

So, can you please shed some light on this?

thanks,
  Gerd

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Gerd Hoffmann]

On Thu, Sep 23, 2021 at 02:19:17PM +0000, Yao, Jiewen wrote:
> All fields in TDX metadata are required. So the current SEV proposal
> (3 fields) does not work for TDX. The extra fields are used to guide
> VMM on how to copy the binary, allocate memory,

--verbose please.

The VMM loads the firmware just fine today without that metadata because
it's defined by the x86 architecture how to the firmware must be loaded.

And note that we are discussing an unified normal/sev/tdx firmware
binary here, so the "we might do something completely different for
tdx in the future" argument isn't very convincing here.

take care,
  Gerd

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Min Xu]

On September 24, 2021 1:28 PM, Gerd Hoffmann wrote:
>   Hi,
> 
> > > SEV hardware does not have a concept of the metadata. To boot SEV
> > > guest we need to pass some information to VMM and in past those
> > > information were passed through SNP_BOOT_BLOCK (GUIDed structure)
> > > but Gerd recommended that it will be good idea if both SEV and TDX
> > > uses a common metadata approach to pass these information. I
> > > personally think it was a good suggestion. So, in SNP series I went
> > > ahead and created a generic metadata structure and  hope that TDX
> > > will build on it. The user of the metadata structure is VMM (qemu,
> > > etc); while launching the guest the VMM knows whether its creating the
> SEV or TDX guest and will process the entries accordingly.
> > >
> > > As per the number of fields in the metadata is concerns, I felt 3
> > > fields (start, size and type) should be good enough for all the
> > > cases. There was a question from Gerd to Min asking why do you need
> > > the dataoffset/rawdatasize etc and I don't remember seeing the answer
> for it.
> >
> > The discussion is in this link.
> > https://edk2.groups.io/g/devel/message/80289
> 
> The question why TDX_BFV_RAW_DATA_OFFSET and
> TDX_BFV_RAW_DATA_SIZE are needed and why TDX_BFV_MEMORY_BASE +
> TDX_BFV_MEMORY_SIZE can't be used is still open.
Here is a BFV metadata.
    37                                                           <1> _Bfv:
    38 00000140 00400800                     <1>   DD TDX_BFV_RAW_DATA_OFFSET
    39 00000144 00C03700                     <1>   DD TDX_BFV_RAW_DATA_SIZE
    40 00000148 0040C8FF00000000    <1>   DQ TDX_BFV_MEMORY_BASE
    41 00000150 00C0370000000000    <1>   DQ TDX_BFV_MEMORY_SIZE
    42 00000158 00000000                      <1>   DD TDX_METADATA_SECTION_TYPE_BFV
    43 0000015C 01000000                      <1>   DD TDX_METADATA_ATTRIBUTES_EXTENDMR

TDX_BFV_RAW_DATA_OFFSET/TDX_BFV_RAW_DATA_SIZE indicates the offset/size of BFV in Ovmf.fd.
TDX_BFV_MEMORY_BASE/ TDX_BFV_MEMORY_SIZE is the physical memory address which is to be mapped.
TDX-QEMU use these information to 1) do the measurement of the BFV 2) map the BFV to the physical memory

> 
> While being at it: The question why "config-b" with a completely different
> initialization code path is needed is still open too.  The tdvf design guide is
> not helpful here.  Although explains what is different in "config-a" vs. "config-
> b" it does not explain the background, i.e. why some features are supported
> by "config-b" only.
The solution of Config-A and Config-B is to address the concerns of One-Binary requirement.
See Erdem Aktas's comments in this link https://edk2.groups.io/g/devel/message/76339
Quote: "What we are asking with "one binary" is: Simply enabling OVMF + a guest OS to boot in a TDX domain without breaking any existing functionality.  Intel should put everything else (specifically related to remote attestation) in a separate platform configuration."

*Config-A* enables a minimum functionality in OvmfPkgX64.dsc without breaking existing functionality.
*Config-B* is a separate platform  configuration file can be used to provide all the security guarantees that TDX provides.

> 
> And I guess these two questions are related.  With "config-a" there is a fixed
> offset between TDX_BFV_RAW_DATA_OFFSET + TDX_BFV_MEMORY_BASE,
> so if you know one of them you can easily calculate the other.  With "config-
> b" this is possibly not the case.
> 
> So, can you please shed some light on this?
As I explained above how BFV metadata is used by TDX-QEMU, there is no different in Config-A and Config-B.

Below are some links discussing the Config-A and Config-B. Hope it is helpful.

Config-A and Config-B:
https://edk2.groups.io/g/devel/message/76367
Erdem Aktas's comments:
https://edk2.groups.io/g/devel/message/76339
Laszlo's comments
https://edk2.groups.io/g/devel/message/76836
https://edk2.groups.io/g/devel/message/76837

Thanks!
Min

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Yao, Jiewen]

Hi Gerd
Having config-a and config-b is proposed by original RedHat rep in EDKII - Laszlo.
We reach the agreement to separate those 2 configuration and AMD SEV is taking same approach.

Are you saying you want to reset the high level plan and unify config-a and config-b into one binary?

Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Friday, September 24, 2021 1:28 PM
> To: Xu, Min M <min.m.xu@intel.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; devel@edk2.groups.io; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Erdem Aktas <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Tom Lendacky <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
>   Hi,
> 
> > > SEV hardware does not have a concept of the metadata. To boot SEV guest
> we
> > > need to pass some information to VMM and in past those information were
> > > passed through SNP_BOOT_BLOCK (GUIDed structure) but Gerd
> recommended
> > > that it will be good idea if both SEV and TDX uses a common metadata
> approach
> > > to pass these information. I personally think it was a good suggestion. So, in
> SNP
> > > series I went ahead and created a generic metadata structure and  hope that
> > > TDX will build on it. The user of the metadata structure is VMM (qemu, etc);
> > > while launching the guest the VMM knows whether its creating the SEV or
> TDX
> > > guest and will process the entries accordingly.
> > >
> > > As per the number of fields in the metadata is concerns, I felt 3 fields (start,
> size
> > > and type) should be good enough for all the cases. There was a question
> from
> > > Gerd to Min asking why do you need the dataoffset/rawdatasize etc and I
> don't
> > > remember seeing the answer for it.
> >
> > The discussion is in this link. https://edk2.groups.io/g/devel/message/80289
> 
> The question why TDX_BFV_RAW_DATA_OFFSET and
> TDX_BFV_RAW_DATA_SIZE are
> needed and why TDX_BFV_MEMORY_BASE + TDX_BFV_MEMORY_SIZE can't be
> used
> is still open.
> 
> While being at it: The question why "config-b" with a completely
> different initialization code path is needed is still open too.  The
> tdvf design guide is not helpful here.  Although explains what is
> different in "config-a" vs. "config-b" it does not explain the
> background, i.e. why some features are supported by "config-b"
> only.
> 
> And I guess these two questions are related.  With "config-a" there is a
> fixed offset between TDX_BFV_RAW_DATA_OFFSET + TDX_BFV_MEMORY_BASE,
> so
> if you know one of them you can easily calculate the other.  With
> "config-b" this is possibly not the case.
> 
> So, can you please shed some light on this?
> 
> thanks,
>   Gerd
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Yao, Jiewen]

That is my question.
AMD has its own extension. TDX has its own extension.
Why we have to unify the firmware binary, and to make both us unconfirmable?
Or do we want to unify ARM/AARch64/RISC-V ?

I agree we can unify as much as possible.
But due to hardware difference i don't think we achieve 100% unifying. 

Thank you
Yao Jiewen

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Friday, September 24, 2021 1:37 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Xu, Min M <min.m.xu@intel.com>; Brijesh Singh <brijesh.singh@amd.com>;
> devel@edk2.groups.io; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Erdem Aktas <erdemaktas@google.com>;
> James Bottomley <jejb@linux.ibm.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
> On Thu, Sep 23, 2021 at 02:19:17PM +0000, Yao, Jiewen wrote:
> > All fields in TDX metadata are required. So the current SEV proposal
> > (3 fields) does not work for TDX. The extra fields are used to guide
> > VMM on how to copy the binary, allocate memory,
> 
> --verbose please.
> 
> The VMM loads the firmware just fine today without that metadata because
> it's defined by the x86 architecture how to the firmware must be loaded.
> 
> And note that we are discussing an unified normal/sev/tdx firmware
> binary here, so the "we might do something completely different for
> tdx in the future" argument isn't very convincing here.
> 
> take care,
>   Gerd

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Yao, Jiewen]

Comment below:

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Friday, September 24, 2021 12:54 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io;
> brijesh.singh@amd.com; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Erdem Aktas <erdemaktas@google.com>;
> James Bottomley <jejb@linux.ibm.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
> On Thu, Sep 23, 2021 at 01:38:52PM +0000, Yao, Jiewen wrote:
> > Good point, Min.
> >
> > If https://github.com/AMDESE/ovmf/blob/snp-
> v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm is the proposal, then I have
> more comment:
> >
> > Type: OVMF_SECTION_TYPE_CODE, OVMF_SECTION_TYPE_VARS are NOT
> used for SEV. I am not sure why they are there.
> 
> tdx needs them (for measurement).  It's not a tdx-specific concept,
> possibly sev-snp wants use that too in the future.
That means this is only for TDX. SEV does not need this type. Then this is TDX specific.

> 
> > Type: OVMF_SECTION_TYPE_CPUID should be SEV specific. TDX does not need
> CPUID page.
> 
> A cpuid page can be used without sev too.
I don't think TDX need this field. This is SEV specific.


> 
> > Type: OVMF_SECTION_TYPE_SEC_MEM also seems for SEV. TDX does not
> need this special memory, such as Page table. It is already covered by code.
> 
> These are "needs pre-validation / pre-acceptance" regions.
> TDX surely needs that too.
I don't think TDX need this. The page table should be covered by CODE already.


> 
> > Type: OVMF_SECTION_TYPE_SNP_SECRETS /
> OVMF_SECTION_TYPE_SNP_SEC_MEM is SEV specific.
> 
> Yes.
> 
> > The SEV table is totally different with TDX metadata table.
> 
> I can't see a fundamental difference.  In both cases the VMM needs
> to know the firmware memory layout for (a) attestation, and (b)
> pre-validating/pre-acceptance of memory, and (c) some
> hardware-specific ranges such as snp secrets page.
> 
> > I really cannot see the benefit to merge into one table.
> 
> Keep reset vector small?
> Have common parser structs and code?

I think it is opposite. This proposal makes reset vector larger, if we need define more structure to satisfy TDX, but it is not needed by SEV. Or Define something purely for SEV, but not useful for TDX.
I don't treat it as benefit. Instead I think it is big burden.


> 
> take care,
>   Gerd

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Gerd Hoffmann]

On Fri, Sep 24, 2021 at 07:32:33AM +0000, Yao, Jiewen wrote:
> Hi Gerd
> Having config-a and config-b is proposed by original RedHat rep in EDKII - Laszlo.
> We reach the agreement to separate those 2 configuration and AMD SEV is taking same approach.
> 
> Are you saying you want to reset the high level plan and unify config-a and config-b into one binary?

There isn't that much of a difference between the normal and amd sev
build.  It has additional drivers and the grub boot loader added, smm
support turned off, network stack removed.

The differences between config-a and config-b are much larger according
to the design document.  config-b has a completely different
initialization code path, skipping the PEI phase.  I see that as a
major problem when it comes to long-term maintenance, and so far nobody
could explain the reason for this.

I'll go read the links to old discussions sent by Min, maybe I find
something there.

Having a config-b with network stack disabled, driver for RTMR trusted
boot added, maybe some other little tweaks but otherwise a boot workflow
identical to config-a is reasonable in my eyes.  Merging it with AmdSev
should also be relatively easy then.

take care,
  Gerd

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Gerd Hoffmann]

On Fri, Sep 24, 2021 at 07:36:10AM +0000, Yao, Jiewen wrote:
> That is my question.
> AMD has its own extension. TDX has its own extension.
> Why we have to unify the firmware binary, and to make both us unconfirmable?

Isn't that the plan anyway?  At least for "config-a" with a basic
feature set?  See other mail just sent for comments on "config-b".

> Or do we want to unify ARM/AARch64/RISC-V ?

Not sure what you are trying to tell me.

take care,
  Gerd

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Gerd Hoffmann]

  Hi,

> > > Type: OVMF_SECTION_TYPE_SEC_MEM also seems for SEV. TDX does not
> > need this special memory, such as Page table. It is already covered by code.
> > 
> > These are "needs pre-validation / pre-acceptance" regions.
> > TDX surely needs that too.
> I don't think TDX need this. The page table should be covered by CODE already.

I think you are wrong here, the patch has this ...

+_OvmfPageTable:
+  DD 0
+  DD 0
+  DQ OVMF_PAGE_TABLE_BASE
+  DQ OVMF_PAGE_TABLE_SIZE
+  DD TDX_METADATA_SECTION_TYPE_TEMP_MEM
+  DD 0

... and a few simliar entries.

> > > I really cannot see the benefit to merge into one table.
> > 
> > Keep reset vector small?
> > Have common parser structs and code?
> 
> I think it is opposite. This proposal makes reset vector larger, if we
> need define more structure to satisfy TDX, but it is not needed by
> SEV.

The sev and tdx specific entries will be there anyway, no matter
whenever we place them into one or two separate tables.

Shared items like the page table memory will be there only once
when we use a unified table, but twice with two separate tables.

take care,
  Gerd

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Yao, Jiewen]

I think we are discussing two topics. Please allow me to separate them.

1) Topic one: A unified build for config-A and config-B

I think we have discussed that before in EDKII, when Laszlo suggested:
A) don't put all TDX features into OvmfPkg.dsc, just put a basic feature them - we call it config-A.
B) Put full TDX feature into another TdvfPkg.dsc - we call it config-B.
Once we finish A) and B), we can evaluate how to merge B into A.
I do recommend you track back or have a discussion in RedHat to see what is high level suggestion from RedHat.


2) Topic two: A unified metadata table for SEV and TDX.

To me, I don't see it is necessary. I would say: I agree with you that we can align the design as much as possible, such as MemEncryption, ExceptionLib, IOMMU, etc.
However, if there is something totally different, I see no benefit to merge.
One example I could give is ACPI MADT table, X86 system defines its own interrupt table (APIC), while ARM system defines its own interrupt table (GIC). There is NO need to define a common interrupt table to cover both X86 and ARM.

I think current two table approach is good enough. TDX owner maintains its own table. SEV owner maintains its own table. Just like APIC table and GIC in ACPI MADT.
Although they are called confidential computing technology, the hardware implementation is different and features are different.
>From software layer, we can have HAL. But it does not mean we only have one common HAL for SEV and TDX. Two different HAL implementation are acceptable.

For the one table proposal, I would like to understand
A) What is the problem statement with current implementation?
B) What is the goal we want to achieve?
C) What is the benefit we can get?

Please be as specific as possible.

BTW: For C), I don't think we will have smaller code size, because we align we have to define some unnecessary field.
For your statement to remove duplication, please give me some real example. The page table example is invalid, because TDX does not need define an page table entry.
SEV requires GHCB, CPUID page but TDX does not. While TDX need indicate which range extend to MRTD, which is NOT. Also TDX metadata table will indicate which region may use AUG page, and which use ADD page in the future. I am not sure if SEV need those info.


Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Friday, September 24, 2021 5:24 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Xu, Min M <min.m.xu@intel.com>; Brijesh Singh <brijesh.singh@amd.com>;
> devel@edk2.groups.io; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Erdem Aktas <erdemaktas@google.com>;
> James Bottomley <jejb@linux.ibm.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
> On Fri, Sep 24, 2021 at 07:36:10AM +0000, Yao, Jiewen wrote:
> > That is my question.
> > AMD has its own extension. TDX has its own extension.
> > Why we have to unify the firmware binary, and to make both us unconfirmable?
> 
> Isn't that the plan anyway?  At least for "config-a" with a basic
> feature set?  See other mail just sent for comments on "config-b".
> 
> > Or do we want to unify ARM/AARch64/RISC-V ?
> 
> Not sure what you are trying to tell me.
> 
> take care,
>   Gerd
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Gerd Hoffmann]

  Hi,

> > The question why TDX_BFV_RAW_DATA_OFFSET and
> > TDX_BFV_RAW_DATA_SIZE are needed and why TDX_BFV_MEMORY_BASE +
> > TDX_BFV_MEMORY_SIZE can't be used is still open.
> Here is a BFV metadata.
>     37                                                           <1> _Bfv:
>     38 00000140 00400800                     <1>   DD TDX_BFV_RAW_DATA_OFFSET
>     39 00000144 00C03700                     <1>   DD TDX_BFV_RAW_DATA_SIZE
>     40 00000148 0040C8FF00000000    <1>   DQ TDX_BFV_MEMORY_BASE
>     41 00000150 00C0370000000000    <1>   DQ TDX_BFV_MEMORY_SIZE
>     42 00000158 00000000                      <1>   DD TDX_METADATA_SECTION_TYPE_BFV
>     43 0000015C 01000000                      <1>   DD TDX_METADATA_ATTRIBUTES_EXTENDMR
> 
> TDX_BFV_RAW_DATA_OFFSET/TDX_BFV_RAW_DATA_SIZE indicates the offset/size of BFV in Ovmf.fd.
> TDX_BFV_MEMORY_BASE/ TDX_BFV_MEMORY_SIZE is the physical memory address which is to be mapped.
> TDX-QEMU use these information to 1) do the measurement of the BFV 2) map the BFV to the physical memory

TDX_BFV_RAW_DATA_SIZE + TDX_BFV_MEMORY_SIZE are identical.  Why do you
need both?  Yes, I know, some entries have RAW_DATA_SIZE=0 because
nothing is loaded for them.  You can also figure that by looking at the
section type.

TDX_BFV_RAW_DATA_OFFSET can be calculated from TDX_BFV_MEMORY_BASE, it's
a fixed offset (depending on firmware size).  At least as long as the
firmware loading uses to the usual x86 workflow (place right below 4G).

Also: When placing the firmware below 4G MEMORY_BASE + MEMORY_SIZE can
be DD.

The attribute field can be added to the ovmf meta data, or we make that
part of the section type.

> *Config-A* enables a minimum functionality in OvmfPkgX64.dsc without
> breaking existing functionality.

> *Config-B* is a separate platform  configuration file can be used to
> provide all the security guarantees that TDX provides.

Why does config-b need a completely different initialization code path
(i.e. skip PEI, see slide 11 of the tdvf design review)?

take care,
  Gerd

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Yao, Jiewen]

You are right. My statement for page table is wrong. Both TDX and SEV need them.

That is NOT our original design. But I can understand why it is changed today.

I compare https://github.com/tianocore/edk2-staging/blob/TDVF/OvmfPkg/ResetVector/X64/TdxMetadata.asm and https://github.com/AMDESE/ovmf/blob/snp-v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm.

There are 8 entries in TDX, and 10 entries in SEV.
2 of them are same, page table and TEMP RAM.
6 entries are TDX unique. 8 entries are SEV unique.

I still feel it is burden to merge them, because some attributes field is NOT required for SEV but needed for TDX.
And TDX parsing tool need rule out SEV entries, and SEV parsing tool need rule out TDX entries.



Thank you
Yao Jiewen

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Friday, September 24, 2021 5:34 PM
> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Xu, Min M <min.m.xu@intel.com>; brijesh.singh@amd.com; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Erdem Aktas <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Tom Lendacky <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
>   Hi,
> 
> > > > Type: OVMF_SECTION_TYPE_SEC_MEM also seems for SEV. TDX does not
> > > need this special memory, such as Page table. It is already covered by code.
> > >
> > > These are "needs pre-validation / pre-acceptance" regions.
> > > TDX surely needs that too.
> > I don't think TDX need this. The page table should be covered by CODE already.
> 
> I think you are wrong here, the patch has this ...
> 
> +_OvmfPageTable:
> +  DD 0
> +  DD 0
> +  DQ OVMF_PAGE_TABLE_BASE
> +  DQ OVMF_PAGE_TABLE_SIZE
> +  DD TDX_METADATA_SECTION_TYPE_TEMP_MEM
> +  DD 0
> 
> ... and a few simliar entries.
> 
> > > > I really cannot see the benefit to merge into one table.
> > >
> > > Keep reset vector small?
> > > Have common parser structs and code?
> >
> > I think it is opposite. This proposal makes reset vector larger, if we
> > need define more structure to satisfy TDX, but it is not needed by
> > SEV.
> 
> The sev and tdx specific entries will be there anyway, no matter
> whenever we place them into one or two separate tables.
> 
> Shared items like the page table memory will be there only once
> when we use a unified table, but twice with two separate tables.
> 
> take care,
>   Gerd

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Brijesh Singh]

On 9/24/21 2:39 AM, Yao, Jiewen wrote:
> Comment below:
>
>> -----Original Message-----
>> From: Gerd Hoffmann <kraxel@redhat.com>
>> Sent: Friday, September 24, 2021 12:54 PM
>> To: Yao, Jiewen <jiewen.yao@intel.com>
>> Cc: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io;
>> brijesh.singh@amd.com; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen,
>> Jordan L <jordan.l.justen@intel.com>; Erdem Aktas <erdemaktas@google.com>;
>> James Bottomley <jejb@linux.ibm.com>; Tom Lendacky
>> <thomas.lendacky@amd.com>
>> Subject: Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
>>
>> On Thu, Sep 23, 2021 at 01:38:52PM +0000, Yao, Jiewen wrote:
>>> Good point, Min.
>>>
>>> If https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Fovmf%2Fblob%2Fsnp-&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C5452ded75af34b9c73aa08d97f2e8426%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637680660025337914%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=3yXoKMmZndyiJkpr2gADbq6KvPLoF1r5sCv32MxK4Ms%3D&amp;reserved=0
>> v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm is the proposal, then I have
>> more comment:
>>> Type: OVMF_SECTION_TYPE_CODE, OVMF_SECTION_TYPE_VARS are NOT
>> used for SEV. I am not sure why they are there.
>>
>> tdx needs them (for measurement).  It's not a tdx-specific concept,
>> possibly sev-snp wants use that too in the future.
> That means this is only for TDX. SEV does not need this type. Then this is TDX specific.

FYI, SEV also measures code or data put by the VMM in the guest memory
space. In SEV, qemu calls a routine to encrypt the pflash unit0 -- while
handling the callback you can convert the GPA to HVA and thus avoid the
section. But having the section available is not going to create any
issues with the SEV and in some cases it may be useful.


>>> Type: OVMF_SECTION_TYPE_CPUID should be SEV specific. TDX does not need
>> CPUID page.
>>
>> A cpuid page can be used without sev too.
> I don't think TDX need this field. This is SEV specific.
>
You are right that CPUID page is currently SEV specific but nothing
stops another VMM to use CPUID page on the non-SEV platform to minimize
the number of VM exits. I think once guest kernel gets support for using
the CPUID page it may become reality sooner.


>>> Type: OVMF_SECTION_TYPE_SEC_MEM also seems for SEV. TDX does not
>> need this special memory, such as Page table. It is already covered by code.
>>
>> These are "needs pre-validation / pre-acceptance" regions.
>> TDX surely needs that too.
> I don't think TDX need this. The page table should be covered by CODE already.
>
Page table is data page and are not covered by the CODE section.  IIUC,
TDX also need to accept memory before the access. There are several data
pages (such page table, stack, heap, ...)  used in the SEC phase; you
have two choice, you either accept them during the SEC phase boot or
accept them before the boot. If accepted before the boot then they will
be measured so that security is not compromised. For simplicity it seems
both SNP and TDX decided to accept those data pages before the boot. The
metadata simply provides the description of those pre accepted pages.

>>> Type: OVMF_SECTION_TYPE_SNP_SECRETS /
>> OVMF_SECTION_TYPE_SNP_SEC_MEM is SEV specific.
>>
>> Yes.
>>
>>> The SEV table is totally different with TDX metadata table.
>> I can't see a fundamental difference.  In both cases the VMM needs
>> to know the firmware memory layout for (a) attestation, and (b)
>> pre-validating/pre-acceptance of memory, and (c) some
>> hardware-specific ranges such as snp secrets page.
>>
>>> I really cannot see the benefit to merge into one table.
>> Keep reset vector small?
>> Have common parser structs and code?
> I think it is opposite. This proposal makes reset vector larger, if we need define more structure to satisfy TDX, but it is not needed by SEV. Or Define something purely for SEV, but not useful for TDX.
> I don't treat it as benefit. Instead I think it is big burden.
>
>
>> take care,
>>   Gerd

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Yao, Jiewen]

Again. Two topics. We need discuss them separately.

Topic 1: TD metadata table is an architecture way to communicate with VMM.
We took the design from PE/COFF image section, which is flexible to support different binary format.
EDKII TDVF is just one possible producer. There could be other producer in the future. We don't want to define something only meet current TDVF need.
The ATTRIBUTE field are separated from TYPE field, because they are different thing. Currently, you may use TYPE to predict what ATTRIBUTE will be. But that is NOT always correct. We are adding more ATTRIBUTE for the future.

Topic 2: In config-B we remove PEI.
I think we should say it in different way: We add PEI back in config-A.
In our original design we totally eliminated PEI, because it is unnecessary. IMHO, it is totally an overdesign in OVMF to include PEI.
However, in order to be compatible with current OVMF and make "minimal" change for config-A, we add PEI back.

Thank you
Yao Jiewen


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Friday, September 24, 2021 6:07 PM
> To: Xu, Min M <min.m.xu@intel.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; devel@edk2.groups.io; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Erdem Aktas <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Tom Lendacky <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
>   Hi,
> 
> > > The question why TDX_BFV_RAW_DATA_OFFSET and
> > > TDX_BFV_RAW_DATA_SIZE are needed and why TDX_BFV_MEMORY_BASE +
> > > TDX_BFV_MEMORY_SIZE can't be used is still open.
> > Here is a BFV metadata.
> >     37                                                           <1> _Bfv:
> >     38 00000140 00400800                     <1>   DD TDX_BFV_RAW_DATA_OFFSET
> >     39 00000144 00C03700                     <1>   DD TDX_BFV_RAW_DATA_SIZE
> >     40 00000148 0040C8FF00000000    <1>   DQ TDX_BFV_MEMORY_BASE
> >     41 00000150 00C0370000000000    <1>   DQ TDX_BFV_MEMORY_SIZE
> >     42 00000158 00000000                      <1>   DD
> TDX_METADATA_SECTION_TYPE_BFV
> >     43 0000015C 01000000                      <1>   DD
> TDX_METADATA_ATTRIBUTES_EXTENDMR
> >
> > TDX_BFV_RAW_DATA_OFFSET/TDX_BFV_RAW_DATA_SIZE indicates the
> offset/size of BFV in Ovmf.fd.
> > TDX_BFV_MEMORY_BASE/ TDX_BFV_MEMORY_SIZE is the physical memory
> address which is to be mapped.
> > TDX-QEMU use these information to 1) do the measurement of the BFV 2) map
> the BFV to the physical memory
> 
> TDX_BFV_RAW_DATA_SIZE + TDX_BFV_MEMORY_SIZE are identical.  Why do
> you
> need both?  Yes, I know, some entries have RAW_DATA_SIZE=0 because
> nothing is loaded for them.  You can also figure that by looking at the
> section type.
> 
> TDX_BFV_RAW_DATA_OFFSET can be calculated from
> TDX_BFV_MEMORY_BASE, it's
> a fixed offset (depending on firmware size).  At least as long as the
> firmware loading uses to the usual x86 workflow (place right below 4G).
> 
> Also: When placing the firmware below 4G MEMORY_BASE + MEMORY_SIZE can
> be DD.
> 
> The attribute field can be added to the ovmf meta data, or we make that
> part of the section type.
> 
> > *Config-A* enables a minimum functionality in OvmfPkgX64.dsc without
> > breaking existing functionality.
> 
> > *Config-B* is a separate platform  configuration file can be used to
> > provide all the security guarantees that TDX provides.
> 
> Why does config-b need a completely different initialization code path
> (i.e. skip PEI, see slide 11 of the tdvf design review)?
> 
> take care,
>   Gerd
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Brijesh Singh]

[-- Attachment #1: Type: text/plain, Size: 1737 bytes --]


On 9/24/21 5:11 AM, Yao, Jiewen wrote:
> You are right. My statement for page table is wrong. Both TDX and SEV need them.
>
> That is NOT our original design. But I can understand why it is changed today.
>
> I compare https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2-staging%2Fblob%2FTDVF%2FOvmfPkg%2FResetVector%2FX64%2FTdxMetadata.asm&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc5ee156422614a843cf408d97f43bc9b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637680751169724127%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=GI6%2BzLASgUtBT9kS%2B%2BVDfzLlImYhZJeETpNfDMIimAk%3D&reserved=0 and https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Fovmf%2Fblob%2Fsnp-v8%2FOvmfPkg%2FResetVector%2FX64%2FOvmfMetadata.asm&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc5ee156422614a843cf408d97f43bc9b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637680751169724127%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Dq2pLkdBsYXFbYM5nmy43yv4DWn7IDUceMpQg2%2F0jYo%3D&reserved=0.
>
> There are 8 entries in TDX, and 10 entries in SEV.
> 2 of them are same, page table and TEMP RAM.
> 6 entries are TDX unique. 8 entries are SEV unique.

In the SEV patches you are seeing more sections because I tried to keep
it in sync with the MEMFD [1] so that its much more readable. In TDX
patches, Min decided to breakdown things a bit further and in some case
skip sections. e.g  in TDX patches the SecPeiTempRamBase is broken into
stack and heap section.

[1] https://github.com/tianocore/edk2/blob/master/OvmfPkg/OvmfPkgX64.fdf#L63


thanks



[-- Attachment #2: Type: text/html, Size: 4038 bytes --]

Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Brijesh Singh]

Hi Min,

On 9/21/21 4:05 AM, Min Xu wrote:
>  ;
>  ; Modified:  EAX, EBX, ECX, EDX
>  ;
>  SetCr3ForPageTables64:
> -
> -    ; Clear the WorkArea header. The SEV probe routines will populate the
> -    ; work area when detected.
> -    mov     byte[WORK_AREA_GUEST_TYPE], 0

Why you are removing the above block ? The workarea hdr must be
initialized to zero, its not safe to assume that the guest memory is
zero'ed in the non-encrypted case.

thanks

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Gerd Hoffmann]

On Fri, Sep 24, 2021 at 05:38:21AM -0500, Brijesh Singh wrote:
> 
> On 9/24/21 5:11 AM, Yao, Jiewen wrote:
> > You are right. My statement for page table is wrong. Both TDX and SEV need them.
> >
> > That is NOT our original design. But I can understand why it is changed today.
> >
> > I compare https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2-staging%2Fblob%2FTDVF%2FOvmfPkg%2FResetVector%2FX64%2FTdxMetadata.asm&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc5ee156422614a843cf408d97f43bc9b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637680751169724127%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=GI6%2BzLASgUtBT9kS%2B%2BVDfzLlImYhZJeETpNfDMIimAk%3D&reserved=0 and https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Fovmf%2Fblob%2Fsnp-v8%2FOvmfPkg%2FResetVector%2FX64%2FOvmfMetadata.asm&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc5ee156422614a843cf408d97f43bc9b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637680751169724127%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Dq2pLkdBsYXFbYM5nmy43yv4DWn7IDUceMpQg2%2F0jYo%3D&reserved=0.
> >
> > There are 8 entries in TDX, and 10 entries in SEV.
> > 2 of them are same, page table and TEMP RAM.
> > 6 entries are TDX unique. 8 entries are SEV unique.
> 
> In the SEV patches you are seeing more sections because I tried to keep
> it in sync with the MEMFD [1] so that its much more readable.

We could add just a single range for stack + heap + pagetables  (+more?)
and comments saying which MEMFD areas are covered by that range, to keep
the table small.

take care,
  Gerd

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Brijesh Singh]

On 9/24/21 6:17 AM, Gerd Hoffmann via groups.io wrote:
> On Fri, Sep 24, 2021 at 05:38:21AM -0500, Brijesh Singh wrote:
>> On 9/24/21 5:11 AM, Yao, Jiewen wrote:
>>> You are right. My statement for page table is wrong. Both TDX and SEV need them.
>>>
>>> That is NOT our original design. But I can understand why it is changed today.
>>>
>>> I compare https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2-staging%2Fblob%2FTDVF%2FOvmfPkg%2FResetVector%2FX64%2FTdxMetadata.asm&data=04%7C01%7Cbrijesh.singh%40amd.com%7C7c131e7f76de43f01ae808d97f4cfb01%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637680790883761087%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=1nG6d2htqsRuvsnzlFTsGazh1f57WzAGG6pxn6sj90w%3D&reserved=0 and https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Fovmf%2Fblob%2Fsnp-v8%2FOvmfPkg%2FResetVector%2FX64%2FOvmfMetadata.asm&data=04%7C01%7Cbrijesh.singh%40amd.com%7C7c131e7f76de43f01ae808d97f4cfb01%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637680790883761087%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=TNcIqgCGnXn3Hl7QTrbePcuWmjtAH9DWUs9c6SENywY%3D&reserved=0.
>>>
>>> There are 8 entries in TDX, and 10 entries in SEV.
>>> 2 of them are same, page table and TEMP RAM.
>>> 6 entries are TDX unique. 8 entries are SEV unique.
>> In the SEV patches you are seeing more sections because I tried to keep
>> it in sync with the MEMFD [1] so that its much more readable.
> We could add just a single range for stack + heap + pagetables  (+more?)
> and comments saying which MEMFD areas are covered by that range, to keep
> the table small.

Sure, that is not an issue at all. As a matter of fact I had only one
section in my original SNP_BOOT_BLOCK GUID to cover the MEMFD region ;)
I think we can live with just 2 to 3 sections common to cover a large
amount of MEMFD, and still keep the code readable.

Section 1: Page table + lockBox + GuidedExtractHandler (0x0 - 0x8000)

Section 2: WorkArea + Ghcbbackup + TempRam (0xb000 - 0x20000)

The region 0x8000 - 0xa000 can be platform specific.

> take care,
>   Gerd
>
>
>
> 
>
>

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Gerd Hoffmann]

On Fri, Sep 24, 2021 at 10:33:35AM +0000, Yao, Jiewen wrote:
> Again. Two topics. We need discuss them separately.
> 
> Topic 1: TD metadata table is an architecture way to communicate with
> VMM.  We took the design from PE/COFF image section, which is flexible
> to support different binary format.
> EDKII TDVF is just one possible producer. There could be other
> producer in the future. We don't want to define something only meet
> current TDVF need.

Hmm.  efi has a kind-of binary format (EFI_FIRMWARE_VOLUME_HEADER).
It's not fully self-contained though, you need to know where the
architecture places the firmware (i.e. just below 4G for x86)
because the load address isn't there.  So I do see the point of
adding other headers adding that.

Possible alternative approach: Define an extension
(EFI_FIRMWARE_VOLUME_EXT_HEADER) for the load address and use that
instead of defining something new.

Still not clear why the size is in there twice.  I think you could
instead use a flag telling whenever a section must be loaded from
the image or not.  This is what COFF and ELF are doing too.

Also not clear why you want stick to 64bit base address.  Loading the
firmware above 4G isn't going to work without also changing a bunch of
other things and it will break backward compatibility anyway.

I think the entry size can be cut in half with something like this:

struct {
	uint32_t file_offset;
	uint32_t load_address;
	uint32_t section_size;
	uint16_t section_type;
	uint16_t section_flags;
};

> Topic 2: In config-B we remove PEI.
> I think we should say it in different way: We add PEI back in config-A.
> In our original design we totally eliminated PEI, because it is unnecessary. IMHO, it is totally an overdesign in OVMF to include PEI.

Granted.  PEI basically allows OEMs to plug their binary PEIMs into
early hardware initialization.  For a full open source firmware there
is little reason to support that, other than maybe using PEIMs from
other edk2 Pkgs unmodified.

But, again, I don't want have two completely different initialization
code paths in OVMF.  We can certainly investigate and discuss dropping
PEI, but that clearly shouldn't be a TDX-only thing.  When we eliminate
PEI, we should do it for all OVMF builds.

take care,
  Gerd

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Yao, Jiewen]

Comment below

> -----Original Message-----
> From: kraxel@redhat.com <kraxel@redhat.com>
> Sent: Friday, September 24, 2021 10:02 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: devel@edk2.groups.io; Xu, Min M <min.m.xu@intel.com>; Brijesh Singh
> <brijesh.singh@amd.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Justen, Jordan L <jordan.l.justen@intel.com>; Erdem Aktas
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> Lendacky <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
> On Fri, Sep 24, 2021 at 10:33:35AM +0000, Yao, Jiewen wrote:
> > Again. Two topics. We need discuss them separately.
> >
> > Topic 1: TD metadata table is an architecture way to communicate with
> > VMM.  We took the design from PE/COFF image section, which is flexible
> > to support different binary format.
> > EDKII TDVF is just one possible producer. There could be other
> > producer in the future. We don't want to define something only meet
> > current TDVF need.
> 
> Hmm.  efi has a kind-of binary format (EFI_FIRMWARE_VOLUME_HEADER).
> It's not fully self-contained though, you need to know where the
> architecture places the firmware (i.e. just below 4G for x86)
> because the load address isn't there.  So I do see the point of
> adding other headers adding that.
>
> Possible alternative approach: Define an extension
> (EFI_FIRMWARE_VOLUME_EXT_HEADER) for the load address and use that
> instead of defining something new.
>

[Jiewen] I would say it is terrible idea to use EFI_FIRMWARE_VOLUME_HEADER.

This is defined by PI specification, the purpose is to have a way to manage the firmware file.
It is very complicated and with overhead only needed for flash.

Intel has multiple technologies requires firmware/hardware interface. None of them uses EFI_FIRMARE_VOLUME_HEADER, because it is unnecessary to carry the complexity.

The most famous firmware/hardware interface is called Firmware Interface Table (FIT) table. - https://software.intel.com/content/dam/develop/external/us/en/documents/firmware-interface-table-bios-specification-r1p2p1.pdf.

Also using EFI_FIRMWARE_VOLUME_HEADER means you have to use PI FV layout, which is another unnecessary limitation. 

I would strongly disagree to using EFI_FIRMWARE_VOLUME_HEADER for metadata table.

> Still not clear why the size is in there twice.  I think you could
> instead use a flag telling whenever a section must be loaded from
> the image or not.  This is what COFF and ELF are doing too.
> 
> Also not clear why you want stick to 64bit base address.  Loading the
> firmware above 4G isn't going to work without also changing a bunch of
> other things and it will break backward compatibility anyway.

[Jiewen] That is our previously experience, when we define a physical address, we always use 64bit to leave room for extension.
Like UEFI specification defines PHYSICAL_ADDRESS to be UINT64 even in IA32 platform.

Let me tell you a story.
In https://github.com/tianocore/edk2/blob/master/SecurityPkg/Include/Ppi/FirmwareVolumeInfoPrehashedFV.h, we defined the physical address to be FvBase.
  UINT32                                     FvBase;
  UINT32                                     FvLength;

It is rare, because in other context, we usually define FvBase to be 64bit.

Later, when we want to enable a host app based unit test for this data structure, we got test crash directly, because the OS app test allocates an above 4G base address, and the test case cast it to UINT32.

Do we really care to save 4 bytes in the PPI definition? I don't think so.
But it just defines in this way and it brings a big burden to enable unit test.
This code has been checked in for 2 years. Till now, I still regret that why we didn't use UINT64 in the beginning.

I have seen other examples to define small size, such as PI Section Size (24 bit), FFS Size (24 bit), HOB size (16 bit). Then we run into problem to support large structure later and we have to figure out ugly work-around to support those cases.

Defining UINT64 gives us enough flexibility for the future, including test in above 4G environment.
I am wondering that, do you really care to save 4 bytes from UINT64 to UINT32 ?

For type, maybe 2^16 is enough. But for flags, I prefer 32bit.

> 
> I think the entry size can be cut in half with something like this:
> 
> struct {
> 	uint32_t file_offset;
> 	uint32_t load_address;
> 	uint32_t section_size;
> 	uint16_t section_type;
> 	uint16_t section_flags;
> };
> 
> > Topic 2: In config-B we remove PEI.
> > I think we should say it in different way: We add PEI back in config-A.
> > In our original design we totally eliminated PEI, because it is unnecessary.
> IMHO, it is totally an overdesign in OVMF to include PEI.
> 
> Granted.  PEI basically allows OEMs to plug their binary PEIMs into
> early hardware initialization.  For a full open source firmware there
> is little reason to support that, other than maybe using PEIMs from
> other edk2 Pkgs unmodified.
> 
> But, again, I don't want have two completely different initialization
> code paths in OVMF.  We can certainly investigate and discuss dropping
> PEI, but that clearly shouldn't be a TDX-only thing.  When we eliminate
> PEI, we should do it for all OVMF builds.

[Jiewen] I think this is out of scope of TDVF config-B patch series.
I don't think it is fair to enable OVMF to remove PEI, just because TDVF does not need PEI. 

If you look around other edk2 platform projects, you can already find some of them does not have PEI. And EmbeddedPkg includes some libs to support those case, such as https://github.com/tianocore/edk2/tree/master/EmbeddedPkg/Library/PrePiHobLib, https://github.com/tianocore/edk2/tree/master/EmbeddedPkg/Library/PrePiLib.
But that does not mean, we need do that for every platform.

Each platform owner can have their own choice.

If you have intertest to remove PEI, I am happy to discuss with you on detail.
However, I would treat "removing PEI from OVMF" and "enable TDVF config-B" be two different tasks.


> 
> take care,
>   Gerd

Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Min Xu]

On September 24, 2021 6:58 PM, Brijesh Singh wrote:
> Hi Min,
> 
> On 9/21/21 4:05 AM, Min Xu wrote:
> >  ;
> >  ; Modified:  EAX, EBX, ECX, EDX
> >  ;
> >  SetCr3ForPageTables64:
> > -
> > -    ; Clear the WorkArea header. The SEV probe routines will populate the
> > -    ; work area when detected.
> > -    mov     byte[WORK_AREA_GUEST_TYPE], 0
> 
> Why you are removing the above block ? The workarea hdr must be initialized
> to zero, its not safe to assume that the guest memory is zero'ed in the non-
> encrypted case.
>
Hi, Brijesh
Please see below explanation (It is in the commit message)
- In Main16 entry point, after TransitionFromReal16To32BitFlat,
   WORK_AREA_GUEST_TYPE is cleared to 0. WORK_AREA_GUEST_TYPE was
   previously cleared in SetCr3ForPageTables64 (see commit ab77b60).
   This doesn't work after TDX is introduced in Ovmf. It is because all
   TDX CPUs (BSP and APs) start to run from 0xfffffff0. In previous code
   WORK_AREA_GUEST_TYPE will be cleared multi-times in TDX guest. So for
   SEV and Legacy guest it is moved to Main16 entry point (after
   TransitionFromReal16To32BitFlat). For TDX guest WORK_AREA_GUEST_TYPE
   is cleared and set in InitTdxWorkarea
> 

Thanks!
Min

Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Brijesh Singh]

Hi Min,


On 9/24/21 7:03 PM, Xu, Min M wrote:
> On September 24, 2021 6:58 PM, Brijesh Singh wrote:
>> Hi Min,
>>
>> On 9/21/21 4:05 AM, Min Xu wrote:
>>>  ;
>>>  ; Modified:  EAX, EBX, ECX, EDX
>>>  ;
>>>  SetCr3ForPageTables64:
>>> -
>>> -    ; Clear the WorkArea header. The SEV probe routines will populate the
>>> -    ; work area when detected.
>>> -    mov     byte[WORK_AREA_GUEST_TYPE], 0
>> Why you are removing the above block ? The workarea hdr must be initialized
>> to zero, its not safe to assume that the guest memory is zero'ed in the non-
>> encrypted case.
>>
> Hi, Brijesh
> Please see below explanation (It is in the commit message)
> - In Main16 entry point, after TransitionFromReal16To32BitFlat,
>    WORK_AREA_GUEST_TYPE is cleared to 0. WORK_AREA_GUEST_TYPE was
>    previously cleared in SetCr3ForPageTables64 (see commit ab77b60).
>    This doesn't work after TDX is introduced in Ovmf. It is because all
>    TDX CPUs (BSP and APs) start to run from 0xfffffff0. In previous code
>    WORK_AREA_GUEST_TYPE will be cleared multi-times in TDX guest. So for
>    SEV and Legacy guest it is moved to Main16 entry point (after
>    TransitionFromReal16To32BitFlat). For TDX guest WORK_AREA_GUEST_TYPE
>    is cleared and set in InitTdxWorkarea

thanks for clarifying it.

This is very busy commit and making several changes at once, so some of
important common code movement is getting lost. Maybe I recommend you to
please break it into multiple. e,g  this particular change can be very
easily broken into two commits

1) Since TDX support need the change in the boot flow, and you are no
longer using the Main.asm from the UefiCpuPkg. This can be a pre-patch
in which you copy UefiCpuPkg/ResetVector/Vtf0/main.asm ->
OvmfPkg/ResetVector/Main.asm and document reason behind the move.

2) Remove clearing of workarea from SetCr3ForPageTables64 to Main.asm

Now that we have override for the Main.asm, I think clearing of the
workarea should be done for all architecture (Ia32, x64) to cover the
cases where someone builds the OVMF for 32bit or IA32 and X64.

thanks

> Thanks!
> Min

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Min Xu]

On September 25, 2021 11:21 AM, Brijesh Singh wrote:
> Hi Min,
> 
> 
> On 9/24/21 7:03 PM, Xu, Min M wrote:
> > On September 24, 2021 6:58 PM, Brijesh Singh wrote:
> >> Hi Min,
> >>
> >> On 9/21/21 4:05 AM, Min Xu wrote:
> >>>  ;
> >>>  ; Modified:  EAX, EBX, ECX, EDX
> >>>  ;
> >>>  SetCr3ForPageTables64:
> >>> -
> >>> -    ; Clear the WorkArea header. The SEV probe routines will populate the
> >>> -    ; work area when detected.
> >>> -    mov     byte[WORK_AREA_GUEST_TYPE], 0
> >> Why you are removing the above block ? The workarea hdr must be
> >> initialized to zero, its not safe to assume that the guest memory is
> >> zero'ed in the non- encrypted case.
> >>
> > Hi, Brijesh
> > Please see below explanation (It is in the commit message)
> > - In Main16 entry point, after TransitionFromReal16To32BitFlat,
> >    WORK_AREA_GUEST_TYPE is cleared to 0. WORK_AREA_GUEST_TYPE was
> >    previously cleared in SetCr3ForPageTables64 (see commit ab77b60).
> >    This doesn't work after TDX is introduced in Ovmf. It is because all
> >    TDX CPUs (BSP and APs) start to run from 0xfffffff0. In previous code
> >    WORK_AREA_GUEST_TYPE will be cleared multi-times in TDX guest. So for
> >    SEV and Legacy guest it is moved to Main16 entry point (after
> >    TransitionFromReal16To32BitFlat). For TDX guest
> WORK_AREA_GUEST_TYPE
> >    is cleared and set in InitTdxWorkarea
> 
> thanks for clarifying it.
> 
> This is very busy commit and making several changes at once, so some of
> important common code movement is getting lost. Maybe I recommend you to
> please break it into multiple. e,g  this particular change can be very easily broken
> into two commits
> 
> 1) Since TDX support need the change in the boot flow, and you are no longer
> using the Main.asm from the UefiCpuPkg. This can be a pre-patch in which you
> copy UefiCpuPkg/ResetVector/Vtf0/main.asm ->
> OvmfPkg/ResetVector/Main.asm and document reason behind the move.
> 
> 2) Remove clearing of workarea from SetCr3ForPageTables64 to Main.asm
> 
> Now that we have override for the Main.asm, I think clearing of the workarea
> should be done for all architecture (Ia32, x64) to cover the cases where
> someone builds the OVMF for 32bit or IA32 and X64.
> 
Thanks for reminder. I have updated the patch-set as you mentioned. But I am waiting for a conclusion of the Metadata, a unified metadata or two separate metadata.

Hoffmann and Jiewen
Do we have a conclusion?

Thanks
Min

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Yao, Jiewen]

I recommend we start with 2 metadata and check in.
It is working today for both SEV and TDX, with KVM and Cloud Hypervisor support.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Sunday, September 26, 2021 7:17 AM
> To: devel@edk2.groups.io; brijesh.singh@amd.com
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Gerd Hoffmann <kraxel@redhat.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: RE: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
> On September 25, 2021 11:21 AM, Brijesh Singh wrote:
> > Hi Min,
> >
> >
> > On 9/24/21 7:03 PM, Xu, Min M wrote:
> > > On September 24, 2021 6:58 PM, Brijesh Singh wrote:
> > >> Hi Min,
> > >>
> > >> On 9/21/21 4:05 AM, Min Xu wrote:
> > >>>  ;
> > >>>  ; Modified:  EAX, EBX, ECX, EDX
> > >>>  ;
> > >>>  SetCr3ForPageTables64:
> > >>> -
> > >>> -    ; Clear the WorkArea header. The SEV probe routines will populate the
> > >>> -    ; work area when detected.
> > >>> -    mov     byte[WORK_AREA_GUEST_TYPE], 0
> > >> Why you are removing the above block ? The workarea hdr must be
> > >> initialized to zero, its not safe to assume that the guest memory is
> > >> zero'ed in the non- encrypted case.
> > >>
> > > Hi, Brijesh
> > > Please see below explanation (It is in the commit message)
> > > - In Main16 entry point, after TransitionFromReal16To32BitFlat,
> > >    WORK_AREA_GUEST_TYPE is cleared to 0. WORK_AREA_GUEST_TYPE was
> > >    previously cleared in SetCr3ForPageTables64 (see commit ab77b60).
> > >    This doesn't work after TDX is introduced in Ovmf. It is because all
> > >    TDX CPUs (BSP and APs) start to run from 0xfffffff0. In previous code
> > >    WORK_AREA_GUEST_TYPE will be cleared multi-times in TDX guest. So for
> > >    SEV and Legacy guest it is moved to Main16 entry point (after
> > >    TransitionFromReal16To32BitFlat). For TDX guest
> > WORK_AREA_GUEST_TYPE
> > >    is cleared and set in InitTdxWorkarea
> >
> > thanks for clarifying it.
> >
> > This is very busy commit and making several changes at once, so some of
> > important common code movement is getting lost. Maybe I recommend you
> to
> > please break it into multiple. e,g  this particular change can be very easily
> broken
> > into two commits
> >
> > 1) Since TDX support need the change in the boot flow, and you are no longer
> > using the Main.asm from the UefiCpuPkg. This can be a pre-patch in which you
> > copy UefiCpuPkg/ResetVector/Vtf0/main.asm ->
> > OvmfPkg/ResetVector/Main.asm and document reason behind the move.
> >
> > 2) Remove clearing of workarea from SetCr3ForPageTables64 to Main.asm
> >
> > Now that we have override for the Main.asm, I think clearing of the workarea
> > should be done for all architecture (Ia32, x64) to cover the cases where
> > someone builds the OVMF for 32bit or IA32 and X64.
> >
> Thanks for reminder. I have updated the patch-set as you mentioned. But I am
> waiting for a conclusion of the Metadata, a unified metadata or two separate
> metadata.
> 
> Hoffmann and Jiewen
> Do we have a conclusion?
> 
> Thanks
> Min

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Gerd Hoffmann]

  Hi,

> > Possible alternative approach: Define an extension
> > (EFI_FIRMWARE_VOLUME_EXT_HEADER) for the load address and use that
> > instead of defining something new.
> 
> [Jiewen] I would say it is terrible idea to use EFI_FIRMWARE_VOLUME_HEADER.
> 
> [ ... details snipped ... ]

Ok, lets scratch the idea then.

> > Still not clear why the size is in there twice.  I think you could
> > instead use a flag telling whenever a section must be loaded from
> > the image or not.  This is what COFF and ELF are doing too.
> > 
> > Also not clear why you want stick to 64bit base address.  Loading the
> > firmware above 4G isn't going to work without also changing a bunch of
> > other things and it will break backward compatibility anyway.
> 
> [Jiewen] That is our previously experience, when we define a physical address, we always use 64bit to leave room for extension.
> Like UEFI specification defines PHYSICAL_ADDRESS to be UINT64 even in IA32 platform.

Sure, physical address space on IA32 is 64G which simply doesn't fit into UINT32 ...

> Defining UINT64 gives us enough flexibility for the future, including test in above 4G environment.
> I am wondering that, do you really care to save 4 bytes from UINT64 to UINT32 ?

Well, MEMFD isn't that big, so we have to care about the size there ...

> For type, maybe 2^16 is enough. But for flags, I prefer 32bit.

Making one 16bit and the other 32bit doesn't make much sense due
to struct padding and alignment.  So with 64-bit load address and
fields reordered so we don't have any padding the struct could look
like this:

struct {
    uint64_t load_address;
    uint32_t file_offset;
    uint32_t section_size;
    uint32_t section_type;
    uint32_t section_flags;
};

> > But, again, I don't want have two completely different initialization
> > code paths in OVMF.  We can certainly investigate and discuss dropping
> > PEI, but that clearly shouldn't be a TDX-only thing.  When we eliminate
> > PEI, we should do it for all OVMF builds.
> 
> [Jiewen] I think this is out of scope of TDVF config-B patch series.
> I don't think it is fair to enable OVMF to remove PEI, just because
> TDVF does not need PEI. 

Hmm?  TDVF is based on OVMF.  My expectation would be that TDVF is
basically OVMF with TDX support added and most code being identical.
So with TDVF being able to boot without PEI most of the work should
already be done ...

> Each platform owner can have their own choice.

Why do you consider TDVF a separate platform?  I see it as OVMF variant.

Or did you just fork OVMF for config-b?  Doing so is certainly easier
for initial development and prototyping, but it's bad in the long run.
Having similar code twice in the code base means more long-term
maintenance work, which isn't fair either.

> If you have intertest to remove PEI, I am happy to discuss with you on
> detail.  However, I would treat "removing PEI from OVMF" and "enable
> TDVF config-B" be two different tasks.

Given TDVF wants boot without PEI the later depends on the former
though.  Or TDVF config-B continues to use the PEI-based boot workflow
for now like config-A does, and we look into removing PEI from OVMF
later.

take care,
  Gerd

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Gerd Hoffmann]

  Hi,

> Thanks for reminder. I have updated the patch-set as you mentioned.
> But I am waiting for a conclusion of the Metadata, a unified metadata
> or two separate metadata.

I still don't see the point in having two different tables for the same
purpose.

take care,
  Gerd

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Yao, Jiewen]

Comment below:

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Monday, September 27, 2021 4:05 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: devel@edk2.groups.io; Xu, Min M <min.m.xu@intel.com>; Brijesh Singh
> <brijesh.singh@amd.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Justen, Jordan L <jordan.l.justen@intel.com>; Erdem Aktas
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> Lendacky <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
>   Hi,
> 
> > > Possible alternative approach: Define an extension
> > > (EFI_FIRMWARE_VOLUME_EXT_HEADER) for the load address and use that
> > > instead of defining something new.
> >
> > [Jiewen] I would say it is terrible idea to use
> EFI_FIRMWARE_VOLUME_HEADER.
> >
> > [ ... details snipped ... ]
> 
> Ok, lets scratch the idea then.
> 
> > > Still not clear why the size is in there twice.  I think you could
> > > instead use a flag telling whenever a section must be loaded from
> > > the image or not.  This is what COFF and ELF are doing too.
> > >
> > > Also not clear why you want stick to 64bit base address.  Loading the
> > > firmware above 4G isn't going to work without also changing a bunch of
> > > other things and it will break backward compatibility anyway.
> >
> > [Jiewen] That is our previously experience, when we define a physical address,
> we always use 64bit to leave room for extension.
> > Like UEFI specification defines PHYSICAL_ADDRESS to be UINT64 even in IA32
> platform.
> 
> Sure, physical address space on IA32 is 64G which simply doesn't fit into
> UINT32 ...
> 
> > Defining UINT64 gives us enough flexibility for the future, including test in
> above 4G environment.
> > I am wondering that, do you really care to save 4 bytes from UINT64 to
> UINT32 ?
> 
> Well, MEMFD isn't that big, so we have to care about the size there ...
> 
> > For type, maybe 2^16 is enough. But for flags, I prefer 32bit.
> 
> Making one 16bit and the other 32bit doesn't make much sense due
> to struct padding and alignment.  So with 64-bit load address and
> fields reordered so we don't have any padding the struct could look
> like this:
> 
> struct {
>     uint64_t load_address;
>     uint32_t file_offset;
>     uint32_t section_size;
>     uint32_t section_type;
>     uint32_t section_flags;
> };

[Jiewen] This data structure does not work in a special use case - A TD may want to have a fixed memory size.

It is TD that tells the VMM how many DRAM should be allocated by using metadata table. Not the case that a VMM tells the TD how many DRAM is allocated by using HOB.

In that special case, the TD_HOB is NOT required. The VMM parses the metadata to allocate the DRAM (AUG page).

The runtime section size must be UINT64, otherwise we cannot support > 4G memory section.
The build time section size can be UINT32. We don't expect to create a >4G binary in near future.
And we need both.

I can understand why you think there is no needed fields, based upon what you see in EDKII/TDVF project.
However, the usage in current TDVF is just a subset, but not all usages.

The TDX metadata structure is carefully designed to support those variants. Also, leaving some room for future is a common practice.
Besides EDKII/TDVF, we are doing other TDX related projects reusing the same metadata structure. (But sorry, I cannot tell more at this time.)
The benefit is that the KVM or cloud hypervisor can have a common logic to handle "TDX boot", instead of using different table in different use cases.

Defining something only feasible in EDKII/OVMF will bring a burden. That is NOT a way we want to go.

Let me say in this way:
I think it is OK, if SEV wants to reuse the existing TDX metadata table. (We need SEV people agree.) Then we can have one metadata table. 
But we cannot reuse the SEV defined metadata table in https://github.com/AMDESE/ovmf/blob/snp-v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm, because it missed some fields we need. If that is the case, then we have to use 2 metadata tables.




> 
> > > But, again, I don't want have two completely different initialization
> > > code paths in OVMF.  We can certainly investigate and discuss dropping
> > > PEI, but that clearly shouldn't be a TDX-only thing.  When we eliminate
> > > PEI, we should do it for all OVMF builds.
> >
> > [Jiewen] I think this is out of scope of TDVF config-B patch series.
> > I don't think it is fair to enable OVMF to remove PEI, just because
> > TDVF does not need PEI.
> 
> Hmm?  TDVF is based on OVMF.  My expectation would be that TDVF is
> basically OVMF with TDX support added and most code being identical.
> So with TDVF being able to boot without PEI most of the work should
> already be done ...
> 
> > Each platform owner can have their own choice.
> 
> Why do you consider TDVF a separate platform?  I see it as OVMF variant.
> 
> Or did you just fork OVMF for config-b?  Doing so is certainly easier
> for initial development and prototyping, but it's bad in the long run.
> Having similar code twice in the code base means more long-term
> maintenance work, which isn't fair either.
> 
> > If you have intertest to remove PEI, I am happy to discuss with you on
> > detail.  However, I would treat "removing PEI from OVMF" and "enable
> > TDVF config-B" be two different tasks.
> 
> Given TDVF wants boot without PEI the later depends on the former
> though.  Or TDVF config-B continues to use the PEI-based boot workflow
> for now like config-A does, and we look into removing PEI from OVMF
> later.

[Jiewen] We don't fork OVMF in config-B.
Instead of we will create new Tdvf/Tdvf.dsc and Tdvf/Tdvf.fdf in config-B, similar to https://github.com/tianocore/edk2/tree/master/OvmfPkg/AmdSev or https://github.com/tianocore/edk2/tree/master/OvmfPkg/Bhyve
That is why I treat it as different platform.

Non-PEI is not a new concept. It is quite mature known configuration.
I reluctant to merge it back to Ovmf.dsc/fdf. The reason is the main Ovmf supports some features (such as S3, TPM) which may depend on PEI modules, but it is NOT needed in TDVF.
We need re-evaluate the effort to enable those features in non-PEI configuration in OVMF. - That is totally unnecessary in TDVF enabling task.
That is why I treat them as separate task. And something the OVMF community may look into in the future.




> 
> take care,
>   Gerd
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Gerd Hoffmann]

  Hi,

> > struct {
> >     uint64_t load_address;
> >     uint32_t file_offset;
> >     uint32_t section_size;
> >     uint32_t section_type;
> >     uint32_t section_flags;
> > };
> 
> [Jiewen] This data structure does not work in a special use case - A
> TD may want to have a fixed memory size.  It is TD that tells the VMM
> how many DRAM should be allocated by using metadata table. Not the
> case that a VMM tells the TD how many DRAM is allocated by using HOB.
> 
> In that special case, the TD_HOB is NOT required. The VMM parses the
> metadata to allocate the DRAM (AUG page).

Hmm.  Not covered in tdx-virtual-firmware-design-guide-rev-1.pdf

> The runtime section size must be UINT64, otherwise we cannot support > 4G memory section.
> The build time section size can be UINT32. We don't expect to create a >4G binary in near future.
> And we need both.

That still doesn't explain why you need two sizes.  Instead of depending
on zero-fill in case MemoryDataSize > RawDataSize you can just use two
entries, simliar to ELF binaries which have separate '.data' and '.bss'
sections too.

> I can understand why you think there is no needed fields, based upon
> what you see in EDKII/TDVF project.  However, the usage in current
> TDVF is just a subset, but not all usages.

So you are doing stuff behind closed doors ...

> The TDX metadata structure is carefully designed to support those
> variants. Also, leaving some room for future is a common practice.
> Besides EDKII/TDVF, we are doing other TDX related projects reusing
> the same metadata structure. (But sorry, I cannot tell more at this
> time.)

... and don't want tell details.  Even the fact that you are doing that
is disclosed only after poking for a while because the patches submitted
leave a bunch of questions open.

This is NOT how Open Source Development works.

If the patches can't speak for themself in cases like this the very
minimum requirement is proper documentation.  It is not acceptable
that I have to ask five times to figure that the format is supposed
to cover use cases beyond TDVF.

> The benefit is that the KVM or cloud hypervisor can have a common
> logic to handle "TDX boot", instead of using different table in
> different use cases.

The benefit of a unified table for tdx and sev is that the VMM can
have common logic to find page ranges which need special
initialization.

But I suspect at that point we are going to trade code sharing at one
place for code sharing at another place.

> I think it is OK, if SEV wants to reuse the existing TDX metadata
> table. (We need SEV people agree.) Then we can have one metadata
> table. 

So, when submitting the next revision of this series, please ...

  (1) Move the tdx metadata changes to a separate patch.
  (2) Add *complete* documentation for the TDX metadata format

... so the SEV people can make up their mind whenever they want use
that or not.

Please do also clarify what the process to allocate section type numbers
(or reserve a number range) for SEV would be.

> [Jiewen] We don't fork OVMF in config-B.
>
> Instead of we will create new Tdvf/Tdvf.dsc and Tdvf/Tdvf.fdf in
> config-B, similar to
> https://github.com/tianocore/edk2/tree/master/OvmfPkg/AmdSev or
> https://github.com/tianocore/edk2/tree/master/OvmfPkg/Bhyve That is
> why I treat it as different platform.

The differences between Ovmf and AmdSev are very small.

Bhyve has more differences, but it's a different hypervisor
so it isn't surprising it has its own PlatformPei.

How does Tdvf handle the platform setup?  It must be done in SEC
somehow, so I suspect you have a (possibly stripped-down) version
of the PlatformPei adapted for SEC?  That is exactly the kind of
code duplication I want avoid.

> I reluctant to merge it back to Ovmf.dsc/fdf.

I don't worry that much about Ovmf.dsc/fdf files.  Whenever we add a
compile-time option (-D ENABLE_TDX) to Ovmf.dsc/fdf or whenever we add a
separate Tdvf.dsc/fdf doesn't make that much of a difference.

I'm more worried about the code duplication and the completely different
(PEI-less) initialization workflow.  When touching the platform setup
code both cases (with/without PEI) must be considered, which increases
development and testing and maintenance effort long-term.

I want less variants, not more.  Ideally I'd like to also get rid of the
OvmfPkgIa32X64.dsc/fdf for example.  It seems some features have a
dependency on PEI running in 32-bit mode though.

> The reason is the main Ovmf supports some features (such as S3, TPM)
> which may depend on PEI modules, but it is NOT needed in TDVF.

So using PEI in OVMF isn't that over-engineered, isn't it?

And I suspect SMM support can be added to the list of features which
depend on the PEI phase (at least when we want reuse the existing common
code in UefiCpuPkg, MdePkg and elsewhere).

> We need re-evaluate the effort to enable those features in non-PEI
> configuration in OVMF. - That is totally unnecessary in TDVF enabling
> task.

Well, it's surely additional upfront work.  But I expect it will pay off
long-term.  Less maintenance work, less testing work, lower risk of
adding regressions due to SEC and PEI init code paths variants running
out of sync.  So "totally unnecessary" only when you ignore the work
needed after the initial merge.

take care,
  Gerd

Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Yao, Jiewen]

For size field, please refer to PE/COFF specification https://docs.microsoft.com/en-us/windows/win32/debug/pe-format

The "Section Table (Section Headers)" defines two fields:
=======================
VirtualSize - The total size of the section when loaded into memory. If this value is greater than SizeOfRawData, the section is zero-padded. This field is valid only for executable images and should be set to zero for object files.

SizeOfRawData - The size of the section (for object files) or the size of the initialized data on disk (for image files). For executable images, this must be a multiple of FileAlignment from the optional header. If this is less than VirtualSize, the remainder of the section is zero-filled. Because the SizeOfRawData field is rounded but the VirtualSize field is not, it is possible for SizeOfRawData to be greater than VirtualSize as well. When a section contains only uninitialized data, this field should be zero.
=======================

We took similar concept here.

RawDataSize == size in the file.
MemoryDataSize == size in the memory. They are totally different concept.

For example, you can have 0xC81 RawDataSize, but the MemoryDataSize is 0x1000.

If one project enforces RawDataSize == MemoryDataSize, then only one size is needed.
But if one project wants to RawDataSize <= MemoryDataSize, then we need two size fields.



Thank you
Yao Jiewen

> -----Original Message-----
> From: kraxel@redhat.com <kraxel@redhat.com>
> Sent: Monday, September 27, 2021 10:59 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: devel@edk2.groups.io; Xu, Min M <min.m.xu@intel.com>; Brijesh Singh
> <brijesh.singh@amd.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Justen, Jordan L <jordan.l.justen@intel.com>; Erdem Aktas
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> Lendacky <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
>   Hi,
> 
> > > struct {
> > >     uint64_t load_address;
> > >     uint32_t file_offset;
> > >     uint32_t section_size;
> > >     uint32_t section_type;
> > >     uint32_t section_flags;
> > > };
> >
> > [Jiewen] This data structure does not work in a special use case - A
> > TD may want to have a fixed memory size.  It is TD that tells the VMM
> > how many DRAM should be allocated by using metadata table. Not the
> > case that a VMM tells the TD how many DRAM is allocated by using HOB.
> >
> > In that special case, the TD_HOB is NOT required. The VMM parses the
> > metadata to allocate the DRAM (AUG page).
> 
> Hmm.  Not covered in tdx-virtual-firmware-design-guide-rev-1.pdf
> 
> > The runtime section size must be UINT64, otherwise we cannot support > 4G
> memory section.
> > The build time section size can be UINT32. We don't expect to create a >4G
> binary in near future.
> > And we need both.
> 
> That still doesn't explain why you need two sizes.  Instead of depending
> on zero-fill in case MemoryDataSize > RawDataSize you can just use two
> entries, simliar to ELF binaries which have separate '.data' and '.bss'
> sections too.
> 
> > I can understand why you think there is no needed fields, based upon
> > what you see in EDKII/TDVF project.  However, the usage in current
> > TDVF is just a subset, but not all usages.
> 
> So you are doing stuff behind closed doors ...
> > The TDX metadata structure is carefully designed to support those
> > variants. Also, leaving some room for future is a common practice.
> > Besides EDKII/TDVF, we are doing other TDX related projects reusing
> > the same metadata structure. (But sorry, I cannot tell more at this
> > time.)
> 
> ... and don't want tell details.  Even the fact that you are doing that
> is disclosed only after poking for a while because the patches submitted
> leave a bunch of questions open.
> 
> This is NOT how Open Source Development works.
> 
> If the patches can't speak for themself in cases like this the very
> minimum requirement is proper documentation.  It is not acceptable
> that I have to ask five times to figure that the format is supposed
> to cover use cases beyond TDVF.
> 
> > The benefit is that the KVM or cloud hypervisor can have a common
> > logic to handle "TDX boot", instead of using different table in
> > different use cases.
> 
> The benefit of a unified table for tdx and sev is that the VMM can
> have common logic to find page ranges which need special
> initialization.
> 
> But I suspect at that point we are going to trade code sharing at one
> place for code sharing at another place.
> 
> > I think it is OK, if SEV wants to reuse the existing TDX metadata
> > table. (We need SEV people agree.) Then we can have one metadata
> > table.
> 
> So, when submitting the next revision of this series, please ...
> 
>   (1) Move the tdx metadata changes to a separate patch.
>   (2) Add *complete* documentation for the TDX metadata format
> 
> ... so the SEV people can make up their mind whenever they want use
> that or not.
> 
> Please do also clarify what the process to allocate section type numbers
> (or reserve a number range) for SEV would be.
> 
> > [Jiewen] We don't fork OVMF in config-B.
> >
> > Instead of we will create new Tdvf/Tdvf.dsc and Tdvf/Tdvf.fdf in
> > config-B, similar to
> > https://github.com/tianocore/edk2/tree/master/OvmfPkg/AmdSev or
> > https://github.com/tianocore/edk2/tree/master/OvmfPkg/Bhyve That is
> > why I treat it as different platform.
> 
> The differences between Ovmf and AmdSev are very small.
> 
> Bhyve has more differences, but it's a different hypervisor
> so it isn't surprising it has its own PlatformPei.
> 
> How does Tdvf handle the platform setup?  It must be done in SEC
> somehow, so I suspect you have a (possibly stripped-down) version
> of the PlatformPei adapted for SEC?  That is exactly the kind of
> code duplication I want avoid.
> 
> > I reluctant to merge it back to Ovmf.dsc/fdf.
> 
> I don't worry that much about Ovmf.dsc/fdf files.  Whenever we add a
> compile-time option (-D ENABLE_TDX) to Ovmf.dsc/fdf or whenever we add a
> separate Tdvf.dsc/fdf doesn't make that much of a difference.
> 
> I'm more worried about the code duplication and the completely different
> (PEI-less) initialization workflow.  When touching the platform setup
> code both cases (with/without PEI) must be considered, which increases
> development and testing and maintenance effort long-term.
> 
> I want less variants, not more.  Ideally I'd like to also get rid of the
> OvmfPkgIa32X64.dsc/fdf for example.  It seems some features have a
> dependency on PEI running in 32-bit mode though.
> 
> > The reason is the main Ovmf supports some features (such as S3, TPM)
> > which may depend on PEI modules, but it is NOT needed in TDVF.
> 
> So using PEI in OVMF isn't that over-engineered, isn't it?
> 
> And I suspect SMM support can be added to the list of features which
> depend on the PEI phase (at least when we want reuse the existing common
> code in UefiCpuPkg, MdePkg and elsewhere).
> 
> > We need re-evaluate the effort to enable those features in non-PEI
> > configuration in OVMF. - That is totally unnecessary in TDVF enabling
> > task.
> 
> Well, it's surely additional upfront work.  But I expect it will pay off
> long-term.  Less maintenance work, less testing work, lower risk of
> adding regressions due to SEC and PEI init code paths variants running
> out of sync.  So "totally unnecessary" only when you ignore the work
> needed after the initial merge.
> 
> take care,
>   Gerd