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

[PATCH V6 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.v6

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/Include/WorkArea.h                   |   3 +-
 OvmfPkg/OvmfPkg.dec                          |  12 +
 OvmfPkg/OvmfPkgDefines.fdf.inc               |  10 +
 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm |  39 +++
 OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm  |  11 +
 OvmfPkg/ResetVector/Ia32/IntelTdx.asm        | 236 +++++++++++++++++++
 OvmfPkg/ResetVector/Ia32/PageTables64.asm    |  21 +-
 OvmfPkg/ResetVector/Main.asm                 | 119 ++++++++++
 OvmfPkg/ResetVector/ResetVector.inf          |  10 +
 OvmfPkg/ResetVector/ResetVector.nasmb        |  42 +++-
 OvmfPkg/ResetVector/X64/IntelTdxMetadata.asm | 102 ++++++++
 11 files changed, 597 insertions(+), 8 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 V6 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. PcdOvmfImageSizeInKb
PcdOvmfImageSizeInKb indicates the size of Ovmf image. It is used to
calculate the offset of TdxMetadata in ResetVectorVtf0.asm.

3. TDX_WORK_AREA
Add Intel TDX definition in the OVMF_WORK_AREA. A new field (SubType)
is added in CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER to record the
sub type of the vm guest.

4. 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

5. 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[WORK_AREA_GUEST_TYPE] is set by BSP to indicate APs can leave
   spin and go.

6. 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.

7. 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.

8. 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.

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

10. OvmfPkg/Include/WorkArea.h
Add a field (SubType) in CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER.

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/Include/WorkArea.h                   |   3 +-
 OvmfPkg/OvmfPkg.dec                          |  12 +
 OvmfPkg/OvmfPkgDefines.fdf.inc               |  10 +
 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm |  39 +++
 OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm  |  11 +
 OvmfPkg/ResetVector/Ia32/IntelTdx.asm        | 236 +++++++++++++++++++
 OvmfPkg/ResetVector/Ia32/PageTables64.asm    |  21 +-
 OvmfPkg/ResetVector/Main.asm                 | 119 ++++++++++
 OvmfPkg/ResetVector/ResetVector.inf          |  10 +
 OvmfPkg/ResetVector/ResetVector.nasmb        |  42 +++-
 OvmfPkg/ResetVector/X64/IntelTdxMetadata.asm | 102 ++++++++
 11 files changed, 597 insertions(+), 8 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/Include/WorkArea.h b/OvmfPkg/Include/WorkArea.h
index c16030e3ac0a..abb804d14a0c 100644
--- a/OvmfPkg/Include/WorkArea.h
+++ b/OvmfPkg/Include/WorkArea.h
@@ -27,7 +27,8 @@ typedef enum {
 //
 typedef struct _CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER {
   UINT8                   GuestType;
-  UINT8                   Reserved1[3];
+  UINT8                   SubType;
+  UINT8                   Reserved1[2];
 } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;
 
 //
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index c37dafad49bb..5216700754db 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -340,6 +340,18 @@
   # 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
+
+  ## Size of the Ovmf image in KB
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfImageSizeInKb|0|UINT32|0x58
 
 [PcdsDynamic, PcdsDynamicEx]
   gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
diff --git a/OvmfPkg/OvmfPkgDefines.fdf.inc b/OvmfPkg/OvmfPkgDefines.fdf.inc
index 3b5e45253916..bde986b6ad4a 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
@@ -66,6 +67,7 @@ DEFINE SECFV_OFFSET      = 0x003CC000
 DEFINE SECFV_SIZE        = 0x34000
 !endif
 
+SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfImageSizeInKb     = $(FD_SIZE_IN_KB)
 SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress     = $(FW_BASE_ADDRESS)
 SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize    = $(FW_SIZE)
 SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize = $(BLOCK_SIZE)
@@ -88,6 +90,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..c3b856754008 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      (OVMF_IMAGE_SIZE_IN_KB * 1024 - (fourGigabytes - 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
+
+    smsw    ax
+    test    al, 1
+    jz      .Real
+BITS 32
+    jmp     Main32
+BITS 16
+.Real:
+    jmp     EarlyBspInitReal16
+
+%endif
+
 ALIGN   16
 
 fourGigabytes:
diff --git a/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm b/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
index c6d0d898bcd1..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..8e84dde24af7
--- /dev/null
+++ b/OvmfPkg/ResetVector/Ia32/IntelTdx.asm
@@ -0,0 +1,236 @@
+;------------------------------------------------------------------------------
+; @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/Subtype of WORK_AREA_GUEST_TYPE so that the following code can use
+    ; these information.
+    ;
+    mov     byte[WORK_AREA_GUEST_TYPE], VM_GUEST_TDX
+    mov     byte[WORK_AREA_GUEST_SUBTYPE], 0
+
+    ;
+    ; 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..d49c7ca37ec9 100644
--- a/OvmfPkg/ResetVector/ResetVector.inf
+++ b/OvmfPkg/ResetVector/ResetVector.inf
@@ -44,6 +44,16 @@
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfImageSizeInKb
+  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..b1141347540b 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -67,19 +67,54 @@
     %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))
   %define WORK_AREA_GUEST_TYPE (FixedPcdGet32 (PcdOvmfWorkAreaBase))
+  %define WORK_AREA_GUEST_SUBTYPE (FixedPcdGet32 (PcdOvmfWorkAreaBase) + 1)
   %define SEV_ES_WORK_AREA (FixedPcdGet32 (PcdSevEsWorkAreaBase))
   %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"
@@ -92,5 +127,6 @@
   %define SEV_LAUNCH_SECRET_SIZE  FixedPcdGet32 (PcdSevLaunchSecretSize)
   %define SEV_FW_HASH_BLOCK_BASE  FixedPcdGet32 (PcdQemuHashTableBase)
   %define SEV_FW_HASH_BLOCK_SIZE  FixedPcdGet32 (PcdQemuHashTableSize)
+  %define OVMF_IMAGE_SIZE_IN_KB   FixedPcdGet32 (PcdOvmfImageSizeInKb)
 %include "Ia16/ResetVectorVtf0.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 V6 1/1] OvmfPkg: Enable TDX in ResetVector

[Brijesh Singh]

Hi Min,

A quick question below.

On 9/14/21 3:50 AM, Min Xu wrote:
> RFC： https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3429&data=04%7C01%7Cbrijesh.singh%40amd.com%7C2cca2f0a7fb44084da2b08d9775cb220%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637672062275443867%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=4zfuIDvTGDNCt%2BD3u7uUR0n6hHDzv%2FI8NkqoUJhsx8Y%3D&reserved=0
>
> 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. PcdOvmfImageSizeInKb
> PcdOvmfImageSizeInKb indicates the size of Ovmf image. It is used to
> calculate the offset of TdxMetadata in ResetVectorVtf0.asm.

In SEV-SNP v7 series, I implemented the metadata support. I did not see
a need for the PcdOvmfImageSizeInKB. Why do you need it? I think your
calculation below will not work if someone is using the OVMF_CODE.fd
instead of OVMF.fd. Have you tried booting with OVMF_CODE.fd ?

thanks

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

[vannapurve]

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

Hi Min, Brijesh,

Regarding:
> diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> ...
> +%ifdef ARCH_IA32
>     nop
>     nop
>     jmp     EarlyBspInitReal16
>
>+%else
>+
>+    smsw    ax

We are having intermittent VM crashes with running this code in AMD-SEV
enabled VMs. As per the AMD64 manual
<https://www.amd.com/system/files/TechDocs/24593.pdf> section 15.8.1,
executing "smsw" instruction doesn't result in bit 63 being set in
EXITINFO1 and KVM ends up emulating "smsw" instruction by trying to read
encrypted guest VM memory as per the code
<https://git.kernel.org/pub/scm/virt/kvm/kvm.git/tree/arch/x86/kvm/svm/svm.c#n2495>.
Since KVM tries to make sense of different random cipher texts in different
boots, it seems to intermittently result in visible issues.

Is this expected behavior or do we miss some configuration or patches that
are recommended by AMD?

Regards,
Vishal

On Tue, Sep 14, 2021 at 4:54 PM Brijesh Singh via groups.io <brijesh.singh=
amd.com@groups.io> wrote:

> Hi Min,
>
> A quick question below.
>
> On 9/14/21 3:50 AM, Min Xu wrote:
> > RFC：
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3429&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C2cca2f0a7fb44084da2b08d9775cb220%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637672062275443867%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=4zfuIDvTGDNCt%2BD3u7uUR0n6hHDzv%2FI8NkqoUJhsx8Y%3D&amp;reserved=0
> >
> > 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. PcdOvmfImageSizeInKb
> > PcdOvmfImageSizeInKb indicates the size of Ovmf image. It is used to
> > calculate the offset of TdxMetadata in ResetVectorVtf0.asm.
>
> In SEV-SNP v7 series, I implemented the metadata support. I did not see
> a need for the PcdOvmfImageSizeInKB. Why do you need it? I think your
> calculation below will not work if someone is using the OVMF_CODE.fd
> instead of OVMF.fd. Have you tried booting with OVMF_CODE.fd ?
>
> thanks
>
>
>
>
> 
>
>
>

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

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

[Brijesh Singh]

Hi Vishal,

On 9/14/21 2:00 PM, Vishal Annapurve wrote:
> Hi Min, Brijesh,
> 
> Regarding:
>> diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>> ...
>> +%ifdef ARCH_IA32
>>     nop
>>     nop
>>     jmp     EarlyBspInitReal16
>>
>>+%else
>>+
>>+    smsw    ax
> 
> We are having intermittent VM crashes with running this code in AMD-SEV 
> enabled VMs. As per the AMD64 manual 
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F24593.pdf&data=04%7C01%7Cbrijesh.singh%40amd.com%7C652023e953924957972a08d977b2031a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637672430875783281%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=VFiIbcV6H4xx5XZd%2F0OZjerSfJwLfUjK7mPU9JHY05E%3D&reserved=0> section 
> 15.8.1, executing "smsw" instruction doesn't result in bit 63 being set 
> in EXITINFO1 and KVM ends up emulating "smsw" instruction by trying to 
> read encrypted guest VM memory as per the code 
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Fvirt%2Fkvm%2Fkvm.git%2Ftree%2Farch%2Fx86%2Fkvm%2Fsvm%2Fsvm.c%23n2495&data=04%7C01%7Cbrijesh.singh%40amd.com%7C652023e953924957972a08d977b2031a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637672430875783281%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=jSw7PLfXjhB8utM7Dxx2P%2F5M3fqvO3q3DBaFW%2Bu03A8%3D&reserved=0>. 
> Since KVM tries to make sense of different random cipher texts in 
> different boots, it seems to intermittently result in visible issues.
> 

The smsw does not provide decode assist, in those cases KVM reads the 
guest memory and tries to decode. With encrypted guest, the memory 
contains the ciphertext and hypervisor will not be able to decode the 
instruction.

But it brings a question to Min, why we are using the smsw ? why cannot 
use mov CRx. The smsw was meant for very old processors (286 or 8086 
etc) and is used for legacy compatibility. The recommendation is to use 
the mov CRx. The mov CRx will provide the decode assist to HV.

I looked at the Intel architecture manual [1] and it also recommends 
using the mov CRx. The text from the Intel doc.

   SMSW is only useful in operating-system software. However,
   it is not a privileged instruction and can be used in
   application programs if CR4.UMIP = 0. It is provided for
   compatibility with the Intel 286 processor. Programs and
   procedures intended to run on IA-32 and Intel 64 processors
   beginning with the Intel386 processors should use the
   MOV CR instruction to load the machine status word.


[1] 
https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-instruction-set-reference-manual-325383.pdf


> Is this expected behavior or do we miss some configuration or patches 
> that are recommended by AMD?
> 

This is expected because the smsw does not provide a decode assist, and 
encrypted guest will have issues with it. Lets understand the reason 
behind using the smsw.

> Regards,
> Vishal
> 
> On Tue, Sep 14, 2021 at 4:54 PM Brijesh Singh via groups.io 
> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgroups.io%2F&data=04%7C01%7Cbrijesh.singh%40amd.com%7C652023e953924957972a08d977b2031a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637672430875793279%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=%2Br4xRTlrqoERthTLJ2YNQAPrKzYX6fid2Q9WNyKybrw%3D&reserved=0> 
> <brijesh.singh=amd.com@groups.io <mailto:amd.com@groups.io>> wrote:
> 
>     Hi Min,
> 
>     A quick question below.
> 
>     On 9/14/21 3:50 AM, Min Xu wrote:
>      > RFC：
>     https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3429&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C2cca2f0a7fb44084da2b08d9775cb220%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637672062275443867%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=4zfuIDvTGDNCt%2BD3u7uUR0n6hHDzv%2FI8NkqoUJhsx8Y%3D&amp;reserved=0
>     <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3429&data=04%7C01%7Cbrijesh.singh%40amd.com%7C652023e953924957972a08d977b2031a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637672430875793279%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=E5bfIvEeyWTgUqnmllwKgSwxycqhzfNnZ72O0i0UKww%3D&reserved=0>
>      >
>      > 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. PcdOvmfImageSizeInKb
>      > PcdOvmfImageSizeInKb indicates the size of Ovmf image. It is used to
>      > calculate the offset of TdxMetadata in ResetVectorVtf0.asm.
> 
>     In SEV-SNP v7 series, I implemented the metadata support. I did not see
>     a need for the PcdOvmfImageSizeInKB. Why do you need it? I think your
>     calculation below will not work if someone is using the OVMF_CODE.fd
>     instead of OVMF.fd. Have you tried booting with OVMF_CODE.fd ?
> 
>     thanks
> 
> 
> 
> 
>     
> 
> 

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

[Min Xu]

On September 14, 2021 7:25 PM, Brijesh Singh wrote:
> 
> Hi Min,
> 
> A quick question below.
> 
> On 9/14/21 3:50 AM, Min Xu wrote:
> > RFC：
> >
> > 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. PcdOvmfImageSizeInKb
> > PcdOvmfImageSizeInKb indicates the size of Ovmf image. It is used to
> > calculate the offset of TdxMetadata in ResetVectorVtf0.asm.
> 
> In SEV-SNP v7 series, I implemented the metadata support. I did not see a
> need for the PcdOvmfImageSizeInKB. Why do you need it? I think your
> calculation below will not work if someone is using the OVMF_CODE.fd
> instead of OVMF.fd. Have you tried booting with OVMF_CODE.fd ?
In the original PoC, TDVF is required to be launched with OVMF.fd (OVMF_CODE.fd and OVMF_VARS.fd is not supported)  because of the TDX-QEMU limitation. So PcdOvmfImageSizeInKb is used to calculate the offset of Metadata (The offset is from fourGigabytes).
But you're right. PcdOvmfImageSizeInKB is not needed. The offset should be from the TDX Metadata offset block in GUIDed chain. 
TDX-QEMU team is aware of the limitation that OVMF_CODE.fd&OVMF_VARS.fd should be supported too, otherwise the SecureBoot does not work with libvirt. They are working on this limitation.

I will remove PcdOvmfImageSizeInKB and update the Metadata offset like below:
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:

Thanks!
Min

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

[Min Xu]

Thanks Vishal and Brijesh.
Sorry I missed the limitation of smsw. MOV CRx should be used instead. I will fix it in the next version.
BTW, as Vishal mentioned in his comments in Github, we didn't have a chance to test SEV features because of the lack of AMD SEV environment. We're setting up the SEV environment now and will run a sanity test before the patch-set is sent out. But we may still need your help on the possible impact of the changes to the SEV features.
Thanks very much!

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Brijesh
> Singh via groups.io
> Sent: Wednesday, September 15, 2021 3:53 AM
> To: Vishal Annapurve <vannapurve@google.com>; devel@edk2.groups.io; Xu,
> Min M <min.m.xu@intel.com>
> Cc: brijesh.singh@amd.com; 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 V6 1/1] OvmfPkg: Enable TDX in ResetVector
> 
> Hi Vishal,
> 
> On 9/14/21 2:00 PM, Vishal Annapurve wrote:
> > Hi Min, Brijesh,
> >
> > Regarding:
> >> diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> >> b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> >> ...
> >> +%ifdef ARCH_IA32
> >>     nop
> >>     nop
> >>     jmp     EarlyBspInitReal16
> >>
> >>+%else
> >>+
> >>+    smsw    ax
> >
> > We are having intermittent VM crashes with running this code in
> > AMD-SEV enabled VMs. As per the AMD64 manual
> >
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> w
> > .amd.com%2Fsystem%2Ffiles%2FTechDocs%2F24593.pdf&data=04%7C01%
> 7Cbrijes
> >
> h.singh%40amd.com%7C652023e953924957972a08d977b2031a%7C3dd8961
> fe4884e6
> >
> 08e11a82d994e183d%7C0%7C0%7C637672430875783281%7CUnknown%7CT
> WFpbGZsb3d
> >
> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%
> 3D%7C3000&sdata=VFiIbcV6H4xx5XZd%2F0OZjerSfJwLfUjK7mPU9JHY05E%3D
> &reserved=0> section 15.8.1, executing "smsw" instruction doesn't result in
> bit 63 being set in EXITINFO1 and KVM ends up emulating "smsw" instruction
> by trying to read encrypted guest VM memory as per the code
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.k
> ernel.org%2Fpub%2Fscm%2Fvirt%2Fkvm%2Fkvm.git%2Ftree%2Farch%2Fx86
> %2Fkvm%2Fsvm%2Fsvm.c%23n2495&data=04%7C01%7Cbrijesh.singh%40am
> d.com%7C652023e953924957972a08d977b2031a%7C3dd8961fe4884e608e1
> 1a82d994e183d%7C0%7C0%7C637672430875783281%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVC
> I6Mn0%3D%7C3000&sdata=jSw7PLfXjhB8utM7Dxx2P%2F5M3fqvO3q3DBaF
> W%2Bu03A8%3D&reserved=0>.
> > Since KVM tries to make sense of different random cipher texts in
> > different boots, it seems to intermittently result in visible issues.
> >
> 
> The smsw does not provide decode assist, in those cases KVM reads the
> guest memory and tries to decode. With encrypted guest, the memory
> contains the ciphertext and hypervisor will not be able to decode the
> instruction.
> 
> But it brings a question to Min, why we are using the smsw ? why cannot
> use mov CRx. The smsw was meant for very old processors (286 or 8086
> etc) and is used for legacy compatibility. The recommendation is to use
> the mov CRx. The mov CRx will provide the decode assist to HV.
> 
> I looked at the Intel architecture manual [1] and it also recommends
> using the mov CRx. The text from the Intel doc.
> 
>    SMSW is only useful in operating-system software. However,
>    it is not a privileged instruction and can be used in
>    application programs if CR4.UMIP = 0. It is provided for
>    compatibility with the Intel 286 processor. Programs and
>    procedures intended to run on IA-32 and Intel 64 processors
>    beginning with the Intel386 processors should use the
>    MOV CR instruction to load the machine status word.
> 
> 
> [1]
> https://www.intel.com/content/dam/www/public/us/en/documents/manual
> s/64-ia-32-architectures-software-developer-instruction-set-reference-
> manual-325383.pdf
> 
> 
> > Is this expected behavior or do we miss some configuration or patches
> > that are recommended by AMD?
> >
> 
> This is expected because the smsw does not provide a decode assist, and
> encrypted guest will have issues with it. Lets understand the reason
> behind using the smsw.
> 
> > Regards,
> > Vishal
> >
> > On Tue, Sep 14, 2021 at 4:54 PM Brijesh Singh via groups.io
> >
> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgrou
> ps.io%2F&data=04%7C01%7Cbrijesh.singh%40amd.com%7C652023e9539249
> 57972a08d977b2031a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0
> %7C637672430875793279%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdat
> a=%2Br4xRTlrqoERthTLJ2YNQAPrKzYX6fid2Q9WNyKybrw%3D&reserved=0>
> > <brijesh.singh=amd.com@groups.io <mailto:amd.com@groups.io>> wrote:
> >
> >     Hi Min,
> >
> >     A quick question below.
> >
> >     On 9/14/21 3:50 AM, Min Xu wrote:
> >      > RFC：
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzi
> lla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3429&amp;data=04%7C01%7Cbr
> ijesh.singh%40amd.com%7C2cca2f0a7fb44084da2b08d9775cb220%7C3dd89
> 61fe4884e608e11a82d994e183d%7C0%7C0%7C637672062275443867%7CUn
> known%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6
> Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=4zfuIDvTGDNCt%2BD3u7u
> UR0n6hHDzv%2FI8NkqoUJhsx8Y%3D&amp;reserved=0
> >
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbug
> zilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3429&data=04%7C01%7Cbrijes
> h.singh%40amd.com%7C652023e953924957972a08d977b2031a%7C3dd8961
> fe4884e608e11a82d994e183d%7C0%7C0%7C637672430875793279%7CUnkn
> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> haWwiLCJXVCI6Mn0%3D%7C3000&sdata=E5bfIvEeyWTgUqnmllwKgSwxycqh
> zfNnZ72O0i0UKww%3D&reserved=0>
> >      >
> >      > 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. PcdOvmfImageSizeInKb
> >      > PcdOvmfImageSizeInKb indicates the size of Ovmf image. It is used to
> >      > calculate the offset of TdxMetadata in ResetVectorVtf0.asm.
> >
> >     In SEV-SNP v7 series, I implemented the metadata support. I did not see
> >     a need for the PcdOvmfImageSizeInKB. Why do you need it? I think your
> >     calculation below will not work if someone is using the OVMF_CODE.fd
> >     instead of OVMF.fd. Have you tried booting with OVMF_CODE.fd ?
> >
> >     thanks
> >
> >
> >
> >
> >
> >
> >
> 
> 
> 
> 

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

[Gerd Hoffmann]

>  typedef struct _CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER {
>    UINT8                   GuestType;
> -  UINT8                   Reserved1[3];
> +  UINT8                   SubType;
> +  UINT8                   Reserved1[2];
>  } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;

I think we should use the same approach (and the same enum)
we are planing to use for the ConfidentialComputing PCD (see
discussion in the other patch series).

take care,
  Gerd

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

[Min Xu]

On September 15, 2021 3:53 AM, Brijesh Singh wrote:
> 
> Hi Vishal,
> 
> On 9/14/21 2:00 PM, Vishal Annapurve wrote:
> > Hi Min, Brijesh,
> >
> > Regarding:
> >> diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> >> b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> >> ...
> >> +%ifdef ARCH_IA32
> >>     nop
> >>     nop
> >>     jmp     EarlyBspInitReal16
> >>
> >>+%else
> >>+
> >>+    smsw    ax
> >
> > We are having intermittent VM crashes with running this code in
> > AMD-SEV enabled VMs. As per the AMD64 manual
> >
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww
> > .amd.com%2Fsystem%2Ffiles%2FTechDocs%2F24593.pdf&data=04%7C01%
> 7Cbrijes
> >
> h.singh%40amd.com%7C652023e953924957972a08d977b2031a%7C3dd896
> 1fe4884e6
> >
> 08e11a82d994e183d%7C0%7C0%7C637672430875783281%7CUnknown%7C
> TWFpbGZsb3d
> >
> 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> D%7C3000&sdata=VFiIbcV6H4xx5XZd%2F0OZjerSfJwLfUjK7mPU9JHY05E%3D
> &reserved=0> section 15.8.1, executing "smsw" instruction doesn't result in bit
> 63 being set in EXITINFO1 and KVM ends up emulating "smsw" instruction by
> trying to read encrypted guest VM memory as per the code
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.ke
> rnel.org%2Fpub%2Fscm%2Fvirt%2Fkvm%2Fkvm.git%2Ftree%2Farch%2Fx86%
> 2Fkvm%2Fsvm%2Fsvm.c%23n2495&data=04%7C01%7Cbrijesh.singh%40amd.
> com%7C652023e953924957972a08d977b2031a%7C3dd8961fe4884e608e11
> a82d994e183d%7C0%7C0%7C637672430875783281%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> 6Mn0%3D%7C3000&sdata=jSw7PLfXjhB8utM7Dxx2P%2F5M3fqvO3q3DBaFW
> %2Bu03A8%3D&reserved=0>.
> > Since KVM tries to make sense of different random cipher texts in
> > different boots, it seems to intermittently result in visible issues.
> >
> 
> The smsw does not provide decode assist, in those cases KVM reads the
> guest memory and tries to decode. With encrypted guest, the memory
> contains the ciphertext and hypervisor will not be able to decode the
> instruction.
> 
> But it brings a question to Min, why we are using the smsw ? why cannot
> use mov CRx. The smsw was meant for very old processors (286 or 8086
> etc) and is used for legacy compatibility. The recommendation is to use
> the mov CRx. The mov CRx will provide the decode assist to HV.

As I mentioned in my last mail, in the beginning I missed the limitation of smsw. 
So I update the code (ResetVectorVtf0.asm) as below using mov CRx.
                                                          <1> BITS 16
   176 00000800 0F20C0              <1>     mov   eax, cr0    <-- previously it was smsw
   177 00000803 A801                  <1>     test    al, 1
   178 00000805 7405                  <1>     jz       .Real
   179                                               <1> BITS 32
   180 00000807 E951FFFFFF      <1>     jmp   Main32
   181                                               <1> BITS 16
   182                                               <1> .Real:
   183 0000080C E939FF              <1>     jmp   EarlyBspInitReal16

I test the code in a AMD SEV server and try to launch a SEV guest. This time it stuck at the *mov eax, cr0*. 
I am curious if *mov eax, cr0* works in real mode in a SEV guest?
I also test the code in a legacy vm guest and td guest, all passed.
Did I miss something?

I followed https://github.com/AMDESE/AMDSEV to set up the AMD SEV environment and successfully launched SEV guest with the OVMF image.

Thanks!
Min

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

[Brijesh Singh]

Hi Min,

On 9/17/21 7:55 AM, Xu, Min M wrote:
...

> 
> As I mentioned in my last mail, in the beginning I missed the limitation of smsw.
> So I update the code (ResetVectorVtf0.asm) as below using mov CRx.
>                                                            <1> BITS 16
>     176 00000800 0F20C0              <1>     mov   eax, cr0    <-- previously it was smsw
>     177 00000803 A801                  <1>     test    al, 1
>     178 00000805 7405                  <1>     jz       .Real
>     179                                               <1> BITS 32
>     180 00000807 E951FFFFFF      <1>     jmp   Main32
>     181                                               <1> BITS 16
>     182                                               <1> .Real:
>     183 0000080C E939FF              <1>     jmp   EarlyBspInitReal16
> 
> I test the code in a AMD SEV server and try to launch a SEV guest. This time it stuck at the *mov eax, cr0*.
> I am curious if *mov eax, cr0* works in real mode in a SEV guest?
> I also test the code in a legacy vm guest and td guest, all passed.
> Did I miss something?
> 
> I followed https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2FAMDSEV&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C735127b019a14c43ed7008d979da674c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637674801211043868%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=8jElJRmB0dVBe0vfhMLCdwZsUqbi6DKhmAA16pbtGnc%3D&amp;reserved=0 to set up the AMD SEV environment and successfully launched SEV guest with the OVMF image.
> 

Hmm, I am not aware of any limitation w.r.t encrypted VMs. I just added 
the below code in my branch and I do not see any issues, my SEV, SEV-ES 
and SEV-SNP all are able to boot fine. And KVM trace confirms that code 
it read

diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm 
b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index f0e509d0672e..98e34332b04c 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -175,9 +175,21 @@ resetVector:
  ;
  ; This is where the processor will begin execution
  ;
+%ifdef ARCH_IA32
      nop
      nop
      jmp     EarlyBspInitReal16
+%else
+    mov     eax, cr0
+    test    al, 1
+    jz      .Real
+BITS 32
+    hlt
+    ;jmp     Main32
+BITS 16
+.Real:
+    jmp     EarlyBspInitReal16
+%endif

  ALIGN   16


And KVM trace:

kvm_exit: vcpu 0 reason npf rip 0xfff0 info1 0x0000000500000014 info2 
0x00000000fffff000 intr_info 0x00000000 error_code 0x00000000
kvm_page_fault: address fffff000 error_code 500000014
kvm_entry: vcpu 0, rip 0xfff0
kvm_exit: vcpu 0 reason read_cr0 rip 0xfff0 info1 0x8000000000000000 
info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
kvm_cr: cr_read 0 = 0x60000010
kvm_entry: vcpu 0, rip 0xfff3

As we can see from the kvm trace, the first instruction here is the Cr0 
read and it was successfully intercepted and rip moved to next instruction.

Can you please provide me KVM trace for your failure case ? Also, 
provide me the output of "lscpu" and "dmesg" from the host.

thanks

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

[Min Xu]

Hi, Brijesh

On September 17, 2021 11:52 PM, Brijesh Singh wrote:
> 
> Hi Min,
> 
> On 9/17/21 7:55 AM, Xu, Min M wrote:
> ...
> 
> >
> > As I mentioned in my last mail, in the beginning I missed the limitation of
> smsw.
> > So I update the code (ResetVectorVtf0.asm) as below using mov CRx.
> >                                                            <1> BITS 16
> >     176 00000800 0F20C0              <1>     mov   eax, cr0    <-- previously it
> was smsw
> >     177 00000803 A801                  <1>     test    al, 1
> >     178 00000805 7405                  <1>     jz       .Real
> >     179                                               <1> BITS 32
> >     180 00000807 E951FFFFFF      <1>     jmp   Main32
> >     181                                               <1> BITS 16
> >     182                                               <1> .Real:
> >     183 0000080C E939FF              <1>     jmp   EarlyBspInitReal16
> >
> > I test the code in a AMD SEV server and try to launch a SEV guest. This time
> it stuck at the *mov eax, cr0*.
> > I am curious if *mov eax, cr0* works in real mode in a SEV guest?
> > I also test the code in a legacy vm guest and td guest, all passed.
> > Did I miss something?
> >
> 
> Hmm, I am not aware of any limitation w.r.t encrypted VMs. I just added the
> below code in my branch and I do not see any issues, my SEV, SEV-ES and
> SEV-SNP all are able to boot fine. And KVM trace confirms that code it read
> 
> diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> index f0e509d0672e..98e34332b04c 100644
> --- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> +++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> @@ -175,9 +175,21 @@ resetVector:
>   ;
>   ; This is where the processor will begin execution
>   ;
> +%ifdef ARCH_IA32
>       nop
>       nop
>       jmp     EarlyBspInitReal16
> +%else
> +    mov     eax, cr0
> +    test    al, 1
> +    jz      .Real
> +BITS 32
> +    hlt
> +    ;jmp     Main32
> +BITS 16
> +.Real:
> +    jmp     EarlyBspInitReal16
> +%endif
> 
>   ALIGN   16
> 
> 
> And KVM trace:
> 
> kvm_exit: vcpu 0 reason npf rip 0xfff0 info1 0x0000000500000014 info2
> 0x00000000fffff000 intr_info 0x00000000 error_code 0x00000000
> kvm_page_fault: address fffff000 error_code 500000014
> kvm_entry: vcpu 0, rip 0xfff0
> kvm_exit: vcpu 0 reason read_cr0 rip 0xfff0 info1 0x8000000000000000
> info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
> kvm_cr: cr_read 0 = 0x60000010
> kvm_entry: vcpu 0, rip 0xfff3
> 
> As we can see from the kvm trace, the first instruction here is the Cr0 read
> and it was successfully intercepted and rip moved to next instruction.
> 
> Can you please provide me KVM trace for your failure case ? Also, provide me
> the output of "lscpu" and "dmesg" from the host.

The OVMF image you tested is built with GCC tool chain, right?

I usually do the development in windows and build the OVMF image with VS2019.
If the new feature works, then I cherry-pick the patch-sets to code base in ubuntu
18.04 and build/test the new feature.

The weird thing is that, with VS2019, even the OVMF image is built from edk2-master, 
such image doesn't work on AMD SEV server either. But if the image is built by Ubuntu 18.04,
it does work on AMD SEV server.

I applied my TDX patch-sets to the code base on my Ubuntu 18.04, and build the image.
This image does work in both AMD SEV server and Intel TDX server.

Thanks!
Min

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

[Brijesh Singh]

Hi Min,

On 9/18/21 12:16 AM, Xu, Min M wrote:
> Hi, Brijesh
>
> On September 17, 2021 11:52 PM, Brijesh Singh wrote:
>> Hi Min,
>>
>> On 9/17/21 7:55 AM, Xu, Min M wrote:
>> ...
>>
>>> As I mentioned in my last mail, in the beginning I missed the limitation of
>> smsw.
>>> So I update the code (ResetVectorVtf0.asm) as below using mov CRx.
>>>                                                            <1> BITS 16
>>>     176 00000800 0F20C0              <1>     mov   eax, cr0    <-- previously it
>> was smsw
>>>     177 00000803 A801                  <1>     test    al, 1
>>>     178 00000805 7405                  <1>     jz       .Real
>>>     179                                               <1> BITS 32
>>>     180 00000807 E951FFFFFF      <1>     jmp   Main32
>>>     181                                               <1> BITS 16
>>>     182                                               <1> .Real:
>>>     183 0000080C E939FF              <1>     jmp   EarlyBspInitReal16
>>>
>>> I test the code in a AMD SEV server and try to launch a SEV guest. This time
>> it stuck at the *mov eax, cr0*.
>>> I am curious if *mov eax, cr0* works in real mode in a SEV guest?
>>> I also test the code in a legacy vm guest and td guest, all passed.
>>> Did I miss something?
>>>
>> Hmm, I am not aware of any limitation w.r.t encrypted VMs. I just added the
>> below code in my branch and I do not see any issues, my SEV, SEV-ES and
>> SEV-SNP all are able to boot fine. And KVM trace confirms that code it read
>>
>> diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>> b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>> index f0e509d0672e..98e34332b04c 100644
>> --- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>> +++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>> @@ -175,9 +175,21 @@ resetVector:
>>   ;
>>   ; This is where the processor will begin execution
>>   ;
>> +%ifdef ARCH_IA32
>>       nop
>>       nop
>>       jmp     EarlyBspInitReal16
>> +%else
>> +    mov     eax, cr0
>> +    test    al, 1
>> +    jz      .Real
>> +BITS 32
>> +    hlt
>> +    ;jmp     Main32
>> +BITS 16
>> +.Real:
>> +    jmp     EarlyBspInitReal16
>> +%endif
>>
>>   ALIGN   16
>>
>>
>> And KVM trace:
>>
>> kvm_exit: vcpu 0 reason npf rip 0xfff0 info1 0x0000000500000014 info2
>> 0x00000000fffff000 intr_info 0x00000000 error_code 0x00000000
>> kvm_page_fault: address fffff000 error_code 500000014
>> kvm_entry: vcpu 0, rip 0xfff0
>> kvm_exit: vcpu 0 reason read_cr0 rip 0xfff0 info1 0x8000000000000000
>> info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
>> kvm_cr: cr_read 0 = 0x60000010
>> kvm_entry: vcpu 0, rip 0xfff3
>>
>> As we can see from the kvm trace, the first instruction here is the Cr0 read
>> and it was successfully intercepted and rip moved to next instruction.
>>
>> Can you please provide me KVM trace for your failure case ? Also, provide me
>> the output of "lscpu" and "dmesg" from the host.
> The OVMF image you tested is built with GCC tool chain, right?

Yes, we have been using the GCC tool chain only.


> I usually do the development in windows and build the OVMF image with VS2019.
> If the new feature works, then I cherry-pick the patch-sets to code base in ubuntu
> 18.04 and build/test the new feature.
>
> The weird thing is that, with VS2019, even the OVMF image is built from edk2-master, 
> such image doesn't work on AMD SEV server either. But if the image is built by Ubuntu 18.04,
> it does work on AMD SEV server.

This seems very strange that we are failing to execute the hand written
assembly code. I am wondering if somehow the VS compiler is generating a
wrong byte code and thus causing a trap on KVM that requires emulation.
Since the guest memory is encrypted, so KVM emulation code will not be
able to decode the instruction bytes and thus leading in repetitive
nested fault. Only way I could verify my theory is if I can get a KVM
trace or an OVMF binary. If you have have KVM trace or OVMF_CODE.fd
handy then please share.


> I applied my TDX patch-sets to the code base on my Ubuntu 18.04, and build the image.
> This image does work in both AMD SEV server and Intel TDX server.
>
> Thanks!
> Min

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

[James Bottomley]

On Sat, 2021-09-18 at 06:30 -0500, Brijesh Singh wrote:
> On 9/18/21 12:16 AM, Xu, Min M wrote:
[...]
> > I usually do the development in windows and build the OVMF image
> > with VS2019.
> > If the new feature works, then I cherry-pick the patch-sets to code
> > base in ubuntu 18.04 and build/test the new feature.
> > 
> > The weird thing is that, with VS2019, even the OVMF image is built
> > from edk2-master, such image doesn't work on AMD SEV server either.
> > But if the image is built by Ubuntu 18.04, it does work on AMD SEV
> > server.
> 
> This seems very strange that we are failing to execute the hand
> written assembly code. I am wondering if somehow the VS compiler is
> generating a wrong byte code and thus causing a trap on KVM that
> requires emulation.

This theory should be verifiable by doing objdump on the windows binary
... can someone do that?

James

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

[Min Xu]

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

Hi, Brijesh

On September 18, 2021 7:30 PM, Brijesh Singh wrote:
> Hi Min,
> 
> On 9/18/21 12:16 AM, Xu, Min M wrote:
> > Hi, Brijesh
> >
> > On September 17, 2021 11:52 PM, Brijesh Singh wrote:
> >> Hi Min,
> >>
> >> On 9/17/21 7:55 AM, Xu, Min M wrote:
> >> ...
> >>
> >>> As I mentioned in my last mail, in the beginning I missed the
> >>> limitation of
> >> smsw.
> >>> So I update the code (ResetVectorVtf0.asm) as below using mov CRx.
> >>>                                                            <1> BITS 16
> >>>     176 00000800 0F20C0              <1>     mov   eax, cr0    <-- previously it
> >> was smsw
> >>>     177 00000803 A801                  <1>     test    al, 1
> >>>     178 00000805 7405                  <1>     jz       .Real
> >>>     179                                               <1> BITS 32
> >>>     180 00000807 E951FFFFFF      <1>     jmp   Main32
> >>>     181                                               <1> BITS 16
> >>>     182                                               <1> .Real:
> >>>     183 0000080C E939FF              <1>     jmp   EarlyBspInitReal16
> >>>
> >>> I test the code in a AMD SEV server and try to launch a SEV guest.
> >>> This time
> >> it stuck at the *mov eax, cr0*.
> >>> I am curious if *mov eax, cr0* works in real mode in a SEV guest?
> >>> I also test the code in a legacy vm guest and td guest, all passed.
> >>> Did I miss something?
> >>>
> >> Hmm, I am not aware of any limitation w.r.t encrypted VMs. I just
> >> added the below code in my branch and I do not see any issues, my
> >> SEV, SEV-ES and SEV-SNP all are able to boot fine. And KVM trace
> >> confirms that code it read
> >>
> >> diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> >> b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> >> index f0e509d0672e..98e34332b04c 100644
> >> --- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> >> +++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> >> @@ -175,9 +175,21 @@ resetVector:
> >>   ;
> >>   ; This is where the processor will begin execution
> >>   ;
> >> +%ifdef ARCH_IA32
> >>       nop
> >>       nop
> >>       jmp     EarlyBspInitReal16
> >> +%else
> >> +    mov     eax, cr0
> >> +    test    al, 1
> >> +    jz      .Real
> >> +BITS 32
> >> +    hlt
> >> +    ;jmp     Main32
> >> +BITS 16
> >> +.Real:
> >> +    jmp     EarlyBspInitReal16
> >> +%endif
> >>
> >>   ALIGN   16
> >>
> >>
> >> And KVM trace:
> >>
> >> kvm_exit: vcpu 0 reason npf rip 0xfff0 info1 0x0000000500000014 info2
> >> 0x00000000fffff000 intr_info 0x00000000 error_code 0x00000000
> >> kvm_page_fault: address fffff000 error_code 500000014
> >> kvm_entry: vcpu 0, rip 0xfff0
> >> kvm_exit: vcpu 0 reason read_cr0 rip 0xfff0 info1 0x8000000000000000
> >> info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
> >> kvm_cr: cr_read 0 = 0x60000010
> >> kvm_entry: vcpu 0, rip 0xfff3
> >>
> >> As we can see from the kvm trace, the first instruction here is the
> >> Cr0 read and it was successfully intercepted and rip moved to next
> instruction.
> >>
> >> Can you please provide me KVM trace for your failure case ? Also,
> >> provide me the output of "lscpu" and "dmesg" from the host.
> > The OVMF image you tested is built with GCC tool chain, right?
> 
> Yes, we have been using the GCC tool chain only.
>
Is VS Tool chain (VS2017, VS2019, etc) supported by AMD SEV in OVMF? 
I am a little nervous when the Ovmf img failed to be launched in AMD SEV
server after my TDX patch is applied.
> 
> 
> > I usually do the development in windows and build the OVMF image with
> VS2019.
> > If the new feature works, then I cherry-pick the patch-sets to code
> > base in ubuntu
> > 18.04 and build/test the new feature.
> >
> > The weird thing is that, with VS2019, even the OVMF image is built
> > from edk2-master, such image doesn't work on AMD SEV server either.
> > But if the image is built by Ubuntu 18.04, it does work on AMD SEV server.
> 
> This seems very strange that we are failing to execute the hand written
> assembly code.
Actually even the OvmfPkg from edk2-master (without any changes) cannot be
launched on AMD SEV server if it is built with VS2019 tool chain.

This is the qemu-kvm used:
$/usr/libexec/qemu-kvm --version
QEMU emulator version 4.2.0 (qemu-kvm-4.2.0-48.module_el8.4.0+885+5e18b468.3)
Copyright (c) 2003-2019 Fabrice Bellard and the QEMU Project developers

This is the launch scripts.

QEMU=/usr/libexec/qemu-kvm
DRIVE=rhel-8.qcow2

${QEMU} \
  -enable-kvm -cpu EPYC -machine q35 \
  -smp 4,maxcpus=64 \
  -m 4096M,slots=5,maxmem=30G \
  -drive if=pflash,format=raw,unit=0,file=OVMF_CODE.fd,readonly \
  -drive if=pflash,format=raw,unit=1,file=OVMF_VARS.fd \
  -netdev user,id=vmnic \
  -device e1000,netdev=vmnic,romfile= \
  -drive file=${DRIVE},if=none,id=disk0,format=qcow2 \
  -device virtio-scsi-pci,id=scsi,disable-legacy=on,iommu_platform=true \
  -device scsi-hd,drive=disk0 \
  -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1 \
  -machine memory-encryption=sev0 \
  -nographic

>I am wondering if somehow the VS compiler is generating a
> wrong byte code and thus causing a trap on KVM that requires emulation.
> Since the guest memory is encrypted, so KVM emulation code will not be
> able to decode the instruction bytes and thus leading in repetitive nested
> fault. Only way I could verify my theory is if I can get a KVM trace or an OVMF
> binary. If you have have KVM trace or OVMF_CODE.fd handy then please
> share.
>
The OVMF_CODE.fd and OVMF_VARS.fd are attached.

The code base is :
ac6388add4 2021-09-15 (HEAD -> master, origin/master, origin/HEAD) ArmPkg/ProcessorSubClassDxe: Fix the format of ProcessorId

This is the build command:
build -p OvmfPkgX64.dsc -a X64 -t VS2019
> 
> 
> > I applied my TDX patch-sets to the code base on my Ubuntu 18.04, and
> build the image.
> > This image does work in both AMD SEV server and Intel TDX server.
> >

Thanks!
Min

[-- Attachment #2: OVMF_VARS.fd --]
[-- Type: application/octet-stream, Size: 540672 bytes --]

[-- Attachment #3: OVMF_CODE.fd --]
[-- Type: application/octet-stream, Size: 3653632 bytes --]

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

[Min Xu]

On September 16, 2021 3:55 PM, Gerd Hoffman wrote:
> 
> >  typedef struct _CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER {
> >    UINT8                   GuestType;
> > -  UINT8                   Reserved1[3];
> > +  UINT8                   SubType;
> > +  UINT8                   Reserved1[2];
> >  } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;
> 

In [PATCH v7 19/31] UefiCpuPkg: Define ConfidentialComputingGuestAttr
There are below ConfidentialComputingGuestAttr:

+  ## This dynamic PCD indicates the memory encryption attribute of the guest.
+  # @Prompt Memory encryption attribute
+  
+ gUefiCpuPkgTokenSpaceGuid.PcdConfidentialComputingGuestAttr|0|UINT64|0
+ x60000017
+
+
+typedef enum {
+  /* The guest is running with memory encryption disabled. */
+  CCAttrNotEncrypted = 0,
+
+  /* The guest is running with AMD SEV memory encryption enabled. */
+  CCAttrAmdSev      = 0x100,
+  CCAttrAmdSevEs    = 0x101,
+  CCAttrAmdSevSnp   = 0x102,
+
+  /* The guest is running with Intel TDX memory encryption enabled. */
+  CCAttrIntelTdx    = 0x200,
+} CONFIDENTIAL_COMPUTING_GUEST_ATTR;
+

ConfidentialComputingGuestAttr is a 64-bit PCD, the byte[1] indicates the Guest type, byte[0] seems the sub type of the guest.

And in the current definition of  CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER:
typedef struct _CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER {
   UINT8                   GuestType;
  UINT8                   Reserved1[3];
} CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;
Byte[0] is the Guest type.

I am not sure what you mean:
> we should use the same approach (and the same enum) we are planing to use 
> for the ConfidentialComputing PCD (see discussion in the other patch series).

Shall we update CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER so that byte[0] is sub type, and byte[1] indicates the Guest type?

Thanks!
Min

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

[Brijesh Singh]

On 9/18/21 10:14 PM, Xu, Min M wrote:
> Hi, Brijesh
> 
> On September 18, 2021 7:30 PM, Brijesh Singh wrote:
>> Hi Min,
>>
>> On 9/18/21 12:16 AM, Xu, Min M wrote:
>>> Hi, Brijesh
>>>
>>> On September 17, 2021 11:52 PM, Brijesh Singh wrote:
>>>> Hi Min,
>>>>
>>>> On 9/17/21 7:55 AM, Xu, Min M wrote:
>>>> ...
>>>>
>>>>> As I mentioned in my last mail, in the beginning I missed the
>>>>> limitation of
>>>> smsw.
>>>>> So I update the code (ResetVectorVtf0.asm) as below using mov CRx.
>>>>>                                                             <1> BITS 16
>>>>>      176 00000800 0F20C0              <1>     mov   eax, cr0    <-- previously it
>>>> was smsw
>>>>>      177 00000803 A801                  <1>     test    al, 1
>>>>>      178 00000805 7405                  <1>     jz       .Real
>>>>>      179                                               <1> BITS 32
>>>>>      180 00000807 E951FFFFFF      <1>     jmp   Main32
>>>>>      181                                               <1> BITS 16
>>>>>      182                                               <1> .Real:
>>>>>      183 0000080C E939FF              <1>     jmp   EarlyBspInitReal16
>>>>>
>>>>> I test the code in a AMD SEV server and try to launch a SEV guest.
>>>>> This time
>>>> it stuck at the *mov eax, cr0*.
>>>>> I am curious if *mov eax, cr0* works in real mode in a SEV guest?
>>>>> I also test the code in a legacy vm guest and td guest, all passed.
>>>>> Did I miss something?
>>>>>
>>>> Hmm, I am not aware of any limitation w.r.t encrypted VMs. I just
>>>> added the below code in my branch and I do not see any issues, my
>>>> SEV, SEV-ES and SEV-SNP all are able to boot fine. And KVM trace
>>>> confirms that code it read
>>>>
>>>> diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>>>> b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>>>> index f0e509d0672e..98e34332b04c 100644
>>>> --- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>>>> +++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>>>> @@ -175,9 +175,21 @@ resetVector:
>>>>    ;
>>>>    ; This is where the processor will begin execution
>>>>    ;
>>>> +%ifdef ARCH_IA32
>>>>        nop
>>>>        nop
>>>>        jmp     EarlyBspInitReal16
>>>> +%else
>>>> +    mov     eax, cr0
>>>> +    test    al, 1
>>>> +    jz      .Real
>>>> +BITS 32
>>>> +    hlt
>>>> +    ;jmp     Main32
>>>> +BITS 16
>>>> +.Real:
>>>> +    jmp     EarlyBspInitReal16
>>>> +%endif
>>>>
>>>>    ALIGN   16
>>>>
>>>>
>>>> And KVM trace:
>>>>
>>>> kvm_exit: vcpu 0 reason npf rip 0xfff0 info1 0x0000000500000014 info2
>>>> 0x00000000fffff000 intr_info 0x00000000 error_code 0x00000000
>>>> kvm_page_fault: address fffff000 error_code 500000014
>>>> kvm_entry: vcpu 0, rip 0xfff0
>>>> kvm_exit: vcpu 0 reason read_cr0 rip 0xfff0 info1 0x8000000000000000
>>>> info2 0x0000000000000000 intr_info 0x00000000 error_code 0x00000000
>>>> kvm_cr: cr_read 0 = 0x60000010
>>>> kvm_entry: vcpu 0, rip 0xfff3
>>>>
>>>> As we can see from the kvm trace, the first instruction here is the
>>>> Cr0 read and it was successfully intercepted and rip moved to next
>> instruction.
>>>>
>>>> Can you please provide me KVM trace for your failure case ? Also,
>>>> provide me the output of "lscpu" and "dmesg" from the host.
>>> The OVMF image you tested is built with GCC tool chain, right?
>>
>> Yes, we have been using the GCC tool chain only.
>>
> Is VS Tool chain (VS2017, VS2019, etc) supported by AMD SEV in OVMF?
> I am a little nervous when the Ovmf img failed to be launched in AMD SEV
> server after my TDX patch is applied.
>>
>>
>>> I usually do the development in windows and build the OVMF image with
>> VS2019.
>>> If the new feature works, then I cherry-pick the patch-sets to code
>>> base in ubuntu
>>> 18.04 and build/test the new feature.
>>>
>>> The weird thing is that, with VS2019, even the OVMF image is built
>>> from edk2-master, such image doesn't work on AMD SEV server either.
>>> But if the image is built by Ubuntu 18.04, it does work on AMD SEV server.
>>
>> This seems very strange that we are failing to execute the hand written
>> assembly code.
> Actually even the OvmfPkg from edk2-master (without any changes) cannot be
> launched on AMD SEV server if it is built with VS2019 tool chain.
> 
> This is the qemu-kvm used:
> $/usr/libexec/qemu-kvm --version
> QEMU emulator version 4.2.0 (qemu-kvm-4.2.0-48.module_el8.4.0+885+5e18b468.3)
> Copyright (c) 2003-2019 Fabrice Bellard and the QEMU Project developers
> 
> This is the launch scripts.
> 
> QEMU=/usr/libexec/qemu-kvm
> DRIVE=rhel-8.qcow2
> 
> ${QEMU} \
>    -enable-kvm -cpu EPYC -machine q35 \
>    -smp 4,maxcpus=64 \
>    -m 4096M,slots=5,maxmem=30G \
>    -drive if=pflash,format=raw,unit=0,file=OVMF_CODE.fd,readonly \
>    -drive if=pflash,format=raw,unit=1,file=OVMF_VARS.fd \
>    -netdev user,id=vmnic \
>    -device e1000,netdev=vmnic,romfile= \
>    -drive file=${DRIVE},if=none,id=disk0,format=qcow2 \
>    -device virtio-scsi-pci,id=scsi,disable-legacy=on,iommu_platform=true \
>    -device scsi-hd,drive=disk0 \
>    -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1 \
>    -machine memory-encryption=sev0 \
>    -nographic
> 
>> I am wondering if somehow the VS compiler is generating a
>> wrong byte code and thus causing a trap on KVM that requires emulation.
>> Since the guest memory is encrypted, so KVM emulation code will not be
>> able to decode the instruction bytes and thus leading in repetitive nested
>> fault. Only way I could verify my theory is if I can get a KVM trace or an OVMF
>> binary. If you have have KVM trace or OVMF_CODE.fd handy then please
>> share.
>>
> The OVMF_CODE.fd and OVMF_VARS.fd are attached.
> 
> The code base is :
> ac6388add4 2021-09-15 (HEAD -> master, origin/master, origin/HEAD) ArmPkg/ProcessorSubClassDxe: Fix the format of ProcessorId
> 
> This is the build command:
> build -p OvmfPkgX64.dsc -a X64 -t VS2019
>>
>>

Actually, I am not able to boot a non SEV guest with your attached 
binary. It appears that sometime during an easy boot, we are getting a 
triple fault (maybe guest is accessing invalid memory) and thus KVM 
issues a SHUTDOWN. The same is seen with the SEV guest.

I don't know what VS compiler is doing to cause this.

-Brijesh

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

[Gerd Hoffmann]

> + gUefiCpuPkgTokenSpaceGuid.PcdConfidentialComputingGuestAttr|0|UINT64|0
> + x60000017

> +typedef enum {
> +  /* The guest is running with memory encryption disabled. */
> +  CCAttrNotEncrypted = 0,
> +
> +  /* The guest is running with AMD SEV memory encryption enabled. */
> +  CCAttrAmdSev      = 0x100,
> +  CCAttrAmdSevEs    = 0x101,
> +  CCAttrAmdSevSnp   = 0x102,
> +
> +  /* The guest is running with Intel TDX memory encryption enabled. */
> +  CCAttrIntelTdx    = 0x200,
> +} CONFIDENTIAL_COMPUTING_GUEST_ATTR;

> ConfidentialComputingGuestAttr is a 64-bit PCD, the byte[1] indicates the Guest type, byte[0] seems the sub type of the guest.
> 
> And in the current definition of  CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER:
> typedef struct _CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER {
>    UINT8                   GuestType;
>   UINT8                   Reserved1[3];
> } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;
> Byte[0] is the Guest type.
> 
> I am not sure what you mean:
> > we should use the same approach (and the same enum) we are planing to use 
> > for the ConfidentialComputing PCD (see discussion in the other patch series).
> 
> Shall we update CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER so that byte[0] is sub type, and byte[1] indicates the Guest type?

The idea is to make GuestType larger (UINT16 is probably enough), then
use the CONFIDENTIAL_COMPUTING_GUEST_ATTR enum for GuestType too, so we
don't have two different confidential computing guest type enumeration
systems in edk2.

So, yes, effectively that would make byte[1] the type (sev/tdx/none)
and byte[0] the sub-type thanks to little endian byte ordering.

take care,
  Gerd

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

[Min Xu]

On September 21, 2021 1:16 PM, Gerd Hoffmann wrote:
> > +
> gUefiCpuPkgTokenSpaceGuid.PcdConfidentialComputingGuestAttr|0|UINT64
> > + |0
> > + x60000017
> 
> > +typedef enum {
> > +  /* The guest is running with memory encryption disabled. */
> > +  CCAttrNotEncrypted = 0,
> > +
> > +  /* The guest is running with AMD SEV memory encryption enabled. */
> > +  CCAttrAmdSev      = 0x100,
> > +  CCAttrAmdSevEs    = 0x101,
> > +  CCAttrAmdSevSnp   = 0x102,
> > +
> > +  /* The guest is running with Intel TDX memory encryption enabled. */
> > +  CCAttrIntelTdx    = 0x200,
> > +} CONFIDENTIAL_COMPUTING_GUEST_ATTR;
> 
> > ConfidentialComputingGuestAttr is a 64-bit PCD, the byte[1] indicates the
> Guest type, byte[0] seems the sub type of the guest.
> >
> > And in the current definition of
> CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER:
> > typedef struct _CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER {
> >    UINT8                   GuestType;
> >   UINT8                   Reserved1[3];
> > } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;
> > Byte[0] is the Guest type.
> >
> > I am not sure what you mean:
> > > we should use the same approach (and the same enum) we are planing
> > > to use for the ConfidentialComputing PCD (see discussion in the other
> patch series).
> >
> > Shall we update CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER so that
> byte[0] is sub type, and byte[1] indicates the Guest type?
> 
> The idea is to make GuestType larger (UINT16 is probably enough), then use
> the CONFIDENTIAL_COMPUTING_GUEST_ATTR enum for GuestType too, so we
> don't have two different confidential computing guest type enumeration
> systems in edk2.
> 
> So, yes, effectively that would make byte[1] the type (sev/tdx/none) and
> byte[0] the sub-type thanks to little endian byte ordering.
>
I see. But such change may impact the existing SEV code in SecMain.c. Maybe there are more existing codes impacted.
I will not change the definition of CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER this time. Maybe in the future this change is needed.
> 
Thanks!
Min