Re: [Patch V5 03/14] UefiCpuPkg: Use CpuPageTableLib to convert SMM paging attribute.

["Ni, Ray" <ray.ni@intel.com>]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Thursday, June 8, 2023 10:28 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [Patch V5 03/14] UefiCpuPkg: Use CpuPageTableLib to convert SMM
> paging attribute.
> 
> Simplify the ConvertMemoryPageAttributes API to convert paging
> attribute by CpuPageTableLib. In the new API, it calls
> PageTableMap() to update the page attributes of a memory range.
> With the PageTableMap() API in CpuPageTableLib, we can remove
> the complicated page table manipulating code.
> 
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           |   3 ++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  28
> +++++++++++++---------------
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf       |   1 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 409
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++---------------------------
> ----------------------------------------------------------------------------------------------
> ----------------------------------------------------------------------------------------------
> ----------------------------------------------------------------------------------------------
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c            |   7 ++++++-
>  5 files changed, 122 insertions(+), 326 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index 34bf6e1a25..9c8107080a 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -1,7 +1,7 @@
>  /** @file
>  Page table manipulation functions for IA-32 processors
> 
> -Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2009 - 2023, Intel Corporation. All rights reserved.<BR>
>  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -31,6 +31,7 @@ SmmInitPageTable (
>    InitializeSpinLock (mPFLock);
> 
>    mPhysicalAddressBits = 32;
> +  mPagingMode          = PagingPae;
> 
>    if (FeaturePcdGet (PcdCpuSmmProfileEnable) ||
>        HEAP_GUARD_NONSTOP_MODE ||
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index a5c2bdd971..ba341cadc6 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -50,6 +50,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include <Library/SmmCpuFeaturesLib.h>
>  #include <Library/PeCoffGetEntryPointLib.h>
>  #include <Library/RegisterCpuFeaturesLib.h>
> +#include <Library/CpuPageTableLib.h>
> 
>  #include <AcpiCpuData.h>
>  #include <CpuHotPlugData.h>
> @@ -260,6 +261,7 @@ extern UINTN                 mNumberOfCpus;
>  extern EFI_SMM_CPU_PROTOCOL  mSmmCpu;
>  extern EFI_MM_MP_PROTOCOL    mSmmMp;
>  extern BOOLEAN               m5LevelPagingNeeded;
> +extern PAGING_MODE           mPagingMode;
> 
>  ///
>  /// The mode of the CPU at the time an SMI occurs
> @@ -1008,11 +1010,10 @@ SetPageTableAttributes (
>    Length from their current attributes to the attributes specified by Attributes.
> 
>    @param[in]   PageTableBase    The page table base.
> -  @param[in]   EnablePML5Paging If PML5 paging is enabled.
> +  @param[in]   PagingMode       The paging mode.
>    @param[in]   BaseAddress      The physical address that is the start address of
> a memory region.
>    @param[in]   Length           The size in bytes of the memory region.
>    @param[in]   Attributes       The bit mask of attributes to set for the memory
> region.
> -  @param[out]  IsSplitted       TRUE means page table splitted. FALSE means
> page table not splitted.
> 
>    @retval EFI_SUCCESS           The attributes were set for the memory region.
>    @retval EFI_ACCESS_DENIED     The attributes for the memory resource
> range specified by
> @@ -1030,12 +1031,11 @@ SetPageTableAttributes (
>  **/
>  EFI_STATUS
>  SmmSetMemoryAttributesEx (
> -  IN  UINTN                 PageTableBase,
> -  IN  BOOLEAN               EnablePML5Paging,
> -  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
> -  IN  UINT64                Length,
> -  IN  UINT64                Attributes,
> -  OUT BOOLEAN               *IsSplitted  OPTIONAL
> +  IN  UINTN             PageTableBase,
> +  IN  PAGING_MODE       PagingMode,
> +  IN  PHYSICAL_ADDRESS  BaseAddress,
> +  IN  UINT64            Length,
> +  IN  UINT64            Attributes
>    );
> 
>  /**
> @@ -1043,34 +1043,32 @@ SmmSetMemoryAttributesEx (
>    Length from their current attributes to the attributes specified by Attributes.
> 
>    @param[in]   PageTableBase    The page table base.
> -  @param[in]   EnablePML5Paging If PML5 paging is enabled.
> +  @param[in]   PagingMode       The paging mode.
>    @param[in]   BaseAddress      The physical address that is the start address of
> a memory region.
>    @param[in]   Length           The size in bytes of the memory region.
>    @param[in]   Attributes       The bit mask of attributes to clear for the memory
> region.
> -  @param[out]  IsSplitted       TRUE means page table splitted. FALSE means
> page table not splitted.
> 
>    @retval EFI_SUCCESS           The attributes were cleared for the memory
> region.
>    @retval EFI_ACCESS_DENIED     The attributes for the memory resource
> range specified by
>                                  BaseAddress and Length cannot be modified.
>    @retval EFI_INVALID_PARAMETER Length is zero.
>                                  Attributes specified an illegal combination of attributes that
> -                                cannot be set together.
> +                                cannot be cleared together.
>    @retval EFI_OUT_OF_RESOURCES  There are not enough system resources
> to modify the attributes of
>                                  the memory resource range.
>    @retval EFI_UNSUPPORTED       The processor does not support one or more
> bytes of the memory
>                                  resource range specified by BaseAddress and Length.
> -                                The bit mask of attributes is not support for the memory
> resource
> +                                The bit mask of attributes is not supported for the memory
> resource
>                                  range specified by BaseAddress and Length.
> 
>  **/
>  EFI_STATUS
>  SmmClearMemoryAttributesEx (
>    IN  UINTN                 PageTableBase,
> -  IN  BOOLEAN               EnablePML5Paging,
> +  IN  PAGING_MODE           PagingMode,
>    IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
>    IN  UINT64                Length,
> -  IN  UINT64                Attributes,
> -  OUT BOOLEAN               *IsSplitted  OPTIONAL
> +  IN  UINT64                Attributes
>    );
> 
>  /**
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> index 158e05e264..38d4e950a4 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> @@ -97,6 +97,7 @@
>    ReportStatusCodeLib
>    SmmCpuFeaturesLib
>    PeCoffGetEntryPointLib
> +  CpuPageTableLib
> 
>  [Protocols]
>    gEfiSmmAccess2ProtocolGuid               ## CONSUMES
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 834a756061..12723e5750 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -1,6 +1,6 @@
>  /** @file
> 
> -Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2016 - 2023, Intel Corporation. All rights reserved.<BR>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -26,14 +26,9 @@ UINTN                            mGcdMemNumberOfDesc = 0;
> 
>  EFI_MEMORY_ATTRIBUTES_TABLE  *mUefiMemoryAttributesTable = NULL;
> 
> -PAGE_ATTRIBUTE_TABLE  mPageAttributeTable[] = {
> -  { Page4K, SIZE_4KB, PAGING_4K_ADDRESS_MASK_64 },
> -  { Page2M, SIZE_2MB, PAGING_2M_ADDRESS_MASK_64 },
> -  { Page1G, SIZE_1GB, PAGING_1G_ADDRESS_MASK_64 },
> -};
> -
> -BOOLEAN  mIsShadowStack      = FALSE;
> -BOOLEAN  m5LevelPagingNeeded = FALSE;
> +BOOLEAN      mIsShadowStack      = FALSE;
> +BOOLEAN      m5LevelPagingNeeded = FALSE;
> +PAGING_MODE  mPagingMode         = PagingModeMax;
> 
>  //
>  // Global variable to keep track current available memory used as page table.
> @@ -185,52 +180,6 @@ AllocatePageTableMemory (
>    return Buffer;
>  }
> 
> -/**
> -  Return length according to page attributes.
> -
> -  @param[in]  PageAttributes   The page attribute of the page entry.
> -
> -  @return The length of page entry.
> -**/
> -UINTN
> -PageAttributeToLength (
> -  IN PAGE_ATTRIBUTE  PageAttribute
> -  )
> -{
> -  UINTN  Index;
> -
> -  for (Index = 0; Index < sizeof (mPageAttributeTable)/sizeof
> (mPageAttributeTable[0]); Index++) {
> -    if (PageAttribute == mPageAttributeTable[Index].Attribute) {
> -      return (UINTN)mPageAttributeTable[Index].Length;
> -    }
> -  }
> -
> -  return 0;
> -}
> -
> -/**
> -  Return address mask according to page attributes.
> -
> -  @param[in]  PageAttributes   The page attribute of the page entry.
> -
> -  @return The address mask of page entry.
> -**/
> -UINTN
> -PageAttributeToMask (
> -  IN PAGE_ATTRIBUTE  PageAttribute
> -  )
> -{
> -  UINTN  Index;
> -
> -  for (Index = 0; Index < sizeof (mPageAttributeTable)/sizeof
> (mPageAttributeTable[0]); Index++) {
> -    if (PageAttribute == mPageAttributeTable[Index].Attribute) {
> -      return (UINTN)mPageAttributeTable[Index].AddressMask;
> -    }
> -  }
> -
> -  return 0;
> -}
> -
>  /**
>    Return page table entry to match the address.
> 
> @@ -353,181 +302,6 @@ GetAttributesFromPageEntry (
>    return Attributes;
>  }
> 
> -/**
> -  Modify memory attributes of page entry.
> -
> -  @param[in]   PageEntry        The page entry.
> -  @param[in]   Attributes       The bit mask of attributes to modify for the
> memory region.
> -  @param[in]   IsSet            TRUE means to set attributes. FALSE means to clear
> attributes.
> -  @param[out]  IsModified       TRUE means page table modified. FALSE means
> page table not modified.
> -**/
> -VOID
> -ConvertPageEntryAttribute (
> -  IN  UINT64   *PageEntry,
> -  IN  UINT64   Attributes,
> -  IN  BOOLEAN  IsSet,
> -  OUT BOOLEAN  *IsModified
> -  )
> -{
> -  UINT64  CurrentPageEntry;
> -  UINT64  NewPageEntry;
> -
> -  CurrentPageEntry = *PageEntry;
> -  NewPageEntry     = CurrentPageEntry;
> -  if ((Attributes & EFI_MEMORY_RP) != 0) {
> -    if (IsSet) {
> -      NewPageEntry &= ~(UINT64)IA32_PG_P;
> -    } else {
> -      NewPageEntry |= IA32_PG_P;
> -    }
> -  }
> -
> -  if ((Attributes & EFI_MEMORY_RO) != 0) {
> -    if (IsSet) {
> -      NewPageEntry &= ~(UINT64)IA32_PG_RW;
> -      if (mIsShadowStack) {
> -        // Environment setup
> -        // ReadOnly page need set Dirty bit for shadow stack
> -        NewPageEntry |= IA32_PG_D;
> -        // Clear user bit for supervisor shadow stack
> -        NewPageEntry &= ~(UINT64)IA32_PG_U;
> -      } else {
> -        // Runtime update
> -        // Clear dirty bit for non shadow stack, to protect RO page.
> -        NewPageEntry &= ~(UINT64)IA32_PG_D;
> -      }
> -    } else {
> -      NewPageEntry |= IA32_PG_RW;
> -    }
> -  }
> -
> -  if ((Attributes & EFI_MEMORY_XP) != 0) {
> -    if (mXdSupported) {
> -      if (IsSet) {
> -        NewPageEntry |= IA32_PG_NX;
> -      } else {
> -        NewPageEntry &= ~IA32_PG_NX;
> -      }
> -    }
> -  }
> -
> -  *PageEntry = NewPageEntry;
> -  if (CurrentPageEntry != NewPageEntry) {
> -    *IsModified = TRUE;
> -    DEBUG ((DEBUG_VERBOSE, "ConvertPageEntryAttribute 0x%lx",
> CurrentPageEntry));
> -    DEBUG ((DEBUG_VERBOSE, "->0x%lx\n", NewPageEntry));
> -  } else {
> -    *IsModified = FALSE;
> -  }
> -}
> -
> -/**
> -  This function returns if there is need to split page entry.
> -
> -  @param[in]  BaseAddress      The base address to be checked.
> -  @param[in]  Length           The length to be checked.
> -  @param[in]  PageEntry        The page entry to be checked.
> -  @param[in]  PageAttribute    The page attribute of the page entry.
> -
> -  @retval SplitAttributes on if there is need to split page entry.
> -**/
> -PAGE_ATTRIBUTE
> -NeedSplitPage (
> -  IN  PHYSICAL_ADDRESS  BaseAddress,
> -  IN  UINT64            Length,
> -  IN  UINT64            *PageEntry,
> -  IN  PAGE_ATTRIBUTE    PageAttribute
> -  )
> -{
> -  UINT64  PageEntryLength;
> -
> -  PageEntryLength = PageAttributeToLength (PageAttribute);
> -
> -  if (((BaseAddress & (PageEntryLength - 1)) == 0) && (Length >=
> PageEntryLength)) {
> -    return PageNone;
> -  }
> -
> -  if (((BaseAddress & PAGING_2M_MASK) != 0) || (Length < SIZE_2MB)) {
> -    return Page4K;
> -  }
> -
> -  return Page2M;
> -}
> -
> -/**
> -  This function splits one page entry to small page entries.
> -
> -  @param[in]  PageEntry        The page entry to be splitted.
> -  @param[in]  PageAttribute    The page attribute of the page entry.
> -  @param[in]  SplitAttribute   How to split the page entry.
> -
> -  @retval RETURN_SUCCESS            The page entry is splitted.
> -  @retval RETURN_UNSUPPORTED        The page entry does not support to be
> splitted.
> -  @retval RETURN_OUT_OF_RESOURCES   No resource to split page entry.
> -**/
> -RETURN_STATUS
> -SplitPage (
> -  IN  UINT64          *PageEntry,
> -  IN  PAGE_ATTRIBUTE  PageAttribute,
> -  IN  PAGE_ATTRIBUTE  SplitAttribute
> -  )
> -{
> -  UINT64  BaseAddress;
> -  UINT64  *NewPageEntry;
> -  UINTN   Index;
> -
> -  ASSERT (PageAttribute == Page2M || PageAttribute == Page1G);
> -
> -  if (PageAttribute == Page2M) {
> -    //
> -    // Split 2M to 4K
> -    //
> -    ASSERT (SplitAttribute == Page4K);
> -    if (SplitAttribute == Page4K) {
> -      NewPageEntry = AllocatePageTableMemory (1);
> -      DEBUG ((DEBUG_VERBOSE, "Split - 0x%x\n", NewPageEntry));
> -      if (NewPageEntry == NULL) {
> -        return RETURN_OUT_OF_RESOURCES;
> -      }
> -
> -      BaseAddress = *PageEntry & PAGING_2M_ADDRESS_MASK_64;
> -      for (Index = 0; Index < SIZE_4KB / sizeof (UINT64); Index++) {
> -        NewPageEntry[Index] = (BaseAddress + SIZE_4KB * Index) |
> mAddressEncMask | ((*PageEntry) & PAGE_PROGATE_BITS);
> -      }
> -
> -      (*PageEntry) = (UINT64)(UINTN)NewPageEntry | mAddressEncMask |
> PAGE_ATTRIBUTE_BITS;
> -      return RETURN_SUCCESS;
> -    } else {
> -      return RETURN_UNSUPPORTED;
> -    }
> -  } else if (PageAttribute == Page1G) {
> -    //
> -    // Split 1G to 2M
> -    // No need support 1G->4K directly, we should use 1G->2M, then 2M->4K
> to get more compact page table.
> -    //
> -    ASSERT (SplitAttribute == Page2M || SplitAttribute == Page4K);
> -    if (((SplitAttribute == Page2M) || (SplitAttribute == Page4K))) {
> -      NewPageEntry = AllocatePageTableMemory (1);
> -      DEBUG ((DEBUG_VERBOSE, "Split - 0x%x\n", NewPageEntry));
> -      if (NewPageEntry == NULL) {
> -        return RETURN_OUT_OF_RESOURCES;
> -      }
> -
> -      BaseAddress = *PageEntry & PAGING_1G_ADDRESS_MASK_64;
> -      for (Index = 0; Index < SIZE_4KB / sizeof (UINT64); Index++) {
> -        NewPageEntry[Index] = (BaseAddress + SIZE_2MB * Index) |
> mAddressEncMask | IA32_PG_PS | ((*PageEntry) & PAGE_PROGATE_BITS);
> -      }
> -
> -      (*PageEntry) = (UINT64)(UINTN)NewPageEntry | mAddressEncMask |
> PAGE_ATTRIBUTE_BITS;
> -      return RETURN_SUCCESS;
> -    } else {
> -      return RETURN_UNSUPPORTED;
> -    }
> -  } else {
> -    return RETURN_UNSUPPORTED;
> -  }
> -}
> -
>  /**
>    This function modifies the page attributes for the memory region specified
> by BaseAddress and
>    Length from their current attributes to the attributes specified by Attributes.
> @@ -535,12 +309,11 @@ SplitPage (
>    Caller should make sure BaseAddress and Length is at page boundary.
> 
>    @param[in]   PageTableBase    The page table base.
> -  @param[in]   EnablePML5Paging If PML5 paging is enabled.
> +  @param[in]   PagingMode       The paging mode.
>    @param[in]   BaseAddress      The physical address that is the start address of
> a memory region.
>    @param[in]   Length           The size in bytes of the memory region.
>    @param[in]   Attributes       The bit mask of attributes to modify for the
> memory region.
>    @param[in]   IsSet            TRUE means to set attributes. FALSE means to clear
> attributes.
> -  @param[out]  IsSplitted       TRUE means page table splitted. FALSE means
> page table not splitted.
>    @param[out]  IsModified       TRUE means page table modified. FALSE means
> page table not modified.
> 
>    @retval RETURN_SUCCESS           The attributes were modified for the
> memory region.
> @@ -559,28 +332,30 @@ SplitPage (
>  RETURN_STATUS
>  ConvertMemoryPageAttributes (
>    IN  UINTN             PageTableBase,
> -  IN  BOOLEAN           EnablePML5Paging,
> +  IN  PAGING_MODE       PagingMode,
>    IN  PHYSICAL_ADDRESS  BaseAddress,
>    IN  UINT64            Length,
>    IN  UINT64            Attributes,
>    IN  BOOLEAN           IsSet,
> -  OUT BOOLEAN           *IsSplitted   OPTIONAL,
>    OUT BOOLEAN           *IsModified   OPTIONAL
>    )
>  {
> -  UINT64                *PageEntry;
> -  PAGE_ATTRIBUTE        PageAttribute;
> -  UINTN                 PageEntryLength;
> -  PAGE_ATTRIBUTE        SplitAttribute;
>    RETURN_STATUS         Status;
> -  BOOLEAN               IsEntryModified;
> +  IA32_MAP_ATTRIBUTE    PagingAttribute;
> +  IA32_MAP_ATTRIBUTE    PagingAttrMask;
> +  UINTN                 PageTableBufferSize;
> +  VOID                  *PageTableBuffer;
>    EFI_PHYSICAL_ADDRESS  MaximumSupportMemAddress;
> +  IA32_MAP_ENTRY        *Map;
> +  UINTN                 Count;
> +  UINTN                 Index;
> 
>    ASSERT (Attributes != 0);
>    ASSERT ((Attributes & ~EFI_MEMORY_ATTRIBUTE_MASK) == 0);
> 
>    ASSERT ((BaseAddress & (SIZE_4KB - 1)) == 0);
>    ASSERT ((Length & (SIZE_4KB - 1)) == 0);
> +  ASSERT (PageTableBase != 0);
> 
>    if (Length == 0) {
>      return RETURN_INVALID_PARAMETER;
> @@ -599,61 +374,89 @@ ConvertMemoryPageAttributes (
>      return RETURN_UNSUPPORTED;
>    }
> 
> -  //  DEBUG ((DEBUG_ERROR, "ConvertMemoryPageAttributes(%x) -
>  %016lx, %016lx, %02lx\n", IsSet, BaseAddress, Length, Attributes));
> -
> -  if (IsSplitted != NULL) {
> -    *IsSplitted = FALSE;
> -  }
> -
>    if (IsModified != NULL) {
>      *IsModified = FALSE;
>    }
> 
> -  //
> -  // Below logic is to check 2M/4K page to make sure we do not waste
> memory.
> -  //
> -  while (Length != 0) {
> -    PageEntry = GetPageTableEntry (PageTableBase, EnablePML5Paging,
> BaseAddress, &PageAttribute);
> -    if (PageEntry == NULL) {
> -      return RETURN_UNSUPPORTED;
> -    }
> +  PagingAttribute.Uint64 = 0;
> +  PagingAttribute.Uint64 = mAddressEncMask | BaseAddress;
> +  PagingAttrMask.Uint64  = 0;
> 
> -    PageEntryLength = PageAttributeToLength (PageAttribute);
> -    SplitAttribute  = NeedSplitPage (BaseAddress, Length, PageEntry,
> PageAttribute);
> -    if (SplitAttribute == PageNone) {
> -      ConvertPageEntryAttribute (PageEntry, Attributes, IsSet,
> &IsEntryModified);
> -      if (IsEntryModified) {
> -        if (IsModified != NULL) {
> -          *IsModified = TRUE;
> -        }
> +  if ((Attributes & EFI_MEMORY_RO) != 0) {
> +    PagingAttrMask.Bits.ReadWrite = 1;
> +    if (IsSet) {
> +      PagingAttribute.Bits.ReadWrite = 0;
> +      PagingAttrMask.Bits.Dirty      = 1;
> +      if (mIsShadowStack) {
> +        // Environment setup
> +        // ReadOnly page need set Dirty bit for shadow stack
> +        PagingAttribute.Bits.Dirty = 1;
> +        // Clear user bit for supervisor shadow stack
> +        PagingAttribute.Bits.UserSupervisor = 0;
> +        PagingAttrMask.Bits.UserSupervisor  = 1;
> +      } else {
> +        // Runtime update
> +        // Clear dirty bit for non shadow stack, to protect RO page.
> +        PagingAttribute.Bits.Dirty = 0;
>        }
> +    } else {
> +      PagingAttribute.Bits.ReadWrite = 1;
> +    }
> +  }
> 
> +  if ((Attributes & EFI_MEMORY_XP) != 0) {
> +    if (mXdSupported) {
> +      PagingAttribute.Bits.Nx = IsSet ? 1 : 0;
> +      PagingAttrMask.Bits.Nx  = 1;
> +    }
> +  }
> +
> +  if ((Attributes & EFI_MEMORY_RP) != 0) {
> +    if (IsSet) {
> +      PagingAttribute.Bits.Present = 0;
>        //
> -      // Convert success, move to next
> +      // When map a range to non-present, all attributes except Present should
> not be provided.
>        //
> -      BaseAddress += PageEntryLength;
> -      Length      -= PageEntryLength;
> +      PagingAttrMask.Uint64       = 0;
> +      PagingAttrMask.Bits.Present = 1;
>      } else {
> -      Status = SplitPage (PageEntry, PageAttribute, SplitAttribute);
> -      if (RETURN_ERROR (Status)) {
> -        return RETURN_UNSUPPORTED;
> -      }
> -
> -      if (IsSplitted != NULL) {
> -        *IsSplitted = TRUE;
> -      }
> -
> -      if (IsModified != NULL) {
> -        *IsModified = TRUE;
> -      }
> +      //
> +      // When map range to present range, provide all attributes.
> +      //
> +      PagingAttribute.Bits.Present = 1;
> +      PagingAttrMask.Uint64        = MAX_UINT64;
> 
>        //
> -      // Just split current page
> -      // Convert success in next around
> +      // By default memory is Ring 3 accessble.
>        //
> +      PagingAttribute.Bits.UserSupervisor = 1;
>      }
>    }
> 
> +  if (PagingAttrMask.Uint64 == 0) {
> +    return RETURN_SUCCESS;
> +  }
> +
> +  PageTableBufferSize = 0;
> +  Status              = PageTableMap (&PageTableBase, PagingMode, NULL,
> &PageTableBufferSize, BaseAddress, Length, &PagingAttribute,
> &PagingAttrMask, IsModified);
> +
> +  if (Status == RETURN_BUFFER_TOO_SMALL) {
> +    PageTableBuffer = AllocatePageTableMemory (EFI_SIZE_TO_PAGES
> (PageTableBufferSize));
> +    ASSERT (PageTableBuffer != NULL);
> +    Status = PageTableMap (&PageTableBase, PagingMode, PageTableBuffer,
> &PageTableBufferSize, BaseAddress, Length, &PagingAttribute,
> &PagingAttrMask, IsModified);
> +  }
> +
> +  if (Status == RETURN_INVALID_PARAMETER) {
> +    //
> +    // The only reason that PageTableMap returns
> RETURN_INVALID_PARAMETER here is to modify other attributes
> +    // of a non-present range but remains the non-present range still as non-
> present.
> +    //
> +    DEBUG ((DEBUG_ERROR, "SMM ConvertMemoryPageAttributes: Only
> change EFI_MEMORY_XP/EFI_MEMORY_RO for non-present range in [0x%lx,
> 0x%lx] is not permitted\n", BaseAddress, BaseAddress + Length));
> +  }
> +
> +  ASSERT_RETURN_ERROR (Status);
> +  ASSERT (PageTableBufferSize == 0);
> +
>    return RETURN_SUCCESS;
>  }
> 
> @@ -697,11 +500,10 @@ FlushTlbForAll (
>    Length from their current attributes to the attributes specified by Attributes.
> 
>    @param[in]   PageTableBase    The page table base.
> -  @param[in]   EnablePML5Paging If PML5 paging is enabled.
> +  @param[in]   PagingMode       The paging mode.
>    @param[in]   BaseAddress      The physical address that is the start address of
> a memory region.
>    @param[in]   Length           The size in bytes of the memory region.
>    @param[in]   Attributes       The bit mask of attributes to set for the memory
> region.
> -  @param[out]  IsSplitted       TRUE means page table splitted. FALSE means
> page table not splitted.
> 
>    @retval EFI_SUCCESS           The attributes were set for the memory region.
>    @retval EFI_ACCESS_DENIED     The attributes for the memory resource
> range specified by
> @@ -720,17 +522,16 @@ FlushTlbForAll (
>  EFI_STATUS
>  SmmSetMemoryAttributesEx (
>    IN  UINTN                 PageTableBase,
> -  IN  BOOLEAN               EnablePML5Paging,
> +  IN  PAGING_MODE           PagingMode,
>    IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
>    IN  UINT64                Length,
> -  IN  UINT64                Attributes,
> -  OUT BOOLEAN               *IsSplitted  OPTIONAL
> +  IN  UINT64                Attributes
>    )
>  {
>    EFI_STATUS  Status;
>    BOOLEAN     IsModified;
> 
> -  Status = ConvertMemoryPageAttributes (PageTableBase, EnablePML5Paging,
> BaseAddress, Length, Attributes, TRUE, IsSplitted, &IsModified);
> +  Status = ConvertMemoryPageAttributes (PageTableBase, PagingMode,
> BaseAddress, Length, Attributes, TRUE, &IsModified);
>    if (!EFI_ERROR (Status)) {
>      if (IsModified) {
>        //
> @@ -748,11 +549,10 @@ SmmSetMemoryAttributesEx (
>    Length from their current attributes to the attributes specified by Attributes.
> 
>    @param[in]   PageTableBase    The page table base.
> -  @param[in]   EnablePML5Paging If PML5 paging is enabled.
> +  @param[in]   PagingMode       The paging mode.
>    @param[in]   BaseAddress      The physical address that is the start address of
> a memory region.
>    @param[in]   Length           The size in bytes of the memory region.
>    @param[in]   Attributes       The bit mask of attributes to clear for the memory
> region.
> -  @param[out]  IsSplitted       TRUE means page table splitted. FALSE means
> page table not splitted.
> 
>    @retval EFI_SUCCESS           The attributes were cleared for the memory
> region.
>    @retval EFI_ACCESS_DENIED     The attributes for the memory resource
> range specified by
> @@ -771,17 +571,16 @@ SmmSetMemoryAttributesEx (
>  EFI_STATUS
>  SmmClearMemoryAttributesEx (
>    IN  UINTN                 PageTableBase,
> -  IN  BOOLEAN               EnablePML5Paging,
> +  IN  PAGING_MODE           PagingMode,
>    IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
>    IN  UINT64                Length,
> -  IN  UINT64                Attributes,
> -  OUT BOOLEAN               *IsSplitted  OPTIONAL
> +  IN  UINT64                Attributes
>    )
>  {
>    EFI_STATUS  Status;
>    BOOLEAN     IsModified;
> 
> -  Status = ConvertMemoryPageAttributes (PageTableBase, EnablePML5Paging,
> BaseAddress, Length, Attributes, FALSE, IsSplitted, &IsModified);
> +  Status = ConvertMemoryPageAttributes (PageTableBase, PagingMode,
> BaseAddress, Length, Attributes, FALSE, &IsModified);
>    if (!EFI_ERROR (Status)) {
>      if (IsModified) {
>        //
> @@ -823,14 +622,10 @@ SmmSetMemoryAttributes (
>    IN  UINT64                Attributes
>    )
>  {
> -  IA32_CR4  Cr4;
> -  UINTN     PageTableBase;
> -  BOOLEAN   Enable5LevelPaging;
> -
> -  PageTableBase      = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64;
> -  Cr4.UintN          = AsmReadCr4 ();
> -  Enable5LevelPaging = (BOOLEAN)(Cr4.Bits.LA57 == 1);
> -  return SmmSetMemoryAttributesEx (PageTableBase, Enable5LevelPaging,
> BaseAddress, Length, Attributes, NULL);
> +  UINTN  PageTableBase;
> +
> +  PageTableBase = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64;
> +  return SmmSetMemoryAttributesEx (PageTableBase, mPagingMode,
> BaseAddress, Length, Attributes);
>  }
> 
>  /**
> @@ -862,14 +657,10 @@ SmmClearMemoryAttributes (
>    IN  UINT64                Attributes
>    )
>  {
> -  IA32_CR4  Cr4;
> -  UINTN     PageTableBase;
> -  BOOLEAN   Enable5LevelPaging;
> -
> -  PageTableBase      = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64;
> -  Cr4.UintN          = AsmReadCr4 ();
> -  Enable5LevelPaging = (BOOLEAN)(Cr4.Bits.LA57 == 1);
> -  return SmmClearMemoryAttributesEx (PageTableBase, Enable5LevelPaging,
> BaseAddress, Length, Attributes, NULL);
> +  UINTN  PageTableBase;
> +
> +  PageTableBase = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64;
> +  return SmmClearMemoryAttributesEx (PageTableBase, mPagingMode,
> BaseAddress, Length, Attributes);
>  }
> 
>  /**
> @@ -891,7 +682,7 @@ SetShadowStack (
>    EFI_STATUS  Status;
> 
>    mIsShadowStack = TRUE;
> -  Status         = SmmSetMemoryAttributesEx (Cr3, m5LevelPagingNeeded,
> BaseAddress, Length, EFI_MEMORY_RO, NULL);
> +  Status         = SmmSetMemoryAttributesEx (Cr3, mPagingMode,
> BaseAddress, Length, EFI_MEMORY_RO);
>    mIsShadowStack = FALSE;
> 
>    return Status;
> @@ -915,7 +706,7 @@ SetNotPresentPage (
>  {
>    EFI_STATUS  Status;
> 
> -  Status = SmmSetMemoryAttributesEx (Cr3, m5LevelPagingNeeded,
> BaseAddress, Length, EFI_MEMORY_RP, NULL);
> +  Status = SmmSetMemoryAttributesEx (Cr3, mPagingMode, BaseAddress,
> Length, EFI_MEMORY_RP);
>    return Status;
>  }
> 
> @@ -1799,7 +1590,7 @@ EnablePageTableProtection (
>      //
>      // Set entire pool including header, used-memory and left free-memory as
> ReadOnly in SMM page table.
>      //
> -    ConvertMemoryPageAttributes (PageTableBase, m5LevelPagingNeeded,
> Address, PoolSize, EFI_MEMORY_RO, TRUE, NULL, NULL);
> +    ConvertMemoryPageAttributes (PageTableBase, mPagingMode, Address,
> PoolSize, EFI_MEMORY_RO, TRUE, NULL);
>      Pool = Pool->NextPool;
>    } while (Pool != HeadPool);
>  }
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 3deb1ffd67..0bed857cae 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -1,7 +1,7 @@
>  /** @file
>  Page Fault (#PF) handler for X64 processors
> 
> -Copyright (c) 2009 - 2022, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2009 - 2023, Intel Corporation. All rights reserved.<BR>
>  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -354,6 +354,11 @@ SmmInitPageTable (
>    m5LevelPagingNeeded           = Is5LevelPagingNeeded ();
>    mPhysicalAddressBits          = CalculateMaximumSupportAddress ();
>    PatchInstructionX86 (gPatch5LevelPagingNeeded, m5LevelPagingNeeded,
> 1);
> +  if (m5LevelPagingNeeded) {
> +    mPagingMode = m1GPageTableSupport ? Paging5Level1GB : Paging5Level;
> +  } else {
> +    mPagingMode = m1GPageTableSupport ? Paging4Level1GB : Paging4Level;
> +  }
>    DEBUG ((DEBUG_INFO, "5LevelPaging Needed             - %d\n",
> m5LevelPagingNeeded));
>    DEBUG ((DEBUG_INFO, "1GPageTable Support             - %d\n",
> m1GPageTableSupport));
>    DEBUG ((DEBUG_INFO, "PcdCpuSmmRestrictedMemoryAccess - %d\n",
> mCpuSmmRestrictedMemoryAccess));
> --
> 2.31.1.windows.1