[PATCH 0/5] restrict memory access per PcdCpuSmmRestrictedMemoryAccess

[PATCH 0/5] restrict memory access per PcdCpuSmmRestrictedMemoryAccess

[Ni, Ray]

NOTE: #5/5 will be pushed in 2nd phase after known close-source code in another
repo is updated to use the new PCD PcdCpuSmmRestrictedMemoryAccess.

Ray Ni (5):
  UefiCpuPkg: Add PcdCpuSmmRestrictedMemoryAccess
  UefiCpuPkg/PiSmmCpu: Use new PCD PcdCpuSmmRestrictedMemoryAccess
  UefiCpuPkg/PiSmmCpu: Restrict access per
    PcdCpuSmmRestrictedMemoryAccess
  UefiCpuPkg: Explain relationship between several SMM PCDs
  UefiCpuPkg: Remove PcdCpuSmmStaticPageTable

 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c     | 14 +++++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c   | 18 +++---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   | 11 ++++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |  4 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c      | 66 +++++++++++++-------
 UefiCpuPkg/UefiCpuPkg.dec                    | 31 +++++----
 6 files changed, 102 insertions(+), 42 deletions(-)

-- 
2.21.0.windows.1

[PATCH 1/5] UefiCpuPkg: Add PcdCpuSmmRestrictedMemoryAccess

[Ni, Ray]

The patch adds a new X64 only PCD PcdCpuSmmRestrictedMemoryAccess.
The PCD indicates access to non-SMRAM memory is restricted to
reserved, runtime and ACPI NVS type after SmmReadyToLock.
MMIO access is always allowed regardless of the value of this PCD.
Loose of such restriction is only required by RAS components in X64
platforms.
The PCD value is considered as constantly TRUE in IA32 platforms.
When the PCD value is TRUE, page table is initialized to cover all
memory spaces and the memory occupied by page table is protected by
page table itself as read-only.

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/UefiCpuPkg.dec | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 86ad61f64b..83acd33612 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -278,6 +278,18 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   # @Prompt Current boot is a power-on reset.
   gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset|FALSE|BOOLEAN|0x0000001B
 
+[PcdsFixedAtBuild.X64, PcdsPatchableInModule.X64, PcdsDynamic.X64, PcdsDynamicEx.X64]
+  ## Indicate access to non-SMRAM memory is restricted to reserved, runtime and ACPI NVS type after SmmReadyToLock.
+  #  MMIO access is always allowed regardless of the value of this PCD.
+  #  Loose of such restriction is only required by RAS components in X64 platforms.
+  #  The PCD value is considered as constantly TRUE in IA32 platforms.
+  #  When the PCD value is TRUE, page table is initialized to cover all memory spaces
+  #  and the memory occupied by page table is protected by page table itself as read-only.
+  #   TRUE  - Access to non-SMRAM memory is restricted to reserved, runtime and ACPI NVS type after SmmReadyToLock.<BR>
+  #   FALSE - Access to any type of non-SMRAM memory after SmmReadyToLock is allowed.<BR>
+  # @Prompt Access to non-SMRAM memory is restricted to reserved, runtime and ACPI NVS type after SmmReadyToLock.
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmRestrictedMemoryAccess|TRUE|BOOLEAN|0x3213210F
+
 [PcdsDynamic, PcdsDynamicEx]
   ## Contains the pointer to a CPU S3 data buffer of structure ACPI_CPU_DATA.
   # @Prompt The pointer to a CPU S3 data buffer.
-- 
2.21.0.windows.1

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

[Ni, Ray]

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

[PATCH 3/5] UefiCpuPkg/PiSmmCpu: Restrict access per PcdCpuSmmRestrictedMemoryAccess

[Ni, Ray]

Today's behavior is to always restrict access to non-SMRAM regardless
the value of PcdCpuSmmRestrictedMemoryAccess.

Because RAS components require to access all non-SMRAM memory, the
patch changes the code logic to honor PcdCpuSmmRestrictedMemoryAccess
so that only when the PCD is true, the restriction takes affect and
page table memory is also protected.

