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

[Min Xu]

On July 27, 2021 3:17 PM, Yao, Jiewen wrote:
> 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.
Agree. Will move them to IntelTdx.asm in the next version.
> 
> 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.
Agree. Will update it in next version.
> 
> 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.
They are in SEV's code. I will extract them out in the next version.
> 
> 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:
> ===============
Agree. Will update it in next version.
> 
> 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?
Sure. I will do it in the next version.
> 
Thank you very much for the comments.

Xu, Min 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78203): https://edk2.groups.io/g/devel/message/78203
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]
-=-=-=-=-=-=-=-=-=-=-=-