Thanks. Looks good to me.

Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

From: Kun Qin <kun.q@outlook.com>
Sent: Thursday, January 21, 2021 9:44 AM
To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Sami Mujawar <sami.mujawar@arm.com>; Supreeth Venkatesh <supreeth.venkatesh@arm.com>
Subject: RE: [edk2-devel] [PATCH v3 05/18] StandaloneMmPkg: StandaloneMmMemLib: Extends support for X64 architecture

Hi Jiewen,

Do you have further concerns about this specific patch? I did created a Bugzilla ticket to track the OS memory protection concern here: 3168 – Add non-MMRAM memory protection for Standalone MM environment (tianocore.org). It introduces a new proposal to allow access of DXE/RT regions from MMRAM, but requires non trivial change. Please let me know if you hold different opinions towards the proposal or this patch.

Thanks,

Kun

From: Kun Qin
Sent: Thursday, January 14, 2021 14:36
To: devel@edk2.groups.io
Cc: Ard Biesheuvel; Sami Mujawar; Jiewen Yao; Supreeth Venkatesh
Subject: [edk2-devel] [PATCH v3 05/18] StandaloneMmPkg: StandaloneMmMemLib: Extends support for X64 architecture

This change extends StandaloneMmMemLib library to support X64
architecture. The implementation is ported from MdePkg/Library/SmmMemLib.

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>

Signed-off-by: Kun Qin <kun.q@outlook.com>
---

Notes:
    v3:
    - Updated destructor description of varibales to pass CI build.

    v2:
    - Added routine to fix bug of not initializing MmRanges [Jiewen]
    - Extends support to x86 instead of x64 only [Hao]

 StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/StandaloneMmMemLibInternal.c |  27 ++++
 StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.c                 |  52 +++++++
 StandaloneMmPkg/Library/StandaloneMmMemLib/X86StandaloneMmMemLibInternal.c      | 155 ++++++++++++++++++++
 StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf               |  13 +-
 4 files changed, 246 insertions(+), 1 deletion(-)

diff --git a/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/StandaloneMmMemLibInternal.c b/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/StandaloneMmMemLibInternal.c
index cb7c5e677a6b..4124959e0435 100644
--- a/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/StandaloneMmMemLibInternal.c
+++ b/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/StandaloneMmMemLibInternal.c
@@ -40,4 +40,31 @@ MmMemLibInternalCalculateMaximumSupportAddress (
   DEBUG ((DEBUG_INFO, "mMmMemLibInternalMaximumSupportAddress = 0x%lx\n", mMmMemLibInternalMaximumSupportAddress));
 }

+/**
+  Initialize cached Mmram Ranges from HOB.
+
+  @retval EFI_UNSUPPORTED   The routine is unable to extract MMRAM information.
+  @retval EFI_SUCCESS       MmRanges are populated successfully.
+
+**/
+EFI_STATUS
+MmMemLibInternalPopulateMmramRanges (
+  VOID
+  )
+{
+  // Not implemented for AARCH64.
+  return EFI_SUCCESS;
+}
+
+/**
+  Deinitialize cached Mmram Ranges.
+
+**/
+VOID
+MmMemLibInternalFreeMmramRanges (
+  VOID
+  )
+{
+  // Not implemented for AARCH64.
+}

diff --git a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.c b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.c
index b533bd8390cd..2737f95315eb 100644
--- a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.c
+++ b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.c
@@ -37,6 +37,27 @@ MmMemLibInternalCalculateMaximumSupportAddress (
   VOID
   );

+/**
+  Initialize cached Mmram Ranges from HOB.
+
+  @retval EFI_UNSUPPORTED   The routine is unable to extract MMRAM information.
+  @retval EFI_SUCCESS       MmRanges are populated successfully.
+
+**/
+EFI_STATUS
+MmMemLibInternalPopulateMmramRanges (
+  VOID
+  );
+
+/**
+  Deinitialize cached Mmram Ranges.
+
+**/
+VOID
+MmMemLibInternalFreeMmramRanges (
+  VOID
+  );
+
 /**
   This function check if the buffer is valid per processor architecture and not overlap with MMRAM.

@@ -253,11 +274,42 @@ MemLibConstructor (
   IN EFI_MM_SYSTEM_TABLE    *MmSystemTable
   )
 {
+  EFI_STATUS Status;

   //
   // Calculate and save maximum support address
   //
   MmMemLibInternalCalculateMaximumSupportAddress ();

+  //
+  // Initialize cached Mmram Ranges from HOB.
+  //
+  Status = MmMemLibInternalPopulateMmramRanges ();
+
+  return Status;
+}
+
+/**
+  Destructor for Mm Mem library.
+
+  @param ImageHandle    The image handle of the process.
+  @param MmSystemTable  The EFI System Table pointer.
+
+  @retval EFI_SUCCESS   The constructor always returns EFI_SUCCESS.
+
+**/
+EFI_STATUS
+EFIAPI
+MemLibDestructor (
+  IN EFI_HANDLE             ImageHandle,
+  IN EFI_MM_SYSTEM_TABLE    *MmSystemTable
+  )
+{
+
+  //
+  // Deinitialize cached Mmram Ranges.
+  //
+  MmMemLibInternalFreeMmramRanges ();
+
   return EFI_SUCCESS;
 }