Because IA32 build doesn't reference this PCD, such restriction
always takes affect in IA32 build.

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/Ia32/PageTbl.c   | 14 ++++++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 18 ++++++++++--------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 11 +++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c    | 14 ++++++++++++++
 4 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index 05fb455936..f891a81112 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -336,3 +336,17 @@ RestoreCr2 (
 {
   return ;
 }
+
+/**
+  Return whether access to non-SMRAM is restricted.
+
+  @retval TRUE  Access to non-SMRAM is restricted.
+  @retval FALSE Access to non-SMRAM is not restricted.
+*/
+BOOLEAN
+IsRestrictedMemoryAccess (
+  VOID
+  )
+{
+  return TRUE;
+}
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index 69a04dfb23..723fd5042f 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -1431,15 +1431,17 @@ PerformRemainingTasks (
     //
     SetMemMapAttributes ();
 
-    //
-    // For outside SMRAM, we only map SMM communication buffer or MMIO.
-    //
-    SetUefiMemMapAttributes ();
+    if (IsRestrictedMemoryAccess ()) {
+      //
+      // For outside SMRAM, we only map SMM communication buffer or MMIO.
+      //
+      SetUefiMemMapAttributes ();
 
-    //
-    // Set page table itself to be read-only
-    //
-    SetPageTableAttributes ();
+      //
+      // Set page table itself to be read-only
+      //
+      SetPageTableAttributes ();
+    }
 
     //
     // Configure SMM Code Access Check feature if available.
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 8c29f1a558..daf977f654 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -1450,4 +1450,15 @@ InitializeDataForMmMp (
   VOID
   );
 
+/**
+  Return whether access to non-SMRAM is restricted.
+
+  @retval TRUE  Access to non-SMRAM is restricted.
+  @retval FALSE Access to non-SMRAM is not restricted.
+*/
+BOOLEAN
+IsRestrictedMemoryAccess (
+  VOID
+  );
+
 #endif
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index 7516f35055..733d107efd 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -1252,3 +1252,17 @@ RestoreCr2 (
     AsmWriteCr2 (Cr2);
   }
 }
+
+/**
+  Return whether access to non-SMRAM is restricted.
+
+  @retval TRUE  Access to non-SMRAM is restricted.
+  @retval FALSE Access to non-SMRAM is not restricted.
+*/
+BOOLEAN
+IsRestrictedMemoryAccess (
+  VOID
+  )
+{
+  return mCpuSmmRestrictedMemoryAccess;
+}
-- 
2.21.0.windows.1

[PATCH 4/5] UefiCpuPkg: Explain relationship between several SMM PCDs

[Ni, Ray]

There are three PCDs that may impact the behavior of each other in
SMM environment:
  PcdCpuSmmProfileEnable
  PcdHeapGuardPropertyMask in MdeModulePkg
  PcdCpuSmmRestrictedMemoryAccess

The patch updates the comments in DEC file to document it.

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/UefiCpuPkg.dec | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 83acd33612..9a03bdd716 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -81,7 +81,8 @@ [Ppis]
 [PcdsFeatureFlag]
   ## Indicates if SMM Profile will be enabled.
   #  If enabled, instruction executions in and data accesses to memory outside of SMRAM will be logged.
-  #  It could not be enabled at the same time with SMM static page table feature (PcdCpuSmmStaticPageTable).
+  #  In X64 build, it could not be enabled when PcdCpuSmmRestrictedMemoryAccess is TRUE.
+  #  In IA32 build, the page table memory is not marked as read-only when it is enabled.
   #  This PCD is only for validation purpose. It should be set to false in production.<BR><BR>
   #   TRUE  - SMM Profile will be enabled.<BR>
   #   FALSE - SMM Profile will be disabled.<BR>
@@ -285,6 +286,11 @@ [PcdsFixedAtBuild.X64, PcdsPatchableInModule.X64, PcdsDynamic.X64, PcdsDynamicEx
   #  The PCD value is considered as constantly TRUE in IA32 platforms.
   #  When the PCD value is TRUE, page table is initialized to cover all memory spaces
   #  and the memory occupied by page table is protected by page table itself as read-only.
+  #  In X64 build, it cannot be enabled at the same time with SMM profile feature (PcdCpuSmmProfileEnable).
+  #  In X64 build, it could not be enabled also at the same time with heap guard feature for SMM
+  #  (PcdHeapGuardPropertyMask in MdeModulePkg).
+  #  In IA32 build, page table memory is not marked as read-only when either SMM profile feature (PcdCpuSmmProfileEnable)
+  #  or heap guard feature for SMM (PcdHeapGuardPropertyMask in MdeModulePkg) is enabled.
   #   TRUE  - Access to non-SMRAM memory is restricted to reserved, runtime and ACPI NVS type after SmmReadyToLock.<BR>
   #   FALSE - Access to any type of non-SMRAM memory after SmmReadyToLock is allowed.<BR>
   # @Prompt Access to non-SMRAM memory is restricted to reserved, runtime and ACPI NVS type after SmmReadyToLock.
-- 
2.21.0.windows.1

[PATCH 5/5] UefiCpuPkg: Remove PcdCpuSmmStaticPageTable

[Ni, Ray]

PcdCpuSmmRestrictedMemoryAccess is introduced to replace
PcdCpuSmmStaticPageTable.

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/UefiCpuPkg.dec | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 9a03bdd716..031a2ccd68 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -247,17 +247,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   # @Prompt The specified AP target C-state for Mwait.
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate|0|UINT8|0x00000007
 
-  ## Indicates if SMM uses static page table.
-  #  If enabled, SMM will not use on-demand paging. SMM will build static page table for all memory.
-  #  This flag only impacts X64 build, because SMM always builds static page table for IA32.
-  #  It could not be enabled at the same time with SMM profile feature (PcdCpuSmmProfileEnable).
-  #  It could not be enabled also at the same time with heap guard feature for SMM
-  #  (PcdHeapGuardPropertyMask in MdeModulePkg).<BR><BR>
-  #   TRUE  - SMM uses static page table for all memory.<BR>
-  #   FALSE - SMM uses static page table for below 4G memory and use on-demand paging for above 4G memory.<BR>
-  # @Prompt Use static page table for all memory in SMM.
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable|TRUE|BOOLEAN|0x3213210D
-
   ## Specifies timeout value in microseconds for the BSP in SMM to wait for all APs to come into SMM.
   # @Prompt AP synchronization timeout value in SMM.
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|1000000|UINT64|0x32132104
-- 
2.21.0.windows.1

Re: [edk2-devel] [PATCH 1/5] UefiCpuPkg: Add PcdCpuSmmRestrictedMemoryAccess

[Laszlo Ersek]

On 08/26/19 00:45, Ni, Ray wrote:
> The patch adds a new X64 only PCD PcdCpuSmmRestrictedMemoryAccess.
> The PCD indicates access to non-SMRAM memory is restricted to
> reserved, runtime and ACPI NVS type after SmmReadyToLock.
> MMIO access is always allowed regardless of the value of this PCD.
> Loose of such restriction is only required by RAS components in X64
> platforms.
> The PCD value is considered as constantly TRUE in IA32 platforms.
> When the PCD value is TRUE, page table is initialized to cover all
> memory spaces and the memory occupied by page table is protected by
> page table itself as read-only.
> 
> 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/UefiCpuPkg.dec | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index 86ad61f64b..83acd33612 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -278,6 +278,18 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>    # @Prompt Current boot is a power-on reset.
>    gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset|FALSE|BOOLEAN|0x0000001B
>  
> +[PcdsFixedAtBuild.X64, PcdsPatchableInModule.X64, PcdsDynamic.X64, PcdsDynamicEx.X64]
> +  ## Indicate access to non-SMRAM memory is restricted to reserved, runtime and ACPI NVS type after SmmReadyToLock.
> +  #  MMIO access is always allowed regardless of the value of this PCD.
> +  #  Loose of such restriction is only required by RAS components in X64 platforms.
> +  #  The PCD value is considered as constantly TRUE in IA32 platforms.
> +  #  When the PCD value is TRUE, page table is initialized to cover all memory spaces
> +  #  and the memory occupied by page table is protected by page table itself as read-only.
> +  #   TRUE  - Access to non-SMRAM memory is restricted to reserved, runtime and ACPI NVS type after SmmReadyToLock.<BR>
> +  #   FALSE - Access to any type of non-SMRAM memory after SmmReadyToLock is allowed.<BR>
> +  # @Prompt Access to non-SMRAM memory is restricted to reserved, runtime and ACPI NVS type after SmmReadyToLock.
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmRestrictedMemoryAccess|TRUE|BOOLEAN|0x3213210F
> +
>  [PcdsDynamic, PcdsDynamicEx]
>    ## Contains the pointer to a CPU S3 data buffer of structure ACPI_CPU_DATA.
>    # @Prompt The pointer to a CPU S3 data buffer.
> 

Please consider updating the UNI file as well.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

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

[Laszlo Ersek]

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 <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);
>    }
>  }
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [edk2-devel] [PATCH 3/5] UefiCpuPkg/PiSmmCpu: Restrict access per PcdCpuSmmRestrictedMemoryAccess

