Re: [PATCH v2 04/16] StandaloneMmPkg: StandaloneMmMemLib: Extends support for X64 architecture

["Yao, Jiewen" <jiewen.yao@intel.com>]

Thanks Qin.
A quick question: May I know what test you have run for this change?

Also, I think this patch only protect the MM memory, but not OS memory. Is that expected?
Will you consider adding OS memory protection later?


> -----Original Message-----
> From: Kun Qin <kun.q@outlook.com>
> Sent: Wednesday, January 6, 2021 2:59 AM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Sami Mujawar
> <sami.mujawar@arm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Supreeth
> Venkatesh <supreeth.venkatesh@arm.com>
> Subject: [PATCH v2 04/16] 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:
>     v2:
>     - Added routine to fix bug of not initializing MmRanges [Jiewen]
>     - Extends support to x86 instead of x64 only [Hao]
> 
> 
> StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/StandaloneM
> mMemLibInternal.c |  26 ++++
> 
> StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib
> .c                 |  52 +++++++
> 
> StandaloneMmPkg/Library/StandaloneMmMemLib/X86StandaloneMmMe
> mLibInternal.c      | 155 ++++++++++++++++++++
> 
> StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib
> .inf               |  13 +-
>  4 files changed, 245 insertions(+), 1 deletion(-)
> 
> diff --git
> a/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/Standalone
> MmMemLibInternal.c
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/Standalone
> MmMemLibInternal.c
> index cb7c5e677a6b..46dfce5cac86 100644
> ---
> a/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/Standalone
> MmMemLibInternal.c
> +++
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/AArch64/Standalone
> MmMemLibInternal.c
> @@ -40,4 +40,30 @@
> 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.
> +}
> +
> +/**
> +  Deinitialize cached Mmram Ranges.
> +
> +**/
> +VOID
> +MmMemLibInternalFreeMmramRanges (
> +  VOID
> +  )
> +{
> +  // Not implemented for AARCH64.
> +}
> 
> diff --git
> a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.c
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.c
> index f82cdb3ba816..f43af2b1cc9b 100644
> ---
> a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.c
> +++
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.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 the library.  free any resources for Mm Mem library
> +
> +  @param ImageHandle    The image handle of the process.
> +  @param SystemTable    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/X86StandaloneMmM
> emLibInternal.c
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/X86StandaloneMm
> MemLibInternal.c
> new file mode 100644
> index 000000000000..1a978541716a
> --- /dev/null
> +++
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/X86StandaloneMm
> MemLibInternal.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/StandaloneMmMem
> Lib.inf
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.inf
> index 49da02e54e6d..062b0d7a1161 100644
> ---
> a/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.inf
> +++
> b/StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMem
> Lib.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