diff --git a/StandaloneMmPkg/Library/StandaloneMmMemLib/X86StandaloneMmMemLibInternal.c b/StandaloneMmPkg/Library/StandaloneMmMemLib/X86StandaloneMmMemLibInternal.c
new file mode 100644
index 000000000000..1a978541716a
--- /dev/null
+++ b/StandaloneMmPkg/Library/StandaloneMmMemLib/X86StandaloneMmMemLibInternal.c
@@ -0,0 +1,155 @@
+/** @file
+  Internal ARCH Specific file of MM memory check library.
+
+  MM memory check library implementation. This library consumes MM_ACCESS_PROTOCOL
+  to get MMRAM information. In order to use this library instance, the platform should produce
+  all MMRAM range via MM_ACCESS_PROTOCOL, including the range for firmware (like MM Core
+  and MM driver) and/or specific dedicated hardware.
+
+  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.<BR>
+  Copyright (c) Microsoft Corporation.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+#include <PiMm.h>
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/DebugLib.h>
+#include <Library/HobLib.h>
+
+#include <Guid/MmCoreData.h>
+#include <Guid/MmramMemoryReserve.h>
+
+//
+// Maximum support address used to check input buffer
+//
+extern EFI_PHYSICAL_ADDRESS  mMmMemLibInternalMaximumSupportAddress;
+extern EFI_MMRAM_DESCRIPTOR *mMmMemLibInternalMmramRanges;
+extern UINTN                 mMmMemLibInternalMmramCount;
+
+/**
+  Calculate and save the maximum support address.
+
+**/
+VOID
+MmMemLibInternalCalculateMaximumSupportAddress (
+  VOID
+  )
+{
+  VOID         *Hob;
+  UINT32       RegEax;
+  UINT8        PhysicalAddressBits;
+
+  //
+  // Get physical address bits supported.
+  //
+  Hob = GetFirstHob (EFI_HOB_TYPE_CPU);
+  if (Hob != NULL) {
+    PhysicalAddressBits = ((EFI_HOB_CPU *) Hob)->SizeOfMemorySpace;
+  } else {
+    AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL);
+    if (RegEax >= 0x80000008) {
+      AsmCpuid (0x80000008, &RegEax, NULL, NULL, NULL);
+      PhysicalAddressBits = (UINT8) RegEax;
+    } else {
+      PhysicalAddressBits = 36;
+    }
+  }
+  //
+  // IA-32e paging translates 48-bit linear addresses to 52-bit physical addresses.
+  //
+  ASSERT (PhysicalAddressBits <= 52);
+  if (PhysicalAddressBits > 48) {
+    PhysicalAddressBits = 48;
+  }
+
+  //
+  // Save the maximum support address in one global variable
+  //
+  mMmMemLibInternalMaximumSupportAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)(LShiftU64 (1, PhysicalAddressBits) - 1);
+  DEBUG ((DEBUG_INFO, "mMmMemLibInternalMaximumSupportAddress = 0x%lx\n", mMmMemLibInternalMaximumSupportAddress));
+}
+
+/**
+  Initialize cached Mmram Ranges from HOB.
+
+  @retval EFI_UNSUPPORTED   The routine is unable to extract MMRAM information.
+  @retval EFI_SUCCESS       MmRanges are populated successfully.
+
+**/
+EFI_STATUS
+MmMemLibInternalPopulateMmramRanges (
+  VOID
+  )
+{
+  VOID                            *HobStart;
+  EFI_HOB_GUID_TYPE               *GuidHob;
+  MM_CORE_DATA_HOB_DATA           *DataInHob;
+  MM_CORE_PRIVATE_DATA            *MmCorePrivateData;
+  EFI_HOB_GUID_TYPE               *MmramRangesHob;
+  EFI_MMRAM_HOB_DESCRIPTOR_BLOCK  *MmramRangesHobData;
+  EFI_MMRAM_DESCRIPTOR            *MmramDescriptors;
+
+  HobStart = GetHobList ();
+  DEBUG ((DEBUG_INFO, "%a - 0x%x\n", __FUNCTION__, HobStart));
+
+  //
+  // Extract MM Core Private context from the Hob. If absent search for
+  // a Hob containing the MMRAM ranges
+  //
+  GuidHob = GetNextGuidHob (&gMmCoreDataHobGuid, HobStart);
+  if (GuidHob == NULL) {
+    MmramRangesHob = GetFirstGuidHob (&gEfiMmPeiMmramMemoryReserveGuid);
+    if (MmramRangesHob == NULL) {
+      return EFI_UNSUPPORTED;
+    }
+
+    MmramRangesHobData = GET_GUID_HOB_DATA (MmramRangesHob);
+    if (MmramRangesHobData == NULL || MmramRangesHobData->Descriptor == NULL) {
+      return EFI_UNSUPPORTED;
+    }
+
+    mMmMemLibInternalMmramCount = MmramRangesHobData->NumberOfMmReservedRegions;
+    MmramDescriptors = MmramRangesHobData->Descriptor;
+  } else {
+    DataInHob = GET_GUID_HOB_DATA (GuidHob);
+    if (DataInHob == NULL) {
+      return EFI_UNSUPPORTED;
+    }
+
+    MmCorePrivateData = (MM_CORE_PRIVATE_DATA *) (UINTN) DataInHob->Address;
+    if (MmCorePrivateData == NULL || MmCorePrivateData->MmramRanges == 0) {
+      return EFI_UNSUPPORTED;
+    }
+
+    mMmMemLibInternalMmramCount = (UINTN) MmCorePrivateData->MmramRangeCount;
+    MmramDescriptors = (EFI_MMRAM_DESCRIPTOR *) (UINTN) MmCorePrivateData->MmramRanges;
+  }
+
+  mMmMemLibInternalMmramRanges = AllocatePool (mMmMemLibInternalMmramCount * sizeof (EFI_MMRAM_DESCRIPTOR));
+  if (mMmMemLibInternalMmramRanges) {
+    CopyMem (mMmMemLibInternalMmramRanges,
+             MmramDescriptors,
+             mMmMemLibInternalMmramCount * sizeof (EFI_MMRAM_DESCRIPTOR));
+  }
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Deinitialize cached Mmram Ranges.
+
+**/
+VOID
+MmMemLibInternalFreeMmramRanges (
+  VOID
+  )
+{
+  if (mMmMemLibInternalMmramRanges != NULL) {
+    FreePool (mMmMemLibInternalMmramRanges);
+  }
+}
+
diff --git a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf
index 49da02e54e6d..062b0d7a1161 100644
--- a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf
@@ -8,6 +8,7 @@
 #
 #  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
 #  Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.<BR>
+#  Copyright (c) Microsoft Corporation.
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -22,16 +23,20 @@ [Defines]
   PI_SPECIFICATION_VERSION       = 0x00010032
   LIBRARY_CLASS                  = MemLib|MM_STANDALONE MM_CORE_STANDALONE
   CONSTRUCTOR                    = MemLibConstructor
+  DESTRUCTOR                     = MemLibDestructor

 #
 # The following information is for reference only and not required by the build tools.
 #
-#  VALID_ARCHITECTURES           = AARCH64
+#  VALID_ARCHITECTURES           = IA32 X64 AARCH64
 #

 [Sources.Common]
   StandaloneMmMemLib.c

+[Sources.IA32, Sources.X64]
+  X86StandaloneMmMemLibInternal.c
+
 [Sources.AARCH64]
   AArch64/StandaloneMmMemLibInternal.c

@@ -42,3 +47,9 @@ [Packages]
 [LibraryClasses]
   BaseMemoryLib
   DebugLib
+  HobLib
+  MemoryAllocationLib
+
+[Guids]
+  gMmCoreDataHobGuid                  ## SOMETIMES_CONSUMES ## HOB
+  gEfiMmPeiMmramMemoryReserveGuid     ## SOMETIMES_CONSUMES ## HOB
--
2.30.0.windows.1