[PATCH v8 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address

[PATCH v8 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address

[Sheng Wei]

When trying to get page table base, if mInternalCr3 is zero, it will use
 the page table from CR3, and reflect the page table depth by CR4 LA57 bit.
If mInternalCr3 is non zero, it will use the page table from mInternalCr3
 and reflect the page table depth of mInternalCr3 at same time.
In the case of X64, we use m5LevelPagingNeeded to reflect the depth of
 the page table. And in the case of IA32, it will not the page table depth
 information.

This patch is a bug fix when enable CET feature with 5 level paging.
The SMM page tables are allocated / initialized in PiCpuSmmEntry().
When CET is enabled, PiCpuSmmEntry() must further modify the attribute of
 shadow stack pages. This page table is not set to CR3 in PiCpuSmmEntry().
 So the page table base address is set to mInternalCr3 for modifty the
 page table attribute. It could not use CR4 LA57 bit to reflect the
 page table depth for mInternalCr3.
So we create a architecture-specific implementation GetPageTable() with
 2 output parameters. One parameter is used to output the page table
 address. Another parameter is used to reflect if it is 5 level paging
 or not.

Correct the Cr3 typo

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3015

Signed-off-by: Sheng Wei <w.sheng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>

Sheng Wei (2):
  UefiCpuPkg/PiSmmCpuDxeSmm: Correct the Cr3 typo
  UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table
    address

 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           | 26 ++++++++++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         | 13 ++++---
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 35 +++++-------------
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c            | 43 ++++++++++++++++++----
 4 files changed, 77 insertions(+), 40 deletions(-)

-- 
2.16.2.windows.1

[PATCH v8 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Correct the Cr3 typo

[Sheng Wei]

Change the variable name from mInternalGr3 to mInternalCr3.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3015

Signed-off-by: Sheng Wei <w.sheng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index ebfc46ad45..d67f036aea 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -32,7 +32,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
   {Page1G,  SIZE_1GB, PAGING_1G_ADDRESS_MASK_64},
 };
 
-UINTN  mInternalGr3;
+UINTN  mInternalCr3;
 
 /**
   Set the internal page table base address.
@@ -46,7 +46,7 @@ SetPageTableBase (
   IN UINTN   Cr3
   )
 {
-  mInternalGr3 = Cr3;
+  mInternalCr3 = Cr3;
 }
 
 /**
@@ -59,8 +59,8 @@ GetPageTableBase (
   VOID
   )
 {
-  if (mInternalGr3 != 0) {
-    return mInternalGr3;
+  if (mInternalCr3 != 0) {
+    return mInternalCr3;
   }
   return (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
 }
@@ -252,7 +252,7 @@ ConvertPageEntryAttribute (
   if ((Attributes & EFI_MEMORY_RO) != 0) {
     if (IsSet) {
       NewPageEntry &= ~(UINT64)IA32_PG_RW;
-      if (mInternalGr3 != 0) {
+      if (mInternalCr3 != 0) {
         // Environment setup
         // ReadOnly page need set Dirty bit for shadow stack
         NewPageEntry |= IA32_PG_D;
-- 
2.16.2.windows.1

[PATCH v8 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address

[Sheng Wei]

When trying to get page table base, if mInternalCr3 is zero, it will use
 the page table from CR3, and reflect the page table depth by CR4 LA57 bit.
If mInternalCr3 is non zero, it will use the page table from mInternalCr3
 and reflect the page table depth of mInternalCr3 at same time.
In the case of X64, we use m5LevelPagingNeeded to reflect the depth of
 the page table. And in the case of IA32, it will not the page table depth
 information.

This patch is a bug fix when enable CET feature with 5 level paging.
The SMM page tables are allocated / initialized in PiCpuSmmEntry().
When CET is enabled, PiCpuSmmEntry() must further modify the attribute of
 shadow stack pages. This page table is not set to CR3 in PiCpuSmmEntry().
 So the page table base address is set to mInternalCr3 for modifty the
 page table attribute. It could not use CR4 LA57 bit to reflect the
 page table depth for mInternalCr3.
So we create a architecture-specific implementation GetPageTable() with
 2 output parameters. One parameter is used to output the page table
 address. Another parameter is used to reflect if it is 5 level paging
 or not.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3015

Signed-off-by: Sheng Wei <w.sheng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           | 26 ++++++++++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         | 13 ++++---
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 29 +++------------
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c            | 43 ++++++++++++++++++----
 4 files changed, 74 insertions(+), 37 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index 2483f2ea84..9c8e2d15ac 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -28,6 +28,26 @@ EnableCet (
   VOID
   );
 
+/**
+  Get page table base address and the depth of the page table.
+
+  @param[out] Base        Page table base address.
+  @param[out] FiveLevels  TRUE means 5 level paging. FALSE means 4 level paging.
+**/
+VOID
+GetPageTable (
+  OUT UINTN   *Base,
+  OUT BOOLEAN *FiveLevels OPTIONAL
+  )
+{
+  *Base = ((mInternalCr3 == 0) ?
+            (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64) :
+            mInternalCr3);
+  if (FiveLevels != NULL) {
+    *FiveLevels = FALSE;
+  }
+}
+
 /**
   Create PageTable for SMM use.
 
@@ -226,6 +246,7 @@ SetPageTableAttributes (
   UINT64                *L1PageTable;
   UINT64                *L2PageTable;
   UINT64                *L3PageTable;
+  UINTN                 PageTableBase;
   BOOLEAN               IsSplitted;
   BOOLEAN               PageTableSplitted;
   BOOLEAN               CetEnabled;
@@ -268,9 +289,10 @@ SetPageTableAttributes (
     DEBUG ((DEBUG_INFO, "Start...\n"));
     PageTableSplitted = FALSE;
 
-    L3PageTable = (UINT64 *)GetPageTableBase ();
+    GetPageTable (&PageTableBase, NULL);
+    L3PageTable = (UINT64 *)PageTableBase;
 
-    SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L3PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted);
+    SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)PageTableBase, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted);
     PageTableSplitted = (PageTableSplitted || IsSplitted);
 
     for (Index3 = 0; Index3 < 4; Index3++) {
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 7fb3a2d9e4..b8aa9e1769 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -263,6 +263,7 @@ extern UINTN                  mMaxNumberOfCpus;
 extern UINTN                  mNumberOfCpus;
 extern EFI_SMM_CPU_PROTOCOL   mSmmCpu;
 extern EFI_MM_MP_PROTOCOL     mSmmMp;
+extern UINTN                  mInternalCr3;
 
 ///
 /// The mode of the CPU at the time an SMI occurs
@@ -942,13 +943,15 @@ SetPageTableAttributes (
   );
 
 /**
-  Return page table base.
+  Get page table base address and the depth of the page table.
 
-  @return page table base.
+  @param[out] Base        Page table base address.
+  @param[out] FiveLevels  TRUE means 5 level paging. FALSE means 4 level paging.
 **/
-UINTN
-GetPageTableBase (
-  VOID
+VOID
+GetPageTable (
+  OUT UINTN   *Base,
+  OUT BOOLEAN *FiveLevels OPTIONAL
   );
 
 /**
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index d67f036aea..766eb1246a 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -49,22 +49,6 @@ SetPageTableBase (
   mInternalCr3 = Cr3;
 }
 
-/**
-  Return page table base.
-
-  @return page table base.
-**/
-UINTN
-GetPageTableBase (
-  VOID
-  )
-{
-  if (mInternalCr3 != 0) {
-    return mInternalCr3;
-  }
-  return (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
-}
-
 /**
   Return length according to page attributes.
 
@@ -131,21 +115,20 @@ GetPageTableEntry (
   UINT64                *L3PageTable;
   UINT64                *L4PageTable;
   UINT64                *L5PageTable;
-  IA32_CR4              Cr4;
+  UINTN                 PageTableBase;
   BOOLEAN               Enable5LevelPaging;
 
+  GetPageTable (&PageTableBase, &Enable5LevelPaging);
+
   Index5 = ((UINTN)RShiftU64 (Address, 48)) & PAGING_PAE_INDEX_MASK;
   Index4 = ((UINTN)RShiftU64 (Address, 39)) & PAGING_PAE_INDEX_MASK;
   Index3 = ((UINTN)Address >> 30) & PAGING_PAE_INDEX_MASK;
   Index2 = ((UINTN)Address >> 21) & PAGING_PAE_INDEX_MASK;
   Index1 = ((UINTN)Address >> 12) & PAGING_PAE_INDEX_MASK;
 
-  Cr4.UintN = AsmReadCr4 ();
-  Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
-
   if (sizeof(UINTN) == sizeof(UINT64)) {
     if (Enable5LevelPaging) {
-      L5PageTable = (UINT64 *)GetPageTableBase ();
+      L5PageTable = (UINT64 *)PageTableBase;
       if (L5PageTable[Index5] == 0) {
         *PageAttribute = PageNone;
         return NULL;
@@ -153,7 +136,7 @@ GetPageTableEntry (
 
       L4PageTable = (UINT64 *)(UINTN)(L5PageTable[Index5] & ~mAddressEncMask & PAGING_4K_ADDRESS_MASK_64);
     } else {
-      L4PageTable = (UINT64 *)GetPageTableBase ();
+      L4PageTable = (UINT64 *)PageTableBase;
     }
     if (L4PageTable[Index4] == 0) {
       *PageAttribute = PageNone;
@@ -162,7 +145,7 @@ GetPageTableEntry (
 
     L3PageTable = (UINT64 *)(UINTN)(L4PageTable[Index4] & ~mAddressEncMask & PAGING_4K_ADDRESS_MASK_64);
   } else {
-    L3PageTable = (UINT64 *)GetPageTableBase ();
+    L3PageTable = (UINT64 *)PageTableBase;
   }
   if (L3PageTable[Index3] == 0) {
     *PageAttribute = PageNone;
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index 810985df20..cdc1fcefc5 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -104,6 +104,35 @@ Is5LevelPagingNeeded (
   }
 }
 
+/**
+  Get page table base address and the depth of the page table.
+
+  @param[out] Base        Page table base address.
+  @param[out] FiveLevels  TRUE means 5 level paging. FALSE means 4 level paging.
+**/
+VOID
+GetPageTable (
+  OUT UINTN   *Base,
+  OUT BOOLEAN *FiveLevels OPTIONAL
+  )
+{
+  IA32_CR4 Cr4;
+
+  if (mInternalCr3 == 0) {
+    *Base = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64;
+    if (FiveLevels != NULL) {
+      Cr4.UintN = AsmReadCr4 ();
+      *FiveLevels = (BOOLEAN)(Cr4.Bits.LA57 == 1);
+    }
+    return;
+  }
+
+  *Base = mInternalCr3;
+  if (FiveLevels != NULL) {
+    *FiveLevels = m5LevelPagingNeeded;
+  }
+}
+
 /**
   Set sub-entries number in entry.
 
@@ -1111,15 +1140,12 @@ SetPageTableAttributes (
   UINT64                *L3PageTable;
   UINT64                *L4PageTable;
   UINT64                *L5PageTable;
+  UINTN                 PageTableBase;
   BOOLEAN               IsSplitted;
   BOOLEAN               PageTableSplitted;
   BOOLEAN               CetEnabled;
-  IA32_CR4              Cr4;
   BOOLEAN               Enable5LevelPaging;
 
-  Cr4.UintN = AsmReadCr4 ();
-  Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
-
   //
   // Don't mark page table memory as read-only if
   //  - no restriction on access to non-SMRAM memory; or
@@ -1163,9 +1189,12 @@ SetPageTableAttributes (
     DEBUG ((DEBUG_INFO, "Start...\n"));
     PageTableSplitted = FALSE;
     L5PageTable = NULL;
+
+    GetPageTable (&PageTableBase, &Enable5LevelPaging);
+
     if (Enable5LevelPaging) {
-      L5PageTable = (UINT64 *)GetPageTableBase ();
-      SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L5PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted);
+      L5PageTable = (UINT64 *)PageTableBase;
+      SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)PageTableBase, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted);
       PageTableSplitted = (PageTableSplitted || IsSplitted);
     }
 
@@ -1176,7 +1205,7 @@ SetPageTableAttributes (
           continue;
         }
       } else {
-        L4PageTable = (UINT64 *)GetPageTableBase ();
+        L4PageTable = (UINT64 *)PageTableBase;
       }
       SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L4PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted);
       PageTableSplitted = (PageTableSplitted || IsSplitted);
-- 
2.16.2.windows.1

Re: [PATCH v8 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address

[Laszlo Ersek]

On 11/16/20 04:18, Sheng Wei wrote:
> When trying to get page table base, if mInternalCr3 is zero, it will use
>  the page table from CR3, and reflect the page table depth by CR4 LA57 bit.
> If mInternalCr3 is non zero, it will use the page table from mInternalCr3
>  and reflect the page table depth of mInternalCr3 at same time.
> In the case of X64, we use m5LevelPagingNeeded to reflect the depth of
>  the page table. And in the case of IA32, it will not the page table depth
>  information.
> 
> This patch is a bug fix when enable CET feature with 5 level paging.
> The SMM page tables are allocated / initialized in PiCpuSmmEntry().
> When CET is enabled, PiCpuSmmEntry() must further modify the attribute of
>  shadow stack pages. This page table is not set to CR3 in PiCpuSmmEntry().
>  So the page table base address is set to mInternalCr3 for modifty the
>  page table attribute. It could not use CR4 LA57 bit to reflect the
>  page table depth for mInternalCr3.
> So we create a architecture-specific implementation GetPageTable() with
>  2 output parameters. One parameter is used to output the page table
>  address. Another parameter is used to reflect if it is 5 level paging
>  or not.
> 
> Correct the Cr3 typo
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3015
> 
> Signed-off-by: Sheng Wei <w.sheng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> 
> Sheng Wei (2):
>   UefiCpuPkg/PiSmmCpuDxeSmm: Correct the Cr3 typo
>   UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table
>     address
> 
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           | 26 ++++++++++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         | 13 ++++---
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 35 +++++-------------
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c            | 43 ++++++++++++++++++----
>  4 files changed, 77 insertions(+), 40 deletions(-)
> 

series
Reviewed-by: Laszlo Ersek <lersek@redhat.com>

The 2nd patch needs an ACK from Eric or Ray; then we can merge this series.

(It is a bugfix so it can go in even during the hard feature freeze. But
we should merge it as soon as we can.)

Thanks,
Laszlo

Re: [PATCH v8 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address

[Sheng Wei]

Hi Eric, Ray,
Could you help to give confirm for merge these patches?
Thank you.
BR
Sheng Wei

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: 2020年11月18日 4:02
> To: Sheng, W <w.sheng@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [PATCH v8 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table
> depth with page table address
> 
> On 11/16/20 04:18, Sheng Wei wrote:
> > When trying to get page table base, if mInternalCr3 is zero, it will
> > use  the page table from CR3, and reflect the page table depth by CR4 LA57
> bit.
> > If mInternalCr3 is non zero, it will use the page table from
> > mInternalCr3  and reflect the page table depth of mInternalCr3 at same time.
> > In the case of X64, we use m5LevelPagingNeeded to reflect the depth of
> > the page table. And in the case of IA32, it will not the page table
> > depth  information.
> >
> > This patch is a bug fix when enable CET feature with 5 level paging.
> > The SMM page tables are allocated / initialized in PiCpuSmmEntry().
> > When CET is enabled, PiCpuSmmEntry() must further modify the attribute
> > of  shadow stack pages. This page table is not set to CR3 in PiCpuSmmEntry().
> >  So the page table base address is set to mInternalCr3 for modifty the
> > page table attribute. It could not use CR4 LA57 bit to reflect the
> > page table depth for mInternalCr3.
> > So we create a architecture-specific implementation GetPageTable()
> > with
> >  2 output parameters. One parameter is used to output the page table
> > address. Another parameter is used to reflect if it is 5 level paging
> > or not.
> >
> > Correct the Cr3 typo
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3015
> >
> > Signed-off-by: Sheng Wei <w.sheng@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> >
> > Sheng Wei (2):
> >   UefiCpuPkg/PiSmmCpuDxeSmm: Correct the Cr3 typo
> >   UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table
> >     address
> >
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           | 26 ++++++++++++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         | 13 ++++---
> >  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 35
> +++++-------------
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c            | 43
> ++++++++++++++++++----
> >  4 files changed, 77 insertions(+), 40 deletions(-)
> >
> 
> series
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> The 2nd patch needs an ACK from Eric or Ray; then we can merge this series.
> 
> (It is a bugfix so it can go in even during the hard feature freeze. But we should
> merge it as soon as we can.)
> 
> Thanks,
> Laszlo

Re: [PATCH v8 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address

[Dong, Eric]

Hi Wei,

Thanks for your patches.

I have include my review-by for it, and create PR https://github.com/tianocore/edk2/pull/1131 for it.

Thanks,
Eric
-----Original Message-----
From: Sheng, W <w.sheng@intel.com> 
Sent: Wednesday, November 18, 2020 9:20 AM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
Cc: Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: RE: [PATCH v8 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address

Hi Eric, Ray,
Could you help to give confirm for merge these patches?
Thank you.
BR
Sheng Wei

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: 2020年11月18日 4:02
> To: Sheng, W <w.sheng@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; 
> Kumar,
> Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [PATCH v8 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page 
> table depth with page table address
> 
> On 11/16/20 04:18, Sheng Wei wrote:
> > When trying to get page table base, if mInternalCr3 is zero, it will 
> > use  the page table from CR3, and reflect the page table depth by 
> > CR4 LA57
> bit.
> > If mInternalCr3 is non zero, it will use the page table from
> > mInternalCr3  and reflect the page table depth of mInternalCr3 at same time.
> > In the case of X64, we use m5LevelPagingNeeded to reflect the depth 
> > of the page table. And in the case of IA32, it will not the page 
> > table depth  information.
> >
> > This patch is a bug fix when enable CET feature with 5 level paging.
> > The SMM page tables are allocated / initialized in PiCpuSmmEntry().
> > When CET is enabled, PiCpuSmmEntry() must further modify the 
> > attribute of  shadow stack pages. This page table is not set to CR3 in PiCpuSmmEntry().
> >  So the page table base address is set to mInternalCr3 for modifty 
> > the page table attribute. It could not use CR4 LA57 bit to reflect 
> > the page table depth for mInternalCr3.
> > So we create a architecture-specific implementation GetPageTable() 
> > with
> >  2 output parameters. One parameter is used to output the page table 
> > address. Another parameter is used to reflect if it is 5 level 
> > paging or not.
> >
> > Correct the Cr3 typo
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3015
> >
> > Signed-off-by: Sheng Wei <w.sheng@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> >
> > Sheng Wei (2):
> >   UefiCpuPkg/PiSmmCpuDxeSmm: Correct the Cr3 typo
> >   UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table
> >     address
> >
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           | 26 ++++++++++++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         | 13 ++++---
> >  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 35
> +++++-------------
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c            | 43
> ++++++++++++++++++----
> >  4 files changed, 77 insertions(+), 40 deletions(-)
> >
> 
> series
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> The 2nd patch needs an ACK from Eric or Ray; then we can merge this series.
> 
> (It is a bugfix so it can go in even during the hard feature freeze. 
> But we should merge it as soon as we can.)
> 
> Thanks,
> Laszlo

Re: [PATCH v8 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address

[Sheng Wei]

Hi Eric
Thank you for the help.
BR
Sheng Wei

> -----Original Message-----
> From: Dong, Eric <eric.dong@intel.com>
> Sent: 2020年11月18日 9:39
> To: Sheng, W <w.sheng@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>
> Subject: RE: [PATCH v8 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table
> depth with page table address
> 
> Hi Wei,
> 
> Thanks for your patches.
> 
> I have include my review-by for it, and create PR
> https://github.com/tianocore/edk2/pull/1131 for it.
> 
> Thanks,
> Eric
> -----Original Message-----
> From: Sheng, W <w.sheng@intel.com>
> Sent: Wednesday, November 18, 2020 9:20 AM
> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Dong, Eric
> <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
> Cc: Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>
> Subject: RE: [PATCH v8 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table
> depth with page table address
> 
> Hi Eric, Ray,
> Could you help to give confirm for merge these patches?
> Thank you.
> BR
> Sheng Wei
> 
> > -----Original Message-----
> > From: Laszlo Ersek <lersek@redhat.com>
> > Sent: 2020年11月18日 4:02
> > To: Sheng, W <w.sheng@intel.com>; devel@edk2.groups.io
> > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > Kumar,
> > Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> > Subject: Re: [PATCH v8 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page
> > table depth with page table address
> >
> > On 11/16/20 04:18, Sheng Wei wrote:
> > > When trying to get page table base, if mInternalCr3 is zero, it will
> > > use  the page table from CR3, and reflect the page table depth by
> > > CR4 LA57
> > bit.
> > > If mInternalCr3 is non zero, it will use the page table from
> > > mInternalCr3  and reflect the page table depth of mInternalCr3 at same
> time.
> > > In the case of X64, we use m5LevelPagingNeeded to reflect the depth
> > > of the page table. And in the case of IA32, it will not the page
> > > table depth  information.
> > >
> > > This patch is a bug fix when enable CET feature with 5 level paging.
> > > The SMM page tables are allocated / initialized in PiCpuSmmEntry().
> > > When CET is enabled, PiCpuSmmEntry() must further modify the
> > > attribute of  shadow stack pages. This page table is not set to CR3 in
> PiCpuSmmEntry().
> > >  So the page table base address is set to mInternalCr3 for modifty
> > > the page table attribute. It could not use CR4 LA57 bit to reflect
> > > the page table depth for mInternalCr3.
> > > So we create a architecture-specific implementation GetPageTable()
> > > with
> > >  2 output parameters. One parameter is used to output the page table
> > > address. Another parameter is used to reflect if it is 5 level
> > > paging or not.
> > >
> > > Correct the Cr3 typo
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3015
> > >
> > > Signed-off-by: Sheng Wei <w.sheng@intel.com>
> > > Cc: Eric Dong <eric.dong@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > >
> > > Sheng Wei (2):
> > >   UefiCpuPkg/PiSmmCpuDxeSmm: Correct the Cr3 typo
> > >   UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table
> > >     address
> > >
> > >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           | 26 ++++++++++++-
> > >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         | 13 ++++---
> > >  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 35
> > +++++-------------
> > >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c            | 43
> > ++++++++++++++++++----
> > >  4 files changed, 77 insertions(+), 40 deletions(-)
> > >
> >
> > series
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >
> > The 2nd patch needs an ACK from Eric or Ray; then we can merge this series.
> >
> > (It is a bugfix so it can go in even during the hard feature freeze.
> > But we should merge it as soon as we can.)
> >
> > Thanks,
> > Laszlo