[Laszlo Ersek]

On 08/26/19 00:45, Ni, Ray wrote:
> Today's behavior is to always restrict access to non-SMRAM regardless
> the value of PcdCpuSmmRestrictedMemoryAccess.
> 
> Because RAS components require to access all non-SMRAM memory, the
> patch changes the code logic to honor PcdCpuSmmRestrictedMemoryAccess
> so that only when the PCD is true, the restriction takes affect and
> page table memory is also protected.
> 
> Because IA32 build doesn't reference this PCD, such restriction
> always takes affect in IA32 build.
> 
> 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/Ia32/PageTbl.c   | 14 ++++++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 18 ++++++++++--------
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 11 +++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c    | 14 ++++++++++++++
>  4 files changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index 05fb455936..f891a81112 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -336,3 +336,17 @@ RestoreCr2 (
>  {
>    return ;
>  }
> +
> +/**
> +  Return whether access to non-SMRAM is restricted.
> +
> +  @retval TRUE  Access to non-SMRAM is restricted.
> +  @retval FALSE Access to non-SMRAM is not restricted.
> +*/
> +BOOLEAN
> +IsRestrictedMemoryAccess (
> +  VOID
> +  )
> +{
> +  return TRUE;
> +}
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index 69a04dfb23..723fd5042f 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -1431,15 +1431,17 @@ PerformRemainingTasks (
>      //
>      SetMemMapAttributes ();
>  
> -    //
> -    // For outside SMRAM, we only map SMM communication buffer or MMIO.
> -    //
> -    SetUefiMemMapAttributes ();
> +    if (IsRestrictedMemoryAccess ()) {
> +      //
> +      // For outside SMRAM, we only map SMM communication buffer or MMIO.
> +      //
> +      SetUefiMemMapAttributes ();
>  
> -    //
> -    // Set page table itself to be read-only
> -    //
> -    SetPageTableAttributes ();
> +      //
> +      // Set page table itself to be read-only
> +      //
> +      SetPageTableAttributes ();
> +    }
>  
>      //
>      // Configure SMM Code Access Check feature if available.
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 8c29f1a558..daf977f654 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1450,4 +1450,15 @@ InitializeDataForMmMp (
>    VOID
>    );
>  
> +/**
> +  Return whether access to non-SMRAM is restricted.
> +
> +  @retval TRUE  Access to non-SMRAM is restricted.
> +  @retval FALSE Access to non-SMRAM is not restricted.
> +*/
> +BOOLEAN
> +IsRestrictedMemoryAccess (
> +  VOID
> +  );
> +
>  #endif
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 7516f35055..733d107efd 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -1252,3 +1252,17 @@ RestoreCr2 (
>      AsmWriteCr2 (Cr2);
>    }
>  }
> +
> +/**
> +  Return whether access to non-SMRAM is restricted.
> +
> +  @retval TRUE  Access to non-SMRAM is restricted.
> +  @retval FALSE Access to non-SMRAM is not restricted.
> +*/
> +BOOLEAN
> +IsRestrictedMemoryAccess (
> +  VOID
> +  )
> +{
> +  return mCpuSmmRestrictedMemoryAccess;
> +}
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [edk2-devel] [PATCH 4/5] UefiCpuPkg: Explain relationship between several SMM PCDs

[Laszlo Ersek]

On 08/26/19 00:45, Ni, Ray wrote:
> There are three PCDs that may impact the behavior of each other in
> SMM environment:
>   PcdCpuSmmProfileEnable
>   PcdHeapGuardPropertyMask in MdeModulePkg
>   PcdCpuSmmRestrictedMemoryAccess
> 
> The patch updates the comments in DEC file to document it.
> 
> 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/UefiCpuPkg.dec | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index 83acd33612..9a03bdd716 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -81,7 +81,8 @@ [Ppis]
>  [PcdsFeatureFlag]
>    ## Indicates if SMM Profile will be enabled.
>    #  If enabled, instruction executions in and data accesses to memory outside of SMRAM will be logged.
> -  #  It could not be enabled at the same time with SMM static page table feature (PcdCpuSmmStaticPageTable).
> +  #  In X64 build, it could not be enabled when PcdCpuSmmRestrictedMemoryAccess is TRUE.
> +  #  In IA32 build, the page table memory is not marked as read-only when it is enabled.
>    #  This PCD is only for validation purpose. It should be set to false in production.<BR><BR>
>    #   TRUE  - SMM Profile will be enabled.<BR>
>    #   FALSE - SMM Profile will be disabled.<BR>
> @@ -285,6 +286,11 @@ [PcdsFixedAtBuild.X64, PcdsPatchableInModule.X64, PcdsDynamic.X64, PcdsDynamicEx
>    #  The PCD value is considered as constantly TRUE in IA32 platforms.
>    #  When the PCD value is TRUE, page table is initialized to cover all memory spaces
>    #  and the memory occupied by page table is protected by page table itself as read-only.
> +  #  In X64 build, it cannot be enabled at the same time with SMM profile feature (PcdCpuSmmProfileEnable).
> +  #  In X64 build, it could not be enabled also at the same time with heap guard feature for SMM
> +  #  (PcdHeapGuardPropertyMask in MdeModulePkg).
> +  #  In IA32 build, page table memory is not marked as read-only when either SMM profile feature (PcdCpuSmmProfileEnable)
> +  #  or heap guard feature for SMM (PcdHeapGuardPropertyMask in MdeModulePkg) is enabled.
>    #   TRUE  - Access to non-SMRAM memory is restricted to reserved, runtime and ACPI NVS type after SmmReadyToLock.<BR>
>    #   FALSE - Access to any type of non-SMRAM memory after SmmReadyToLock is allowed.<BR>
>    # @Prompt Access to non-SMRAM memory is restricted to reserved, runtime and ACPI NVS type after SmmReadyToLock.
> 

Please consider updating the UNI file as well.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [edk2-devel] [PATCH 5/5] UefiCpuPkg: Remove PcdCpuSmmStaticPageTable

[Laszlo Ersek]

On 08/26/19 00:45, Ni, Ray wrote:
> PcdCpuSmmRestrictedMemoryAccess is introduced to replace
> PcdCpuSmmStaticPageTable.
> 
> 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/UefiCpuPkg.dec | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index 9a03bdd716..031a2ccd68 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -247,17 +247,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>    # @Prompt The specified AP target C-state for Mwait.
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate|0|UINT8|0x00000007
>  
> -  ## Indicates if SMM uses static page table.
> -  #  If enabled, SMM will not use on-demand paging. SMM will build static page table for all memory.
> -  #  This flag only impacts X64 build, because SMM always builds static page table for IA32.
> -  #  It could not be enabled at the same time with SMM profile feature (PcdCpuSmmProfileEnable).
> -  #  It could not be enabled also at the same time with heap guard feature for SMM
> -  #  (PcdHeapGuardPropertyMask in MdeModulePkg).<BR><BR>
> -  #   TRUE  - SMM uses static page table for all memory.<BR>
> -  #   FALSE - SMM uses static page table for below 4G memory and use on-demand paging for above 4G memory.<BR>
> -  # @Prompt Use static page table for all memory in SMM.
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable|TRUE|BOOLEAN|0x3213210D
> -
>    ## Specifies timeout value in microseconds for the BSP in SMM to wait for all APs to come into SMM.
>    # @Prompt AP synchronization timeout value in SMM.
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|1000000|UINT64|0x32132104
> 

Please update the UNI file as well.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 1/5] UefiCpuPkg: Add PcdCpuSmmRestrictedMemoryAccess

[Dong, Eric]

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 1/5] UefiCpuPkg: Add PcdCpuSmmRestrictedMemoryAccess
> 
> The patch adds a new X64 only PCD PcdCpuSmmRestrictedMemoryAccess.
> The PCD indicates access to non-SMRAM memory is restricted to reserved,
> runtime and ACPI NVS type after SmmReadyToLock.
> MMIO access is always allowed regardless of the value of this PCD.
> Loose of such restriction is only required by RAS components in X64 platforms.
> The PCD value is considered as constantly TRUE in IA32 platforms.
> When the PCD value is TRUE, page table is initialized to cover all memory
> spaces and the memory occupied by page table is protected by page table
> itself as read-only.
> 
> 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/UefiCpuPkg.dec | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec index
> 86ad61f64b..83acd33612 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -278,6 +278,18 @@ [PcdsFixedAtBuild, PcdsPatchableInModule,
> PcdsDynamic, PcdsDynamicEx]
>    # @Prompt Current boot is a power-on reset.
> 
> gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset|FALSE|BOOLEAN|0x000000
> 1B
> 
> +[PcdsFixedAtBuild.X64, PcdsPatchableInModule.X64, PcdsDynamic.X64,
> +PcdsDynamicEx.X64]
> +  ## Indicate access to non-SMRAM memory is restricted to reserved,
> runtime and ACPI NVS type after SmmReadyToLock.
> +  #  MMIO access is always allowed regardless of the value of this PCD.
> +  #  Loose of such restriction is only required by RAS components in X64
> platforms.
> +  #  The PCD value is considered as constantly TRUE in IA32 platforms.
> +  #  When the PCD value is TRUE, page table is initialized to cover all
> +memory spaces
> +  #  and the memory occupied by page table is protected by page table itself
> as read-only.
> +  #   TRUE  - Access to non-SMRAM memory is restricted to reserved, runtime
> and ACPI NVS type after SmmReadyToLock.<BR>
> +  #   FALSE - Access to any type of non-SMRAM memory after
> SmmReadyToLock is allowed.<BR>
> +  # @Prompt Access to non-SMRAM memory is restricted to reserved,
> runtime and ACPI NVS type after SmmReadyToLock.
> +
> +gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmRestrictedMemoryAccess|TRUE|B
> OOLEAN|
> +0x3213210F
> +
>  [PcdsDynamic, PcdsDynamicEx]
>    ## Contains the pointer to a CPU S3 data buffer of structure ACPI_CPU_DATA.
>    # @Prompt The pointer to a CPU S3 data buffer.
> --
> 2.21.0.windows.1

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

