Thanks Min.
Many thanks for splitting SEV stuff to a standalone file. That is very good
start.
Some other comments for your consideration:
1) There is no need to create a standalone Init32.asm and ReloadFlat32.asm.
They are only needed in TDX so far. Please keep it in TDX.
2) I do not see the absolute need to create multiple patches for
ResetVector.nasm/ResetVectorVtf0.asm to add TDX stuff one by one. That always
makes me feel you miss something in the beginning. Since all the patches are
adding TDX support, I think we can add them one time.
3) The strategy I take to review the patch is to compare the ResetVector in
UefiCpuPkg and OvmfPkg.
If they are similar, I am at ease. If they are different, I would ask why.
For example, OVMF version Flat32ToFlat64.asm missing the CR4/CR0/EFER_MSR
handling in normal mode. I am not sure why. A potential bug? We had better make
them consistent.
4) I can understand difference in PageTables64.asm. UefiCpuPkg uses ROM page
table, while OVMF uses runtime crated page table. That is OK.
However, it is hard for me to understand how SEV/TDX hack the build page table.
I still recommend we move SEV hook to SEV file, and TDX hook to TDX file.
If we can use below patter, that can help me a lot to understand the logic.
===============
SetCr3ForPageTables64:
xor edx, edx
PreBuildPageTableHookSev
PreBuildPageTableHookTdx
BuildPageTables:
XXXXXX
PostBuildPageTableHookSev
PostBuildPageTableHookTdx
SetCr3:
===============
5) There are too many noise in ResetVectorVtf0.asm.
Can we move SEV and TDX related GUID definition to a standalone SevVtf0.asm and
TdxVtf0.asm?
Thank you
Yao Jiewen
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Min Xu
> Sent: Tuesday, July 27, 2021 1:42 PM
> To: devel@edk2.groups.io
> Cc: Xu, Min M <min.m...@intel.com>; Ard Biesheuvel
> <ardb+tianoc...@kernel.org>; Justen, Jordan L <jordan.l.jus...@intel.com>;
> Brijesh Singh <brijesh.si...@amd.com>; Erdem Aktas
> <erdemak...@google.com>; James Bottomley <j...@linux.ibm.com>; Yao,
> Jiewen <jiewen....@intel.com>; Tom Lendacky <thomas.lenda...@amd.com>
> Subject: [edk2-devel] [PATCH V3 00/10] Add Intel TDX support in
> OvmfPkg/ResetVector
>
> 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 wave1 which adds Intel TDX support in OvmfPkg/ResetVector.
> Note: TDX only works in X64.
>
> According to the comments in https://edk2.groups.io/g/devel/message/78152
> and https://edk2.groups.io/g/devel/message/78151, PageTables64.asm and
> Flat32ToFlat64.asm are refined. SEV and TDX related routines are separated
> to AmdSev.asm and IntelTdx.asm.
>
> Patch 1 add the PCDs of BFV/CFV. BFV is the code part of the image. CFV is
> the configuration part. BFV is measured by VMM and CFV is measured by TDVF
> itself.
>
> Patch 2 add TdxMetadata in OvmfPkg/ResetVector. It describes the
> information about the image so that VMM can do the initialization and
> measurement based on these information.
>
> Patch 3 set the initial value of Td mailbox and its related macros used in
> ResetVector.
>
> Patch 4 define macro of TDX_PT_ADDR which indicates the Tdx extra page
> tables.
>
> Patch 5 is IntelTdx.asm which includes below routines used in ResetVector.
> - IsTdx
> - InitTdx
> - PostSetCr3PageTables64Tdx
>
> Patch 6 is AmdSev.asm which includes SEV routines. These routines are
> extracted from the previous PageTables64.asm/Flat32ToFlat64.asm.
>
> Patch 7 load the GDT and set CR0, then jump to Flat32 protected mode. This
> is because GDT is not set in the initial stage of ResetVector.
>
> Patch 8 is Init32.asm. It is the entry point of doing the 32-bit protected
> mode initialization. ReloadFlat32 and InitTdx are called. In the future if
> SEV has something to initialize, InitSev (for example) can be called in
> turn.
>
> Patch 9 create Main.asm in OvmfPkg/ResetVector. It is based on the
> suggestion in https://edk2.groups.io/g/devel/message/78152. In this commit
> it is simply copied from UefiCpuPkg. In the next commit this Main.asm is
> updated to add a new Entry (Main32) to support Tdx.
>
> In Patch 10, all above are put together. Init32.asm call ReloadFlat32
> to jump to 32-bit protected mode, then InitTdx initialize TDX_WORK_AREA
> to record 'TDXG' flags. After that page tables are built and set, then
> jump to SecEntry.
>
> [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
>
> [ReviewComment-1]: https://edk2.groups.io/g/devel/message/78151
>
> [ReviewComment-2]: https://edk2.groups.io/g/devel/message/78152
>
> Code is at https://github.com/mxu9/edk2/tree/tdvf_wave1.v3
>
> 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+tianoc...@kernel.org>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Brijesh Singh <brijesh.si...@amd.com>
> Cc: Erdem Aktas <erdemak...@google.com>
> Cc: James Bottomley <j...@linux.ibm.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Signed-off-by: Min Xu <min.m...@intel.com>
>
> Min Xu (10):
> OvmfPkg: Add Tdx BFV/CFV PCDs and PcdOvmfImageSizeInKb
> OvmfPkg: Add Tdx metadata
> OvmfPkg: Set TdMailbox initial value and macros
> OvmfPkg: Add TDX_PT_ADDR defition in ResetVector.nasmb
> OvmfPkg: Add IntelTdx.asm in ResetVector
> OvmfPkg: Add AmdSev.asm in ResetVector
> OvmfPkg: Add ReloadFlat32
> OvmfPkg: Add Init32
> OvmfPkg: Create Main.asm in ResetVector
> OvmfPkg: Update ResetVector to support Tdx
>
> OvmfPkg/OvmfPkg.dec | 13 +
> OvmfPkg/OvmfPkgDefines.fdf.inc | 12 +-
> OvmfPkg/OvmfPkgX64.fdf | 6 +
> OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 38 ++
> OvmfPkg/ResetVector/Ia32/AmdSev.asm | 526 +++++++++++++++++++
> OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm | 110 +---
> OvmfPkg/ResetVector/Ia32/Init32.asm | 32 ++
> OvmfPkg/ResetVector/Ia32/IntelTdx.asm | 172 ++++++
> OvmfPkg/ResetVector/Ia32/PageTables64.asm | 478 +++--------------
> OvmfPkg/ResetVector/Ia32/ReloadFlat32.asm | 44 ++
> OvmfPkg/ResetVector/Main.asm | 119 +++++
> OvmfPkg/ResetVector/ResetVector.inf | 11 +-
> OvmfPkg/ResetVector/ResetVector.nasmb | 66 ++-
> OvmfPkg/ResetVector/X64/TdxMetadata.asm | 97 ++++
> 14 files changed, 1224 insertions(+), 500 deletions(-)
> create mode 100644 OvmfPkg/ResetVector/Ia32/AmdSev.asm
> create mode 100644 OvmfPkg/ResetVector/Ia32/Init32.asm
> create mode 100644 OvmfPkg/ResetVector/Ia32/IntelTdx.asm
> create mode 100644 OvmfPkg/ResetVector/Ia32/ReloadFlat32.asm
> create mode 100644 OvmfPkg/ResetVector/Main.asm
> create mode 100644 OvmfPkg/ResetVector/X64/TdxMetadata.asm
>
> --
> 2.29.2.windows.2
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78202): https://edk2.groups.io/g/devel/message/78202
Mute This Topic: https://groups.io/mt/84476057/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
