Laszlo, I agree with your changes. Will include your changes in V2. > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek > Sent: Monday, July 1, 2019 8:45 PM > To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com> > Cc: Dong, Eric <eric.d...@intel.com> > Subject: Re: [edk2-devel] [PATCH 3/3] UefiCpuPkg/PiSmmCpu: Enable 5 level > paging when CPU supports > > 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 (#43166): https://edk2.groups.io/g/devel/message/43166 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] -=-=-=-=-=-=-=-=-=-=-=-