[Dong, Eric]

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

Re: [PATCH 3/5] UefiCpuPkg/PiSmmCpu: Restrict access per PcdCpuSmmRestrictedMemoryAccess

[Dong, Eric]

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 3/5] UefiCpuPkg/PiSmmCpu: Restrict access per
> PcdCpuSmmRestrictedMemoryAccess
> 
> Today's behavior is to always restrict access to non-SMRAM regardless the
> value of PcdCpuSmmRestrictedMemoryAccess.
> 
> Because RAS components require to access all non-SMRAM memory, the
> patch changes the code logic to honor PcdCpuSmmRestrictedMemoryAccess
> so that only when the PCD is true, the restriction takes affect and page table
> memory is also protected.
> 
> Because IA32 build doesn't reference this PCD, such restriction always takes
> affect in IA32 build.
> 
> 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/Ia32/PageTbl.c   | 14 ++++++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 18 ++++++++++--------
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 11 +++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c    | 14 ++++++++++++++
>  4 files changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index 05fb455936..f891a81112 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -336,3 +336,17 @@ RestoreCr2 (
>  {
>    return ;
>  }
> +
> +/**
> +  Return whether access to non-SMRAM is restricted.
> +
> +  @retval TRUE  Access to non-SMRAM is restricted.
> +  @retval FALSE Access to non-SMRAM is not restricted.
> +*/
> +BOOLEAN
> +IsRestrictedMemoryAccess (
> +  VOID
> +  )
> +{
> +  return TRUE;
> +}
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index 69a04dfb23..723fd5042f 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -1431,15 +1431,17 @@ PerformRemainingTasks (
>      //
>      SetMemMapAttributes ();
> 
> -    //
> -    // For outside SMRAM, we only map SMM communication buffer or MMIO.
> -    //
> -    SetUefiMemMapAttributes ();
> +    if (IsRestrictedMemoryAccess ()) {
> +      //
> +      // For outside SMRAM, we only map SMM communication buffer or MMIO.
> +      //
> +      SetUefiMemMapAttributes ();
> 
> -    //
> -    // Set page table itself to be read-only
> -    //
> -    SetPageTableAttributes ();
> +      //
> +      // Set page table itself to be read-only
> +      //
> +      SetPageTableAttributes ();
> +    }
> 
>      //
>      // Configure SMM Code Access Check feature if available.
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 8c29f1a558..daf977f654 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1450,4 +1450,15 @@ InitializeDataForMmMp (
>    VOID
>    );
> 
> +/**
> +  Return whether access to non-SMRAM is restricted.
> +
> +  @retval TRUE  Access to non-SMRAM is restricted.
> +  @retval FALSE Access to non-SMRAM is not restricted.
> +*/
> +BOOLEAN
> +IsRestrictedMemoryAccess (
> +  VOID
> +  );
> +
>  #endif
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 7516f35055..733d107efd 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -1252,3 +1252,17 @@ RestoreCr2 (
>      AsmWriteCr2 (Cr2);
>    }
>  }
> +
> +/**
> +  Return whether access to non-SMRAM is restricted.
> +
> +  @retval TRUE  Access to non-SMRAM is restricted.
> +  @retval FALSE Access to non-SMRAM is not restricted.
> +*/
> +BOOLEAN
> +IsRestrictedMemoryAccess (
> +  VOID
> +  )
> +{
> +  return mCpuSmmRestrictedMemoryAccess; }
> --
> 2.21.0.windows.1

Re: [PATCH 4/5] UefiCpuPkg: Explain relationship between several SMM PCDs

[Dong, Eric]

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 4/5] UefiCpuPkg: Explain relationship between several SMM
> PCDs
> 
> There are three PCDs that may impact the behavior of each other in SMM
> environment:
>   PcdCpuSmmProfileEnable
>   PcdHeapGuardPropertyMask in MdeModulePkg
>   PcdCpuSmmRestrictedMemoryAccess
> 
> The patch updates the comments in DEC file to document it.
> 
> 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/UefiCpuPkg.dec | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec index
> 83acd33612..9a03bdd716 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -81,7 +81,8 @@ [Ppis]
>  [PcdsFeatureFlag]
>    ## Indicates if SMM Profile will be enabled.
>    #  If enabled, instruction executions in and data accesses to memory outside
> of SMRAM will be logged.
> -  #  It could not be enabled at the same time with SMM static page table
> feature (PcdCpuSmmStaticPageTable).
> +  #  In X64 build, it could not be enabled when
> PcdCpuSmmRestrictedMemoryAccess is TRUE.
> +  #  In IA32 build, the page table memory is not marked as read-only when it is
> enabled.
>    #  This PCD is only for validation purpose. It should be set to false in
> production.<BR><BR>
>    #   TRUE  - SMM Profile will be enabled.<BR>
>    #   FALSE - SMM Profile will be disabled.<BR>
> @@ -285,6 +286,11 @@ [PcdsFixedAtBuild.X64, PcdsPatchableInModule.X64,
> PcdsDynamic.X64, PcdsDynamicEx
>    #  The PCD value is considered as constantly TRUE in IA32 platforms.
>    #  When the PCD value is TRUE, page table is initialized to cover all memory
> spaces
>    #  and the memory occupied by page table is protected by page table itself as
> read-only.
> +  #  In X64 build, it cannot be enabled at the same time with SMM profile
> feature (PcdCpuSmmProfileEnable).
> +  #  In X64 build, it could not be enabled also at the same time with
> + heap guard feature for SMM  #  (PcdHeapGuardPropertyMask in
> MdeModulePkg).
> +  #  In IA32 build, page table memory is not marked as read-only when
> + either SMM profile feature (PcdCpuSmmProfileEnable)  #  or heap guard
> feature for SMM (PcdHeapGuardPropertyMask in MdeModulePkg) is enabled.
>    #   TRUE  - Access to non-SMRAM memory is restricted to reserved, runtime
> and ACPI NVS type after SmmReadyToLock.<BR>
>    #   FALSE - Access to any type of non-SMRAM memory after
> SmmReadyToLock is allowed.<BR>
>    # @Prompt Access to non-SMRAM memory is restricted to reserved, runtime
> and ACPI NVS type after SmmReadyToLock.
> --
> 2.21.0.windows.1

