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, 26 Aug 2019 10:12:57 -0700 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BF7D68CF1AE; Mon, 26 Aug 2019 17:12:56 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-34.ams2.redhat.com [10.36.117.34]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5AD85450F; Mon, 26 Aug 2019 17:12:55 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 2/5] UefiCpuPkg/PiSmmCpu: Use new PCD PcdCpuSmmRestrictedMemoryAccess To: devel@edk2.groups.io, ray.ni@intel.com Cc: Eric Dong , Jiewen Yao References: <20190825224513.171572-1-ray.ni@intel.com> <20190825224513.171572-3-ray.ni@intel.com> From: "Laszlo Ersek" Message-ID: Date: Mon, 26 Aug 2019 19:12:54 +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: <20190825224513.171572-3-ray.ni@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.69]); Mon, 26 Aug 2019 17:12:56 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/26/19 00:45, Ni, Ray wrote: > The patch changes PiSmmCpu driver to consume PCD > PcdCpuSmmRestrictedMemoryAccess. > Because the behavior controlled by PcdCpuSmmStaticPageTable in > original code is not changed after switching to > PcdCpuSmmRestrictedMemoryAccess. > > The functionality is not impacted by this patch. > > Signed-off-by: Ray Ni > Cc: Eric Dong > Cc: Jiewen Yao > Cc: Laszlo Ersek > --- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 4 +- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 52 ++++++++++++-------- > 2 files changed, 34 insertions(+), 22 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > index da0308c47f..b12b2691f8 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > @@ -133,7 +133,6 @@ [Pcd] > gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress ## SOMETIMES_PRODUCES > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmCodeAccessCheckEnable ## CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode ## CONSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable ## CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmShadowStackSize ## SOMETIMES_CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask ## CONSUMES > @@ -141,6 +140,9 @@ [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask ## CONSUMES > gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask ## CONSUMES > > +[Pcd.X64] > + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmRestrictedMemoryAccess ## CONSUMES > + > [Depex] > gEfiMpServiceProtocolGuid > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > index d60c404a3d..7516f35055 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -15,7 +15,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > LIST_ENTRY mPagePool = INITIALIZE_LIST_HEAD_VARIABLE (mPagePool); > BOOLEAN m1GPageTableSupport = FALSE; > -BOOLEAN mCpuSmmStaticPageTable; > +BOOLEAN mCpuSmmRestrictedMemoryAccess; > BOOLEAN m5LevelPagingSupport; > X86_ASSEMBLY_PATCH_LABEL gPatch5LevelPagingSupport; > > @@ -334,15 +334,15 @@ SmmInitPageTable ( > // > InitializeSpinLock (mPFLock); > > - mCpuSmmStaticPageTable = PcdGetBool (PcdCpuSmmStaticPageTable); > - m1GPageTableSupport = Is1GPageSupport (); > - m5LevelPagingSupport = Is5LevelPagingSupport (); > - mPhysicalAddressBits = CalculateMaximumSupportAddress (); > + mCpuSmmRestrictedMemoryAccess = PcdGetBool (PcdCpuSmmRestrictedMemoryAccess); > + 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)); > - DEBUG ((DEBUG_INFO, "PhysicalAddressBits - %d\n", mPhysicalAddressBits)); > + DEBUG ((DEBUG_INFO, "5LevelPaging Support - %d\n", m5LevelPagingSupport)); > + DEBUG ((DEBUG_INFO, "1GPageTable Support - %d\n", m1GPageTableSupport)); > + DEBUG ((DEBUG_INFO, "PcdCpuSmmRestrictedMemoryAccess - %d\n", mCpuSmmRestrictedMemoryAccess)); > + DEBUG ((DEBUG_INFO, "PhysicalAddressBits - %d\n", mPhysicalAddressBits)); > // > // Generate PAE page table for the first 4GB memory space > // > @@ -385,7 +385,11 @@ SmmInitPageTable ( > PTEntry = Pml5Entry; > } > > - if (mCpuSmmStaticPageTable) { > + if (mCpuSmmRestrictedMemoryAccess) { > + // > + // When access to non-SMRAM memory is restricted, create page table > + // that covers all memory space. > + // > SetStaticPageTable ((UINTN)PTEntry); > } else { > // > @@ -972,7 +976,7 @@ SmiPFHandler ( > > PFAddress = AsmReadCr2 (); > > - if (mCpuSmmStaticPageTable && (PFAddress >= LShiftU64 (1, (mPhysicalAddressBits - 1)))) { > + if (mCpuSmmRestrictedMemoryAccess && (PFAddress >= LShiftU64 (1, (mPhysicalAddressBits - 1)))) { > DumpCpuContext (InterruptType, SystemContext); > DEBUG ((DEBUG_ERROR, "Do not support address 0x%lx by processor!\n", PFAddress)); > CpuDeadLoop (); > @@ -1049,7 +1053,7 @@ SmiPFHandler ( > goto Exit; > } > > - if (mCpuSmmStaticPageTable && IsSmmCommBufferForbiddenAddress (PFAddress)) { > + if (mCpuSmmRestrictedMemoryAccess && IsSmmCommBufferForbiddenAddress (PFAddress)) { > DumpCpuContext (InterruptType, SystemContext); > DEBUG ((DEBUG_ERROR, "Access SMM communication forbidden address (0x%lx)!\n", PFAddress)); > DEBUG_CODE ( > @@ -1100,26 +1104,26 @@ SetPageTableAttributes ( > Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1); > > // > - // Don't do this if > - // - no static page table; or > + // Don't mark page table memory as read-only if > + // - no restriction on access to non-SMRAM memory; or > // - SMM heap guard feature enabled; or > // BIT2: SMM page guard enabled > // BIT3: SMM pool guard enabled > // - SMM profile feature enabled > // > - if (!mCpuSmmStaticPageTable || > + if (!mCpuSmmRestrictedMemoryAccess || > ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT3 | BIT2)) != 0) || > FeaturePcdGet (PcdCpuSmmProfileEnable)) { > // > - // Static paging and heap guard could not be enabled at the same time. > + // Restriction on access to non-SMRAM memory and heap guard could not be enabled at the same time. > // > - ASSERT (!(mCpuSmmStaticPageTable && > + ASSERT (!(mCpuSmmRestrictedMemoryAccess && > (PcdGet8 (PcdHeapGuardPropertyMask) & (BIT3 | BIT2)) != 0)); > > // > - // Static paging and SMM profile could not be enabled at the same time. > + // Restriction on access to non-SMRAM memory and SMM profile could not be enabled at the same time. > // > - ASSERT (!(mCpuSmmStaticPageTable && FeaturePcdGet (PcdCpuSmmProfileEnable))); > + ASSERT (!(mCpuSmmRestrictedMemoryAccess && FeaturePcdGet (PcdCpuSmmProfileEnable))); > return ; > } > > @@ -1223,7 +1227,10 @@ SaveCr2 ( > OUT UINTN *Cr2 > ) > { > - if (!mCpuSmmStaticPageTable) { > + if (!mCpuSmmRestrictedMemoryAccess) { > + // > + // On-demand paging is enabled when access to non-SMRAM is not restricted. > + // > *Cr2 = AsmReadCr2 (); > } > } > @@ -1238,7 +1245,10 @@ RestoreCr2 ( > IN UINTN Cr2 > ) > { > - if (!mCpuSmmStaticPageTable) { > + if (!mCpuSmmRestrictedMemoryAccess) { > + // > + // On-demand paging is enabled when access to non-SMRAM is not restricted. > + // > AsmWriteCr2 (Cr2); > } > } > Reviewed-by: Laszlo Ersek