[edk2-devel] [PATCH v2 1/3] UefiCpuPkg/CpuPageTableLib: Init local variable before using it.

[edk2-devel] [PATCH v2 1/3] UefiCpuPkg/CpuPageTableLib: Init local variable before using it.

[Zhiguang Liu]

The local variable OneOfPagingEntry is used before initialized, this
may cause reserved bit in page table entry is set especially in PAE
paging mode. The bug is random because it depends on the value in
stack.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index eff02619fa..36b2c4e6a3 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -338,7 +338,7 @@ PageTableLibMapInLevel (
   ParentAttribute             = &LocalParentAttribute;
 
   OriginalParentPagingEntry.Uint64 = ParentPagingEntry->Uint64;
-
+  OneOfPagingEntry.Uint64          = 0;
   //
   // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1 << 21) or 4K (1 << 12).
   //
@@ -367,8 +367,6 @@ PageTableLibMapInLevel (
       if (RETURN_ERROR (Status)) {
         return Status;
       }
-
-      OneOfPagingEntry.Pnle.Uint64 = 0;
     } else {
       PageTableLibSetPle (Level, &OneOfPagingEntry, 0, &PleBAttribute, &AllOneMask);
     }
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111889): https://edk2.groups.io/g/devel/message/111889
Mute This Topic: https://groups.io/mt/102889278/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 2/3] UefiCpuPkg/CpuPageTableLib/TestCase: Refine test case for PAE paging.

[Zhiguang Liu]

Refine test case:
1. Check PAE paging reserved bits is zero.
2. Set stack as random value.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 .../CpuPageTableLib/UnitTest/RandomTest.c     | 24 ++++++++++++++++++-
 .../CpuPageTableLib/UnitTest/TestHelper.c     | 14 ++++++++---
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
index f7a77d00e7..9ac3188be0 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
@@ -138,6 +138,23 @@ RandomBoolean (
   return ((Probability > ((UINT8)Random64 (0, 100))) ? TRUE : FALSE);
 }
 
+/**
+  Set 8K stack as random value.
+**/
+VOID
+SetRandomStack (
+  VOID
+  )
+{
+  UINT64  Buffer[SIZE_1KB];
+  UINTN   Index;
+
+  for (Index = 0; Index < SIZE_1KB; Index++) {
+    Buffer[Index] = Random64 (0, MAX_UINT64);
+    Buffer[Index] = Buffer[Index];
+  }
+}
+
 /**
   Check if the Page table entry is valid
 
@@ -670,6 +687,7 @@ SingleMapEntryTest (
   IsNotPresent              = FALSE;
   IsModified                = FALSE;
 
+  SetRandomStack ();
   GenerateSingleRandomMapEntry (MaxAddress, MapEntrys);
   LastMapEntry = &MapEntrys->Maps[MapsIndex];
   Status       = PageTableParse (*PageTable, PagingMode, NULL, &MapCount);
@@ -1039,7 +1057,11 @@ TestCaseforRandomTest (
   mSupportedBit.Bits.Pat            = 1;
   mSupportedBit.Bits.Global         = 1;
   mSupportedBit.Bits.ProtectionKey  = 0xF;
-  mSupportedBit.Bits.Nx             = 1;
+  if (((CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT *)Context)->PagingMode == PagingPae) {
+    mSupportedBit.Bits.ProtectionKey = 0;
+  }
+
+  mSupportedBit.Bits.Nx = 1;
 
   mRandomOption = ((CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT *)Context)->RandomOption;
   mNumberIndex  = 0;
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
index 67776255c2..d2c50a6c8a 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
@@ -9,6 +9,7 @@
 #include "CpuPageTableLibUnitTest.h"
 #include "../CpuPageTable.h"
 
+#define IA32_PAE_RESERVED_MASK  0x7FF0000000000000ull
 //
 // Global Data to validate if the page table is legal
 // mValidMaskNoLeaf[0] is not used
@@ -95,6 +96,7 @@ InitGlobalData (
   @param[in]   Level          the level of PagingEntry.
   @param[in]   MaxLeafLevel   Max leaf entry level.
   @param[in]   LinearAddress  The linear address verified.
+  @param[in]   PagingMode     The paging mode.
 
   @retval  Leaf entry.
 **/