Re: [PATCH 5/5] UefiCpuPkg: Remove PcdCpuSmmStaticPageTable

[Dong, Eric]

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 5/5] UefiCpuPkg: Remove PcdCpuSmmStaticPageTable
> 
> PcdCpuSmmRestrictedMemoryAccess is introduced to replace
> PcdCpuSmmStaticPageTable.
> 
> 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/UefiCpuPkg.dec | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec index
> 9a03bdd716..031a2ccd68 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -247,17 +247,6 @@ [PcdsFixedAtBuild, PcdsPatchableInModule,
> PcdsDynamic, PcdsDynamicEx]
>    # @Prompt The specified AP target C-state for Mwait.
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate|0|UINT8|0x00000007
> 
> -  ## Indicates if SMM uses static page table.
> -  #  If enabled, SMM will not use on-demand paging. SMM will build static page
> table for all memory.
> -  #  This flag only impacts X64 build, because SMM always builds static page
> table for IA32.
> -  #  It could not be enabled at the same time with SMM profile feature
> (PcdCpuSmmProfileEnable).
> -  #  It could not be enabled also at the same time with heap guard feature for
> SMM
> -  #  (PcdHeapGuardPropertyMask in MdeModulePkg).<BR><BR>
> -  #   TRUE  - SMM uses static page table for all memory.<BR>
> -  #   FALSE - SMM uses static page table for below 4G memory and use on-
> demand paging for above 4G memory.<BR>
> -  # @Prompt Use static page table for all memory in SMM.
> -
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable|TRUE|BOOLEAN|0
> x3213210D
> -
>    ## Specifies timeout value in microseconds for the BSP in SMM to wait for all
> APs to come into SMM.
>    # @Prompt AP synchronization timeout value in SMM.
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|1000000|UINT64|0
> x32132104
> --
> 2.21.0.windows.1

