Re: [PATCH] MdeModulePkg/PiSmmCore: AllocatePool should use MemoryType.

["Fan, Jeff" <jeff.fan@intel.com>]

/**
+  Convert a UEFI memory type to SMM pool type.
+
+  @param[in]  PoolType              Type of pool to allocate.
[Jeff] Typo. *PoolType* should be *MemoryType*.

Reviewed-by: Jeff Fan <jeff.fan@intel.com> with this typo fix.

-----Original Message-----
From: Yao, Jiewen 
Sent: Thursday, December 01, 2016 4:23 PM
To: edk2-devel@lists.01.org
Cc: Fan, Jeff; Kinney, Michael D; Laszlo Ersek
Subject: [PATCH] MdeModulePkg/PiSmmCore: AllocatePool should use MemoryType.

PiSmmCore supports page level protection based upon the Memory Type
(EfiRuntimeServicesCode/EfiRuntimeServicesData) and PE image.

However, the Memory Type information is ignored in AllocatePool().
If a caller calls AllocatePool with EfiRuntimeServicesCode, the final memory is still allocated as EfiRuntimeServicesData.

This patch supports AllocatePool with EfiRuntimeServicesCode.

Cc: Jeff Fan <jeff.fan@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
---
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.h          |  13 ++-
 MdeModulePkg/Core/PiSmmCore/Pool.c               |  66 +++++++++---
 MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c | 114 +++++++++++---------
 3 files changed, 124 insertions(+), 69 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
index e2fee54..8df1e50 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
@@ -1109,8 +1109,9 @@ extern LIST_ENTRY  mSmmMemoryMap;  #define MAX_POOL_INDEX  (MAX_POOL_SHIFT - MIN_POOL_SHIFT + 1)
 
 typedef struct {
-  UINTN        Size;
-  BOOLEAN      Available;
+  UINTN           Size;
+  BOOLEAN         Available;
+  EFI_MEMORY_TYPE Type;
 } POOL_HEADER;
 
 typedef struct {
@@ -1118,6 +1119,12 @@ typedef struct {
   LIST_ENTRY   Link;
 } FREE_POOL_HEADER;
 
-extern LIST_ENTRY  mSmmPoolLists[MAX_POOL_INDEX];
+typedef enum {
+  SmmPoolTypeCode,
+  SmmPoolTypeData,
+  SmmPoolTypeMax,
+} SMM_POOL_TYPE;
+
+extern LIST_ENTRY  mSmmPoolLists[SmmPoolTypeMax][MAX_POOL_INDEX];
 
 #endif
diff --git a/MdeModulePkg/Core/PiSmmCore/Pool.c b/MdeModulePkg/Core/PiSmmCore/Pool.c
index dcfd13e..6fb426c 100644
--- a/MdeModulePkg/Core/PiSmmCore/Pool.c
+++ b/MdeModulePkg/Core/PiSmmCore/Pool.c
@@ -14,7 +14,7 @@
 
 #include "PiSmmCore.h"
 
-LIST_ENTRY  mSmmPoolLists[MAX_POOL_INDEX];
+LIST_ENTRY  mSmmPoolLists[SmmPoolTypeMax][MAX_POOL_INDEX];
 //
 // To cache the SMRAM base since when Loading modules At fixed address feature is enabled,  // all module is assigned an offset relative the SMRAM base in build time.
@@ -22,6 +22,30 @@ LIST_ENTRY  mSmmPoolLists[MAX_POOL_INDEX];
 GLOBAL_REMOVE_IF_UNREFERENCED  EFI_PHYSICAL_ADDRESS       gLoadModuleAtFixAddressSmramBase = 0;
 
 /**
+  Convert a UEFI memory type to SMM pool type.
+
+  @param[in]  PoolType              Type of pool to allocate.
+
+  @return SMM pool type
+**/
+SMM_POOL_TYPE
+UefiMemoryTypeToSmmPoolType (
+  IN  EFI_MEMORY_TYPE   MemoryType
+  )
+{
+  ASSERT ((MemoryType == EfiRuntimeServicesCode) || (MemoryType == 
+EfiRuntimeServicesData));
+  switch (MemoryType) {
+  case EfiRuntimeServicesCode:
+    return SmmPoolTypeCode;
+  case EfiRuntimeServicesData:
+    return SmmPoolTypeData;
+  default:
+    return SmmPoolTypeMax;
+  }
+}
+
+
+/**
   Called to initialize the memory service.
 
   @param   SmramRangeCount       Number of SMRAM Regions
@@ -35,15 +59,18 @@ SmmInitializeMemoryServices (
   )
 {
   UINTN                  Index;
- 	UINT64                 SmmCodeSize;
- 	UINTN                  CurrentSmramRangesIndex;
- 	UINT64                 MaxSize;
+  UINT64                 SmmCodeSize;
+  UINTN                  CurrentSmramRangesIndex;
+  UINT64                 MaxSize;
+  UINTN                  SmmPoolTypeIndex;
 
   //
   // Initialize Pool list
   //
-  for (Index = ARRAY_SIZE (mSmmPoolLists); Index > 0;) {
-    InitializeListHead (&mSmmPoolLists[--Index]);
+  for (SmmPoolTypeIndex = 0; SmmPoolTypeIndex < SmmPoolTypeMax; SmmPoolTypeIndex++) {
+    for (Index = 0; Index < ARRAY_SIZE (mSmmPoolLists[SmmPoolTypeIndex]); Index++) {
+      InitializeListHead (&mSmmPoolLists[SmmPoolTypeIndex][Index]);
+    }
   }
   CurrentSmramRangesIndex = 0;
   //
@@ -117,6 +144,7 @@ SmmInitializeMemoryServices (
 /**
   Internal Function. Allocate a pool by specified PoolIndex.
 
+  @param  PoolType              Type of pool to allocate.
   @param  PoolIndex             Index which indicate the Pool size.
   @param  FreePoolHdr           The returned Free pool.
 
@@ -126,6 +154,7 @@ SmmInitializeMemoryServices (  **/  EFI_STATUS  InternalAllocPoolByIndex (
+  IN  EFI_MEMORY_TYPE   PoolType,
   IN  UINTN             PoolIndex,
   OUT FREE_POOL_HEADER  **FreePoolHdr
   )