@@ -103,13 +105,18 @@ IsPageTableEntryValid (
   IN IA32_PAGING_ENTRY  *PagingEntry,
   IN UINTN              Level,
   IN UINTN              MaxLeafLevel,
-  IN UINT64             Address
+  IN UINT64             Address,
+  IN PAGING_MODE        PagingMode
   )
 {
   UINT64             Index;
   IA32_PAGING_ENTRY  *ChildPageEntry;
   UNIT_TEST_STATUS   Status;
 
+  if (PagingMode == PagingPae) {
+    UT_ASSERT_EQUAL (PagingEntry->Uint64 & IA32_PAE_RESERVED_MASK, 0);
+  }
+
   if (PagingEntry->Pce.Present == 0) {
     return UNIT_TEST_PASSED;
   }
@@ -142,7 +149,7 @@ IsPageTableEntryValid (
 
   ChildPageEntry = (IA32_PAGING_ENTRY  *)(UINTN)(IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&PagingEntry->Pnle));
   for (Index = 0; Index < 512; Index++) {
-    Status = IsPageTableEntryValid (&ChildPageEntry[Index], Level-1, MaxLeafLevel, Address + (Index<<(9*(Level-1) + 3)));
+    Status = IsPageTableEntryValid (&ChildPageEntry[Index], Level-1, MaxLeafLevel, Address + (Index<<(9*(Level-1) + 3)), PagingMode);
     if (Status != UNIT_TEST_PASSED) {
       return Status;
     }
@@ -190,9 +197,10 @@ IsPageTableValid (
     if (PagingMode == PagingPae) {
       UT_ASSERT_EQUAL (PagingEntry[Index].PdptePae.Bits.MustBeZero, 0);
       UT_ASSERT_EQUAL (PagingEntry[Index].PdptePae.Bits.MustBeZero2, 0);
+      UT_ASSERT_EQUAL (PagingEntry[Index].PdptePae.Bits.MustBeZero3, 0);
     }
 
-    Status = IsPageTableEntryValid (&PagingEntry[Index], MaxLevel, MaxLeafLevel, Index << (9 * MaxLevel + 3));
+    Status = IsPageTableEntryValid (&PagingEntry[Index], MaxLevel, MaxLeafLevel, Index << (9 * MaxLevel + 3), PagingMode);
     if (Status != UNIT_TEST_PASSED) {
       return Status;
     }
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111890): https://edk2.groups.io/g/devel/message/111890
Mute This Topic: https://groups.io/mt/102889279/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/3] UefiCpuPkg/CpuPageTableLib/TestCase: Refine test case for PAE paging.

[Ni, Ray]

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

Thanks,
Ray
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Thursday, November 30, 2023 2:29 PM
> To: devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH v2 2/3] UefiCpuPkg/CpuPageTableLib/TestCase: Refine test
> case for PAE paging.
> 
> Refine test case:
> 1. Check PAE paging reserved bits is zero.
> 2. Set stack as random value.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  .../CpuPageTableLib/UnitTest/RandomTest.c     | 24 ++++++++++++++++++-
>  .../CpuPageTableLib/UnitTest/TestHelper.c     | 14 ++++++++---
>  2 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> index f7a77d00e7..9ac3188be0 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> @@ -138,6 +138,23 @@ RandomBoolean (
>    return ((Probability > ((UINT8)Random64 (0, 100))) ? TRUE : FALSE);
>  }
> 
> +/**
> +  Set 8K stack as random value.
> +**/
> +VOID
> +SetRandomStack (
> +  VOID
> +  )
> +{
> +  UINT64  Buffer[SIZE_1KB];
> +  UINTN   Index;
> +
> +  for (Index = 0; Index < SIZE_1KB; Index++) {
> +    Buffer[Index] = Random64 (0, MAX_UINT64);
> +    Buffer[Index] = Buffer[Index];
> +  }
> +}
> +
>  /**
>    Check if the Page table entry is valid
> 
> @@ -670,6 +687,7 @@ SingleMapEntryTest (
>    IsNotPresent              = FALSE;
>    IsModified                = FALSE;
> 
> +  SetRandomStack ();
>    GenerateSingleRandomMapEntry (MaxAddress, MapEntrys);
>    LastMapEntry = &MapEntrys->Maps[MapsIndex];
>    Status       = PageTableParse (*PageTable, PagingMode, NULL, &MapCount);
> @@ -1039,7 +1057,11 @@ TestCaseforRandomTest (
>    mSupportedBit.Bits.Pat            = 1;
>    mSupportedBit.Bits.Global         = 1;
>    mSupportedBit.Bits.ProtectionKey  = 0xF;
> -  mSupportedBit.Bits.Nx             = 1;
> +  if (((CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT *)Context)-
> >PagingMode == PagingPae) {
> +    mSupportedBit.Bits.ProtectionKey = 0;
> +  }
> +
> +  mSupportedBit.Bits.Nx = 1;
> 
>    mRandomOption = ((CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT
> *)Context)->RandomOption;
>    mNumberIndex  = 0;
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> index 67776255c2..d2c50a6c8a 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> @@ -9,6 +9,7 @@
>  #include "CpuPageTableLibUnitTest.h"
>  #include "../CpuPageTable.h"
> 
> +#define IA32_PAE_RESERVED_MASK  0x7FF0000000000000ull
>  //
>  // Global Data to validate if the page table is legal
>  // mValidMaskNoLeaf[0] is not used
> @@ -95,6 +96,7 @@ InitGlobalData (
>    @param[in]   Level          the level of PagingEntry.
>    @param[in]   MaxLeafLevel   Max leaf entry level.
>    @param[in]   LinearAddress  The linear address verified.
> +  @param[in]   PagingMode     The paging mode.
> 
>    @retval  Leaf entry.
>  **/
> @@ -103,13 +105,18 @@ IsPageTableEntryValid (
>    IN IA32_PAGING_ENTRY  *PagingEntry,
>    IN UINTN              Level,
>    IN UINTN              MaxLeafLevel,
> -  IN UINT64             Address
> +  IN UINT64             Address,
> +  IN PAGING_MODE        PagingMode
>    )
>  {
>    UINT64             Index;
>    IA32_PAGING_ENTRY  *ChildPageEntry;
>    UNIT_TEST_STATUS   Status;
> 
> +  if (PagingMode == PagingPae) {
> +    UT_ASSERT_EQUAL (PagingEntry->Uint64 & IA32_PAE_RESERVED_MASK,
> 0);
> +  }
> +
>    if (PagingEntry->Pce.Present == 0) {
>      return UNIT_TEST_PASSED;
>    }
> @@ -142,7 +149,7 @@ IsPageTableEntryValid (
> 
>    ChildPageEntry = (IA32_PAGING_ENTRY
> *)(UINTN)(IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&PagingEntry->Pnle));
>    for (Index = 0; Index < 512; Index++) {
> -    Status = IsPageTableEntryValid (&ChildPageEntry[Index], Level-1,
> MaxLeafLevel, Address + (Index<<(9*(Level-1) + 3)));
> +    Status = IsPageTableEntryValid (&ChildPageEntry[Index], Level-1,
> MaxLeafLevel, Address + (Index<<(9*(Level-1) + 3)), PagingMode);
>      if (Status != UNIT_TEST_PASSED) {
>        return Status;
>      }
> @@ -190,9 +197,10 @@ IsPageTableValid (
>      if (PagingMode == PagingPae) {
>        UT_ASSERT_EQUAL (PagingEntry[Index].PdptePae.Bits.MustBeZero, 0);
>        UT_ASSERT_EQUAL (PagingEntry[Index].PdptePae.Bits.MustBeZero2, 0);
> +      UT_ASSERT_EQUAL (PagingEntry[Index].PdptePae.Bits.MustBeZero3, 0);
>      }
> 
> -    Status = IsPageTableEntryValid (&PagingEntry[Index], MaxLevel,
> MaxLeafLevel, Index << (9 * MaxLevel + 3));
> +    Status = IsPageTableEntryValid (&PagingEntry[Index], MaxLevel,
> MaxLeafLevel, Index << (9 * MaxLevel + 3), PagingMode);
>      if (Status != UNIT_TEST_PASSED) {
>        return Status;
>      }
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111978): https://edk2.groups.io/g/devel/message/111978
Mute This Topic: https://groups.io/mt/102889279/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 3/3] UefiCpuPkg/CpuMpPei: Use CpuPageTableLib to set memory attribute.

[Zhiguang Liu]

Currently, there are code to set memory attribute in CpuMpPei module.
However, the code doesn't handle the case of 5 level paging.
Use the CpuPageTableLib to set memory attribute for two purpose:
1. Add 5 level paging support
2. Clean up code

Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 UefiCpuPkg/CpuMpPei/CpuPaging.c | 317 +++++++-------------------------
 1 file changed, 69 insertions(+), 248 deletions(-)

diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c b/UefiCpuPkg/CpuMpPei/CpuPaging.c
index b7ddb0005b..2dd7237141 100644
--- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
+++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
@@ -15,50 +15,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Guid/MigratedFvInfo.h>
 
 #include "CpuMpPei.h"
-
-#define IA32_PG_P   BIT0
-#define IA32_PG_RW  BIT1
-#define IA32_PG_U   BIT2
-#define IA32_PG_A   BIT5
-#define IA32_PG_D   BIT6
-#define IA32_PG_PS  BIT7
-#define IA32_PG_NX  BIT63
-
-#define PAGE_ATTRIBUTE_BITS  (IA32_PG_RW | IA32_PG_P)
-#define PAGE_PROGATE_BITS    (IA32_PG_D | IA32_PG_A | IA32_PG_NX | IA32_PG_U | \
-                               PAGE_ATTRIBUTE_BITS)
-
-#define PAGING_PAE_INDEX_MASK        0x1FF
-#define PAGING_4K_ADDRESS_MASK_64    0x000FFFFFFFFFF000ull
-#define PAGING_2M_ADDRESS_MASK_64    0x000FFFFFFFE00000ull
-#define PAGING_1G_ADDRESS_MASK_64    0x000FFFFFC0000000ull
-#define PAGING_512G_ADDRESS_MASK_64  0x000FFF8000000000ull
-
-typedef enum {
-  PageNone = 0,
-  PageMin  = 1,
-  Page4K   = PageMin,
-  Page2M   = 2,
-  Page1G   = 3,
-  Page512G = 4,
-  PageMax  = Page512G
-} PAGE_ATTRIBUTE;
-
-typedef struct {
-  PAGE_ATTRIBUTE    Attribute;
-  UINT64            Length;
-  UINT64            AddressMask;
-  UINTN             AddressBitOffset;
-  UINTN             AddressBitLength;
-} PAGE_ATTRIBUTE_TABLE;
-
-PAGE_ATTRIBUTE_TABLE  mPageAttributeTable[] = {
-  { PageNone, 0,          0,                           0,  0 },
-  { Page4K,   SIZE_4KB,   PAGING_4K_ADDRESS_MASK_64,   12, 9 },
-  { Page2M,   SIZE_2MB,   PAGING_2M_ADDRESS_MASK_64,   21, 9 },
-  { Page1G,   SIZE_1GB,   PAGING_1G_ADDRESS_MASK_64,   30, 9 },
-  { Page512G, SIZE_512GB, PAGING_512G_ADDRESS_MASK_64, 39, 9 },
-};
+#define PAGING_4K_ADDRESS_MASK_64  0x000FFFFFFFFFF000ull
 
 EFI_PEI_NOTIFY_DESCRIPTOR  mPostMemNotifyList[] = {
   {
@@ -117,237 +74,101 @@ AllocatePageTableMemory (
   return Address;
 }
 
-/**
-  Get the type of top level page table.
-
-  @retval Page512G  PML4 paging.
-  @retval Page1G    PAE paging.
-
-**/
-PAGE_ATTRIBUTE
-GetPageTableTopLevelType (
-  VOID
-  )
-{
-  MSR_IA32_EFER_REGISTER  MsrEfer;
-
-  MsrEfer.Uint64 = AsmReadMsr64 (MSR_CORE_IA32_EFER);
-
-  return (MsrEfer.Bits.LMA == 1) ? Page512G : Page1G;
-}
-
-/**
-  Return page table entry matching the address.
-
-  @param[in]   Address          The address to be checked.
-  @param[out]  PageAttributes   The page attribute of the page entry.
-
-  @return The page entry.
-**/
-VOID *
-GetPageTableEntry (
-  IN  PHYSICAL_ADDRESS  Address,
-  OUT PAGE_ATTRIBUTE    *PageAttribute
-  )
-{
-  INTN    Level;
-  UINTN   Index;
-  UINT64  *PageTable;
-  UINT64  AddressEncMask;
-
-  AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask);
-  PageTable      = (UINT64 *)(UINTN)(AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
-  for (Level = (INTN)GetPageTableTopLevelType (); Level > 0; --Level) {
-    Index  = (UINTN)RShiftU64 (Address, mPageAttributeTable[Level].AddressBitOffset);
-    Index &= PAGING_PAE_INDEX_MASK;
-
-    //
-    // No mapping?
-    //
-    if (PageTable[Index] == 0) {
-      *PageAttribute = PageNone;
-      return NULL;
-    }
-
-    //
-    // Page memory?
-    //
-    if (((PageTable[Index] & IA32_PG_PS) != 0) || (Level == PageMin)) {
-      *PageAttribute = (PAGE_ATTRIBUTE)Level;
-      return &PageTable[Index];
-    }
-
-    //
-    // Page directory or table
-    //
-    PageTable = (UINT64 *)(UINTN)(PageTable[Index] &
-                                  ~AddressEncMask &
-                                  PAGING_4K_ADDRESS_MASK_64);
-  }
-
-  *PageAttribute = PageNone;
-  return NULL;
-}
-
-/**
-  This function splits one page entry to smaller 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.
-  @param[in]  Recursively      Do the split recursively or not.
-
-  @retval RETURN_SUCCESS            The page entry is splitted.
-  @retval RETURN_INVALID_PARAMETER  If target page attribute is invalid
-  @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,
-  IN  BOOLEAN         Recursively
-  )
-{
-  UINT64          BaseAddress;
-  UINT64          *NewPageEntry;
-  UINTN           Index;
-  UINT64          AddressEncMask;
-  PAGE_ATTRIBUTE  SplitTo;
-
-  if ((SplitAttribute == PageNone) || (SplitAttribute >= PageAttribute)) {
-    ASSERT (SplitAttribute != PageNone);
-    ASSERT (SplitAttribute < PageAttribute);
-    return RETURN_INVALID_PARAMETER;
-  }
-
-  NewPageEntry = AllocatePageTableMemory (1);
-  if (NewPageEntry == NULL) {
-    ASSERT (NewPageEntry != NULL);
-    return RETURN_OUT_OF_RESOURCES;
-  }
-
-  //
-  // One level down each step to achieve more compact page table.
-  //
-  SplitTo        = PageAttribute - 1;
-  AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) &
-                   mPageAttributeTable[SplitTo].AddressMask;
-  BaseAddress = *PageEntry &
-                ~PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) &
-                mPageAttributeTable[PageAttribute].AddressMask;
-  for (Index = 0; Index < SIZE_4KB / sizeof (UINT64); Index++) {
-    NewPageEntry[Index] = BaseAddress | AddressEncMask |
-                          ((*PageEntry) & PAGE_PROGATE_BITS);
-
-    if (SplitTo != PageMin) {
-      NewPageEntry[Index] |= IA32_PG_PS;
-    }
-
-    if (Recursively && (SplitTo > SplitAttribute)) {
-      SplitPage (&NewPageEntry[Index], SplitTo, SplitAttribute, Recursively);
-    }
-
-    BaseAddress += mPageAttributeTable[SplitTo].Length;
-  }
-
-  (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask | PAGE_ATTRIBUTE_BITS;
-
-  return RETURN_SUCCESS;
-}
-
 /**
   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.
+  by BaseAddress and Length to not present.
 
   Caller should make sure BaseAddress and Length is at page boundary.
 
   @param[in]   BaseAddress      Start address of a memory region.
   @param[in]   Length           Size in bytes of the memory region.
-  @param[in]   Attributes       Bit mask of attributes to modify.
-
-  @retval RETURN_SUCCESS            The attributes were modified for the memory
-                                    region.
-  @retval RETURN_INVALID_PARAMETER  Length is zero; or,
-                                    Attributes specified an illegal combination
-                                    of attributes that cannot be set together; or
-                                    Addressis not 4KB aligned.
+
+  @retval RETURN_SUCCESS            The memory region is changed to not present.
   @retval RETURN_OUT_OF_RESOURCES   There are not enough system resources to modify
                                     the attributes.
   @retval RETURN_UNSUPPORTED        Cannot modify the attributes of given memory.
 
 **/
 RETURN_STATUS
-EFIAPI
-ConvertMemoryPageAttributes (
+ConvertMemoryPageToNotPresent (
   IN  PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64            Length,
-  IN  UINT64            Attributes
+  IN  UINT64            Length
   )
 {
-  UINT64                *PageEntry;
-  PAGE_ATTRIBUTE        PageAttribute;
-  RETURN_STATUS         Status;
-  EFI_PHYSICAL_ADDRESS  MaximumAddress;
-
-  if ((Length == 0) ||
-      ((BaseAddress & (SIZE_4KB - 1)) != 0) ||
-      ((Length & (SIZE_4KB - 1)) != 0))
-  {
-    ASSERT (Length > 0);
-    ASSERT ((BaseAddress & (SIZE_4KB - 1)) == 0);
-    ASSERT ((Length & (SIZE_4KB - 1)) == 0);
-
-    return RETURN_INVALID_PARAMETER;
-  }
-
-  MaximumAddress = (EFI_PHYSICAL_ADDRESS)MAX_UINT32;
-  if ((BaseAddress > MaximumAddress) ||
-      (Length > MaximumAddress) ||
-      (BaseAddress > MaximumAddress - (Length - 1)))
-  {
-    return RETURN_UNSUPPORTED;
-  }
-
-  //
-  // Below logic is to check 2M/4K page to make sure we do not waste memory.
-  //
-  while (Length != 0) {
-    PageEntry = GetPageTableEntry (BaseAddress, &PageAttribute);
-    if (PageEntry == NULL) {
-      return RETURN_UNSUPPORTED;
-    }
+  EFI_STATUS                  Status;
+  UINTN                       PageTable;
+  EFI_PHYSICAL_ADDRESS        Buffer;
+  UINTN                       BufferSize;
+  IA32_MAP_ATTRIBUTE          MapAttribute;
+  IA32_MAP_ATTRIBUTE          MapMask;
+  PAGING_MODE                 PagingMode;
+  IA32_CR4                    Cr4;
+  BOOLEAN                     Page5LevelSupport;
+  UINT32                      RegEax;
+  BOOLEAN                     Page1GSupport;
+  CPUID_EXTENDED_CPU_SIG_EDX  RegEdx;
+
+  if (sizeof (UINTN) == sizeof (UINT64)) {
+    //
+    // Check Page5Level Support or not.
+    //
+    Cr4.UintN         = AsmReadCr4 ();
+    Page5LevelSupport = (Cr4.Bits.LA57 ? TRUE : FALSE);
 
-    if (PageAttribute != Page4K) {
-      Status = SplitPage (PageEntry, PageAttribute, Page4K, FALSE);
-      if (RETURN_ERROR (Status)) {
-        return Status;
+    //
+    // Check Page1G Support or not.
+    //
+    Page1GSupport = FALSE;
+    AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
+    if (RegEax >= CPUID_EXTENDED_CPU_SIG) {
+      AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, &RegEdx.Uint32);
+      if (RegEdx.Bits.Page1GB != 0) {
+        Page1GSupport = TRUE;
       }
-
-      //
-      // Do it again until the page is 4K.
-      //
-      continue;
     }
 
     //
-    // Just take care of 'present' bit for Stack Guard.
+    // Decide Paging Mode according Page5LevelSupport & Page1GSupport.
     //
-    if ((Attributes & IA32_PG_P) != 0) {
-      *PageEntry |= (UINT64)IA32_PG_P;
+    if (Page5LevelSupport) {
+      PagingMode = Page1GSupport ? Paging5Level1GB : Paging5Level;
     } else {
-      *PageEntry &= ~((UINT64)IA32_PG_P);
+      PagingMode = Page1GSupport ? Paging4Level1GB : Paging4Level;
     }
+  } else {
+    PagingMode = PagingPae;
+  }
+
+  MapAttribute.Uint64  = 0;
+  MapMask.Uint64       = 0;
+  MapMask.Bits.Present = 1;
+  PageTable            = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64;
+  BufferSize           = 0;
 
+  //
+  // Get required buffer size for the pagetable that will be created.
+  //
+  Status = PageTableMap (&PageTable, PagingMode, 0, &BufferSize, BaseAddress, Length, &MapAttribute, &MapMask, NULL);
+  if (Status == EFI_BUFFER_TOO_SMALL) {
     //
-    // Convert success, move to next
+    // Allocate required Buffer.
     //
-    BaseAddress += SIZE_4KB;
-    Length      -= SIZE_4KB;
+    Status = PeiServicesAllocatePages (
+               EfiBootServicesData,
+               EFI_SIZE_TO_PAGES (BufferSize),
+               &Buffer
+               );
+    ASSERT_EFI_ERROR (Status);
+    if (EFI_ERROR (Status)) {
+      return EFI_OUT_OF_RESOURCES;
+    }
+
+    Status = PageTableMap (&PageTable, PagingMode, (VOID *)(UINTN)Buffer, &BufferSize, BaseAddress, Length, &MapAttribute, &MapMask, NULL);
   }
 
-  return RETURN_SUCCESS;
+  ASSERT_EFI_ERROR (Status);
+  AsmWriteCr3 (PageTable);
+  return Status;
 }
 
 /**
@@ -516,7 +337,7 @@ SetupStackGuardPage (
     //
     // Set Guard page at stack base address.
     //
-    ConvertMemoryPageAttributes (StackBase, EFI_PAGE_SIZE, 0);
+    ConvertMemoryPageToNotPresent (StackBase, EFI_PAGE_SIZE);
     DEBUG ((
       DEBUG_INFO,
       "Stack Guard set at %lx [cpu%lu]!\n",
@@ -599,7 +420,7 @@ MemoryDiscoveredPpiNotifyCallback (
     // Enable #PF exception, so if the code access SPI after disable NEM, it will generate
     // the exception to avoid potential vulnerability.
     //
-    ConvertMemoryPageAttributes (MigratedFvInfo->FvOrgBase, MigratedFvInfo->FvLength, 0);
+    ConvertMemoryPageToNotPresent (MigratedFvInfo->FvOrgBase, MigratedFvInfo->FvLength);
 
     Hob.Raw = GET_NEXT_HOB (Hob);
     Hob.Raw = GetNextGuidHob (&gEdkiiMigratedFvInfoGuid, Hob.Raw);
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111891): https://edk2.groups.io/g/devel/message/111891
Mute This Topic: https://groups.io/mt/102889280/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/CpuMpPei: Use CpuPageTableLib to set memory attribute.

[Ni, Ray]

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

Thanks,
Ray
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Thursday, November 30, 2023 2:29 PM
> To: devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH v2 3/3] UefiCpuPkg/CpuMpPei: Use CpuPageTableLib to set
> memory attribute.
> 
> Currently, there are code to set memory attribute in CpuMpPei module.
> However, the code doesn't handle the case of 5 level paging.
> Use the CpuPageTableLib to set memory attribute for two purpose:
> 1. Add 5 level paging support
> 2. Clean up code
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  UefiCpuPkg/CpuMpPei/CpuPaging.c | 317 +++++++-------------------------
>  1 file changed, 69 insertions(+), 248 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> index b7ddb0005b..2dd7237141 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> @@ -15,50 +15,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include <Guid/MigratedFvInfo.h>
> 
>  #include "CpuMpPei.h"
> -
> -#define IA32_PG_P   BIT0
> -#define IA32_PG_RW  BIT1
> -#define IA32_PG_U   BIT2
> -#define IA32_PG_A   BIT5
> -#define IA32_PG_D   BIT6
> -#define IA32_PG_PS  BIT7
> -#define IA32_PG_NX  BIT63
> -
> -#define PAGE_ATTRIBUTE_BITS  (IA32_PG_RW | IA32_PG_P)
> -#define PAGE_PROGATE_BITS    (IA32_PG_D | IA32_PG_A | IA32_PG_NX |
> IA32_PG_U | \
> -                               PAGE_ATTRIBUTE_BITS)
> -
> -#define PAGING_PAE_INDEX_MASK        0x1FF
> -#define PAGING_4K_ADDRESS_MASK_64    0x000FFFFFFFFFF000ull
> -#define PAGING_2M_ADDRESS_MASK_64    0x000FFFFFFFE00000ull
> -#define PAGING_1G_ADDRESS_MASK_64    0x000FFFFFC0000000ull
> -#define PAGING_512G_ADDRESS_MASK_64  0x000FFF8000000000ull
> -
> -typedef enum {
> -  PageNone = 0,
> -  PageMin  = 1,
> -  Page4K   = PageMin,
> -  Page2M   = 2,
> -  Page1G   = 3,
> -  Page512G = 4,
> -  PageMax  = Page512G
> -} PAGE_ATTRIBUTE;
> -
> -typedef struct {
> -  PAGE_ATTRIBUTE    Attribute;
> -  UINT64            Length;
> -  UINT64            AddressMask;
> -  UINTN             AddressBitOffset;
> -  UINTN             AddressBitLength;
> -} PAGE_ATTRIBUTE_TABLE;
> -
> -PAGE_ATTRIBUTE_TABLE  mPageAttributeTable[] = {
> -  { PageNone, 0,          0,                           0,  0 },
> -  { Page4K,   SIZE_4KB,   PAGING_4K_ADDRESS_MASK_64,   12, 9 },
> -  { Page2M,   SIZE_2MB,   PAGING_2M_ADDRESS_MASK_64,   21, 9 },
> -  { Page1G,   SIZE_1GB,   PAGING_1G_ADDRESS_MASK_64,   30, 9 },
> -  { Page512G, SIZE_512GB, PAGING_512G_ADDRESS_MASK_64, 39, 9 },
> -};
> +#define PAGING_4K_ADDRESS_MASK_64  0x000FFFFFFFFFF000ull
> 
>  EFI_PEI_NOTIFY_DESCRIPTOR  mPostMemNotifyList[] = {
>    {
> @@ -117,237 +74,101 @@ AllocatePageTableMemory (
>    return Address;
>  }
> 
> -/**
> -  Get the type of top level page table.
> -
> -  @retval Page512G  PML4 paging.
> -  @retval Page1G    PAE paging.
> -
> -**/
> -PAGE_ATTRIBUTE
> -GetPageTableTopLevelType (
> -  VOID
> -  )
> -{
> -  MSR_IA32_EFER_REGISTER  MsrEfer;
> -
> -  MsrEfer.Uint64 = AsmReadMsr64 (MSR_CORE_IA32_EFER);
> -
> -  return (MsrEfer.Bits.LMA == 1) ? Page512G : Page1G;
> -}
> -
> -/**
> -  Return page table entry matching the address.
> -
> -  @param[in]   Address          The address to be checked.
> -  @param[out]  PageAttributes   The page attribute of the page entry.
> -
> -  @return The page entry.
> -**/
> -VOID *
> -GetPageTableEntry (
> -  IN  PHYSICAL_ADDRESS  Address,
> -  OUT PAGE_ATTRIBUTE    *PageAttribute
> -  )
> -{
> -  INTN    Level;
> -  UINTN   Index;
> -  UINT64  *PageTable;
> -  UINT64  AddressEncMask;
> -
> -  AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask);
> -  PageTable      = (UINT64 *)(UINTN)(AsmReadCr3 () &
> PAGING_4K_ADDRESS_MASK_64);
> -  for (Level = (INTN)GetPageTableTopLevelType (); Level > 0; --Level) {
> -    Index  = (UINTN)RShiftU64 (Address,
> mPageAttributeTable[Level].AddressBitOffset);
> -    Index &= PAGING_PAE_INDEX_MASK;
> -
> -    //
> -    // No mapping?
> -    //
> -    if (PageTable[Index] == 0) {
> -      *PageAttribute = PageNone;
> -      return NULL;
> -    }
> -
> -    //
> -    // Page memory?
> -    //
> -    if (((PageTable[Index] & IA32_PG_PS) != 0) || (Level == PageMin)) {
> -      *PageAttribute = (PAGE_ATTRIBUTE)Level;
> -      return &PageTable[Index];
> -    }
> -
> -    //
> -    // Page directory or table
> -    //
> -    PageTable = (UINT64 *)(UINTN)(PageTable[Index] &
> -                                  ~AddressEncMask &
> -                                  PAGING_4K_ADDRESS_MASK_64);
> -  }
> -
> -  *PageAttribute = PageNone;
> -  return NULL;
> -}
> -
> -/**
> -  This function splits one page entry to smaller 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.
> -  @param[in]  Recursively      Do the split recursively or not.
> -
> -  @retval RETURN_SUCCESS            The page entry is splitted.
> -  @retval RETURN_INVALID_PARAMETER  If target page attribute is invalid
> -  @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,
> -  IN  BOOLEAN         Recursively
> -  )
> -{
> -  UINT64          BaseAddress;
> -  UINT64          *NewPageEntry;
> -  UINTN           Index;
> -  UINT64          AddressEncMask;
> -  PAGE_ATTRIBUTE  SplitTo;
> -
> -  if ((SplitAttribute == PageNone) || (SplitAttribute >= PageAttribute)) {
> -    ASSERT (SplitAttribute != PageNone);
> -    ASSERT (SplitAttribute < PageAttribute);
> -    return RETURN_INVALID_PARAMETER;
> -  }
> -
> -  NewPageEntry = AllocatePageTableMemory (1);
> -  if (NewPageEntry == NULL) {
> -    ASSERT (NewPageEntry != NULL);
> -    return RETURN_OUT_OF_RESOURCES;
> -  }
> -
> -  //
> -  // One level down each step to achieve more compact page table.
> -  //
> -  SplitTo        = PageAttribute - 1;
> -  AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) &
> -                   mPageAttributeTable[SplitTo].AddressMask;
> -  BaseAddress = *PageEntry &
> -                ~PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) &
> -                mPageAttributeTable[PageAttribute].AddressMask;
> -  for (Index = 0; Index < SIZE_4KB / sizeof (UINT64); Index++) {
> -    NewPageEntry[Index] = BaseAddress | AddressEncMask |
> -                          ((*PageEntry) & PAGE_PROGATE_BITS);
> -
> -    if (SplitTo != PageMin) {
> -      NewPageEntry[Index] |= IA32_PG_PS;
> -    }
> -
> -    if (Recursively && (SplitTo > SplitAttribute)) {
> -      SplitPage (&NewPageEntry[Index], SplitTo, SplitAttribute, Recursively);
> -    }
> -
> -    BaseAddress += mPageAttributeTable[SplitTo].Length;
> -  }
> -
> -  (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask |
> PAGE_ATTRIBUTE_BITS;
> -
> -  return RETURN_SUCCESS;
> -}
> -
>  /**
>    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.
> +  by BaseAddress and Length to not present.
> 
>    Caller should make sure BaseAddress and Length is at page boundary.
> 
>    @param[in]   BaseAddress      Start address of a memory region.
>    @param[in]   Length           Size in bytes of the memory region.
> -  @param[in]   Attributes       Bit mask of attributes to modify.
> -
> -  @retval RETURN_SUCCESS            The attributes were modified for the
> memory
> -                                    region.
> -  @retval RETURN_INVALID_PARAMETER  Length is zero; or,
> -                                    Attributes specified an illegal combination
> -                                    of attributes that cannot be set together; or
> -                                    Addressis not 4KB aligned.
> +
> +  @retval RETURN_SUCCESS            The memory region is changed to not
> present.
>    @retval RETURN_OUT_OF_RESOURCES   There are not enough system
> resources to modify
>                                      the attributes.
>    @retval RETURN_UNSUPPORTED        Cannot modify the attributes of given
> memory.
> 
>  **/
>  RETURN_STATUS
> -EFIAPI
> -ConvertMemoryPageAttributes (
> +ConvertMemoryPageToNotPresent (
>    IN  PHYSICAL_ADDRESS  BaseAddress,
> -  IN  UINT64            Length,
> -  IN  UINT64            Attributes
> +  IN  UINT64            Length
>    )
>  {
> -  UINT64                *PageEntry;
> -  PAGE_ATTRIBUTE        PageAttribute;
> -  RETURN_STATUS         Status;
> -  EFI_PHYSICAL_ADDRESS  MaximumAddress;
> -
> -  if ((Length == 0) ||
> -      ((BaseAddress & (SIZE_4KB - 1)) != 0) ||
> -      ((Length & (SIZE_4KB - 1)) != 0))
> -  {
> -    ASSERT (Length > 0);
> -    ASSERT ((BaseAddress & (SIZE_4KB - 1)) == 0);
> -    ASSERT ((Length & (SIZE_4KB - 1)) == 0);
> -
> -    return RETURN_INVALID_PARAMETER;
> -  }
> -
> -  MaximumAddress = (EFI_PHYSICAL_ADDRESS)MAX_UINT32;
> -  if ((BaseAddress > MaximumAddress) ||
> -      (Length > MaximumAddress) ||
> -      (BaseAddress > MaximumAddress - (Length - 1)))
> -  {
> -    return RETURN_UNSUPPORTED;
> -  }
> -
> -  //
> -  // Below logic is to check 2M/4K page to make sure we do not waste
> memory.
> -  //
> -  while (Length != 0) {
> -    PageEntry = GetPageTableEntry (BaseAddress, &PageAttribute);
> -    if (PageEntry == NULL) {
> -      return RETURN_UNSUPPORTED;
> -    }
> +  EFI_STATUS                  Status;
> +  UINTN                       PageTable;
> +  EFI_PHYSICAL_ADDRESS        Buffer;
> +  UINTN                       BufferSize;
> +  IA32_MAP_ATTRIBUTE          MapAttribute;
> +  IA32_MAP_ATTRIBUTE          MapMask;
> +  PAGING_MODE                 PagingMode;
> +  IA32_CR4                    Cr4;
> +  BOOLEAN                     Page5LevelSupport;
> +  UINT32                      RegEax;
> +  BOOLEAN                     Page1GSupport;
> +  CPUID_EXTENDED_CPU_SIG_EDX  RegEdx;
> +
> +  if (sizeof (UINTN) == sizeof (UINT64)) {
> +    //
> +    // Check Page5Level Support or not.
> +    //
> +    Cr4.UintN         = AsmReadCr4 ();
> +    Page5LevelSupport = (Cr4.Bits.LA57 ? TRUE : FALSE);
> 
> -    if (PageAttribute != Page4K) {
> -      Status = SplitPage (PageEntry, PageAttribute, Page4K, FALSE);
> -      if (RETURN_ERROR (Status)) {
> -        return Status;
> +    //
> +    // Check Page1G Support or not.
> +    //
> +    Page1GSupport = FALSE;
> +    AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
> +    if (RegEax >= CPUID_EXTENDED_CPU_SIG) {
> +      AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL,
> &RegEdx.Uint32);
> +      if (RegEdx.Bits.Page1GB != 0) {
> +        Page1GSupport = TRUE;
>        }
> -
> -      //
> -      // Do it again until the page is 4K.
> -      //
> -      continue;
>      }
> 
>      //
> -    // Just take care of 'present' bit for Stack Guard.
> +    // Decide Paging Mode according Page5LevelSupport & Page1GSupport.
>      //
> -    if ((Attributes & IA32_PG_P) != 0) {
> -      *PageEntry |= (UINT64)IA32_PG_P;
> +    if (Page5LevelSupport) {
> +      PagingMode = Page1GSupport ? Paging5Level1GB : Paging5Level;
>      } else {
> -      *PageEntry &= ~((UINT64)IA32_PG_P);
> +      PagingMode = Page1GSupport ? Paging4Level1GB : Paging4Level;
>      }
> +  } else {
> +    PagingMode = PagingPae;
> +  }
> +
> +  MapAttribute.Uint64  = 0;
> +  MapMask.Uint64       = 0;
> +  MapMask.Bits.Present = 1;
> +  PageTable            = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64;
> +  BufferSize           = 0;
> 
> +  //
> +  // Get required buffer size for the pagetable that will be created.
> +  //
> +  Status = PageTableMap (&PageTable, PagingMode, 0, &BufferSize,
> BaseAddress, Length, &MapAttribute, &MapMask, NULL);
> +  if (Status == EFI_BUFFER_TOO_SMALL) {
>      //
> -    // Convert success, move to next
> +    // Allocate required Buffer.
>      //
> -    BaseAddress += SIZE_4KB;
> -    Length      -= SIZE_4KB;
> +    Status = PeiServicesAllocatePages (
> +               EfiBootServicesData,
> +               EFI_SIZE_TO_PAGES (BufferSize),
> +               &Buffer
> +               );
> +    ASSERT_EFI_ERROR (Status);
> +    if (EFI_ERROR (Status)) {
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +
> +    Status = PageTableMap (&PageTable, PagingMode, (VOID *)(UINTN)Buffer,
> &BufferSize, BaseAddress, Length, &MapAttribute, &MapMask, NULL);
>    }
> 
> -  return RETURN_SUCCESS;
> +  ASSERT_EFI_ERROR (Status);
> +  AsmWriteCr3 (PageTable);
> +  return Status;
>  }
> 
>  /**
> @@ -516,7 +337,7 @@ SetupStackGuardPage (
>      //
>      // Set Guard page at stack base address.
>      //
> -    ConvertMemoryPageAttributes (StackBase, EFI_PAGE_SIZE, 0);
> +    ConvertMemoryPageToNotPresent (StackBase, EFI_PAGE_SIZE);
>      DEBUG ((
>        DEBUG_INFO,
>        "Stack Guard set at %lx [cpu%lu]!\n",
> @@ -599,7 +420,7 @@ MemoryDiscoveredPpiNotifyCallback (
>      // Enable #PF exception, so if the code access SPI after disable NEM, it will
> generate
>      // the exception to avoid potential vulnerability.
>      //
> -    ConvertMemoryPageAttributes (MigratedFvInfo->FvOrgBase,
> MigratedFvInfo->FvLength, 0);
> +    ConvertMemoryPageToNotPresent (MigratedFvInfo->FvOrgBase,
> MigratedFvInfo->FvLength);
> 
>      Hob.Raw = GET_NEXT_HOB (Hob);
>      Hob.Raw = GetNextGuidHob (&gEdkiiMigratedFvInfoGuid, Hob.Raw);
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111979): https://edk2.groups.io/g/devel/message/111979
Mute This Topic: https://groups.io/mt/102889280/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/CpuMpPei: Use CpuPageTableLib to set memory attribute.

[Laszlo Ersek]

On 12/1/23 09:42, Ni, Ray wrote:
> Reviewed-by: Ray Ni <ray.ni@intel.com>

This series seems to have been merged meanwhile -- that's good, because
I could have usefully reviewed only patch#1, and only ACK patches #2 and
#3, due to time constraints.

However... please do report on the list whenever a series is merged! I
started reviewing patch#1, only to find that it was already upstream. I
could have saved that time if I had seen, on the list, a statement of
merging.

So, for the record: commit range
ef3fde64aa78598a4c21556629936fb228390e8c..7e18c9a788e543ab71cdc0485989cf5d00cdccc2.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112347): https://edk2.groups.io/g/devel/message/112347
Mute This Topic: https://groups.io/mt/102889280/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/3] UefiCpuPkg/CpuPageTableLib: Init local variable before using it.

[Ni, Ray]

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

Thanks,
Ray
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Thursday, November 30, 2023 2:29 PM
> To: devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH v2 1/3] UefiCpuPkg/CpuPageTableLib: Init local variable
> before using it.
> 
> The local variable OneOfPagingEntry is used before initialized, this
> may cause reserved bit in page table entry is set especially in PAE
> paging mode. The bug is random because it depends on the value in
> stack.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index eff02619fa..36b2c4e6a3 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -338,7 +338,7 @@ PageTableLibMapInLevel (
>    ParentAttribute             = &LocalParentAttribute;
> 
>    OriginalParentPagingEntry.Uint64 = ParentPagingEntry->Uint64;
> -
> +  OneOfPagingEntry.Uint64          = 0;
>    //
>    // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1 << 21)
> or 4K (1 << 12).
>    //
> @@ -367,8 +367,6 @@ PageTableLibMapInLevel (
>        if (RETURN_ERROR (Status)) {
>          return Status;
>        }
> -
> -      OneOfPagingEntry.Pnle.Uint64 = 0;
>      } else {
>        PageTableLibSetPle (Level, &OneOfPagingEntry, 0, &PleBAttribute,
> &AllOneMask);
>      }
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111977): https://edk2.groups.io/g/devel/message/111977
Mute This Topic: https://groups.io/mt/102889278/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-