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

["Brijesh Singh" <brijesh.singh@amd.com>]

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%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637629670106503221%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=5JPmffFHuQiLiG954z68vNLT2h1nwuURTRpfsczCm%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.groups.io%2Fg%2Fdevel%2Fmessage%2F78152&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637629670106503221%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=LKpyRRcngy4H71bVhCbvZMOxy4QMMr%2BbE7lGmEgLIHA%3D&amp;reserved=0
>> and https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F78151&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637629670106503221%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=%2B7S7%2BsNkHC8LWl5zWNbNt0QF8RY5ICUVYobUrYklYTM%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.groups.io%2Fg%2Fdevel%2Fmessage%2F78152&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637629670106513208%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=J7naeEX7mJhoMD4f0LDWEUNEjiYD7DHSJICQFxqf8v8%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%2Fsoftware.intel.com%2Fcontent%2Fdam%2Fdevelop%2Fexternal%2Fus%2Fen%2F&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637629670106513208%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=OXsNermcOOWzwl%2BtznnK3fGtH3HD4bmvqXVNR8SAPPI%3D&amp;reserved=0
>> documents/tdx-whitepaper-final9-17.pdf
>>
>> [TDVF]: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsoftware.intel.com%2Fcontent%2Fdam%2Fdevelop%2Fexternal%2Fus%2Fen%2F&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637629670106513208%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=OXsNermcOOWzwl%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.groups.io%2Fg%2Fdevel%2Fmessage%2F78151&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637629670106513208%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=CjaSzffSZ1wEMFJ09uqZiHR%2FNcCefJ%2Bi4g2iplKxIiw%3D&amp;reserved=0
>>
>> [ReviewComment-2]: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F78152&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637629670106513208%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=J7naeEX7mJhoMD4f0LDWEUNEjiYD7DHSJICQFxqf8v8%3D&amp;reserved=0
>>
>> Code is at https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmxu9%2Fedk2%2Ftree%2Ftdvf_wave1.v3&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637629670106513208%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=9ly05aaxRWbNLBR8PrNncQL%2B%2BrzhnKlk3kthNVs9kIU%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.groups.io%2Fg%2Fdevel%2Fmessage%2F77675&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C845afb67ce2d47b1309308d950ce8012%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637629670106513208%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=osU4QOWl3FWRZ94VyD%2B0VHse7%2BCxUSbiVBtWf7WmbcQ%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
>>
>>
>>
>> 
>>