@@ -133,25 +162,29 @@ InternalAllocPoolByIndex (
   EFI_STATUS            Status;
   FREE_POOL_HEADER      *Hdr;
   EFI_PHYSICAL_ADDRESS  Address;
+  SMM_POOL_TYPE         SmmPoolType;
+
+  SmmPoolType = UefiMemoryTypeToSmmPoolType(PoolType);
 
   ASSERT (PoolIndex <= MAX_POOL_INDEX);
   Status = EFI_SUCCESS;
   Hdr = NULL;
   if (PoolIndex == MAX_POOL_INDEX) {
-    Status = SmmInternalAllocatePages (AllocateAnyPages, EfiRuntimeServicesData, EFI_SIZE_TO_PAGES (MAX_POOL_SIZE << 1), &Address);
+    Status = SmmInternalAllocatePages (AllocateAnyPages, PoolType, 
+ EFI_SIZE_TO_PAGES (MAX_POOL_SIZE << 1), &Address);
     if (EFI_ERROR (Status)) {
       return EFI_OUT_OF_RESOURCES;
     }
     Hdr = (FREE_POOL_HEADER *) (UINTN) Address;
-  } else if (!IsListEmpty (&mSmmPoolLists[PoolIndex])) {
-    Hdr = BASE_CR (GetFirstNode (&mSmmPoolLists[PoolIndex]), FREE_POOL_HEADER, Link);
+  } else if (!IsListEmpty (&mSmmPoolLists[SmmPoolType][PoolIndex])) {
+    Hdr = BASE_CR (GetFirstNode 
+ (&mSmmPoolLists[SmmPoolType][PoolIndex]), FREE_POOL_HEADER, Link);
     RemoveEntryList (&Hdr->Link);
   } else {
-    Status = InternalAllocPoolByIndex (PoolIndex + 1, &Hdr);
+    Status = InternalAllocPoolByIndex (PoolType, PoolIndex + 1, &Hdr);
     if (!EFI_ERROR (Status)) {
       Hdr->Header.Size >>= 1;
       Hdr->Header.Available = TRUE;
-      InsertHeadList (&mSmmPoolLists[PoolIndex], &Hdr->Link);
+      Hdr->Header.Type = PoolType;
+      InsertHeadList (&mSmmPoolLists[SmmPoolType][PoolIndex], 
+ &Hdr->Link);
       Hdr = (FREE_POOL_HEADER*)((UINT8*)Hdr + Hdr->Header.Size);
     }
   }
@@ -159,6 +192,7 @@ InternalAllocPoolByIndex (
   if (!EFI_ERROR (Status)) {
     Hdr->Header.Size = MIN_POOL_SIZE << PoolIndex;
     Hdr->Header.Available = FALSE;
+    Hdr->Header.Type = PoolType;
   }
 
   *FreePoolHdr = Hdr;
@@ -178,16 +212,19 @@ InternalFreePoolByIndex (
   IN FREE_POOL_HEADER  *FreePoolHdr
   )
 {
-  UINTN  PoolIndex;
+  UINTN                 PoolIndex;
+  SMM_POOL_TYPE         SmmPoolType;
 
   ASSERT ((FreePoolHdr->Header.Size & (FreePoolHdr->Header.Size - 1)) == 0);
   ASSERT (((UINTN)FreePoolHdr & (FreePoolHdr->Header.Size - 1)) == 0);
   ASSERT (FreePoolHdr->Header.Size >= MIN_POOL_SIZE);
 
+  SmmPoolType = UefiMemoryTypeToSmmPoolType(FreePoolHdr->Header.Type);
+
   PoolIndex = (UINTN) (HighBitSet32 ((UINT32)FreePoolHdr->Header.Size) - MIN_POOL_SHIFT);
   FreePoolHdr->Header.Available = TRUE;
   ASSERT (PoolIndex < MAX_POOL_INDEX);
-  InsertHeadList (&mSmmPoolLists[PoolIndex], &FreePoolHdr->Link);
+  InsertHeadList (&mSmmPoolLists[SmmPoolType][PoolIndex], 
+ &FreePoolHdr->Link);
   return EFI_SUCCESS;
 }
 
@@ -234,6 +271,7 @@ SmmInternalAllocatePool (
     PoolHdr = (POOL_HEADER*)(UINTN)Address;
     PoolHdr->Size = EFI_PAGES_TO_SIZE (Size);
     PoolHdr->Available = FALSE;
+    PoolHdr->Type = PoolType;
     *Buffer = PoolHdr + 1;
     return Status;
   }
@@ -244,7 +282,7 @@ SmmInternalAllocatePool (
     PoolIndex++;
   }
 
-  Status = InternalAllocPoolByIndex (PoolIndex, &FreePoolHdr);
+  Status = InternalAllocPoolByIndex (PoolType, PoolIndex, 
+ &FreePoolHdr);
   if (!EFI_ERROR(Status)) {
     *Buffer = &FreePoolHdr->Header + 1;
   }
diff --git a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
index d983cef..dda9f12 100644
--- a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
+++ b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
@@ -1596,6 +1596,7 @@ SmramProfileGetDataSize (
   FREE_POOL_HEADER                  *Pool;
   UINTN                             PoolListIndex;
   UINTN                             Index;
+  UINTN                             SmmPoolTypeIndex;
 
   ContextData = GetSmramProfileContext ();
   if (ContextData == NULL) {
@@ -1638,19 +1639,20 @@ SmramProfileGetDataSize (
        Node = Node->BackLink) {
     Index++;
   }
-  for (PoolListIndex = 0; PoolListIndex < MAX_POOL_INDEX; PoolListIndex++) {
-    FreePoolList = &mSmmPoolLists[PoolListIndex];
-    for (Node = FreePoolList->BackLink;
-         Node != FreePoolList;
-         Node = Node->BackLink) {
-      Pool = BASE_CR (Node, FREE_POOL_HEADER, Link);
-      if (Pool->Header.Available) {
-        Index++;
+  for (SmmPoolTypeIndex = 0; SmmPoolTypeIndex < SmmPoolTypeMax; SmmPoolTypeIndex++) {
+    for (PoolListIndex = 0; PoolListIndex < MAX_POOL_INDEX; PoolListIndex++) {
+      FreePoolList = &mSmmPoolLists[SmmPoolTypeIndex][PoolListIndex];
+      for (Node = FreePoolList->BackLink;
+           Node != FreePoolList;
+           Node = Node->BackLink) {
+        Pool = BASE_CR (Node, FREE_POOL_HEADER, Link);
+        if (Pool->Header.Available) {
+          Index++;
+        }
       }
     }
   }
 
-
   TotalSize += (sizeof (MEMORY_PROFILE_FREE_MEMORY) + Index * sizeof (MEMORY_PROFILE_DESCRIPTOR));
   TotalSize += (sizeof (MEMORY_PROFILE_MEMORY_RANGE) + mFullSmramRangeCount * sizeof (MEMORY_PROFILE_DESCRIPTOR));
 
@@ -1698,6 +1700,7 @@ SmramProfileCopyData (
   UINT64                          RemainingSize;
   UINTN                           PdbSize;
   UINTN                           ActionStringSize;
+  UINTN                           SmmPoolTypeIndex;
 
   ContextData = GetSmramProfileContext ();
   if (ContextData == NULL) {
@@ -1785,14 +1788,16 @@ SmramProfileCopyData (
            Node = Node->BackLink) {
         Index++;
       }
-      for (PoolListIndex = 0; PoolListIndex < MAX_POOL_INDEX; PoolListIndex++) {
-        FreePoolList = &mSmmPoolLists[MAX_POOL_INDEX - PoolListIndex - 1];
-        for (Node = FreePoolList->BackLink;
-             Node != FreePoolList;
-             Node = Node->BackLink) {
-          Pool = BASE_CR (Node, FREE_POOL_HEADER, Link);
-          if (Pool->Header.Available) {
-            Index++;
+      for (SmmPoolTypeIndex = 0; SmmPoolTypeIndex < SmmPoolTypeMax; SmmPoolTypeIndex++) {
+        for (PoolListIndex = 0; PoolListIndex < MAX_POOL_INDEX; PoolListIndex++) {
+          FreePoolList = &mSmmPoolLists[SmmPoolTypeIndex][MAX_POOL_INDEX - PoolListIndex - 1];
+          for (Node = FreePoolList->BackLink;
+               Node != FreePoolList;
+               Node = Node->BackLink) {
+            Pool = BASE_CR (Node, FREE_POOL_HEADER, Link);
+            if (Pool->Header.Available) {
+              Index++;
+            }
           }
         }
       }
@@ -1827,29 +1832,31 @@ SmramProfileCopyData (
     }
     Offset += sizeof (MEMORY_PROFILE_DESCRIPTOR);
   }
-  for (PoolListIndex = 0; PoolListIndex < MAX_POOL_INDEX; PoolListIndex++) {
-    FreePoolList = &mSmmPoolLists[MAX_POOL_INDEX - PoolListIndex - 1];
-    for (Node = FreePoolList->BackLink;
-         Node != FreePoolList;
-         Node = Node->BackLink) {
-      Pool = BASE_CR (Node, FREE_POOL_HEADER, Link);
-      if (Pool->Header.Available) {
-        if (*ProfileOffset < (Offset + sizeof (MEMORY_PROFILE_DESCRIPTOR))) {
-          if (RemainingSize >= sizeof (MEMORY_PROFILE_DESCRIPTOR)) {
-            MemoryProfileDescriptor = ProfileBuffer;
-            MemoryProfileDescriptor->Header.Signature = MEMORY_PROFILE_DESCRIPTOR_SIGNATURE;
-            MemoryProfileDescriptor->Header.Length = sizeof (MEMORY_PROFILE_DESCRIPTOR);
-            MemoryProfileDescriptor->Header.Revision = MEMORY_PROFILE_DESCRIPTOR_REVISION;
-            MemoryProfileDescriptor->Address = (PHYSICAL_ADDRESS) (UINTN) Pool;
-            MemoryProfileDescriptor->Size = Pool->Header.Size;
-
-            RemainingSize -= sizeof (MEMORY_PROFILE_DESCRIPTOR);
-            ProfileBuffer = (UINT8 *) ProfileBuffer + sizeof (MEMORY_PROFILE_DESCRIPTOR);
-          } else {
-            goto Done;
+  for (SmmPoolTypeIndex = 0; SmmPoolTypeIndex < SmmPoolTypeMax; SmmPoolTypeIndex++) {
+    for (PoolListIndex = 0; PoolListIndex < MAX_POOL_INDEX; PoolListIndex++) {
+      FreePoolList = &mSmmPoolLists[SmmPoolTypeIndex][MAX_POOL_INDEX - PoolListIndex - 1];
+      for (Node = FreePoolList->BackLink;
+           Node != FreePoolList;
+           Node = Node->BackLink) {
+        Pool = BASE_CR (Node, FREE_POOL_HEADER, Link);
+        if (Pool->Header.Available) {
+          if (*ProfileOffset < (Offset + sizeof (MEMORY_PROFILE_DESCRIPTOR))) {
+            if (RemainingSize >= sizeof (MEMORY_PROFILE_DESCRIPTOR)) {
+              MemoryProfileDescriptor = ProfileBuffer;
+              MemoryProfileDescriptor->Header.Signature = MEMORY_PROFILE_DESCRIPTOR_SIGNATURE;
+              MemoryProfileDescriptor->Header.Length = sizeof (MEMORY_PROFILE_DESCRIPTOR);
+              MemoryProfileDescriptor->Header.Revision = MEMORY_PROFILE_DESCRIPTOR_REVISION;
+              MemoryProfileDescriptor->Address = (PHYSICAL_ADDRESS) (UINTN) Pool;
+              MemoryProfileDescriptor->Size = Pool->Header.Size;
+
+              RemainingSize -= sizeof (MEMORY_PROFILE_DESCRIPTOR);
+              ProfileBuffer = (UINT8 *) ProfileBuffer + sizeof (MEMORY_PROFILE_DESCRIPTOR);
+            } else {
+              goto Done;
+            }
           }
+          Offset += sizeof (MEMORY_PROFILE_DESCRIPTOR);
         }
-        Offset += sizeof (MEMORY_PROFILE_DESCRIPTOR);
       }
     }
   }
@@ -2577,6 +2584,7 @@ DumpFreePoolList (
   UINTN                         PoolListIndex;
   MEMORY_PROFILE_CONTEXT_DATA   *ContextData;
   BOOLEAN                       SmramProfileGettingStatus;
+  UINTN                         SmmPoolTypeIndex;
 
   ContextData = GetSmramProfileContext ();
   if (ContextData == NULL) {
@@ -2586,23 +2594,25 @@ DumpFreePoolList (
   SmramProfileGettingStatus = mSmramProfileGettingStatus;
   mSmramProfileGettingStatus = TRUE;
 
-  DEBUG ((EFI_D_INFO, "======= SmramProfile begin =======\n"));
-
-  for (PoolListIndex = 0; PoolListIndex < MAX_POOL_INDEX; PoolListIndex++) {
-    DEBUG ((EFI_D_INFO, "FreePoolList (%d):\n", PoolListIndex));
-    FreePoolList = &mSmmPoolLists[PoolListIndex];
-    for (Node = FreePoolList->BackLink, Index = 0;
-         Node != FreePoolList;
-         Node = Node->BackLink, Index++) {
-      Pool = BASE_CR (Node, FREE_POOL_HEADER, Link);
-      DEBUG ((EFI_D_INFO, "  Index - 0x%x\n", Index));
-      DEBUG ((EFI_D_INFO, "    PhysicalStart - 0x%016lx\n", (PHYSICAL_ADDRESS) (UINTN) Pool));
-      DEBUG ((EFI_D_INFO, "    Size          - 0x%08x\n", Pool->Header.Size));
-      DEBUG ((EFI_D_INFO, "    Available     - 0x%02x\n", Pool->Header.Available));
+  DEBUG ((DEBUG_INFO, "======= SmramProfile begin =======\n"));
+
+  for (SmmPoolTypeIndex = 0; SmmPoolTypeIndex < SmmPoolTypeMax; SmmPoolTypeIndex++) {
+    for (PoolListIndex = 0; PoolListIndex < MAX_POOL_INDEX; PoolListIndex++) {
+      DEBUG ((DEBUG_INFO, "FreePoolList(%d)(%d):\n", SmmPoolTypeIndex, PoolListIndex));
+      FreePoolList = &mSmmPoolLists[SmmPoolTypeIndex][PoolListIndex];
+      for (Node = FreePoolList->BackLink, Index = 0;
+           Node != FreePoolList;
+           Node = Node->BackLink, Index++) {
+        Pool = BASE_CR (Node, FREE_POOL_HEADER, Link);
+        DEBUG ((DEBUG_INFO, "  Index - 0x%x\n", Index));
+        DEBUG ((DEBUG_INFO, "    PhysicalStart - 0x%016lx\n", (PHYSICAL_ADDRESS) (UINTN) Pool));
+        DEBUG ((DEBUG_INFO, "    Size          - 0x%08x\n", Pool->Header.Size));
+        DEBUG ((DEBUG_INFO, "    Available     - 0x%02x\n", Pool->Header.Available));
+      }
     }
   }
 
-  DEBUG ((EFI_D_INFO, "======= SmramProfile end =======\n"));
+  DEBUG ((DEBUG_INFO, "======= SmramProfile end =======\n"));
 
   mSmramProfileGettingStatus = SmramProfileGettingStatus;  }
--
2.7.4.windows.1