On 06/28/19 08:47, Ni, Ray wrote: > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1946 > > The patch changes SMM environment to use 5 level paging when CPU > supports it. > > Signed-off-by: Ray Ni <ray...@intel.com> > Cc: Eric Dong <eric.d...@intel.com> > --- > .../PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 20 +- > UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 272 ++++++---- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 483 ++++++++++++------ > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 12 + > .../PiSmmCpuDxeSmm/X64/SmmProfileArch.c | 72 ++- > 5 files changed, 559 insertions(+), 300 deletions(-)
This patch does not build with GCC, because: > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > index 3d5d663d99..c088010327 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -16,6 +16,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > LIST_ENTRY mPagePool = > INITIALIZE_LIST_HEAD_VARIABLE (mPagePool); > BOOLEAN m1GPageTableSupport = FALSE; > BOOLEAN mCpuSmmStaticPageTable; > +BOOLEAN m5LevelPagingSupport; as-is, two translation units *define* (allocate) "m5LevelPagingSupport": "PageTbl.c" above, and "SmiEntry.nasm". And that breaks the build with GCC (it should break the build with VS as well, because it is a bug in the code). However, I'm not suggesting that we add "extern" to "PageTbl.c". Because, I don't think we should reintroduce DBs into the PiSmmCpuDxeSmm assembly code. That would be a regression for: https://bugzilla.tianocore.org/show_bug.cgi?id=866 We should *especially* not reintroduce the dual use of a byte - both for binary instruction encoding, - and as a data value (for steering C-language code). PiSmmCpuDxeSmm had that kind of code before, but I eliminated it. For example, in commit 3c5ce64f23c4 ("UefiCpuPkg/PiSmmCpuDxeSmm: patch "XdSupported" with PatchInstructionX86()", 2018-04-04). Therefore, please incorporate the following update, into patch #3: > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm > index b5e0405b3b00..ae79bf024bf0 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm > @@ -69,7 +69,7 @@ extern ASM_PFX(mXdSupported) > global ASM_PFX(gPatchXdSupported) > global ASM_PFX(gPatchSmiStack) > global ASM_PFX(gPatchSmiCr3) > -global ASM_PFX(m5LevelPagingSupport) > +global ASM_PFX(gPatch5LevelPagingSupport) > global ASM_PFX(gcSmiHandlerTemplate) > global ASM_PFX(gcSmiHandlerSize) > > @@ -126,8 +126,8 @@ ASM_PFX(gPatchSmiCr3): > mov cr3, rax > mov eax, 0x668 ; as cr4.PGE is not set here, > refresh cr3 > > - DB 0xb1 ; mov cl, m5LevelPagingSupport > -ASM_PFX(m5LevelPagingSupport): DB 0 > + mov cl, strict byte 0 ; source operand will be patched > +ASM_PFX(gPatch5LevelPagingSupport): > cmp cl, 0 > je SkipEnable5LevelPaging > ; > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > index c08801032766..c31160735a37 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -17,6 +17,7 @@ LIST_ENTRY mPagePool = > INITIALIZE_LIST_HEAD_VARIABLE (m > BOOLEAN m1GPageTableSupport = FALSE; > BOOLEAN mCpuSmmStaticPageTable; > BOOLEAN m5LevelPagingSupport; > +X86_ASSEMBLY_PATCH_LABEL gPatch5LevelPagingSupport; > > /** > Disable CET. > @@ -337,6 +338,7 @@ SmmInitPageTable ( > m1GPageTableSupport = Is1GPageSupport (); > m5LevelPagingSupport = Is5LevelPagingSupport (); > mPhysicalAddressBits = CalculateMaximumSupportAddress (); > + PatchInstructionX86 (gPatch5LevelPagingSupport, m5LevelPagingSupport, 1); > DEBUG ((DEBUG_INFO, "5LevelPaging Support - %d\n", > m5LevelPagingSupport)); > DEBUG ((DEBUG_INFO, "1GPageTable Support - %d\n", > m1GPageTableSupport)); > DEBUG ((DEBUG_INFO, "PcdCpuSmmStaticPageTable - %d\n", > mCpuSmmStaticPageTable)); With this update, the build succeeds, and a quick regression-test has passed for me (using OVMF/IA32X64). I'll try to do deeper testing if you agree with this update. Thanks, Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43125): https://edk2.groups.io/g/devel/message/43125 Mute This Topic: https://groups.io/mt/32239521/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
