From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 01 Jul 2019 05:45:06 -0700 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6576081DE5; Mon, 1 Jul 2019 12:44:51 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (unknown [10.36.118.14]) by smtp.corp.redhat.com (Postfix) with ESMTP id 42D341B4B0; Mon, 1 Jul 2019 12:44:48 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 3/3] UefiCpuPkg/PiSmmCpu: Enable 5 level paging when CPU supports To: devel@edk2.groups.io, ray.ni@intel.com Cc: Eric Dong References: <20190628064700.549472-1-ray.ni@intel.com> <20190628064700.549472-4-ray.ni@intel.com> From: "Laszlo Ersek" Message-ID: Date: Mon, 1 Jul 2019 14:44:47 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190628064700.549472-4-ray.ni@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Mon, 01 Jul 2019 12:45:00 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Eric Dong > --- > .../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