Re: [PATCH 2/5] UefiCpuPkg/PiSmmCpu: Use new PCD PcdCpuSmmRestrictedMemoryAccess

["Dong, Eric" <eric.dong@intel.com>]

Reviewed-by: Eric Dong <eric.dong@intel.com>

> -----Original Message-----
> From: Ni, Ray
> Sent: Monday, August 26, 2019 6:45 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH 2/5] UefiCpuPkg/PiSmmCpu: Use new PCD
> PcdCpuSmmRestrictedMemoryAccess
> 
> 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 <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  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);
>    }
>  }
> --
> 2.21.0.windows.1