Re: [edk2-devel] [PATCH 0/5] restrict memory access per PcdCpuSmmRestrictedMemoryAccess

[Laszlo Ersek]

Hi Ray,

On 08/26/19 00:45, Ni, Ray wrote:
> NOTE: #5/5 will be pushed in 2nd phase after known close-source code in another
> repo is updated to use the new PCD PcdCpuSmmRestrictedMemoryAccess.

you've now pushed patch 5/5 as commit 136dad095660, but you forgot to
update the UNI file, which is what I requested here:

  https://edk2.groups.io/g/devel/message/46364

Right now we have:

$ git grep -l PcdCpuSmmStaticPageTable
UefiCpuPkg/UefiCpuPkg.uni

Please send a follow-up patch for the UNI file.

Thanks,
Laszlo

> Ray Ni (5):
>   UefiCpuPkg: Add PcdCpuSmmRestrictedMemoryAccess
>   UefiCpuPkg/PiSmmCpu: Use new PCD PcdCpuSmmRestrictedMemoryAccess
>   UefiCpuPkg/PiSmmCpu: Restrict access per
>     PcdCpuSmmRestrictedMemoryAccess
>   UefiCpuPkg: Explain relationship between several SMM PCDs
>   UefiCpuPkg: Remove PcdCpuSmmStaticPageTable
> 
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c     | 14 +++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c   | 18 +++---
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   | 11 ++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |  4 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c      | 66 +++++++++++++-------
>  UefiCpuPkg/UefiCpuPkg.dec                    | 31 +++++----
>  6 files changed, 102 insertions(+), 42 deletions(-)
> 

