Re: [edk2-devel] [PATCH V3 00/10] Add Intel TDX support in OvmfPkg/ResetVector

["Yao, Jiewen" <jiewen.yao@intel.com>]

Thank Brijesh.
Yes, I just notice that.

I am reviewing SNP patch series. It may take me some time to review all 28 patches.

Thank you
Yao Jiewen


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Brijesh
> Singh via groups.io
> Sent: Tuesday, July 27, 2021 7:24 PM
> To: Yao, Jiewen <jiewen.yao@intel.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>; Erdem Aktas
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> Lendacky <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V3 00/10] Add Intel TDX support in
> OvmfPkg/ResetVector
> 
> Hi,
> 
> 
> On 7/27/21 2:16 AM, Yao, Jiewen wrote:
> > Thanks Min.
> > Many thanks for splitting SEV stuff to a standalone file. That is very good start.
> 
> I am not sure if you noticed but the SEV refactoring was already
> included in the SNP series, I extracted those patches from SNP series
> and posted now directly for the acceptance. This will give a well tested
> SEV refactor logic which Min can build upon. The refactor series is
> tested on non-SEV and SEV guest.
> 
> thanks
> 
> >
> > 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.xu@intel.com>; Ard Biesheuvel
> >> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> >> Brijesh Singh <brijesh.singh@amd.com>; Erdem Aktas
> >> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Yao,
> >> Jiewen <jiewen.yao@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> >> Subject: [edk2-devel] [PATCH V3 00/10] Add Intel TDX support in
> >> OvmfPkg/ResetVector
> >>
> >> REF:
> 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%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe48
> 84e608e11a82d994e183d%7C0%7C0%7C637629670106503221%7CUnknown%
> 7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVCI6Mn0%3D%7C1000&amp;sdata=5JPmffFHuQiLiG954z68vNLT2h1nwuURT
> RpfsczCm%2F0%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.
> >>
> >> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.gr
> oups.io%2Fg%2Fdevel%2Fmessage%2F78152&amp;data=04%7C01%7Cbrijesh.si
> ngh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884
> e608e11a82d994e183d%7C0%7C0%7C637629670106503221%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C1000&amp;sdata=LKpyRRcngy4H71bVhCbvZMOxy4QMMr%2B
> bE7lGmEgLIHA%3D&amp;reserved=0
> >> and
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.gr
> oups.io%2Fg%2Fdevel%2Fmessage%2F78151&amp;data=04%7C01%7Cbrijesh.si
> ngh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884
> e608e11a82d994e183d%7C0%7C0%7C637629670106503221%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C1000&amp;sdata=%2B7S7%2BsNkHC8LWl5zWNbNt0QF8RY5I
> CUVYobUrYklYTM%3D&amp;reserved=0, 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.gr
> oups.io%2Fg%2Fdevel%2Fmessage%2F78152&amp;data=04%7C01%7Cbrijesh.si
> ngh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884
> e608e11a82d994e183d%7C0%7C0%7C637629670106513208%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C1000&amp;sdata=J7naeEX7mJhoMD4f0LDWEUNEjiYD7DHSJI
> CQFxqf8v8%3D&amp;reserved=0. 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsoftwar
> e.intel.com%2Fcontent%2Fdam%2Fdevelop%2Fexternal%2Fus%2Fen%2F&amp;
> data=04%7C01%7Cbrijesh.singh%40amd.com%7C845afb67ce2d47b1309308d95
> 0ce8012%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6376296701
> 06513208%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
> uMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=OXsNermcOO
> Wzwl%2BtznnK3fGtH3HD4bmvqXVNR8SAPPI%3D&amp;reserved=0
> >> documents/tdx-whitepaper-final9-17.pdf
> >>
> >> [TDVF]:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsoftwar
> e.intel.com%2Fcontent%2Fdam%2Fdevelop%2Fexternal%2Fus%2Fen%2F&amp;
> data=04%7C01%7Cbrijesh.singh%40amd.com%7C845afb67ce2d47b1309308d95
> 0ce8012%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6376296701
> 06513208%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
> uMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=OXsNermcOO
> Wzwl%2BtznnK3fGtH3HD4bmvqXVNR8SAPPI%3D&amp;reserved=0
> >> documents/tdx-virtual-firmware-design-guide-rev-1.pdf
> >>
> >> [ReviewComment-1]:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.gr
> oups.io%2Fg%2Fdevel%2Fmessage%2F78151&amp;data=04%7C01%7Cbrijesh.si
> ngh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884
> e608e11a82d994e183d%7C0%7C0%7C637629670106513208%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C1000&amp;sdata=CjaSzffSZ1wEMFJ09uqZiHR%2FNcCefJ%2Bi4
> g2iplKxIiw%3D&amp;reserved=0
> >>
> >> [ReviewComment-2]:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.gr
> oups.io%2Fg%2Fdevel%2Fmessage%2F78152&amp;data=04%7C01%7Cbrijesh.si
> ngh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884
> e608e11a82d994e183d%7C0%7C0%7C637629670106513208%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C1000&amp;sdata=J7naeEX7mJhoMD4f0LDWEUNEjiYD7DHSJI
> CQFxqf8v8%3D&amp;reserved=0
> >>
> >> Code is at
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
> om%2Fmxu9%2Fedk2%2Ftree%2Ftdvf_wave1.v3&amp;data=04%7C01%7Cbrijes
> h.singh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4
> 884e608e11a82d994e183d%7C0%7C0%7C637629670106513208%7CUnknown
> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi
> LCJXVCI6Mn0%3D%7C1000&amp;sdata=9ly05aaxRWbNLBR8PrNncQL%2B%2Br
> zhnKlk3kthNVs9kIU%3D&amp;reserved=0
> >>
> >> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.gr
> oups.io%2Fg%2Fdevel%2Fmessage%2F77675&amp;data=04%7C01%7Cbrijesh.si
> ngh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884
> e608e11a82d994e183d%7C0%7C0%7C637629670106513208%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C1000&amp;sdata=osU4QOWl3FWRZ94VyD%2B0VHse7%2BCx
> USbiVBtWf7WmbcQ%3D&amp;reserved=0
> >>
> >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> >> 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 (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
> >>
> >>
> >>
> >>
> >>
> 
> 
> 
>