[edk2-devel] [PATCH V3 00/10] Add Intel TDX support in OvmfPkg/ResetVector
Yao, Jiewen
jiewen.yao at intel.com
Tue Jul 27 11:41:45 UTC 2021
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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Brijesh
> Singh via groups.io
> Sent: Tuesday, July 27, 2021 7:24 PM
> To: Yao, Jiewen <jiewen.yao at intel.com>; devel at edk2.groups.io; Xu, Min M
> <min.m.xu at intel.com>
> Cc: brijesh.singh at amd.com; Ard Biesheuvel <ardb+tianocore at kernel.org>;
> Justen, Jordan L <jordan.l.justen at intel.com>; Erdem Aktas
> <erdemaktas at google.com>; James Bottomley <jejb at linux.ibm.com>; Tom
> Lendacky <thomas.lendacky at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Min Xu
> >> Sent: Tuesday, July 27, 2021 1:42 PM
> >> To: devel at edk2.groups.io
> >> Cc: Xu, Min M <min.m.xu at intel.com>; Ard Biesheuvel
> >> <ardb+tianocore at kernel.org>; Justen, Jordan L <jordan.l.justen at intel.com>;
> >> Brijesh Singh <brijesh.singh at amd.com>; Erdem Aktas
> >> <erdemaktas at google.com>; James Bottomley <jejb at linux.ibm.com>; Yao,
> >> Jiewen <jiewen.yao at intel.com>; Tom Lendacky
> <thomas.lendacky at 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&data=04%7C01%7Cbrijesh.
> singh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe48
> 84e608e11a82d994e183d%7C0%7C0%7C637629670106503221%7CUnknown%
> 7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVCI6Mn0%3D%7C1000&sdata=5JPmffFHuQiLiG954z68vNLT2h1nwuURT
> RpfsczCm%2F0%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.
> >>
> >> 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&data=04%7C01%7Cbrijesh.si
> ngh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884
> e608e11a82d994e183d%7C0%7C0%7C637629670106503221%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C1000&sdata=LKpyRRcngy4H71bVhCbvZMOxy4QMMr%2B
> bE7lGmEgLIHA%3D&reserved=0
> >> and
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.gr
> oups.io%2Fg%2Fdevel%2Fmessage%2F78151&data=04%7C01%7Cbrijesh.si
> ngh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884
> e608e11a82d994e183d%7C0%7C0%7C637629670106503221%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C1000&sdata=%2B7S7%2BsNkHC8LWl5zWNbNt0QF8RY5I
> CUVYobUrYklYTM%3D&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&data=04%7C01%7Cbrijesh.si
> ngh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884
> e608e11a82d994e183d%7C0%7C0%7C637629670106513208%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C1000&sdata=J7naeEX7mJhoMD4f0LDWEUNEjiYD7DHSJI
> CQFxqf8v8%3D&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&
> data=04%7C01%7Cbrijesh.singh%40amd.com%7C845afb67ce2d47b1309308d95
> 0ce8012%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6376296701
> 06513208%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
> uMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=OXsNermcOO
> Wzwl%2BtznnK3fGtH3HD4bmvqXVNR8SAPPI%3D&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&
> data=04%7C01%7Cbrijesh.singh%40amd.com%7C845afb67ce2d47b1309308d95
> 0ce8012%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6376296701
> 06513208%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
> uMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=OXsNermcOO
> Wzwl%2BtznnK3fGtH3HD4bmvqXVNR8SAPPI%3D&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&data=04%7C01%7Cbrijesh.si
> ngh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884
> e608e11a82d994e183d%7C0%7C0%7C637629670106513208%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C1000&sdata=CjaSzffSZ1wEMFJ09uqZiHR%2FNcCefJ%2Bi4
> g2iplKxIiw%3D&reserved=0
> >>
> >> [ReviewComment-2]:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.gr
> oups.io%2Fg%2Fdevel%2Fmessage%2F78152&data=04%7C01%7Cbrijesh.si
> ngh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884
> e608e11a82d994e183d%7C0%7C0%7C637629670106513208%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C1000&sdata=J7naeEX7mJhoMD4f0LDWEUNEjiYD7DHSJI
> CQFxqf8v8%3D&reserved=0
> >>
> >> Code is at
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
> om%2Fmxu9%2Fedk2%2Ftree%2Ftdvf_wave1.v3&data=04%7C01%7Cbrijes
> h.singh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4
> 884e608e11a82d994e183d%7C0%7C0%7C637629670106513208%7CUnknown
> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi
> LCJXVCI6Mn0%3D%7C1000&sdata=9ly05aaxRWbNLBR8PrNncQL%2B%2Br
> zhnKlk3kthNVs9kIU%3D&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&data=04%7C01%7Cbrijesh.si
> ngh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884
> e608e11a82d994e183d%7C0%7C0%7C637629670106513208%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C1000&sdata=osU4QOWl3FWRZ94VyD%2B0VHse7%2BCx
> USbiVBtWf7WmbcQ%3D&reserved=0
> >>
> >> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
> >> Cc: Jordan Justen <jordan.l.justen at intel.com>
> >> Cc: Brijesh Singh <brijesh.singh at amd.com>
> >> Cc: Erdem Aktas <erdemaktas at google.com>
> >> Cc: James Bottomley <jejb at linux.ibm.com>
> >> Cc: Jiewen Yao <jiewen.yao at intel.com>
> >> Cc: Tom Lendacky <thomas.lendacky at amd.com>
> >> Signed-off-by: Min Xu <min.m.xu at 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 (#78217): https://edk2.groups.io/g/devel/message/78217
Mute This Topic: https://groups.io/mt/84476057/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list