Re: [edk2-devel] [PATCH 0/5] restrict memory access per PcdCpuSmmRestrictedMemoryAccess

[Ni, Ray]

Thanks for reminding. I will do that.
> On Sep 20, 2019, at 1:44 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> Hi Ray,
> 
>> On 08/26/19 00:45, Ni, Ray wrote:
>> NOTE: #5/5 will be pushed in 2nd phase after known close-source code in another
>> repo is updated to use the new PCD PcdCpuSmmRestrictedMemoryAccess.
> 
> you've now pushed patch 5/5 as commit 136dad095660, but you forgot to
> update the UNI file, which is what I requested here:
> 
>  https://edk2.groups.io/g/devel/message/46364
> 
> Right now we have:
> 
> $ git grep -l PcdCpuSmmStaticPageTable
> UefiCpuPkg/UefiCpuPkg.uni
> 
> Please send a follow-up patch for the UNI file.
> 
> Thanks,
> Laszlo
> 
>> Ray Ni (5):
>>  UefiCpuPkg: Add PcdCpuSmmRestrictedMemoryAccess
>>  UefiCpuPkg/PiSmmCpu: Use new PCD PcdCpuSmmRestrictedMemoryAccess
>>  UefiCpuPkg/PiSmmCpu: Restrict access per
>>    PcdCpuSmmRestrictedMemoryAccess
>>  UefiCpuPkg: Explain relationship between several SMM PCDs
>>  UefiCpuPkg: Remove PcdCpuSmmStaticPageTable
>> 
>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c     | 14 +++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c   | 18 +++---
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   | 11 ++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |  4 +-
>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c      | 66 +++++++++++++-------
>> UefiCpuPkg/UefiCpuPkg.dec                    | 31 +++++----
>> 6 files changed, 102 insertions(+), 42 deletions(-)
>> 
> 
> 
> 
>