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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
