[PATCH 0/4] Support 5-level paging in DXE long mode

[PATCH 0/4] Support 5-level paging in DXE long mode

[Ni, Ray]

Ray Ni (4):
  UefiCpuPkg/MpInitLib: Enable 5-level paging for AP when BSP's enabled
  UefiCpuPkg/CpuDxe: Support parsing 5-level page table
  MdeModulePkg/DxeIpl: Introduce PCD PcdUse5LevelPageTable
  MdeModulePkg/DxeIpl: Create 5-level page table for long mode

 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf       |   1 +
 .../Core/DxeIplPeim/X64/VirtualMemory.c       | 227 ++++++++++++------
 MdeModulePkg/MdeModulePkg.dec                 |   7 +
 UefiCpuPkg/CpuDxe/CpuPageTable.c              |  22 +-
 UefiCpuPkg/Library/MpInitLib/MpLib.c          |  11 +
 UefiCpuPkg/Library/MpInitLib/MpLib.h          |   6 +-
 UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc    |   3 +-
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm |  14 +-
 8 files changed, 209 insertions(+), 82 deletions(-)

-- 
2.21.0.windows.1

[PATCH 1/4] UefiCpuPkg/MpInitLib: Enable 5-level paging for AP when BSP's enabled

[Ni, Ray]

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

MpInitLib is the library that's responsible to wake up APs to provide
MP PPI and Protocol services.

The patch synchronizes BSP's CR4.LA57 to each AP's CR4.LA57.
Without this change, AP may enter to GP fault when BSP's 5-level page
table is set to AP during AP wakes up.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c          | 11 +++++++++++
 UefiCpuPkg/Library/MpInitLib/MpLib.h          |  6 +++++-
 UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc    |  3 ++-
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 14 +++++++++++++-
 4 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 6f51bc4ebf..e4691315e9 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -790,6 +790,7 @@ FillExchangeInfoData (
   volatile MP_CPU_EXCHANGE_INFO    *ExchangeInfo;
   UINTN                            Size;
   IA32_SEGMENT_DESCRIPTOR          *Selector;
+  IA32_CR4                         Cr4;
 
   ExchangeInfo                  = CpuMpData->MpCpuExchangeInfo;
   ExchangeInfo->Lock            = 0;
@@ -814,6 +815,16 @@ FillExchangeInfoData (
 
   ExchangeInfo->InitializeFloatingPointUnitsAddress = (UINTN)InitializeFloatingPointUnits;
 
+  //
+  // We can check either CPUID(7).ECX[bit16] or check CR4.LA57[bit12]
+  //  to determin whether 5-Level Paging is enabled.
+  // Using latter way is simpler because it also eliminates the needs to
+  //  check whether platform wants to enable it.
+  //
+  Cr4.UintN = AsmReadCr4 ();
+  ExchangeInfo->Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
+  DEBUG ((DEBUG_INFO, "CpuMp: 5-Level Paging = %d\n", ExchangeInfo->Enable5LevelPaging));
+
   //
   // Get the BSP's data of GDT and IDT
   //
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index f89037c59e..fa7d6b32e9 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -1,7 +1,7 @@
 /** @file
   Common header file for MP Initialize Library.
 
-  Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -185,6 +185,10 @@ typedef struct {
   UINT16                ModeTransitionSegment;
   UINT32                ModeHighMemory;
   UINT16                ModeHighSegment;
+  //
+  // Enable5LevelPaging indicates whether 5-level paging is enabled in long mode.
+  //
+  UINTN                 Enable5LevelPaging;
 } MP_CPU_EXCHANGE_INFO;
 
 #pragma pack()
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
index 467f54a860..58ef369342 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
@@ -1,5 +1,5 @@
 ;------------------------------------------------------------------------------ ;
-; Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR>
+; Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
 ; SPDX-License-Identifier: BSD-2-Clause-Patent
 ;
 ; Module Name:
@@ -40,3 +40,4 @@ ModeTransitionMemoryLocation        equ  LockLocation + 94h
 ModeTransitionSegmentLocation       equ  LockLocation + 98h
 ModeHighMemoryLocation              equ  LockLocation + 9Ah
 ModeHighSegmentLocation             equ  LockLocation + 9Eh
+Enable5LevelPagingLocation          equ  LockLocation + 0A0h
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index cea90f3d4d..b563c2ed3e 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -1,5 +1,5 @@
 ;------------------------------------------------------------------------------ ;
-; Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
+; Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
 ; SPDX-License-Identifier: BSD-2-Clause-Patent
 ;
 ; Module Name:
@@ -100,6 +100,18 @@ SkipEnableExecuteDisableBit:
     ;
     mov        eax, cr4
     bts        eax, 5
+
+    mov        esi, Enable5LevelPagingLocation
+    cmp        byte [ebx + esi], 0
+    jz         SkipEnable5Paging
+
+    ;
+    ; Enable 5 Level Paging
+    ;
+    bts        eax, 12                     ; Set LA57=1.
+
+SkipEnable5Paging:
+
     mov        cr4, eax
 
     ;
-- 
2.21.0.windows.1

Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: Enable 5-level paging for AP when BSP's enabled

[Laszlo Ersek]

Later (after more feedback has been collected), I would like to
regression-test this series; for now, just some superficial comments:

On 07/22/19 10:15, Ni, Ray wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
> 
> MpInitLib is the library that's responsible to wake up APs to provide
> MP PPI and Protocol services.
> 
> The patch synchronizes BSP's CR4.LA57 to each AP's CR4.LA57.
> Without this change, AP may enter to GP fault when BSP's 5-level page
> table is set to AP during AP wakes up.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c          | 11 +++++++++++
>  UefiCpuPkg/Library/MpInitLib/MpLib.h          |  6 +++++-
>  UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc    |  3 ++-
>  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 14 +++++++++++++-
>  4 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 6f51bc4ebf..e4691315e9 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -790,6 +790,7 @@ FillExchangeInfoData (
>    volatile MP_CPU_EXCHANGE_INFO    *ExchangeInfo;
>    UINTN                            Size;
>    IA32_SEGMENT_DESCRIPTOR          *Selector;
> +  IA32_CR4                         Cr4;
>  
>    ExchangeInfo                  = CpuMpData->MpCpuExchangeInfo;
>    ExchangeInfo->Lock            = 0;
> @@ -814,6 +815,16 @@ FillExchangeInfoData (
>  
>    ExchangeInfo->InitializeFloatingPointUnitsAddress = (UINTN)InitializeFloatingPointUnits;
>  
> +  //
> +  // We can check either CPUID(7).ECX[bit16] or check CR4.LA57[bit12]
> +  //  to determin whether 5-Level Paging is enabled.
> +  // Using latter way is simpler because it also eliminates the needs to
> +  //  check whether platform wants to enable it.
> +  //
> +  Cr4.UintN = AsmReadCr4 ();

(1) Are the above checks (CPUID and CR4) interchangeable on AMD
processors too?

> +  ExchangeInfo->Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
> +  DEBUG ((DEBUG_INFO, "CpuMp: 5-Level Paging = %d\n", ExchangeInfo->Enable5LevelPaging));
> +
>    //
>    // Get the BSP's data of GDT and IDT
>    //

(2) Quite unimportant comment, but I might as well make it:

- In library code, it's best to refer to actual module names with
gEfiCallerBaseName. "CpuMp" isn't ideal in this log message.

- "ExchangeInfo->Enable5LevelPaging" is a UINTN; we shouldn't log it
with %d. The portable logging for UINTN is to cast it to UINT64, and
print it with %Lu. In this particular case, we know it's either 0 or 1,
so we can print it with %d too, but then we should cast it to INT32.

> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index f89037c59e..fa7d6b32e9 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -1,7 +1,7 @@
>  /** @file
>    Common header file for MP Initialize Library.
>  
> -  Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  **/
> @@ -185,6 +185,10 @@ typedef struct {
>    UINT16                ModeTransitionSegment;
>    UINT32                ModeHighMemory;
>    UINT16                ModeHighSegment;
> +  //
> +  // Enable5LevelPaging indicates whether 5-level paging is enabled in long mode.
> +  //
> +  UINTN                 Enable5LevelPaging;
>  } MP_CPU_EXCHANGE_INFO;
>  
>  #pragma pack()
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> index 467f54a860..58ef369342 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> @@ -1,5 +1,5 @@
>  ;------------------------------------------------------------------------------ ;
> -; Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR>
> +; Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
>  ; SPDX-License-Identifier: BSD-2-Clause-Patent
>  ;
>  ; Module Name:
> @@ -40,3 +40,4 @@ ModeTransitionMemoryLocation        equ  LockLocation + 94h
>  ModeTransitionSegmentLocation       equ  LockLocation + 98h
>  ModeHighMemoryLocation              equ  LockLocation + 9Ah
>  ModeHighSegmentLocation             equ  LockLocation + 9Eh
> +Enable5LevelPagingLocation          equ  LockLocation + 0A0h

(3) Any particular reason for "0A0h" rather than just "A0h"?

> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index cea90f3d4d..b563c2ed3e 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -1,5 +1,5 @@
>  ;------------------------------------------------------------------------------ ;
> -; Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
> +; Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
>  ; SPDX-License-Identifier: BSD-2-Clause-Patent
>  ;
>  ; Module Name:
> @@ -100,6 +100,18 @@ SkipEnableExecuteDisableBit:
>      ;
>      mov        eax, cr4
>      bts        eax, 5
> +
> +    mov        esi, Enable5LevelPagingLocation
> +    cmp        byte [ebx + esi], 0

(4) If we use a byte comparison here, why don't we make the field itself
a UINT8 (or even BOOLEAN)? The MP_CPU_EXCHANGE_INFO structure is packed.

(If we still want to use a whole UINTN for this purpose, then I think
the zero comparison should cover the whole field.)

> +    jz         SkipEnable5Paging
> +
> +    ;
> +    ; Enable 5 Level Paging
> +    ;
> +    bts        eax, 12                     ; Set LA57=1.
> +
> +SkipEnable5Paging:

(5) Not too important, but we might as well be consistent with the
naming elsewhere, and call this "SkipEnable5LevelPaging". Up to you.

Thanks
Laszlo

> +
>      mov        cr4, eax
>  
>      ;
> 

Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: Enable 5-level paging for AP when BSP's enabled

[Ni, Ray]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, July 23, 2019 5:15 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: Enable 5-level
> paging for AP when BSP's enabled
> 
> Later (after more feedback has been collected), I would like to regression-
> test this series; for now, just some superficial comments:
> 
> On 07/22/19 10:15, Ni, Ray wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
> >
> > MpInitLib is the library that's responsible to wake up APs to provide
> > MP PPI and Protocol services.
> >
> > The patch synchronizes BSP's CR4.LA57 to each AP's CR4.LA57.
> > Without this change, AP may enter to GP fault when BSP's 5-level page
> > table is set to AP during AP wakes up.
> >
> > Signed-off-by: Ray Ni <ray.ni@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > ---
> >  UefiCpuPkg/Library/MpInitLib/MpLib.c          | 11 +++++++++++
> >  UefiCpuPkg/Library/MpInitLib/MpLib.h          |  6 +++++-
> >  UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc    |  3 ++-
> >  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 14 +++++++++++++-
> >  4 files changed, 31 insertions(+), 3 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index 6f51bc4ebf..e4691315e9 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -790,6 +790,7 @@ FillExchangeInfoData (
> >    volatile MP_CPU_EXCHANGE_INFO    *ExchangeInfo;
> >    UINTN                            Size;
> >    IA32_SEGMENT_DESCRIPTOR          *Selector;
> > +  IA32_CR4                         Cr4;
> >
> >    ExchangeInfo                  = CpuMpData->MpCpuExchangeInfo;
> >    ExchangeInfo->Lock            = 0;
> > @@ -814,6 +815,16 @@ FillExchangeInfoData (
> >
> >    ExchangeInfo->InitializeFloatingPointUnitsAddress =
> > (UINTN)InitializeFloatingPointUnits;
> >
> > +  //
> > +  // We can check either CPUID(7).ECX[bit16] or check CR4.LA57[bit12]
> > + //  to determin whether 5-Level Paging is enabled.
> > +  // Using latter way is simpler because it also eliminates the needs
> > + to  //  check whether platform wants to enable it.
> > +  //
> > +  Cr4.UintN = AsmReadCr4 ();
> 
> (1) Are the above checks (CPUID and CR4) interchangeable on AMD
> processors too?

I am not sure about that really.
Since Intel and AMD has magically aligned each bit in control registers for
quite a long period, I don't see big risk here for 5-level paging support on
future AMD processors😊

> 
> > +  ExchangeInfo->Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
> > + DEBUG ((DEBUG_INFO, "CpuMp: 5-Level Paging = %d\n",
> > + ExchangeInfo->Enable5LevelPaging));
> > +
> >    //
> >    // Get the BSP's data of GDT and IDT
> >    //
> 
> (2) Quite unimportant comment, but I might as well make it:
> 
> - In library code, it's best to refer to actual module names with
> gEfiCallerBaseName. "CpuMp" isn't ideal in this log message.
> 
> - "ExchangeInfo->Enable5LevelPaging" is a UINTN; we shouldn't log it
> with %d. The portable logging for UINTN is to cast it to UINT64, and print it
> with %Lu. In this particular case, we know it's either 0 or 1, so we can print it
> with %d too, but then we should cast it to INT32.

Sure. Will address them in V2.

> 
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > index f89037c59e..fa7d6b32e9 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > @@ -1,7 +1,7 @@
> >  /** @file
> >    Common header file for MP Initialize Library.
> >
> > -  Copyright (c) 2016 - 2018, Intel Corporation. All rights
> > reserved.<BR>
> > +  Copyright (c) 2016 - 2019, Intel Corporation. All rights
> > + reserved.<BR>
> >    SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >  **/
> > @@ -185,6 +185,10 @@ typedef struct {
> >    UINT16                ModeTransitionSegment;
> >    UINT32                ModeHighMemory;
> >    UINT16                ModeHighSegment;
> > +  //
> > +  // Enable5LevelPaging indicates whether 5-level paging is enabled in long
> mode.
> > +  //
> > +  UINTN                 Enable5LevelPaging;
> >  } MP_CPU_EXCHANGE_INFO;
> >
> >  #pragma pack()
> > diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> > b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> > index 467f54a860..58ef369342 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> > +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> > @@ -1,5 +1,5 @@
> >
> > ;---------------------------------------------------------------------
> > --------- ; -; Copyright (c) 2015 - 2017, Intel Corporation. All
> > rights reserved.<BR>
> > +; Copyright (c) 2015 - 2019, Intel Corporation. All rights
> > +reserved.<BR>
> >  ; SPDX-License-Identifier: BSD-2-Clause-Patent  ;  ; Module Name:
> > @@ -40,3 +40,4 @@ ModeTransitionMemoryLocation        equ
> LockLocation + 94h
> >  ModeTransitionSegmentLocation       equ  LockLocation + 98h
> >  ModeHighMemoryLocation              equ  LockLocation + 9Ah
> >  ModeHighSegmentLocation             equ  LockLocation + 9Eh
> > +Enable5LevelPagingLocation          equ  LockLocation + 0A0h
> 
> (3) Any particular reason for "0A0h" rather than just "A0h"?

Originally I had some idea to make sure "0-9" in the beginning.
Now you ask, as long as build passes I don't see reason to have 0 as prefix.

> 
> > diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> > b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> > index cea90f3d4d..b563c2ed3e 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> > +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> > @@ -1,5 +1,5 @@
> >
> > ;---------------------------------------------------------------------
> > --------- ; -; Copyright (c) 2015 - 2018, Intel Corporation. All
> > rights reserved.<BR>
> > +; Copyright (c) 2015 - 2019, Intel Corporation. All rights
> > +reserved.<BR>
> >  ; SPDX-License-Identifier: BSD-2-Clause-Patent  ;  ; Module Name:
> > @@ -100,6 +100,18 @@ SkipEnableExecuteDisableBit:
> >      ;
> >      mov        eax, cr4
> >      bts        eax, 5
> > +
> > +    mov        esi, Enable5LevelPagingLocation
> > +    cmp        byte [ebx + esi], 0
> 
> (4) If we use a byte comparison here, why don't we make the field itself a
> UINT8 (or even BOOLEAN)? The MP_CPU_EXCHANGE_INFO structure is
> packed.

Sure. Will address them in V2.

> 
> (If we still want to use a whole UINTN for this purpose, then I think the zero
> comparison should cover the whole field.)
> 
> > +    jz         SkipEnable5Paging
> > +
> > +    ;
> > +    ; Enable 5 Level Paging
> > +    ;
> > +    bts        eax, 12                     ; Set LA57=1.
> > +
> > +SkipEnable5Paging:
> 
> (5) Not too important, but we might as well be consistent with the naming
> elsewhere, and call this "SkipEnable5LevelPaging". Up to you.
> 
Sure. Will address them in V2.
> Thanks
> Laszlo
> 
> > +
> >      mov        cr4, eax
> >
> >      ;
> >

[PATCH 2/4] UefiCpuPkg/CpuDxe: Support parsing 5-level page table

[Ni, Ray]

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

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index c369b44f12..8e959eb2b7 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -1,7 +1,7 @@
 /** @file
   Page table management support.
 
-  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2017 - 2019, Intel Corporation. All rights reserved.<BR>
   Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -276,25 +276,43 @@ GetPageTableEntry (
   UINTN                 Index2;
   UINTN                 Index3;
   UINTN                 Index4;
+  UINTN                 Index5;
   UINT64                *L1PageTable;
   UINT64                *L2PageTable;
   UINT64                *L3PageTable;
   UINT64                *L4PageTable;
+  UINT64                *L5PageTable;
   UINT64                AddressEncMask;
+  IA32_CR4              Cr4;
+  BOOLEAN               Enable5LevelPaging;
 
   ASSERT (PagingContext != NULL);
 
+  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);
+
   // Make sure AddressEncMask is contained to smallest supported address field.
   //
   AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) & PAGING_1G_ADDRESS_MASK_64;
 
   if (PagingContext->MachineType == IMAGE_FILE_MACHINE_X64) {
-    L4PageTable = (UINT64 *)(UINTN)PagingContext->ContextData.X64.PageTableBase;
+    if (Enable5LevelPaging) {
+      L5PageTable = (UINT64 *)(UINTN)PagingContext->ContextData.X64.PageTableBase;
+      if (L5PageTable[Index5] == 0) {
+        *PageAttribute = PageNone;
+        return NULL;
+      }
+
+      L4PageTable = (UINT64 *)(UINTN)(L5PageTable[Index5] & ~AddressEncMask & PAGING_4K_ADDRESS_MASK_64);
+    } else {
+      L4PageTable = (UINT64 *)(UINTN)PagingContext->ContextData.X64.PageTableBase;
+    }
     if (L4PageTable[Index4] == 0) {
       *PageAttribute = PageNone;
       return NULL;
-- 
2.21.0.windows.1

Re: [edk2-devel] [PATCH 2/4] UefiCpuPkg/CpuDxe: Support parsing 5-level page table

[Laszlo Ersek]

On 07/22/19 10:15, Ni, Ray wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index c369b44f12..8e959eb2b7 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Page table management support.
>  
> -  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2017 - 2019, Intel Corporation. All rights reserved.<BR>
>    Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -276,25 +276,43 @@ GetPageTableEntry (
>    UINTN                 Index2;
>    UINTN                 Index3;
>    UINTN                 Index4;
> +  UINTN                 Index5;
>    UINT64                *L1PageTable;
>    UINT64                *L2PageTable;
>    UINT64                *L3PageTable;
>    UINT64                *L4PageTable;
> +  UINT64                *L5PageTable;
>    UINT64                AddressEncMask;
> +  IA32_CR4              Cr4;
> +  BOOLEAN               Enable5LevelPaging;
>  
>    ASSERT (PagingContext != NULL);
>  
> +  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);
> +
>    // Make sure AddressEncMask is contained to smallest supported address field.
>    //
>    AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) & PAGING_1G_ADDRESS_MASK_64;
>  
>    if (PagingContext->MachineType == IMAGE_FILE_MACHINE_X64) {
> -    L4PageTable = (UINT64 *)(UINTN)PagingContext->ContextData.X64.PageTableBase;
> +    if (Enable5LevelPaging) {
> +      L5PageTable = (UINT64 *)(UINTN)PagingContext->ContextData.X64.PageTableBase;
> +      if (L5PageTable[Index5] == 0) {
> +        *PageAttribute = PageNone;
> +        return NULL;
> +      }
> +
> +      L4PageTable = (UINT64 *)(UINTN)(L5PageTable[Index5] & ~AddressEncMask & PAGING_4K_ADDRESS_MASK_64);
> +    } else {
> +      L4PageTable = (UINT64 *)(UINTN)PagingContext->ContextData.X64.PageTableBase;
> +    }
>      if (L4PageTable[Index4] == 0) {
>        *PageAttribute = PageNone;
>        return NULL;
> 

The patch seems to be a no-op if Enable5LevelPaging is FALSE, so that's
good.

Questions:

(1) Same question as under patch #1: is the CR4 check reliable on AMD
processors too?

(2) Should we read CR4 (or call CPUID) every time this function is
invoked? Can we perform the check at CpuDxe startup, and cache the result?

(3) Should we consider the PCD (from patch #3) before accessing the
hardware (CR4 / CPUID alike)?

Thanks
Laszlo

Re: [edk2-devel] [PATCH 2/4] UefiCpuPkg/CpuDxe: Support parsing 5-level page table

[Ni, Ray]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, July 23, 2019 5:23 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH 2/4] UefiCpuPkg/CpuDxe: Support parsing
> 5-level page table
> 
> On 07/22/19 10:15, Ni, Ray wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
> >
> > Signed-off-by: Ray Ni <ray.ni@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > ---
> >  UefiCpuPkg/CpuDxe/CpuPageTable.c | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > index c369b44f12..8e959eb2b7 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >    Page table management support.
> >
> > -  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> > +  Copyright (c) 2017 - 2019, Intel Corporation. All rights
> > + reserved.<BR>
> >    Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> >
> >    SPDX-License-Identifier: BSD-2-Clause-Patent @@ -276,25 +276,43 @@
> > GetPageTableEntry (
> >    UINTN                 Index2;
> >    UINTN                 Index3;
> >    UINTN                 Index4;
> > +  UINTN                 Index5;
> >    UINT64                *L1PageTable;
> >    UINT64                *L2PageTable;
> >    UINT64                *L3PageTable;
> >    UINT64                *L4PageTable;
> > +  UINT64                *L5PageTable;
> >    UINT64                AddressEncMask;
> > +  IA32_CR4              Cr4;
> > +  BOOLEAN               Enable5LevelPaging;
> >
> >    ASSERT (PagingContext != NULL);
> >
> > +  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);
> > +
> >    // Make sure AddressEncMask is contained to smallest supported address
> field.
> >    //
> >    AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask)
> &
> > PAGING_1G_ADDRESS_MASK_64;
> >
> >    if (PagingContext->MachineType == IMAGE_FILE_MACHINE_X64) {
> > -    L4PageTable = (UINT64 *)(UINTN)PagingContext-
> >ContextData.X64.PageTableBase;
> > +    if (Enable5LevelPaging) {
> > +      L5PageTable = (UINT64 *)(UINTN)PagingContext-
> >ContextData.X64.PageTableBase;
> > +      if (L5PageTable[Index5] == 0) {
> > +        *PageAttribute = PageNone;
> > +        return NULL;
> > +      }
> > +
> > +      L4PageTable = (UINT64 *)(UINTN)(L5PageTable[Index5] &
> ~AddressEncMask & PAGING_4K_ADDRESS_MASK_64);
> > +    } else {
> > +      L4PageTable = (UINT64 *)(UINTN)PagingContext-
> >ContextData.X64.PageTableBase;
> > +    }
> >      if (L4PageTable[Index4] == 0) {
> >        *PageAttribute = PageNone;
> >        return NULL;
> >
> 
> The patch seems to be a no-op if Enable5LevelPaging is FALSE, so that's good.
> 
> Questions:
> 
> (1) Same question as under patch #1: is the CR4 check reliable on AMD
> processors too?

Replied in 1/4 thread.

> 
> (2) Should we read CR4 (or call CPUID) every time this function is invoked?
> Can we perform the check at CpuDxe startup, and cache the result?
It's a good suggestion. Will address it in V2.


> 
> (3) Should we consider the PCD (from patch #3) before accessing the
> hardware (CR4 / CPUID alike)?
I want to avoid touching PCD in this CPU driver.
All the information is taken in DxeIpl and pass to CPU driver through Cr4.LA57.

> 
> Thanks
> Laszlo

Re: [edk2-devel] [PATCH 2/4] UefiCpuPkg/CpuDxe: Support parsing 5-level page table

[Laszlo Ersek]

On 07/24/19 09:03, Ni, Ray wrote:
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Tuesday, July 23, 2019 5:23 PM
>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
>> Cc: Dong, Eric <eric.dong@intel.com>
>> Subject: Re: [edk2-devel] [PATCH 2/4] UefiCpuPkg/CpuDxe: Support parsing
>> 5-level page table
>>
>> On 07/22/19 10:15, Ni, Ray wrote:
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
>>>
>>> Signed-off-by: Ray Ni <ray.ni@intel.com>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 22 ++++++++++++++++++++--
>>>  1 file changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> index c369b44f12..8e959eb2b7 100644
>>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> @@ -1,7 +1,7 @@
>>>  /** @file
>>>    Page table management support.
>>>
>>> -  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>>> +  Copyright (c) 2017 - 2019, Intel Corporation. All rights
>>> + reserved.<BR>
>>>    Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
>>>
>>>    SPDX-License-Identifier: BSD-2-Clause-Patent @@ -276,25 +276,43 @@
>>> GetPageTableEntry (
>>>    UINTN                 Index2;
>>>    UINTN                 Index3;
>>>    UINTN                 Index4;
>>> +  UINTN                 Index5;
>>>    UINT64                *L1PageTable;
>>>    UINT64                *L2PageTable;
>>>    UINT64                *L3PageTable;
>>>    UINT64                *L4PageTable;
>>> +  UINT64                *L5PageTable;
>>>    UINT64                AddressEncMask;
>>> +  IA32_CR4              Cr4;
>>> +  BOOLEAN               Enable5LevelPaging;
>>>
>>>    ASSERT (PagingContext != NULL);
>>>
>>> +  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);
>>> +
>>>    // Make sure AddressEncMask is contained to smallest supported address
>> field.
>>>    //
>>>    AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask)
>> &
>>> PAGING_1G_ADDRESS_MASK_64;
>>>
>>>    if (PagingContext->MachineType == IMAGE_FILE_MACHINE_X64) {
>>> -    L4PageTable = (UINT64 *)(UINTN)PagingContext-
>>> ContextData.X64.PageTableBase;
>>> +    if (Enable5LevelPaging) {
>>> +      L5PageTable = (UINT64 *)(UINTN)PagingContext-
>>> ContextData.X64.PageTableBase;
>>> +      if (L5PageTable[Index5] == 0) {
>>> +        *PageAttribute = PageNone;
>>> +        return NULL;
>>> +      }
>>> +
>>> +      L4PageTable = (UINT64 *)(UINTN)(L5PageTable[Index5] &
>> ~AddressEncMask & PAGING_4K_ADDRESS_MASK_64);
>>> +    } else {
>>> +      L4PageTable = (UINT64 *)(UINTN)PagingContext-
>>> ContextData.X64.PageTableBase;
>>> +    }
>>>      if (L4PageTable[Index4] == 0) {
>>>        *PageAttribute = PageNone;
>>>        return NULL;
>>>
>>
>> The patch seems to be a no-op if Enable5LevelPaging is FALSE, so that's good.
>>
>> Questions:
>>
>> (1) Same question as under patch #1: is the CR4 check reliable on AMD
>> processors too?
> 
> Replied in 1/4 thread.
> 
>>
>> (2) Should we read CR4 (or call CPUID) every time this function is invoked?
>> Can we perform the check at CpuDxe startup, and cache the result?
> It's a good suggestion. Will address it in V2.
> 
> 
>>
>> (3) Should we consider the PCD (from patch #3) before accessing the
>> hardware (CR4 / CPUID alike)?
> I want to avoid touching PCD in this CPU driver.
> All the information is taken in DxeIpl and pass to CPU driver through Cr4.LA57.

Makes sense, thanks.
Laszlo

[PATCH 3/4] MdeModulePkg/DxeIpl: Introduce PCD PcdUse5LevelPageTable

[Ni, Ray]

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

The PCD indicates if 5-Level Paging will be enabled in long mode.
5-Level Paging will not be enabled when the PCD is TRUE but CPU
doesn't support 5-Level Paging.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/MdeModulePkg.dec | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 12e0bbf579..21388595a9 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1991,6 +1991,13 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   # @Prompt The address mask when memory encryption is enabled.
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0|UINT64|0x30001047
 
+  ## Indicates if 5-Level Paging will be enabled in long mode. 5-Level Paging will not be enabled
+  #  when the PCD is TRUE but CPU doesn't support 5-Level Paging.
+  #   TRUE  - 5-Level Paging will be enabled.<BR>
+  #   FALSE - 5-Level Paging will not be enabled.<BR>
+  # @Prompt Enable 5-Level Paging support in long mode. 
+  gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable|FALSE|BOOLEAN|0x0001105F
+
   ## Capsule In Ram is to use memory to deliver the capsules that will be processed after system
   #  reset.<BR><BR>
   #  This PCD indicates if the Capsule In Ram is supported.<BR>
-- 
2.21.0.windows.1

Re: [edk2-devel] [PATCH 3/4] MdeModulePkg/DxeIpl: Introduce PCD PcdUse5LevelPageTable

[Wu, Hao A]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni,
> Ray
> Sent: Monday, July 22, 2019 4:16 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric; Laszlo Ersek
> Subject: [edk2-devel] [PATCH 3/4] MdeModulePkg/DxeIpl: Introduce PCD
> PcdUse5LevelPageTable
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
> 
> The PCD indicates if 5-Level Paging will be enabled in long mode.
> 5-Level Paging will not be enabled when the PCD is TRUE but CPU
> doesn't support 5-Level Paging.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  MdeModulePkg/MdeModulePkg.dec | 7 +++++++


Hello,

Please help to add the PCD description string definitions in MdeModulePkg.uni.

One question, is this PCD introduced for the consideration of memory
consumption by the page table entries? Or is there any other purpose?

Best Regards,
Hao Wu


>  1 file changed, 7 insertions(+)
> 
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec
> index 12e0bbf579..21388595a9 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -1991,6 +1991,13 @@ [PcdsFixedAtBuild, PcdsPatchableInModule,
> PcdsDynamic, PcdsDynamicEx]
>    # @Prompt The address mask when memory encryption is enabled.
> 
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
> ask|0x0|UINT64|0x30001047
> 
> +  ## Indicates if 5-Level Paging will be enabled in long mode. 5-Level Paging
> will not be enabled
> +  #  when the PCD is TRUE but CPU doesn't support 5-Level Paging.
> +  #   TRUE  - 5-Level Paging will be enabled.<BR>
> +  #   FALSE - 5-Level Paging will not be enabled.<BR>
> +  # @Prompt Enable 5-Level Paging support in long mode.
> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable|FALSE|BOOLE
> AN|0x0001105F
> +
>    ## Capsule In Ram is to use memory to deliver the capsules that will be
> processed after system
>    #  reset.<BR><BR>
>    #  This PCD indicates if the Capsule In Ram is supported.<BR>
> --
> 2.21.0.windows.1
> 
> 
> 

Re: [edk2-devel] [PATCH 3/4] MdeModulePkg/DxeIpl: Introduce PCD PcdUse5LevelPageTable

[Ni, Ray]

Hao,
Certain OS loaders may fail to boot when 5-level paging is used. PCD can be used to restrict to 4-level paging.

> -----Original Message-----
> From: Wu, Hao A
> Sent: Tuesday, July 23, 2019 10:06 AM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [edk2-devel] [PATCH 3/4] MdeModulePkg/DxeIpl: Introduce PCD PcdUse5LevelPageTable
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni,
> > Ray
> > Sent: Monday, July 22, 2019 4:16 PM
> > To: devel@edk2.groups.io
> > Cc: Dong, Eric; Laszlo Ersek
> > Subject: [edk2-devel] [PATCH 3/4] MdeModulePkg/DxeIpl: Introduce PCD
> > PcdUse5LevelPageTable
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
> >
> > The PCD indicates if 5-Level Paging will be enabled in long mode.
> > 5-Level Paging will not be enabled when the PCD is TRUE but CPU
> > doesn't support 5-Level Paging.
> >
> > Signed-off-by: Ray Ni <ray.ni@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > ---
> >  MdeModulePkg/MdeModulePkg.dec | 7 +++++++
> 
> 
> Hello,
> 
> Please help to add the PCD description string definitions in MdeModulePkg.uni.
> 
> One question, is this PCD introduced for the consideration of memory
> consumption by the page table entries? Or is there any other purpose?
> 
> Best Regards,
> Hao Wu
> 
> 
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec
> > index 12e0bbf579..21388595a9 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -1991,6 +1991,13 @@ [PcdsFixedAtBuild, PcdsPatchableInModule,
> > PcdsDynamic, PcdsDynamicEx]
> >    # @Prompt The address mask when memory encryption is enabled.
> >
> > gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
> > ask|0x0|UINT64|0x30001047
> >
> > +  ## Indicates if 5-Level Paging will be enabled in long mode. 5-Level Paging
> > will not be enabled
> > +  #  when the PCD is TRUE but CPU doesn't support 5-Level Paging.
> > +  #   TRUE  - 5-Level Paging will be enabled.<BR>
> > +  #   FALSE - 5-Level Paging will not be enabled.<BR>
> > +  # @Prompt Enable 5-Level Paging support in long mode.
> > +
> > gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable|FALSE|BOOLE
> > AN|0x0001105F
> > +
> >    ## Capsule In Ram is to use memory to deliver the capsules that will be
> > processed after system
> >    #  reset.<BR><BR>
> >    #  This PCD indicates if the Capsule In Ram is supported.<BR>
> > --
> > 2.21.0.windows.1
> >
> >
> > 

Re: [PATCH 3/4] MdeModulePkg/DxeIpl: Introduce PCD PcdUse5LevelPageTable

[Dong, Eric]

Reviewed-by: Eric Dong <eric.dong@intel.com>

> -----Original Message-----
> From: Ni, Ray
> Sent: Monday, July 22, 2019 4:16 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH 3/4] MdeModulePkg/DxeIpl: Introduce PCD
> PcdUse5LevelPageTable
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
> 
> The PCD indicates if 5-Level Paging will be enabled in long mode.
> 5-Level Paging will not be enabled when the PCD is TRUE but CPU doesn't
> support 5-Level Paging.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  MdeModulePkg/MdeModulePkg.dec | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec index 12e0bbf579..21388595a9
> 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -1991,6 +1991,13 @@ [PcdsFixedAtBuild, PcdsPatchableInModule,
> PcdsDynamic, PcdsDynamicEx]
>    # @Prompt The address mask when memory encryption is enabled.
> 
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
> ask|0x0|UINT64|0x30001047
> 
> +  ## Indicates if 5-Level Paging will be enabled in long mode. 5-Level
> + Paging will not be enabled  #  when the PCD is TRUE but CPU doesn't
> support 5-Level Paging.
> +  #   TRUE  - 5-Level Paging will be enabled.<BR>
> +  #   FALSE - 5-Level Paging will not be enabled.<BR>
> +  # @Prompt Enable 5-Level Paging support in long mode.
> +
> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable|FALSE|BOOLE
> AN|0x0
> + 001105F
> +
>    ## Capsule In Ram is to use memory to deliver the capsules that will be
> processed after system
>    #  reset.<BR><BR>
>    #  This PCD indicates if the Capsule In Ram is supported.<BR>
> --
> 2.21.0.windows.1

[PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level page table for long mode

[Ni, Ray]

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

DxeIpl is responsible to create page table for DXE phase running
either in long mode or in 32bit mode with certain protection
mechanism enabled (refer to ToBuildPageTable()).

The patch updates DxeIpl to create 5-level page table for DXE phase
running in long mode when PcdUse5LevelPageTable is TRUE and CPU
supports 5-level page table.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf       |   1 +
 .../Core/DxeIplPeim/X64/VirtualMemory.c       | 227 ++++++++++++------
 2 files changed, 151 insertions(+), 77 deletions(-)

diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index abc3217b01..98bc17fc9d 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -110,6 +110,7 @@ [Pcd.IA32,Pcd.X64]
   gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask    ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask               ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                       ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable                  ## SOMETIMES_CONSUMES
 
 [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ## SOMETIMES_CONSUMES
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
index edc38e4525..a5bcdcc734 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -15,7 +15,7 @@
     2) IA-32 Intel(R) Architecture Software Developer's Manual Volume 2:Instruction Set Reference, Intel
     3) IA-32 Intel(R) Architecture Software Developer's Manual Volume 3:System Programmer's Guide, Intel
 
-Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
 Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -626,14 +626,19 @@ CreateIdentityMappingPageTables (
   )
 {
   UINT32                                        RegEax;
+  UINT32                                        RegEbx;
+  UINT32                                        RegEcx;
   UINT32                                        RegEdx;
   UINT8                                         PhysicalAddressBits;
   EFI_PHYSICAL_ADDRESS                          PageAddress;
+  UINTN                                         IndexOfPml5Entries;
   UINTN                                         IndexOfPml4Entries;
   UINTN                                         IndexOfPdpEntries;
   UINTN                                         IndexOfPageDirectoryEntries;
+  UINT32                                        NumberOfPml5EntriesNeeded;
   UINT32                                        NumberOfPml4EntriesNeeded;
   UINT32                                        NumberOfPdpEntriesNeeded;
+  PAGE_MAP_AND_DIRECTORY_POINTER                *PageMapLevel5Entry;
   PAGE_MAP_AND_DIRECTORY_POINTER                *PageMapLevel4Entry;
   PAGE_MAP_AND_DIRECTORY_POINTER                *PageMap;
   PAGE_MAP_AND_DIRECTORY_POINTER                *PageDirectoryPointerEntry;
@@ -641,9 +646,11 @@ CreateIdentityMappingPageTables (
   UINTN                                         TotalPagesNum;
   UINTN                                         BigPageAddress;
   VOID                                          *Hob;
+  BOOLEAN                                       Page5LevelSupport;
   BOOLEAN                                       Page1GSupport;
   PAGE_TABLE_1G_ENTRY                           *PageDirectory1GEntry;
   UINT64                                        AddressEncMask;
+  IA32_CR4                                      Cr4;
 
   //
   // Make sure AddressEncMask is contained to smallest supported address field
@@ -677,33 +684,66 @@ CreateIdentityMappingPageTables (
     }
   }
 
+  Page5LevelSupport = FALSE;
+  if (PcdGetBool (PcdUse5LevelPageTable)) {
+    AsmCpuidEx (0x7, 0, &RegEax, &RegEbx, &RegEcx, &RegEdx);
+    DEBUG ((DEBUG_INFO, "Cpuid(7/0): %08x/%08x/%08x/%08x\n", RegEax, RegEbx, RegEcx, RegEdx));
+    if ((RegEcx & BIT16) != 0) {
+      Page5LevelSupport = TRUE;
+    }
+  }
+
+  DEBUG ((DEBUG_INFO, "AddressBits/5LevelPaging/1GPage = %d/%d/%d\n", PhysicalAddressBits, Page5LevelSupport, Page1GSupport));
+
   //
-  // IA-32e paging translates 48-bit linear addresses to 52-bit physical addresses.
+  // IA-32e paging translates 48-bit linear addresses to 52-bit physical addresses
+  //  when 5-Level Paging is disabled,
+  //  due to either unsupported by HW, or disabled by PCD.
   //
   ASSERT (PhysicalAddressBits <= 52);
-  if (PhysicalAddressBits > 48) {
+  if (!Page5LevelSupport && PhysicalAddressBits > 48) {
     PhysicalAddressBits = 48;
   }
 
   //
   // Calculate the table entries needed.
   //
-  if (PhysicalAddressBits <= 39 ) {
-    NumberOfPml4EntriesNeeded = 1;
-    NumberOfPdpEntriesNeeded = (UINT32)LShiftU64 (1, (PhysicalAddressBits - 30));
-  } else {
-    NumberOfPml4EntriesNeeded = (UINT32)LShiftU64 (1, (PhysicalAddressBits - 39));
-    NumberOfPdpEntriesNeeded = 512;
+  NumberOfPml5EntriesNeeded = 1;
+  if (PhysicalAddressBits > 48) {
+    NumberOfPml5EntriesNeeded = (UINT32) LShiftU64 (1, PhysicalAddressBits - 48);
+    PhysicalAddressBits = 48;
   }
 
+  NumberOfPml4EntriesNeeded = 1;
+  if (PhysicalAddressBits > 39) {
+    NumberOfPml4EntriesNeeded = (UINT32) LShiftU64 (1, PhysicalAddressBits - 39);
+    PhysicalAddressBits = 39;
+  }
+
+  NumberOfPdpEntriesNeeded = 1;
+  ASSERT (PhysicalAddressBits > 30);
+  NumberOfPdpEntriesNeeded = (UINT32) LShiftU64 (1, PhysicalAddressBits - 30);
+
   //
   // Pre-allocate big pages to avoid later allocations.
   //
   if (!Page1GSupport) {
-    TotalPagesNum = (NumberOfPdpEntriesNeeded + 1) * NumberOfPml4EntriesNeeded + 1;
+    TotalPagesNum = ((NumberOfPdpEntriesNeeded + 1) * NumberOfPml4EntriesNeeded + 1) * NumberOfPml5EntriesNeeded + 1;
   } else {
-    TotalPagesNum = NumberOfPml4EntriesNeeded + 1;
+    TotalPagesNum = (NumberOfPml4EntriesNeeded + 1) * NumberOfPml5EntriesNeeded + 1;
+  }
+
+  //
+  // Substract the one page occupied by PML5 entries if 5-Level Paging is disabled.
+  //
+  if (!Page5LevelSupport) {
+    TotalPagesNum--;
   }
+
+  DEBUG ((DEBUG_INFO, "Pml5/Pml4/Pdp/TotalPage = %d/%d/%d/%d\n",
+    NumberOfPml5EntriesNeeded, NumberOfPml4EntriesNeeded,
+    NumberOfPdpEntriesNeeded, TotalPagesNum));
+
   BigPageAddress = (UINTN) AllocatePageTableMemory (TotalPagesNum);
   ASSERT (BigPageAddress != 0);
 
@@ -711,92 +751,125 @@ CreateIdentityMappingPageTables (
   // By architecture only one PageMapLevel4 exists - so lets allocate storage for it.
   //
   PageMap         = (VOID *) BigPageAddress;
-  BigPageAddress += SIZE_4KB;
-
-  PageMapLevel4Entry = PageMap;
-  PageAddress        = 0;
-  for (IndexOfPml4Entries = 0; IndexOfPml4Entries < NumberOfPml4EntriesNeeded; IndexOfPml4Entries++, PageMapLevel4Entry++) {
+  if (Page5LevelSupport) {
     //
-    // Each PML4 entry points to a page of Page Directory Pointer entires.
-    // So lets allocate space for them and fill them in in the IndexOfPdpEntries loop.
+    // By architecture only one PageMapLevel5 exists - so lets allocate storage for it.
     //
-    PageDirectoryPointerEntry = (VOID *) BigPageAddress;
-    BigPageAddress += SIZE_4KB;
+    PageMapLevel5Entry = PageMap;
+    BigPageAddress    += SIZE_4KB;
+  }
+  PageAddress        = 0;
 
+  for ( IndexOfPml5Entries = 0
+      ; IndexOfPml5Entries < NumberOfPml5EntriesNeeded
+      ; IndexOfPml5Entries++, PageMapLevel5Entry++) {
     //
-    // Make a PML4 Entry
+    // Each PML5 entry points to a page of PML4 entires.
+    // So lets allocate space for them and fill them in in the IndexOfPml4Entries loop.
+    // When 5-Level Paging is disabled, below allocation happens only once.
     //
-    PageMapLevel4Entry->Uint64 = (UINT64)(UINTN)PageDirectoryPointerEntry | AddressEncMask;
-    PageMapLevel4Entry->Bits.ReadWrite = 1;
-    PageMapLevel4Entry->Bits.Present = 1;
+    PageMapLevel4Entry = (VOID *) BigPageAddress;
+    BigPageAddress    += SIZE_4KB;
 
-    if (Page1GSupport) {
-      PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
+    if (Page5LevelSupport) {
+      //
+      // Make a PML5 Entry
+      //
+      PageMapLevel5Entry->Uint64 = (UINT64) (UINTN) PageMapLevel4Entry;
+      PageMapLevel5Entry->Bits.ReadWrite = 1;
+      PageMapLevel5Entry->Bits.Present   = 1;
+    }
 
-      for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress += SIZE_1GB) {
-        if (ToSplitPageTable (PageAddress, SIZE_1GB, StackBase, StackSize)) {
-          Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry, StackBase, StackSize);
-        } else {
-          //
-          // Fill in the Page Directory entries
-          //
-          PageDirectory1GEntry->Uint64 = (UINT64)PageAddress | AddressEncMask;
-          PageDirectory1GEntry->Bits.ReadWrite = 1;
-          PageDirectory1GEntry->Bits.Present = 1;
-          PageDirectory1GEntry->Bits.MustBe1 = 1;
-        }
-      }
-    } else {
-      for (IndexOfPdpEntries = 0; IndexOfPdpEntries < NumberOfPdpEntriesNeeded; IndexOfPdpEntries++, PageDirectoryPointerEntry++) {
-        //
-        // Each Directory Pointer entries points to a page of Page Directory entires.
-        // So allocate space for them and fill them in in the IndexOfPageDirectoryEntries loop.
-        //
-        PageDirectoryEntry = (VOID *) BigPageAddress;
-        BigPageAddress += SIZE_4KB;
+    for ( IndexOfPml4Entries = 0
+        ; IndexOfPml4Entries < (NumberOfPml5EntriesNeeded == 1 ? NumberOfPml4EntriesNeeded : 512)
+        ; IndexOfPml4Entries++, PageMapLevel4Entry++) {
+      //
+      // Each PML4 entry points to a page of Page Directory Pointer entires.
+      // So lets allocate space for them and fill them in in the IndexOfPdpEntries loop.
+      //
+      PageDirectoryPointerEntry = (VOID *) BigPageAddress;
+      BigPageAddress += SIZE_4KB;
 
-        //
-        // Fill in a Page Directory Pointer Entries
-        //
-        PageDirectoryPointerEntry->Uint64 = (UINT64)(UINTN)PageDirectoryEntry | AddressEncMask;
-        PageDirectoryPointerEntry->Bits.ReadWrite = 1;
-        PageDirectoryPointerEntry->Bits.Present = 1;
+      //
+      // Make a PML4 Entry
+      //
+      PageMapLevel4Entry->Uint64 = (UINT64)(UINTN)PageDirectoryPointerEntry | AddressEncMask;
+      PageMapLevel4Entry->Bits.ReadWrite = 1;
+      PageMapLevel4Entry->Bits.Present = 1;
 
-        for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress += SIZE_2MB) {
-          if (ToSplitPageTable (PageAddress, SIZE_2MB, StackBase, StackSize)) {
-            //
-            // Need to split this 2M page that covers NULL or stack range.
-            //
-            Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry, StackBase, StackSize);
+      if (Page1GSupport) {
+        PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
+
+        for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress += SIZE_1GB) {
+          if (ToSplitPageTable (PageAddress, SIZE_1GB, StackBase, StackSize)) {
+            Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry, StackBase, StackSize);
           } else {
             //
             // Fill in the Page Directory entries
             //
-            PageDirectoryEntry->Uint64 = (UINT64)PageAddress | AddressEncMask;
-            PageDirectoryEntry->Bits.ReadWrite = 1;
-            PageDirectoryEntry->Bits.Present = 1;
-            PageDirectoryEntry->Bits.MustBe1 = 1;
+            PageDirectory1GEntry->Uint64 = (UINT64)PageAddress | AddressEncMask;
+            PageDirectory1GEntry->Bits.ReadWrite = 1;
+            PageDirectory1GEntry->Bits.Present = 1;
+            PageDirectory1GEntry->Bits.MustBe1 = 1;
           }
         }
-      }
+      } else {
+        for ( IndexOfPdpEntries = 0
+            ; IndexOfPdpEntries < (NumberOfPml4EntriesNeeded == 1 ? NumberOfPdpEntriesNeeded : 512)
+            ; IndexOfPdpEntries++, PageDirectoryPointerEntry++) {
+          //
+          // Each Directory Pointer entries points to a page of Page Directory entires.
+          // So allocate space for them and fill them in in the IndexOfPageDirectoryEntries loop.
+          //
+          PageDirectoryEntry = (VOID *) BigPageAddress;
+          BigPageAddress += SIZE_4KB;
 
-      for (; IndexOfPdpEntries < 512; IndexOfPdpEntries++, PageDirectoryPointerEntry++) {
-        ZeroMem (
-          PageDirectoryPointerEntry,
-          sizeof(PAGE_MAP_AND_DIRECTORY_POINTER)
-          );
+          //
+          // Fill in a Page Directory Pointer Entries
+          //
+          PageDirectoryPointerEntry->Uint64 = (UINT64)(UINTN)PageDirectoryEntry | AddressEncMask;
+          PageDirectoryPointerEntry->Bits.ReadWrite = 1;
+          PageDirectoryPointerEntry->Bits.Present = 1;
+
+          for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress += SIZE_2MB) {
+            if (ToSplitPageTable (PageAddress, SIZE_2MB, StackBase, StackSize)) {
+              //
+              // Need to split this 2M page that covers NULL or stack range.
+              //
+              Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry, StackBase, StackSize);
+            } else {
+              //
+              // Fill in the Page Directory entries
+              //
+              PageDirectoryEntry->Uint64 = (UINT64)PageAddress | AddressEncMask;
+              PageDirectoryEntry->Bits.ReadWrite = 1;
+              PageDirectoryEntry->Bits.Present = 1;
+              PageDirectoryEntry->Bits.MustBe1 = 1;
+            }
+          }
+        }
+
+        //
+        // Fill with null entry for unused PDPTE
+        //
+        ZeroMem (PageDirectoryPointerEntry, (512 - IndexOfPdpEntries) * sizeof(PAGE_MAP_AND_DIRECTORY_POINTER));
       }
     }
+
+    //
+    // For the PML4 entries we are not using fill in a null entry.
+    //
+    ZeroMem (PageMapLevel4Entry, (512 - IndexOfPml4Entries) * sizeof (PAGE_MAP_AND_DIRECTORY_POINTER));
   }
 
-  //
-  // For the PML4 entries we are not using fill in a null entry.
-  //
-  for (; IndexOfPml4Entries < 512; IndexOfPml4Entries++, PageMapLevel4Entry++) {
-    ZeroMem (
-      PageMapLevel4Entry,
-      sizeof (PAGE_MAP_AND_DIRECTORY_POINTER)
-      );
+  if (Page5LevelSupport) {
+    Cr4.UintN = AsmReadCr4 ();
+    Cr4.Bits.LA57 = 1;
+    AsmWriteCr4 (Cr4.UintN);
+    //
+    // For the PML5 entries we are not using fill in a null entry.
+    //
+    ZeroMem (PageMapLevel5Entry, (512 - IndexOfPml5Entries) * sizeof (PAGE_MAP_AND_DIRECTORY_POINTER));
   }
 
   //
-- 
2.21.0.windows.1

Re: [edk2-devel] [PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level page table for long mode

[Wu, Hao A]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni,
> Ray
> Sent: Monday, July 22, 2019 4:16 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric; Laszlo Ersek
> Subject: [edk2-devel] [PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level
> page table for long mode
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
> 
> DxeIpl is responsible to create page table for DXE phase running
> either in long mode or in 32bit mode with certain protection
> mechanism enabled (refer to ToBuildPageTable()).
> 
> The patch updates DxeIpl to create 5-level page table for DXE phase
> running in long mode when PcdUse5LevelPageTable is TRUE and CPU
> supports 5-level page table.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf       |   1 +
>  .../Core/DxeIplPeim/X64/VirtualMemory.c       | 227 ++++++++++++------
>  2 files changed, 151 insertions(+), 77 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index abc3217b01..98bc17fc9d 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -110,6 +110,7 @@ [Pcd.IA32,Pcd.X64]
> 
> gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
> ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
> ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                       ##
> CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable                  ##
> SOMETIMES_CONSUMES
> 
>  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ##
> SOMETIMES_CONSUMES
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index edc38e4525..a5bcdcc734 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -15,7 +15,7 @@
>      2) IA-32 Intel(R) Architecture Software Developer's Manual Volume
> 2:Instruction Set Reference, Intel
>      3) IA-32 Intel(R) Architecture Software Developer's Manual Volume
> 3:System Programmer's Guide, Intel
> 
> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -626,14 +626,19 @@ CreateIdentityMappingPageTables (
>    )
>  {
>    UINT32                                        RegEax;
> +  UINT32                                        RegEbx;
> +  UINT32                                        RegEcx;
>    UINT32                                        RegEdx;
>    UINT8                                         PhysicalAddressBits;
>    EFI_PHYSICAL_ADDRESS                          PageAddress;
> +  UINTN                                         IndexOfPml5Entries;
>    UINTN                                         IndexOfPml4Entries;
>    UINTN                                         IndexOfPdpEntries;
>    UINTN                                         IndexOfPageDirectoryEntries;
> +  UINT32                                        NumberOfPml5EntriesNeeded;
>    UINT32                                        NumberOfPml4EntriesNeeded;
>    UINT32                                        NumberOfPdpEntriesNeeded;
> +  PAGE_MAP_AND_DIRECTORY_POINTER                *PageMapLevel5Entry;
>    PAGE_MAP_AND_DIRECTORY_POINTER                *PageMapLevel4Entry;
>    PAGE_MAP_AND_DIRECTORY_POINTER                *PageMap;
>    PAGE_MAP_AND_DIRECTORY_POINTER
> *PageDirectoryPointerEntry;
> @@ -641,9 +646,11 @@ CreateIdentityMappingPageTables (
>    UINTN                                         TotalPagesNum;
>    UINTN                                         BigPageAddress;
>    VOID                                          *Hob;
> +  BOOLEAN                                       Page5LevelSupport;
>    BOOLEAN                                       Page1GSupport;
>    PAGE_TABLE_1G_ENTRY                           *PageDirectory1GEntry;
>    UINT64                                        AddressEncMask;
> +  IA32_CR4                                      Cr4;
> 
>    //
>    // Make sure AddressEncMask is contained to smallest supported address
> field
> @@ -677,33 +684,66 @@ CreateIdentityMappingPageTables (
>      }
>    }
> 
> +  Page5LevelSupport = FALSE;
> +  if (PcdGetBool (PcdUse5LevelPageTable)) {
> +    AsmCpuidEx (0x7, 0, &RegEax, &RegEbx, &RegEcx, &RegEdx);
> +    DEBUG ((DEBUG_INFO, "Cpuid(7/0): %08x/%08x/%08x/%08x\n", RegEax,
> RegEbx, RegEcx, RegEdx));
> +    if ((RegEcx & BIT16) != 0) {
> +      Page5LevelSupport = TRUE;
> +    }
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "AddressBits/5LevelPaging/1GPage
> = %d/%d/%d\n", PhysicalAddressBits, Page5LevelSupport, Page1GSupport));
> +
>    //
> -  // IA-32e paging translates 48-bit linear addresses to 52-bit physical
> addresses.
> +  // IA-32e paging translates 48-bit linear addresses to 52-bit physical
> addresses
> +  //  when 5-Level Paging is disabled,
> +  //  due to either unsupported by HW, or disabled by PCD.
>    //
>    ASSERT (PhysicalAddressBits <= 52);
> -  if (PhysicalAddressBits > 48) {
> +  if (!Page5LevelSupport && PhysicalAddressBits > 48) {
>      PhysicalAddressBits = 48;
>    }
> 
>    //
>    // Calculate the table entries needed.
>    //
> -  if (PhysicalAddressBits <= 39 ) {
> -    NumberOfPml4EntriesNeeded = 1;
> -    NumberOfPdpEntriesNeeded = (UINT32)LShiftU64 (1,
> (PhysicalAddressBits - 30));
> -  } else {
> -    NumberOfPml4EntriesNeeded = (UINT32)LShiftU64 (1,
> (PhysicalAddressBits - 39));
> -    NumberOfPdpEntriesNeeded = 512;
> +  NumberOfPml5EntriesNeeded = 1;
> +  if (PhysicalAddressBits > 48) {
> +    NumberOfPml5EntriesNeeded = (UINT32) LShiftU64 (1,
> PhysicalAddressBits - 48);
> +    PhysicalAddressBits = 48;
>    }
> 
> +  NumberOfPml4EntriesNeeded = 1;
> +  if (PhysicalAddressBits > 39) {
> +    NumberOfPml4EntriesNeeded = (UINT32) LShiftU64 (1,
> PhysicalAddressBits - 39);
> +    PhysicalAddressBits = 39;
> +  }
> +
> +  NumberOfPdpEntriesNeeded = 1;
> +  ASSERT (PhysicalAddressBits > 30);
> +  NumberOfPdpEntriesNeeded = (UINT32) LShiftU64 (1, PhysicalAddressBits
> - 30);
> +
>    //
>    // Pre-allocate big pages to avoid later allocations.
>    //
>    if (!Page1GSupport) {
> -    TotalPagesNum = (NumberOfPdpEntriesNeeded + 1) *
> NumberOfPml4EntriesNeeded + 1;
> +    TotalPagesNum = ((NumberOfPdpEntriesNeeded + 1) *
> NumberOfPml4EntriesNeeded + 1) * NumberOfPml5EntriesNeeded + 1;
>    } else {
> -    TotalPagesNum = NumberOfPml4EntriesNeeded + 1;
> +    TotalPagesNum = (NumberOfPml4EntriesNeeded + 1) *
> NumberOfPml5EntriesNeeded + 1;
> +  }
> +
> +  //
> +  // Substract the one page occupied by PML5 entries if 5-Level Paging is
> disabled.
> +  //
> +  if (!Page5LevelSupport) {
> +    TotalPagesNum--;
>    }
> +
> +  DEBUG ((DEBUG_INFO, "Pml5/Pml4/Pdp/TotalPage = %d/%d/%d/%d\n",
> +    NumberOfPml5EntriesNeeded, NumberOfPml4EntriesNeeded,
> +    NumberOfPdpEntriesNeeded, TotalPagesNum));
> +
>    BigPageAddress = (UINTN) AllocatePageTableMemory (TotalPagesNum);
>    ASSERT (BigPageAddress != 0);
> 
> @@ -711,92 +751,125 @@ CreateIdentityMappingPageTables (
>    // By architecture only one PageMapLevel4 exists - so lets allocate storage
> for it.
>    //
>    PageMap         = (VOID *) BigPageAddress;
> -  BigPageAddress += SIZE_4KB;
> -
> -  PageMapLevel4Entry = PageMap;
> -  PageAddress        = 0;
> -  for (IndexOfPml4Entries = 0; IndexOfPml4Entries <
> NumberOfPml4EntriesNeeded; IndexOfPml4Entries++,
> PageMapLevel4Entry++) {
> +  if (Page5LevelSupport) {
>      //
> -    // Each PML4 entry points to a page of Page Directory Pointer entires.
> -    // So lets allocate space for them and fill them in in the IndexOfPdpEntries
> loop.
> +    // By architecture only one PageMapLevel5 exists - so lets allocate storage
> for it.
>      //
> -    PageDirectoryPointerEntry = (VOID *) BigPageAddress;
> -    BigPageAddress += SIZE_4KB;
> +    PageMapLevel5Entry = PageMap;
> +    BigPageAddress    += SIZE_4KB;
> +  }
> +  PageAddress        = 0;
> 
> +  for ( IndexOfPml5Entries = 0
> +      ; IndexOfPml5Entries < NumberOfPml5EntriesNeeded
> +      ; IndexOfPml5Entries++, PageMapLevel5Entry++) {
>      //
> -    // Make a PML4 Entry
> +    // Each PML5 entry points to a page of PML4 entires.
> +    // So lets allocate space for them and fill them in in the
> IndexOfPml4Entries loop.
> +    // When 5-Level Paging is disabled, below allocation happens only once.
>      //
> -    PageMapLevel4Entry->Uint64 =
> (UINT64)(UINTN)PageDirectoryPointerEntry | AddressEncMask;
> -    PageMapLevel4Entry->Bits.ReadWrite = 1;
> -    PageMapLevel4Entry->Bits.Present = 1;
> +    PageMapLevel4Entry = (VOID *) BigPageAddress;
> +    BigPageAddress    += SIZE_4KB;
> 
> -    if (Page1GSupport) {
> -      PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
> +    if (Page5LevelSupport) {
> +      //
> +      // Make a PML5 Entry
> +      //
> +      PageMapLevel5Entry->Uint64 = (UINT64) (UINTN) PageMapLevel4Entry;


Hello,

Do we need to bitwise-or with 'AddressEncMask' here?

Best Regards,
Hao Wu


> +      PageMapLevel5Entry->Bits.ReadWrite = 1;
> +      PageMapLevel5Entry->Bits.Present   = 1;
> +    }
> 
> -      for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512;
> IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress +=
> SIZE_1GB) {
> -        if (ToSplitPageTable (PageAddress, SIZE_1GB, StackBase, StackSize)) {
> -          Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry,
> StackBase, StackSize);
> -        } else {
> -          //
> -          // Fill in the Page Directory entries
> -          //
> -          PageDirectory1GEntry->Uint64 = (UINT64)PageAddress |
> AddressEncMask;
> -          PageDirectory1GEntry->Bits.ReadWrite = 1;
> -          PageDirectory1GEntry->Bits.Present = 1;
> -          PageDirectory1GEntry->Bits.MustBe1 = 1;
> -        }
> -      }
> -    } else {
> -      for (IndexOfPdpEntries = 0; IndexOfPdpEntries <
> NumberOfPdpEntriesNeeded; IndexOfPdpEntries++,
> PageDirectoryPointerEntry++) {
> -        //
> -        // Each Directory Pointer entries points to a page of Page Directory
> entires.
> -        // So allocate space for them and fill them in in the
> IndexOfPageDirectoryEntries loop.
> -        //
> -        PageDirectoryEntry = (VOID *) BigPageAddress;
> -        BigPageAddress += SIZE_4KB;
> +    for ( IndexOfPml4Entries = 0
> +        ; IndexOfPml4Entries < (NumberOfPml5EntriesNeeded == 1 ?
> NumberOfPml4EntriesNeeded : 512)
> +        ; IndexOfPml4Entries++, PageMapLevel4Entry++) {
> +      //
> +      // Each PML4 entry points to a page of Page Directory Pointer entires.
> +      // So lets allocate space for them and fill them in in the
> IndexOfPdpEntries loop.
> +      //
> +      PageDirectoryPointerEntry = (VOID *) BigPageAddress;
> +      BigPageAddress += SIZE_4KB;
> 
> -        //
> -        // Fill in a Page Directory Pointer Entries
> -        //
> -        PageDirectoryPointerEntry->Uint64 =
> (UINT64)(UINTN)PageDirectoryEntry | AddressEncMask;
> -        PageDirectoryPointerEntry->Bits.ReadWrite = 1;
> -        PageDirectoryPointerEntry->Bits.Present = 1;
> +      //
> +      // Make a PML4 Entry
> +      //
> +      PageMapLevel4Entry->Uint64 =
> (UINT64)(UINTN)PageDirectoryPointerEntry | AddressEncMask;
> +      PageMapLevel4Entry->Bits.ReadWrite = 1;
> +      PageMapLevel4Entry->Bits.Present = 1;
> 
> -        for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries <
> 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress +=
> SIZE_2MB) {
> -          if (ToSplitPageTable (PageAddress, SIZE_2MB, StackBase, StackSize)) {
> -            //
> -            // Need to split this 2M page that covers NULL or stack range.
> -            //
> -            Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry,
> StackBase, StackSize);
> +      if (Page1GSupport) {
> +        PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
> +
> +        for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries <
> 512; IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress
> += SIZE_1GB) {
> +          if (ToSplitPageTable (PageAddress, SIZE_1GB, StackBase, StackSize)) {
> +            Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry,
> StackBase, StackSize);
>            } else {
>              //
>              // Fill in the Page Directory entries
>              //
> -            PageDirectoryEntry->Uint64 = (UINT64)PageAddress |
> AddressEncMask;
> -            PageDirectoryEntry->Bits.ReadWrite = 1;
> -            PageDirectoryEntry->Bits.Present = 1;
> -            PageDirectoryEntry->Bits.MustBe1 = 1;
> +            PageDirectory1GEntry->Uint64 = (UINT64)PageAddress |
> AddressEncMask;
> +            PageDirectory1GEntry->Bits.ReadWrite = 1;
> +            PageDirectory1GEntry->Bits.Present = 1;
> +            PageDirectory1GEntry->Bits.MustBe1 = 1;
>            }
>          }
> -      }
> +      } else {
> +        for ( IndexOfPdpEntries = 0
> +            ; IndexOfPdpEntries < (NumberOfPml4EntriesNeeded == 1 ?
> NumberOfPdpEntriesNeeded : 512)
> +            ; IndexOfPdpEntries++, PageDirectoryPointerEntry++) {
> +          //
> +          // Each Directory Pointer entries points to a page of Page Directory
> entires.
> +          // So allocate space for them and fill them in in the
> IndexOfPageDirectoryEntries loop.
> +          //
> +          PageDirectoryEntry = (VOID *) BigPageAddress;
> +          BigPageAddress += SIZE_4KB;
> 
> -      for (; IndexOfPdpEntries < 512; IndexOfPdpEntries++,
> PageDirectoryPointerEntry++) {
> -        ZeroMem (
> -          PageDirectoryPointerEntry,
> -          sizeof(PAGE_MAP_AND_DIRECTORY_POINTER)
> -          );
> +          //
> +          // Fill in a Page Directory Pointer Entries
> +          //
> +          PageDirectoryPointerEntry->Uint64 =
> (UINT64)(UINTN)PageDirectoryEntry | AddressEncMask;
> +          PageDirectoryPointerEntry->Bits.ReadWrite = 1;
> +          PageDirectoryPointerEntry->Bits.Present = 1;
> +
> +          for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries <
> 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress +=
> SIZE_2MB) {
> +            if (ToSplitPageTable (PageAddress, SIZE_2MB, StackBase, StackSize)) {
> +              //
> +              // Need to split this 2M page that covers NULL or stack range.
> +              //
> +              Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry,
> StackBase, StackSize);
> +            } else {
> +              //
> +              // Fill in the Page Directory entries
> +              //
> +              PageDirectoryEntry->Uint64 = (UINT64)PageAddress |
> AddressEncMask;
> +              PageDirectoryEntry->Bits.ReadWrite = 1;
> +              PageDirectoryEntry->Bits.Present = 1;
> +              PageDirectoryEntry->Bits.MustBe1 = 1;
> +            }
> +          }
> +        }
> +
> +        //
> +        // Fill with null entry for unused PDPTE
> +        //
> +        ZeroMem (PageDirectoryPointerEntry, (512 - IndexOfPdpEntries) *
> sizeof(PAGE_MAP_AND_DIRECTORY_POINTER));
>        }
>      }
> +
> +    //
> +    // For the PML4 entries we are not using fill in a null entry.
> +    //
> +    ZeroMem (PageMapLevel4Entry, (512 - IndexOfPml4Entries) * sizeof
> (PAGE_MAP_AND_DIRECTORY_POINTER));
>    }
> 
> -  //
> -  // For the PML4 entries we are not using fill in a null entry.
> -  //
> -  for (; IndexOfPml4Entries < 512; IndexOfPml4Entries++,
> PageMapLevel4Entry++) {
> -    ZeroMem (
> -      PageMapLevel4Entry,
> -      sizeof (PAGE_MAP_AND_DIRECTORY_POINTER)
> -      );
> +  if (Page5LevelSupport) {
> +    Cr4.UintN = AsmReadCr4 ();
> +    Cr4.Bits.LA57 = 1;
> +    AsmWriteCr4 (Cr4.UintN);
> +    //
> +    // For the PML5 entries we are not using fill in a null entry.
> +    //
> +    ZeroMem (PageMapLevel5Entry, (512 - IndexOfPml5Entries) * sizeof
> (PAGE_MAP_AND_DIRECTORY_POINTER));
>    }
> 
>    //
> --
> 2.21.0.windows.1
> 
> 
> 

Re: [edk2-devel] [PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level page table for long mode

[Laszlo Ersek]

(+ Brijesh, and one comment below)

On 07/23/19 04:05, Wu, Hao A wrote:
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni,
>> Ray
>> Sent: Monday, July 22, 2019 4:16 PM
>> To: devel@edk2.groups.io
>> Cc: Dong, Eric; Laszlo Ersek
>> Subject: [edk2-devel] [PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level
>> page table for long mode
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
>>
>> DxeIpl is responsible to create page table for DXE phase running
>> either in long mode or in 32bit mode with certain protection
>> mechanism enabled (refer to ToBuildPageTable()).
>>
>> The patch updates DxeIpl to create 5-level page table for DXE phase
>> running in long mode when PcdUse5LevelPageTable is TRUE and CPU
>> supports 5-level page table.
>>
>> Signed-off-by: Ray Ni <ray.ni@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf       |   1 +
>>  .../Core/DxeIplPeim/X64/VirtualMemory.c       | 227 ++++++++++++------
>>  2 files changed, 151 insertions(+), 77 deletions(-)
>>
>> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>> index abc3217b01..98bc17fc9d 100644
>> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>> @@ -110,6 +110,7 @@ [Pcd.IA32,Pcd.X64]
>>
>> gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
>> ## CONSUMES
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
>> ## CONSUMES
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                       ##
>> CONSUMES
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable                  ##
>> SOMETIMES_CONSUMES
>>
>>  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ##
>> SOMETIMES_CONSUMES
>> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
>> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
>> index edc38e4525..a5bcdcc734 100644
>> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
>> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
>> @@ -15,7 +15,7 @@
>>      2) IA-32 Intel(R) Architecture Software Developer's Manual Volume
>> 2:Instruction Set Reference, Intel
>>      3) IA-32 Intel(R) Architecture Software Developer's Manual Volume
>> 3:System Programmer's Guide, Intel
>>
>> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>>  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
>>
>>  SPDX-License-Identifier: BSD-2-Clause-Patent
>> @@ -626,14 +626,19 @@ CreateIdentityMappingPageTables (
>>    )
>>  {
>>    UINT32                                        RegEax;
>> +  UINT32                                        RegEbx;
>> +  UINT32                                        RegEcx;
>>    UINT32                                        RegEdx;
>>    UINT8                                         PhysicalAddressBits;
>>    EFI_PHYSICAL_ADDRESS                          PageAddress;
>> +  UINTN                                         IndexOfPml5Entries;
>>    UINTN                                         IndexOfPml4Entries;
>>    UINTN                                         IndexOfPdpEntries;
>>    UINTN                                         IndexOfPageDirectoryEntries;
>> +  UINT32                                        NumberOfPml5EntriesNeeded;
>>    UINT32                                        NumberOfPml4EntriesNeeded;
>>    UINT32                                        NumberOfPdpEntriesNeeded;
>> +  PAGE_MAP_AND_DIRECTORY_POINTER                *PageMapLevel5Entry;
>>    PAGE_MAP_AND_DIRECTORY_POINTER                *PageMapLevel4Entry;
>>    PAGE_MAP_AND_DIRECTORY_POINTER                *PageMap;
>>    PAGE_MAP_AND_DIRECTORY_POINTER
>> *PageDirectoryPointerEntry;
>> @@ -641,9 +646,11 @@ CreateIdentityMappingPageTables (
>>    UINTN                                         TotalPagesNum;
>>    UINTN                                         BigPageAddress;
>>    VOID                                          *Hob;
>> +  BOOLEAN                                       Page5LevelSupport;
>>    BOOLEAN                                       Page1GSupport;
>>    PAGE_TABLE_1G_ENTRY                           *PageDirectory1GEntry;
>>    UINT64                                        AddressEncMask;
>> +  IA32_CR4                                      Cr4;
>>
>>    //
>>    // Make sure AddressEncMask is contained to smallest supported address
>> field
>> @@ -677,33 +684,66 @@ CreateIdentityMappingPageTables (
>>      }
>>    }
>>
>> +  Page5LevelSupport = FALSE;
>> +  if (PcdGetBool (PcdUse5LevelPageTable)) {
>> +    AsmCpuidEx (0x7, 0, &RegEax, &RegEbx, &RegEcx, &RegEdx);
>> +    DEBUG ((DEBUG_INFO, "Cpuid(7/0): %08x/%08x/%08x/%08x\n", RegEax,
>> RegEbx, RegEcx, RegEdx));
>> +    if ((RegEcx & BIT16) != 0) {
>> +      Page5LevelSupport = TRUE;
>> +    }
>> +  }
>> +
>> +  DEBUG ((DEBUG_INFO, "AddressBits/5LevelPaging/1GPage
>> = %d/%d/%d\n", PhysicalAddressBits, Page5LevelSupport, Page1GSupport));
>> +
>>    //
>> -  // IA-32e paging translates 48-bit linear addresses to 52-bit physical
>> addresses.
>> +  // IA-32e paging translates 48-bit linear addresses to 52-bit physical
>> addresses
>> +  //  when 5-Level Paging is disabled,
>> +  //  due to either unsupported by HW, or disabled by PCD.
>>    //
>>    ASSERT (PhysicalAddressBits <= 52);
>> -  if (PhysicalAddressBits > 48) {
>> +  if (!Page5LevelSupport && PhysicalAddressBits > 48) {
>>      PhysicalAddressBits = 48;
>>    }
>>
>>    //
>>    // Calculate the table entries needed.
>>    //
>> -  if (PhysicalAddressBits <= 39 ) {
>> -    NumberOfPml4EntriesNeeded = 1;
>> -    NumberOfPdpEntriesNeeded = (UINT32)LShiftU64 (1,
>> (PhysicalAddressBits - 30));
>> -  } else {
>> -    NumberOfPml4EntriesNeeded = (UINT32)LShiftU64 (1,
>> (PhysicalAddressBits - 39));
>> -    NumberOfPdpEntriesNeeded = 512;
>> +  NumberOfPml5EntriesNeeded = 1;
>> +  if (PhysicalAddressBits > 48) {
>> +    NumberOfPml5EntriesNeeded = (UINT32) LShiftU64 (1,
>> PhysicalAddressBits - 48);
>> +    PhysicalAddressBits = 48;
>>    }
>>
>> +  NumberOfPml4EntriesNeeded = 1;
>> +  if (PhysicalAddressBits > 39) {
>> +    NumberOfPml4EntriesNeeded = (UINT32) LShiftU64 (1,
>> PhysicalAddressBits - 39);
>> +    PhysicalAddressBits = 39;
>> +  }
>> +
>> +  NumberOfPdpEntriesNeeded = 1;
>> +  ASSERT (PhysicalAddressBits > 30);
>> +  NumberOfPdpEntriesNeeded = (UINT32) LShiftU64 (1, PhysicalAddressBits
>> - 30);
>> +
>>    //
>>    // Pre-allocate big pages to avoid later allocations.
>>    //
>>    if (!Page1GSupport) {
>> -    TotalPagesNum = (NumberOfPdpEntriesNeeded + 1) *
>> NumberOfPml4EntriesNeeded + 1;
>> +    TotalPagesNum = ((NumberOfPdpEntriesNeeded + 1) *
>> NumberOfPml4EntriesNeeded + 1) * NumberOfPml5EntriesNeeded + 1;
>>    } else {
>> -    TotalPagesNum = NumberOfPml4EntriesNeeded + 1;
>> +    TotalPagesNum = (NumberOfPml4EntriesNeeded + 1) *
>> NumberOfPml5EntriesNeeded + 1;
>> +  }
>> +
>> +  //
>> +  // Substract the one page occupied by PML5 entries if 5-Level Paging is
>> disabled.
>> +  //
>> +  if (!Page5LevelSupport) {
>> +    TotalPagesNum--;
>>    }
>> +
>> +  DEBUG ((DEBUG_INFO, "Pml5/Pml4/Pdp/TotalPage = %d/%d/%d/%d\n",
>> +    NumberOfPml5EntriesNeeded, NumberOfPml4EntriesNeeded,
>> +    NumberOfPdpEntriesNeeded, TotalPagesNum));
>> +
>>    BigPageAddress = (UINTN) AllocatePageTableMemory (TotalPagesNum);
>>    ASSERT (BigPageAddress != 0);
>>
>> @@ -711,92 +751,125 @@ CreateIdentityMappingPageTables (
>>    // By architecture only one PageMapLevel4 exists - so lets allocate storage
>> for it.
>>    //
>>    PageMap         = (VOID *) BigPageAddress;
>> -  BigPageAddress += SIZE_4KB;
>> -
>> -  PageMapLevel4Entry = PageMap;
>> -  PageAddress        = 0;
>> -  for (IndexOfPml4Entries = 0; IndexOfPml4Entries <
>> NumberOfPml4EntriesNeeded; IndexOfPml4Entries++,
>> PageMapLevel4Entry++) {
>> +  if (Page5LevelSupport) {
>>      //
>> -    // Each PML4 entry points to a page of Page Directory Pointer entires.
>> -    // So lets allocate space for them and fill them in in the IndexOfPdpEntries
>> loop.
>> +    // By architecture only one PageMapLevel5 exists - so lets allocate storage
>> for it.
>>      //
>> -    PageDirectoryPointerEntry = (VOID *) BigPageAddress;
>> -    BigPageAddress += SIZE_4KB;
>> +    PageMapLevel5Entry = PageMap;
>> +    BigPageAddress    += SIZE_4KB;
>> +  }
>> +  PageAddress        = 0;
>>
>> +  for ( IndexOfPml5Entries = 0
>> +      ; IndexOfPml5Entries < NumberOfPml5EntriesNeeded
>> +      ; IndexOfPml5Entries++, PageMapLevel5Entry++) {
>>      //
>> -    // Make a PML4 Entry
>> +    // Each PML5 entry points to a page of PML4 entires.
>> +    // So lets allocate space for them and fill them in in the
>> IndexOfPml4Entries loop.
>> +    // When 5-Level Paging is disabled, below allocation happens only once.
>>      //
>> -    PageMapLevel4Entry->Uint64 =
>> (UINT64)(UINTN)PageDirectoryPointerEntry | AddressEncMask;
>> -    PageMapLevel4Entry->Bits.ReadWrite = 1;
>> -    PageMapLevel4Entry->Bits.Present = 1;
>> +    PageMapLevel4Entry = (VOID *) BigPageAddress;
>> +    BigPageAddress    += SIZE_4KB;
>>
>> -    if (Page1GSupport) {
>> -      PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
>> +    if (Page5LevelSupport) {
>> +      //
>> +      // Make a PML5 Entry
>> +      //
>> +      PageMapLevel5Entry->Uint64 = (UINT64) (UINTN) PageMapLevel4Entry;
> 
> 
> Hello,
> 
> Do we need to bitwise-or with 'AddressEncMask' here?

Indeed it crossed my mind how this extension intersects with SEV.

I didn't ask the question myself because I thought 5-level paging didn't
apply to the AMD processors that supported SEV. But maybe we should
figure that out now regardless.

Thanks
Laszlo

> Best Regards,
> Hao Wu
> 
> 
>> +      PageMapLevel5Entry->Bits.ReadWrite = 1;
>> +      PageMapLevel5Entry->Bits.Present   = 1;
>> +    }
>>
>> -      for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512;
>> IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress +=
>> SIZE_1GB) {
>> -        if (ToSplitPageTable (PageAddress, SIZE_1GB, StackBase, StackSize)) {
>> -          Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry,
>> StackBase, StackSize);
>> -        } else {
>> -          //
>> -          // Fill in the Page Directory entries
>> -          //
>> -          PageDirectory1GEntry->Uint64 = (UINT64)PageAddress |
>> AddressEncMask;
>> -          PageDirectory1GEntry->Bits.ReadWrite = 1;
>> -          PageDirectory1GEntry->Bits.Present = 1;
>> -          PageDirectory1GEntry->Bits.MustBe1 = 1;
>> -        }
>> -      }
>> -    } else {
>> -      for (IndexOfPdpEntries = 0; IndexOfPdpEntries <
>> NumberOfPdpEntriesNeeded; IndexOfPdpEntries++,
>> PageDirectoryPointerEntry++) {
>> -        //
>> -        // Each Directory Pointer entries points to a page of Page Directory
>> entires.
>> -        // So allocate space for them and fill them in in the
>> IndexOfPageDirectoryEntries loop.
>> -        //
>> -        PageDirectoryEntry = (VOID *) BigPageAddress;
>> -        BigPageAddress += SIZE_4KB;
>> +    for ( IndexOfPml4Entries = 0
>> +        ; IndexOfPml4Entries < (NumberOfPml5EntriesNeeded == 1 ?
>> NumberOfPml4EntriesNeeded : 512)
>> +        ; IndexOfPml4Entries++, PageMapLevel4Entry++) {
>> +      //
>> +      // Each PML4 entry points to a page of Page Directory Pointer entires.
>> +      // So lets allocate space for them and fill them in in the
>> IndexOfPdpEntries loop.
>> +      //
>> +      PageDirectoryPointerEntry = (VOID *) BigPageAddress;
>> +      BigPageAddress += SIZE_4KB;
>>
>> -        //
>> -        // Fill in a Page Directory Pointer Entries
>> -        //
>> -        PageDirectoryPointerEntry->Uint64 =
>> (UINT64)(UINTN)PageDirectoryEntry | AddressEncMask;
>> -        PageDirectoryPointerEntry->Bits.ReadWrite = 1;
>> -        PageDirectoryPointerEntry->Bits.Present = 1;
>> +      //
>> +      // Make a PML4 Entry
>> +      //
>> +      PageMapLevel4Entry->Uint64 =
>> (UINT64)(UINTN)PageDirectoryPointerEntry | AddressEncMask;
>> +      PageMapLevel4Entry->Bits.ReadWrite = 1;
>> +      PageMapLevel4Entry->Bits.Present = 1;
>>
>> -        for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries <
>> 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress +=
>> SIZE_2MB) {
>> -          if (ToSplitPageTable (PageAddress, SIZE_2MB, StackBase, StackSize)) {
>> -            //
>> -            // Need to split this 2M page that covers NULL or stack range.
>> -            //
>> -            Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry,
>> StackBase, StackSize);
>> +      if (Page1GSupport) {
>> +        PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
>> +
>> +        for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries <
>> 512; IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress
>> += SIZE_1GB) {
>> +          if (ToSplitPageTable (PageAddress, SIZE_1GB, StackBase, StackSize)) {
>> +            Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry,
>> StackBase, StackSize);
>>            } else {
>>              //
>>              // Fill in the Page Directory entries
>>              //
>> -            PageDirectoryEntry->Uint64 = (UINT64)PageAddress |
>> AddressEncMask;
>> -            PageDirectoryEntry->Bits.ReadWrite = 1;
>> -            PageDirectoryEntry->Bits.Present = 1;
>> -            PageDirectoryEntry->Bits.MustBe1 = 1;
>> +            PageDirectory1GEntry->Uint64 = (UINT64)PageAddress |
>> AddressEncMask;
>> +            PageDirectory1GEntry->Bits.ReadWrite = 1;
>> +            PageDirectory1GEntry->Bits.Present = 1;
>> +            PageDirectory1GEntry->Bits.MustBe1 = 1;
>>            }
>>          }
>> -      }
>> +      } else {
>> +        for ( IndexOfPdpEntries = 0
>> +            ; IndexOfPdpEntries < (NumberOfPml4EntriesNeeded == 1 ?
>> NumberOfPdpEntriesNeeded : 512)
>> +            ; IndexOfPdpEntries++, PageDirectoryPointerEntry++) {
>> +          //
>> +          // Each Directory Pointer entries points to a page of Page Directory
>> entires.
>> +          // So allocate space for them and fill them in in the
>> IndexOfPageDirectoryEntries loop.
>> +          //
>> +          PageDirectoryEntry = (VOID *) BigPageAddress;
>> +          BigPageAddress += SIZE_4KB;
>>
>> -      for (; IndexOfPdpEntries < 512; IndexOfPdpEntries++,
>> PageDirectoryPointerEntry++) {
>> -        ZeroMem (
>> -          PageDirectoryPointerEntry,
>> -          sizeof(PAGE_MAP_AND_DIRECTORY_POINTER)
>> -          );
>> +          //
>> +          // Fill in a Page Directory Pointer Entries
>> +          //
>> +          PageDirectoryPointerEntry->Uint64 =
>> (UINT64)(UINTN)PageDirectoryEntry | AddressEncMask;
>> +          PageDirectoryPointerEntry->Bits.ReadWrite = 1;
>> +          PageDirectoryPointerEntry->Bits.Present = 1;
>> +
>> +          for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries <
>> 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress +=
>> SIZE_2MB) {
>> +            if (ToSplitPageTable (PageAddress, SIZE_2MB, StackBase, StackSize)) {
>> +              //
>> +              // Need to split this 2M page that covers NULL or stack range.
>> +              //
>> +              Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry,
>> StackBase, StackSize);
>> +            } else {
>> +              //
>> +              // Fill in the Page Directory entries
>> +              //
>> +              PageDirectoryEntry->Uint64 = (UINT64)PageAddress |
>> AddressEncMask;
>> +              PageDirectoryEntry->Bits.ReadWrite = 1;
>> +              PageDirectoryEntry->Bits.Present = 1;
>> +              PageDirectoryEntry->Bits.MustBe1 = 1;
>> +            }
>> +          }
>> +        }
>> +
>> +        //
>> +        // Fill with null entry for unused PDPTE
>> +        //
>> +        ZeroMem (PageDirectoryPointerEntry, (512 - IndexOfPdpEntries) *
>> sizeof(PAGE_MAP_AND_DIRECTORY_POINTER));
>>        }
>>      }
>> +
>> +    //
>> +    // For the PML4 entries we are not using fill in a null entry.
>> +    //
>> +    ZeroMem (PageMapLevel4Entry, (512 - IndexOfPml4Entries) * sizeof
>> (PAGE_MAP_AND_DIRECTORY_POINTER));
>>    }
>>
>> -  //
>> -  // For the PML4 entries we are not using fill in a null entry.
>> -  //
>> -  for (; IndexOfPml4Entries < 512; IndexOfPml4Entries++,
>> PageMapLevel4Entry++) {
>> -    ZeroMem (
>> -      PageMapLevel4Entry,
>> -      sizeof (PAGE_MAP_AND_DIRECTORY_POINTER)
>> -      );
>> +  if (Page5LevelSupport) {
>> +    Cr4.UintN = AsmReadCr4 ();
>> +    Cr4.Bits.LA57 = 1;
>> +    AsmWriteCr4 (Cr4.UintN);
>> +    //
>> +    // For the PML5 entries we are not using fill in a null entry.
>> +    //
>> +    ZeroMem (PageMapLevel5Entry, (512 - IndexOfPml5Entries) * sizeof
>> (PAGE_MAP_AND_DIRECTORY_POINTER));
>>    }
>>
>>    //
>> --
>> 2.21.0.windows.1
>>
>>
>> 
> 

Re: [edk2-devel] [PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level page table for long mode

[Singh, Brijesh]

On 7/23/19 2:48 AM, Laszlo Ersek wrote:
> (+ Brijesh, and one comment below)
> 
> On 07/23/19 04:05, Wu, Hao A wrote:
>>> -----Original Message-----
>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Ni,
>>> Ray
>>> Sent: Monday, July 22, 2019 4:16 PM
>>> To: devel@edk2.groups.io
>>> Cc: Dong, Eric; Laszlo Ersek
>>> Subject: [edk2-devel] [PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level
>>> page table for long mode
>>>
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
>>>
>>> DxeIpl is responsible to create page table for DXE phase running
>>> either in long mode or in 32bit mode with certain protection
>>> mechanism enabled (refer to ToBuildPageTable()).
>>>
>>> The patch updates DxeIpl to create 5-level page table for DXE phase
>>> running in long mode when PcdUse5LevelPageTable is TRUE and CPU
>>> supports 5-level page table.
>>>
>>> Signed-off-by: Ray Ni <ray.ni@intel.com>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>   MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf       |   1 +
>>>   .../Core/DxeIplPeim/X64/VirtualMemory.c       | 227 ++++++++++++------
>>>   2 files changed, 151 insertions(+), 77 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>>> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>>> index abc3217b01..98bc17fc9d 100644
>>> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>>> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>>> @@ -110,6 +110,7 @@ [Pcd.IA32,Pcd.X64]
>>>
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
>>> ## CONSUMES
>>>     gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
>>> ## CONSUMES
>>>     gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                       ##
>>> CONSUMES
>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable                  ##
>>> SOMETIMES_CONSUMES
>>>
>>>   [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ##
>>> SOMETIMES_CONSUMES
>>> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
>>> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
>>> index edc38e4525..a5bcdcc734 100644
>>> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
>>> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
>>> @@ -15,7 +15,7 @@
>>>       2) IA-32 Intel(R) Architecture Software Developer's Manual Volume
>>> 2:Instruction Set Reference, Intel
>>>       3) IA-32 Intel(R) Architecture Software Developer's Manual Volume
>>> 3:System Programmer's Guide, Intel
>>>
>>> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>>> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>>>   Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
>>>
>>>   SPDX-License-Identifier: BSD-2-Clause-Patent
>>> @@ -626,14 +626,19 @@ CreateIdentityMappingPageTables (
>>>     )
>>>   {
>>>     UINT32                                        RegEax;
>>> +  UINT32                                        RegEbx;
>>> +  UINT32                                        RegEcx;
>>>     UINT32                                        RegEdx;
>>>     UINT8                                         PhysicalAddressBits;
>>>     EFI_PHYSICAL_ADDRESS                          PageAddress;
>>> +  UINTN                                         IndexOfPml5Entries;
>>>     UINTN                                         IndexOfPml4Entries;
>>>     UINTN                                         IndexOfPdpEntries;
>>>     UINTN                                         IndexOfPageDirectoryEntries;
>>> +  UINT32                                        NumberOfPml5EntriesNeeded;
>>>     UINT32                                        NumberOfPml4EntriesNeeded;
>>>     UINT32                                        NumberOfPdpEntriesNeeded;
>>> +  PAGE_MAP_AND_DIRECTORY_POINTER                *PageMapLevel5Entry;
>>>     PAGE_MAP_AND_DIRECTORY_POINTER                *PageMapLevel4Entry;
>>>     PAGE_MAP_AND_DIRECTORY_POINTER                *PageMap;
>>>     PAGE_MAP_AND_DIRECTORY_POINTER
>>> *PageDirectoryPointerEntry;
>>> @@ -641,9 +646,11 @@ CreateIdentityMappingPageTables (
>>>     UINTN                                         TotalPagesNum;
>>>     UINTN                                         BigPageAddress;
>>>     VOID                                          *Hob;
>>> +  BOOLEAN                                       Page5LevelSupport;
>>>     BOOLEAN                                       Page1GSupport;
>>>     PAGE_TABLE_1G_ENTRY                           *PageDirectory1GEntry;
>>>     UINT64                                        AddressEncMask;
>>> +  IA32_CR4                                      Cr4;
>>>
>>>     //
>>>     // Make sure AddressEncMask is contained to smallest supported address
>>> field
>>> @@ -677,33 +684,66 @@ CreateIdentityMappingPageTables (
>>>       }
>>>     }
>>>
>>> +  Page5LevelSupport = FALSE;
>>> +  if (PcdGetBool (PcdUse5LevelPageTable)) {
>>> +    AsmCpuidEx (0x7, 0, &RegEax, &RegEbx, &RegEcx, &RegEdx);
>>> +    DEBUG ((DEBUG_INFO, "Cpuid(7/0): %08x/%08x/%08x/%08x\n", RegEax,
>>> RegEbx, RegEcx, RegEdx));
>>> +    if ((RegEcx & BIT16) != 0) {
>>> +      Page5LevelSupport = TRUE;
>>> +    }
>>> +  }
>>> +
>>> +  DEBUG ((DEBUG_INFO, "AddressBits/5LevelPaging/1GPage
>>> = %d/%d/%d\n", PhysicalAddressBits, Page5LevelSupport, Page1GSupport));
>>> +
>>>     //
>>> -  // IA-32e paging translates 48-bit linear addresses to 52-bit physical
>>> addresses.
>>> +  // IA-32e paging translates 48-bit linear addresses to 52-bit physical
>>> addresses
>>> +  //  when 5-Level Paging is disabled,
>>> +  //  due to either unsupported by HW, or disabled by PCD.
>>>     //
>>>     ASSERT (PhysicalAddressBits <= 52);
>>> -  if (PhysicalAddressBits > 48) {
>>> +  if (!Page5LevelSupport && PhysicalAddressBits > 48) {
>>>       PhysicalAddressBits = 48;
>>>     }
>>>
>>>     //
>>>     // Calculate the table entries needed.
>>>     //
>>> -  if (PhysicalAddressBits <= 39 ) {
>>> -    NumberOfPml4EntriesNeeded = 1;
>>> -    NumberOfPdpEntriesNeeded = (UINT32)LShiftU64 (1,
>>> (PhysicalAddressBits - 30));
>>> -  } else {
>>> -    NumberOfPml4EntriesNeeded = (UINT32)LShiftU64 (1,
>>> (PhysicalAddressBits - 39));
>>> -    NumberOfPdpEntriesNeeded = 512;
>>> +  NumberOfPml5EntriesNeeded = 1;
>>> +  if (PhysicalAddressBits > 48) {
>>> +    NumberOfPml5EntriesNeeded = (UINT32) LShiftU64 (1,
>>> PhysicalAddressBits - 48);
>>> +    PhysicalAddressBits = 48;
>>>     }
>>>
>>> +  NumberOfPml4EntriesNeeded = 1;
>>> +  if (PhysicalAddressBits > 39) {
>>> +    NumberOfPml4EntriesNeeded = (UINT32) LShiftU64 (1,
>>> PhysicalAddressBits - 39);
>>> +    PhysicalAddressBits = 39;
>>> +  }
>>> +
>>> +  NumberOfPdpEntriesNeeded = 1;
>>> +  ASSERT (PhysicalAddressBits > 30);
>>> +  NumberOfPdpEntriesNeeded = (UINT32) LShiftU64 (1, PhysicalAddressBits
>>> - 30);
>>> +
>>>     //
>>>     // Pre-allocate big pages to avoid later allocations.
>>>     //
>>>     if (!Page1GSupport) {
>>> -    TotalPagesNum = (NumberOfPdpEntriesNeeded + 1) *
>>> NumberOfPml4EntriesNeeded + 1;
>>> +    TotalPagesNum = ((NumberOfPdpEntriesNeeded + 1) *
>>> NumberOfPml4EntriesNeeded + 1) * NumberOfPml5EntriesNeeded + 1;
>>>     } else {
>>> -    TotalPagesNum = NumberOfPml4EntriesNeeded + 1;
>>> +    TotalPagesNum = (NumberOfPml4EntriesNeeded + 1) *
>>> NumberOfPml5EntriesNeeded + 1;
>>> +  }
>>> +
>>> +  //
>>> +  // Substract the one page occupied by PML5 entries if 5-Level Paging is
>>> disabled.
>>> +  //
>>> +  if (!Page5LevelSupport) {
>>> +    TotalPagesNum--;
>>>     }
>>> +
>>> +  DEBUG ((DEBUG_INFO, "Pml5/Pml4/Pdp/TotalPage = %d/%d/%d/%d\n",
>>> +    NumberOfPml5EntriesNeeded, NumberOfPml4EntriesNeeded,
>>> +    NumberOfPdpEntriesNeeded, TotalPagesNum));
>>> +
>>>     BigPageAddress = (UINTN) AllocatePageTableMemory (TotalPagesNum);
>>>     ASSERT (BigPageAddress != 0);
>>>
>>> @@ -711,92 +751,125 @@ CreateIdentityMappingPageTables (
>>>     // By architecture only one PageMapLevel4 exists - so lets allocate storage
>>> for it.
>>>     //
>>>     PageMap         = (VOID *) BigPageAddress;
>>> -  BigPageAddress += SIZE_4KB;
>>> -
>>> -  PageMapLevel4Entry = PageMap;
>>> -  PageAddress        = 0;
>>> -  for (IndexOfPml4Entries = 0; IndexOfPml4Entries <
>>> NumberOfPml4EntriesNeeded; IndexOfPml4Entries++,
>>> PageMapLevel4Entry++) {
>>> +  if (Page5LevelSupport) {
>>>       //
>>> -    // Each PML4 entry points to a page of Page Directory Pointer entires.
>>> -    // So lets allocate space for them and fill them in in the IndexOfPdpEntries
>>> loop.
>>> +    // By architecture only one PageMapLevel5 exists - so lets allocate storage
>>> for it.
>>>       //
>>> -    PageDirectoryPointerEntry = (VOID *) BigPageAddress;
>>> -    BigPageAddress += SIZE_4KB;
>>> +    PageMapLevel5Entry = PageMap;
>>> +    BigPageAddress    += SIZE_4KB;
>>> +  }
>>> +  PageAddress        = 0;
>>>
>>> +  for ( IndexOfPml5Entries = 0
>>> +      ; IndexOfPml5Entries < NumberOfPml5EntriesNeeded
>>> +      ; IndexOfPml5Entries++, PageMapLevel5Entry++) {
>>>       //
>>> -    // Make a PML4 Entry
>>> +    // Each PML5 entry points to a page of PML4 entires.
>>> +    // So lets allocate space for them and fill them in in the
>>> IndexOfPml4Entries loop.
>>> +    // When 5-Level Paging is disabled, below allocation happens only once.
>>>       //
>>> -    PageMapLevel4Entry->Uint64 =
>>> (UINT64)(UINTN)PageDirectoryPointerEntry | AddressEncMask;
>>> -    PageMapLevel4Entry->Bits.ReadWrite = 1;
>>> -    PageMapLevel4Entry->Bits.Present = 1;
>>> +    PageMapLevel4Entry = (VOID *) BigPageAddress;
>>> +    BigPageAddress    += SIZE_4KB;
>>>
>>> -    if (Page1GSupport) {
>>> -      PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
>>> +    if (Page5LevelSupport) {
>>> +      //
>>> +      // Make a PML5 Entry
>>> +      //
>>> +      PageMapLevel5Entry->Uint64 = (UINT64) (UINTN) PageMapLevel4Entry;
>>
>>
>> Hello,
>>
>> Do we need to bitwise-or with 'AddressEncMask' here?
> 
> Indeed it crossed my mind how this extension intersects with SEV.
> 
> I didn't ask the question myself because I thought 5-level paging didn't
> apply to the AMD processors that supported SEV. But maybe we should
> figure that out now regardless.
> 

We should apply the PAGE_ENC mask while creating the 5-level pgtable
entries (similar to what we do today). The current available processor
which support SEV does not support 5-level pgtable but I expect things
will change in future generation.

-Brijesh

> Thanks
> Laszlo
> 
>> Best Regards,
>> Hao Wu
>>
>>
>>> +      PageMapLevel5Entry->Bits.ReadWrite = 1;
>>> +      PageMapLevel5Entry->Bits.Present   = 1;
>>> +    }
>>>
>>> -      for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512;
>>> IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress +=
>>> SIZE_1GB) {
>>> -        if (ToSplitPageTable (PageAddress, SIZE_1GB, StackBase, StackSize)) {
>>> -          Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry,
>>> StackBase, StackSize);
>>> -        } else {
>>> -          //
>>> -          // Fill in the Page Directory entries
>>> -          //
>>> -          PageDirectory1GEntry->Uint64 = (UINT64)PageAddress |
>>> AddressEncMask;
>>> -          PageDirectory1GEntry->Bits.ReadWrite = 1;
>>> -          PageDirectory1GEntry->Bits.Present = 1;
>>> -          PageDirectory1GEntry->Bits.MustBe1 = 1;
>>> -        }
>>> -      }
>>> -    } else {
>>> -      for (IndexOfPdpEntries = 0; IndexOfPdpEntries <
>>> NumberOfPdpEntriesNeeded; IndexOfPdpEntries++,
>>> PageDirectoryPointerEntry++) {
>>> -        //
>>> -        // Each Directory Pointer entries points to a page of Page Directory
>>> entires.
>>> -        // So allocate space for them and fill them in in the
>>> IndexOfPageDirectoryEntries loop.
>>> -        //
>>> -        PageDirectoryEntry = (VOID *) BigPageAddress;
>>> -        BigPageAddress += SIZE_4KB;
>>> +    for ( IndexOfPml4Entries = 0
>>> +        ; IndexOfPml4Entries < (NumberOfPml5EntriesNeeded == 1 ?
>>> NumberOfPml4EntriesNeeded : 512)
>>> +        ; IndexOfPml4Entries++, PageMapLevel4Entry++) {
>>> +      //
>>> +      // Each PML4 entry points to a page of Page Directory Pointer entires.
>>> +      // So lets allocate space for them and fill them in in the
>>> IndexOfPdpEntries loop.
>>> +      //
>>> +      PageDirectoryPointerEntry = (VOID *) BigPageAddress;
>>> +      BigPageAddress += SIZE_4KB;
>>>
>>> -        //
>>> -        // Fill in a Page Directory Pointer Entries
>>> -        //
>>> -        PageDirectoryPointerEntry->Uint64 =
>>> (UINT64)(UINTN)PageDirectoryEntry | AddressEncMask;
>>> -        PageDirectoryPointerEntry->Bits.ReadWrite = 1;
>>> -        PageDirectoryPointerEntry->Bits.Present = 1;
>>> +      //
>>> +      // Make a PML4 Entry
>>> +      //
>>> +      PageMapLevel4Entry->Uint64 =
>>> (UINT64)(UINTN)PageDirectoryPointerEntry | AddressEncMask;
>>> +      PageMapLevel4Entry->Bits.ReadWrite = 1;
>>> +      PageMapLevel4Entry->Bits.Present = 1;
>>>
>>> -        for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries <
>>> 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress +=
>>> SIZE_2MB) {
>>> -          if (ToSplitPageTable (PageAddress, SIZE_2MB, StackBase, StackSize)) {
>>> -            //
>>> -            // Need to split this 2M page that covers NULL or stack range.
>>> -            //
>>> -            Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry,
>>> StackBase, StackSize);
>>> +      if (Page1GSupport) {
>>> +        PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
>>> +
>>> +        for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries <
>>> 512; IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress
>>> += SIZE_1GB) {
>>> +          if (ToSplitPageTable (PageAddress, SIZE_1GB, StackBase, StackSize)) {
>>> +            Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry,
>>> StackBase, StackSize);
>>>             } else {
>>>               //
>>>               // Fill in the Page Directory entries
>>>               //
>>> -            PageDirectoryEntry->Uint64 = (UINT64)PageAddress |
>>> AddressEncMask;
>>> -            PageDirectoryEntry->Bits.ReadWrite = 1;
>>> -            PageDirectoryEntry->Bits.Present = 1;
>>> -            PageDirectoryEntry->Bits.MustBe1 = 1;
>>> +            PageDirectory1GEntry->Uint64 = (UINT64)PageAddress |
>>> AddressEncMask;
>>> +            PageDirectory1GEntry->Bits.ReadWrite = 1;
>>> +            PageDirectory1GEntry->Bits.Present = 1;
>>> +            PageDirectory1GEntry->Bits.MustBe1 = 1;
>>>             }
>>>           }
>>> -      }
>>> +      } else {
>>> +        for ( IndexOfPdpEntries = 0
>>> +            ; IndexOfPdpEntries < (NumberOfPml4EntriesNeeded == 1 ?
>>> NumberOfPdpEntriesNeeded : 512)
>>> +            ; IndexOfPdpEntries++, PageDirectoryPointerEntry++) {
>>> +          //
>>> +          // Each Directory Pointer entries points to a page of Page Directory
>>> entires.
>>> +          // So allocate space for them and fill them in in the
>>> IndexOfPageDirectoryEntries loop.
>>> +          //
>>> +          PageDirectoryEntry = (VOID *) BigPageAddress;
>>> +          BigPageAddress += SIZE_4KB;
>>>
>>> -      for (; IndexOfPdpEntries < 512; IndexOfPdpEntries++,
>>> PageDirectoryPointerEntry++) {
>>> -        ZeroMem (
>>> -          PageDirectoryPointerEntry,
>>> -          sizeof(PAGE_MAP_AND_DIRECTORY_POINTER)
>>> -          );
>>> +          //
>>> +          // Fill in a Page Directory Pointer Entries
>>> +          //
>>> +          PageDirectoryPointerEntry->Uint64 =
>>> (UINT64)(UINTN)PageDirectoryEntry | AddressEncMask;
>>> +          PageDirectoryPointerEntry->Bits.ReadWrite = 1;
>>> +          PageDirectoryPointerEntry->Bits.Present = 1;
>>> +
>>> +          for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries <
>>> 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress +=
>>> SIZE_2MB) {
>>> +            if (ToSplitPageTable (PageAddress, SIZE_2MB, StackBase, StackSize)) {
>>> +              //
>>> +              // Need to split this 2M page that covers NULL or stack range.
>>> +              //
>>> +              Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry,
>>> StackBase, StackSize);
>>> +            } else {
>>> +              //
>>> +              // Fill in the Page Directory entries
>>> +              //
>>> +              PageDirectoryEntry->Uint64 = (UINT64)PageAddress |
>>> AddressEncMask;
>>> +              PageDirectoryEntry->Bits.ReadWrite = 1;
>>> +              PageDirectoryEntry->Bits.Present = 1;
>>> +              PageDirectoryEntry->Bits.MustBe1 = 1;
>>> +            }
>>> +          }
>>> +        }
>>> +
>>> +        //
>>> +        // Fill with null entry for unused PDPTE
>>> +        //
>>> +        ZeroMem (PageDirectoryPointerEntry, (512 - IndexOfPdpEntries) *
>>> sizeof(PAGE_MAP_AND_DIRECTORY_POINTER));
>>>         }
>>>       }
>>> +
>>> +    //
>>> +    // For the PML4 entries we are not using fill in a null entry.
>>> +    //
>>> +    ZeroMem (PageMapLevel4Entry, (512 - IndexOfPml4Entries) * sizeof
>>> (PAGE_MAP_AND_DIRECTORY_POINTER));
>>>     }
>>>
>>> -  //
>>> -  // For the PML4 entries we are not using fill in a null entry.
>>> -  //
>>> -  for (; IndexOfPml4Entries < 512; IndexOfPml4Entries++,
>>> PageMapLevel4Entry++) {
>>> -    ZeroMem (
>>> -      PageMapLevel4Entry,
>>> -      sizeof (PAGE_MAP_AND_DIRECTORY_POINTER)
>>> -      );
>>> +  if (Page5LevelSupport) {
>>> +    Cr4.UintN = AsmReadCr4 ();
>>> +    Cr4.Bits.LA57 = 1;
>>> +    AsmWriteCr4 (Cr4.UintN);
>>> +    //
>>> +    // For the PML5 entries we are not using fill in a null entry.
>>> +    //
>>> +    ZeroMem (PageMapLevel5Entry, (512 - IndexOfPml5Entries) * sizeof
>>> (PAGE_MAP_AND_DIRECTORY_POINTER));
>>>     }
>>>
>>>     //
>>> --
>>> 2.21.0.windows.1
>>>
>>>
>>> 
>>
> 

Re: [edk2-devel] [PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level page table for long mode

[Laszlo Ersek]

On 07/22/19 10:15, Ni, Ray wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
> 
> DxeIpl is responsible to create page table for DXE phase running
> either in long mode or in 32bit mode with certain protection
> mechanism enabled (refer to ToBuildPageTable()).
> 
> The patch updates DxeIpl to create 5-level page table for DXE phase
> running in long mode when PcdUse5LevelPageTable is TRUE and CPU
> supports 5-level page table.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf       |   1 +
>  .../Core/DxeIplPeim/X64/VirtualMemory.c       | 227 ++++++++++++------
>  2 files changed, 151 insertions(+), 77 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index abc3217b01..98bc17fc9d 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -110,6 +110,7 @@ [Pcd.IA32,Pcd.X64]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask    ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask               ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                       ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable                  ## SOMETIMES_CONSUMES
>  
>  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ## SOMETIMES_CONSUMES
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index edc38e4525..a5bcdcc734 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -15,7 +15,7 @@
>      2) IA-32 Intel(R) Architecture Software Developer's Manual Volume 2:Instruction Set Reference, Intel
>      3) IA-32 Intel(R) Architecture Software Developer's Manual Volume 3:System Programmer's Guide, Intel
>  
> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
>  
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -626,14 +626,19 @@ CreateIdentityMappingPageTables (
>    )
>  {
>    UINT32                                        RegEax;
> +  UINT32                                        RegEbx;
> +  UINT32                                        RegEcx;
>    UINT32                                        RegEdx;
>    UINT8                                         PhysicalAddressBits;
>    EFI_PHYSICAL_ADDRESS                          PageAddress;
> +  UINTN                                         IndexOfPml5Entries;
>    UINTN                                         IndexOfPml4Entries;
>    UINTN                                         IndexOfPdpEntries;
>    UINTN                                         IndexOfPageDirectoryEntries;
> +  UINT32                                        NumberOfPml5EntriesNeeded;
>    UINT32                                        NumberOfPml4EntriesNeeded;
>    UINT32                                        NumberOfPdpEntriesNeeded;
> +  PAGE_MAP_AND_DIRECTORY_POINTER                *PageMapLevel5Entry;
>    PAGE_MAP_AND_DIRECTORY_POINTER                *PageMapLevel4Entry;
>    PAGE_MAP_AND_DIRECTORY_POINTER                *PageMap;
>    PAGE_MAP_AND_DIRECTORY_POINTER                *PageDirectoryPointerEntry;
> @@ -641,9 +646,11 @@ CreateIdentityMappingPageTables (
>    UINTN                                         TotalPagesNum;
>    UINTN                                         BigPageAddress;
>    VOID                                          *Hob;
> +  BOOLEAN                                       Page5LevelSupport;
>    BOOLEAN                                       Page1GSupport;
>    PAGE_TABLE_1G_ENTRY                           *PageDirectory1GEntry;
>    UINT64                                        AddressEncMask;
> +  IA32_CR4                                      Cr4;
>  
>    //
>    // Make sure AddressEncMask is contained to smallest supported address field
> @@ -677,33 +684,66 @@ CreateIdentityMappingPageTables (
>      }
>    }
>  
> +  Page5LevelSupport = FALSE;
> +  if (PcdGetBool (PcdUse5LevelPageTable)) {
> +    AsmCpuidEx (0x7, 0, &RegEax, &RegEbx, &RegEcx, &RegEdx);
> +    DEBUG ((DEBUG_INFO, "Cpuid(7/0): %08x/%08x/%08x/%08x\n", RegEax, RegEbx, RegEcx, RegEdx));
> +    if ((RegEcx & BIT16) != 0) {

(1) Would it be possible to use macro names here, for Index and SubIndex
in AsmCpuidEx(), and a "better" macro name than BIT16, in the RegEcx check?

> +      Page5LevelSupport = TRUE;
> +    }
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "AddressBits/5LevelPaging/1GPage = %d/%d/%d\n", PhysicalAddressBits, Page5LevelSupport, Page1GSupport));
> +

(2) Can we format this log message as:

  AddressBits=%d 5LevelPaging=%d 1GPage=%d

?

>    //
> -  // IA-32e paging translates 48-bit linear addresses to 52-bit physical addresses.
> +  // IA-32e paging translates 48-bit linear addresses to 52-bit physical addresses
> +  //  when 5-Level Paging is disabled,
> +  //  due to either unsupported by HW, or disabled by PCD.
>    //
>    ASSERT (PhysicalAddressBits <= 52);
> -  if (PhysicalAddressBits > 48) {
> +  if (!Page5LevelSupport && PhysicalAddressBits > 48) {
>      PhysicalAddressBits = 48;
>    }
>  
>    //
>    // Calculate the table entries needed.
>    //
> -  if (PhysicalAddressBits <= 39 ) {
> -    NumberOfPml4EntriesNeeded = 1;
> -    NumberOfPdpEntriesNeeded = (UINT32)LShiftU64 (1, (PhysicalAddressBits - 30));
> -  } else {
> -    NumberOfPml4EntriesNeeded = (UINT32)LShiftU64 (1, (PhysicalAddressBits - 39));
> -    NumberOfPdpEntriesNeeded = 512;
> +  NumberOfPml5EntriesNeeded = 1;
> +  if (PhysicalAddressBits > 48) {
> +    NumberOfPml5EntriesNeeded = (UINT32) LShiftU64 (1, PhysicalAddressBits - 48);
> +    PhysicalAddressBits = 48;
>    }
>  
> +  NumberOfPml4EntriesNeeded = 1;
> +  if (PhysicalAddressBits > 39) {
> +    NumberOfPml4EntriesNeeded = (UINT32) LShiftU64 (1, PhysicalAddressBits - 39);
> +    PhysicalAddressBits = 39;
> +  }
> +
> +  NumberOfPdpEntriesNeeded = 1;
> +  ASSERT (PhysicalAddressBits > 30);
> +  NumberOfPdpEntriesNeeded = (UINT32) LShiftU64 (1, PhysicalAddressBits - 30);
> +
>    //
>    // Pre-allocate big pages to avoid later allocations.
>    //
>    if (!Page1GSupport) {
> -    TotalPagesNum = (NumberOfPdpEntriesNeeded + 1) * NumberOfPml4EntriesNeeded + 1;
> +    TotalPagesNum = ((NumberOfPdpEntriesNeeded + 1) * NumberOfPml4EntriesNeeded + 1) * NumberOfPml5EntriesNeeded + 1;
>    } else {
> -    TotalPagesNum = NumberOfPml4EntriesNeeded + 1;
> +    TotalPagesNum = (NumberOfPml4EntriesNeeded + 1) * NumberOfPml5EntriesNeeded + 1;
> +  }
> +
> +  //
> +  // Substract the one page occupied by PML5 entries if 5-Level Paging is disabled.
> +  //
> +  if (!Page5LevelSupport) {
> +    TotalPagesNum--;
>    }
> +
> +  DEBUG ((DEBUG_INFO, "Pml5/Pml4/Pdp/TotalPage = %d/%d/%d/%d\n",
> +    NumberOfPml5EntriesNeeded, NumberOfPml4EntriesNeeded,
> +    NumberOfPdpEntriesNeeded, TotalPagesNum));
> +

(3) Same comment about log message formatting as in (2).

(4) Please log UINT32 values with %u, not %d.

(5) Please log UINTN values with %Lu (and cast them to UINT64 first).

(6) More generally, can we please replace this patch with three patches,
in the series?

- the first patch should extract the TotalPagesNum calculation to a
separate *library* function (it should be a public function in a BASE or
PEIM library)

- the second patch should extend the TotalPagesNum calculation in the
library function to 5-level paging

- the third patch should be the remaining code from the present patch.

Here's why: in OvmfPkg/PlatformPei, the GetPeiMemoryCap() function
duplicates this calculation. In OVMF, PEI-phase memory allocations can
be dominated by the page tables built for 64-bit DXE, and so
OvmfPkg/PlatformPei must know, for sizing the permanent PEI RAM, how
much RAM the DXE IPL PEIM will need for those page tables.

I would *really* dislike copying this update (for 5-level paging) from
CreateIdentityMappingPageTables() to GetPeiMemoryCap(). Instead, the
calculation should be extracted to a library function, and then OVMF
(and all other platforms affected similarly) could call the new function.

If this is not feasible or very much out of scope, then I guess we'll
just keep the PCD as FALSE in OVMF, for the time being.

I'll let others review the rest of this patch.

Thanks
Laszlo

>    BigPageAddress = (UINTN) AllocatePageTableMemory (TotalPagesNum);
>    ASSERT (BigPageAddress != 0);
>  
> @@ -711,92 +751,125 @@ CreateIdentityMappingPageTables (
>    // By architecture only one PageMapLevel4 exists - so lets allocate storage for it.
>    //
>    PageMap         = (VOID *) BigPageAddress;
> -  BigPageAddress += SIZE_4KB;
> -
> -  PageMapLevel4Entry = PageMap;
> -  PageAddress        = 0;
> -  for (IndexOfPml4Entries = 0; IndexOfPml4Entries < NumberOfPml4EntriesNeeded; IndexOfPml4Entries++, PageMapLevel4Entry++) {
> +  if (Page5LevelSupport) {
>      //
> -    // Each PML4 entry points to a page of Page Directory Pointer entires.
> -    // So lets allocate space for them and fill them in in the IndexOfPdpEntries loop.
> +    // By architecture only one PageMapLevel5 exists - so lets allocate storage for it.
>      //
> -    PageDirectoryPointerEntry = (VOID *) BigPageAddress;
> -    BigPageAddress += SIZE_4KB;
> +    PageMapLevel5Entry = PageMap;
> +    BigPageAddress    += SIZE_4KB;
> +  }
> +  PageAddress        = 0;
>  
> +  for ( IndexOfPml5Entries = 0
> +      ; IndexOfPml5Entries < NumberOfPml5EntriesNeeded
> +      ; IndexOfPml5Entries++, PageMapLevel5Entry++) {
>      //
> -    // Make a PML4 Entry
> +    // Each PML5 entry points to a page of PML4 entires.
> +    // So lets allocate space for them and fill them in in the IndexOfPml4Entries loop.
> +    // When 5-Level Paging is disabled, below allocation happens only once.
>      //
> -    PageMapLevel4Entry->Uint64 = (UINT64)(UINTN)PageDirectoryPointerEntry | AddressEncMask;
> -    PageMapLevel4Entry->Bits.ReadWrite = 1;
> -    PageMapLevel4Entry->Bits.Present = 1;
> +    PageMapLevel4Entry = (VOID *) BigPageAddress;
> +    BigPageAddress    += SIZE_4KB;
>  
> -    if (Page1GSupport) {
> -      PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
> +    if (Page5LevelSupport) {
> +      //
> +      // Make a PML5 Entry
> +      //
> +      PageMapLevel5Entry->Uint64 = (UINT64) (UINTN) PageMapLevel4Entry;
> +      PageMapLevel5Entry->Bits.ReadWrite = 1;
> +      PageMapLevel5Entry->Bits.Present   = 1;
> +    }
>  
> -      for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress += SIZE_1GB) {
> -        if (ToSplitPageTable (PageAddress, SIZE_1GB, StackBase, StackSize)) {
> -          Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry, StackBase, StackSize);
> -        } else {
> -          //
> -          // Fill in the Page Directory entries
> -          //
> -          PageDirectory1GEntry->Uint64 = (UINT64)PageAddress | AddressEncMask;
> -          PageDirectory1GEntry->Bits.ReadWrite = 1;
> -          PageDirectory1GEntry->Bits.Present = 1;
> -          PageDirectory1GEntry->Bits.MustBe1 = 1;
> -        }
> -      }
> -    } else {
> -      for (IndexOfPdpEntries = 0; IndexOfPdpEntries < NumberOfPdpEntriesNeeded; IndexOfPdpEntries++, PageDirectoryPointerEntry++) {
> -        //
> -        // Each Directory Pointer entries points to a page of Page Directory entires.
> -        // So allocate space for them and fill them in in the IndexOfPageDirectoryEntries loop.
> -        //
> -        PageDirectoryEntry = (VOID *) BigPageAddress;
> -        BigPageAddress += SIZE_4KB;
> +    for ( IndexOfPml4Entries = 0
> +        ; IndexOfPml4Entries < (NumberOfPml5EntriesNeeded == 1 ? NumberOfPml4EntriesNeeded : 512)
> +        ; IndexOfPml4Entries++, PageMapLevel4Entry++) {
> +      //
> +      // Each PML4 entry points to a page of Page Directory Pointer entires.
> +      // So lets allocate space for them and fill them in in the IndexOfPdpEntries loop.
> +      //
> +      PageDirectoryPointerEntry = (VOID *) BigPageAddress;
> +      BigPageAddress += SIZE_4KB;
>  
> -        //
> -        // Fill in a Page Directory Pointer Entries
> -        //
> -        PageDirectoryPointerEntry->Uint64 = (UINT64)(UINTN)PageDirectoryEntry | AddressEncMask;
> -        PageDirectoryPointerEntry->Bits.ReadWrite = 1;
> -        PageDirectoryPointerEntry->Bits.Present = 1;
> +      //
> +      // Make a PML4 Entry
> +      //
> +      PageMapLevel4Entry->Uint64 = (UINT64)(UINTN)PageDirectoryPointerEntry | AddressEncMask;
> +      PageMapLevel4Entry->Bits.ReadWrite = 1;
> +      PageMapLevel4Entry->Bits.Present = 1;
>  
> -        for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress += SIZE_2MB) {
> -          if (ToSplitPageTable (PageAddress, SIZE_2MB, StackBase, StackSize)) {
> -            //
> -            // Need to split this 2M page that covers NULL or stack range.
> -            //
> -            Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry, StackBase, StackSize);
> +      if (Page1GSupport) {
> +        PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
> +
> +        for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress += SIZE_1GB) {
> +          if (ToSplitPageTable (PageAddress, SIZE_1GB, StackBase, StackSize)) {
> +            Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry, StackBase, StackSize);
>            } else {
>              //
>              // Fill in the Page Directory entries
>              //
> -            PageDirectoryEntry->Uint64 = (UINT64)PageAddress | AddressEncMask;
> -            PageDirectoryEntry->Bits.ReadWrite = 1;
> -            PageDirectoryEntry->Bits.Present = 1;
> -            PageDirectoryEntry->Bits.MustBe1 = 1;
> +            PageDirectory1GEntry->Uint64 = (UINT64)PageAddress | AddressEncMask;
> +            PageDirectory1GEntry->Bits.ReadWrite = 1;
> +            PageDirectory1GEntry->Bits.Present = 1;
> +            PageDirectory1GEntry->Bits.MustBe1 = 1;
>            }
>          }
> -      }
> +      } else {
> +        for ( IndexOfPdpEntries = 0
> +            ; IndexOfPdpEntries < (NumberOfPml4EntriesNeeded == 1 ? NumberOfPdpEntriesNeeded : 512)
> +            ; IndexOfPdpEntries++, PageDirectoryPointerEntry++) {
> +          //
> +          // Each Directory Pointer entries points to a page of Page Directory entires.
> +          // So allocate space for them and fill them in in the IndexOfPageDirectoryEntries loop.
> +          //
> +          PageDirectoryEntry = (VOID *) BigPageAddress;
> +          BigPageAddress += SIZE_4KB;
>  
> -      for (; IndexOfPdpEntries < 512; IndexOfPdpEntries++, PageDirectoryPointerEntry++) {
> -        ZeroMem (
> -          PageDirectoryPointerEntry,
> -          sizeof(PAGE_MAP_AND_DIRECTORY_POINTER)
> -          );
> +          //
> +          // Fill in a Page Directory Pointer Entries
> +          //
> +          PageDirectoryPointerEntry->Uint64 = (UINT64)(UINTN)PageDirectoryEntry | AddressEncMask;
> +          PageDirectoryPointerEntry->Bits.ReadWrite = 1;
> +          PageDirectoryPointerEntry->Bits.Present = 1;
> +
> +          for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress += SIZE_2MB) {
> +            if (ToSplitPageTable (PageAddress, SIZE_2MB, StackBase, StackSize)) {
> +              //
> +              // Need to split this 2M page that covers NULL or stack range.
> +              //
> +              Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry, StackBase, StackSize);
> +            } else {
> +              //
> +              // Fill in the Page Directory entries
> +              //
> +              PageDirectoryEntry->Uint64 = (UINT64)PageAddress | AddressEncMask;
> +              PageDirectoryEntry->Bits.ReadWrite = 1;
> +              PageDirectoryEntry->Bits.Present = 1;
> +              PageDirectoryEntry->Bits.MustBe1 = 1;
> +            }
> +          }
> +        }
> +
> +        //
> +        // Fill with null entry for unused PDPTE
> +        //
> +        ZeroMem (PageDirectoryPointerEntry, (512 - IndexOfPdpEntries) * sizeof(PAGE_MAP_AND_DIRECTORY_POINTER));
>        }
>      }
> +
> +    //
> +    // For the PML4 entries we are not using fill in a null entry.
> +    //
> +    ZeroMem (PageMapLevel4Entry, (512 - IndexOfPml4Entries) * sizeof (PAGE_MAP_AND_DIRECTORY_POINTER));
>    }
>  
> -  //
> -  // For the PML4 entries we are not using fill in a null entry.
> -  //
> -  for (; IndexOfPml4Entries < 512; IndexOfPml4Entries++, PageMapLevel4Entry++) {
> -    ZeroMem (
> -      PageMapLevel4Entry,
> -      sizeof (PAGE_MAP_AND_DIRECTORY_POINTER)
> -      );
> +  if (Page5LevelSupport) {
> +    Cr4.UintN = AsmReadCr4 ();
> +    Cr4.Bits.LA57 = 1;
> +    AsmWriteCr4 (Cr4.UintN);
> +    //
> +    // For the PML5 entries we are not using fill in a null entry.
> +    //
> +    ZeroMem (PageMapLevel5Entry, (512 - IndexOfPml5Entries) * sizeof (PAGE_MAP_AND_DIRECTORY_POINTER));
>    }
>  
>    //
> 

Re: [edk2-devel] [PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level page table for long mode

[Ni, Ray]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Tuesday, July 23, 2019 5:46 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level page table for long mode
> 
> On 07/22/19 10:15, Ni, Ray wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
> >
> > DxeIpl is responsible to create page table for DXE phase running
> > either in long mode or in 32bit mode with certain protection
> > mechanism enabled (refer to ToBuildPageTable()).
> >
> > The patch updates DxeIpl to create 5-level page table for DXE phase
> > running in long mode when PcdUse5LevelPageTable is TRUE and CPU
> > supports 5-level page table.
> >
> > Signed-off-by: Ray Ni <ray.ni@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > ---
> >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf       |   1 +
> >  .../Core/DxeIplPeim/X64/VirtualMemory.c       | 227 ++++++++++++------
> >  2 files changed, 151 insertions(+), 77 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > index abc3217b01..98bc17fc9d 100644
> > --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > @@ -110,6 +110,7 @@ [Pcd.IA32,Pcd.X64]
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask    ## CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask               ## CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                       ## CONSUMES
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable                  ## SOMETIMES_CONSUMES
> >
> >  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ## SOMETIMES_CONSUMES
> > diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> > index edc38e4525..a5bcdcc734 100644
> > --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> > +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> > @@ -15,7 +15,7 @@
> >      2) IA-32 Intel(R) Architecture Software Developer's Manual Volume 2:Instruction Set Reference, Intel
> >      3) IA-32 Intel(R) Architecture Software Developer's Manual Volume 3:System Programmer's Guide, Intel
> >
> > -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> > +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
> >  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> >
> >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > @@ -626,14 +626,19 @@ CreateIdentityMappingPageTables (
> >    )
> >  {
> >    UINT32                                        RegEax;
> > +  UINT32                                        RegEbx;
> > +  UINT32                                        RegEcx;
> >    UINT32                                        RegEdx;
> >    UINT8                                         PhysicalAddressBits;
> >    EFI_PHYSICAL_ADDRESS                          PageAddress;
> > +  UINTN                                         IndexOfPml5Entries;
> >    UINTN                                         IndexOfPml4Entries;
> >    UINTN                                         IndexOfPdpEntries;
> >    UINTN                                         IndexOfPageDirectoryEntries;
> > +  UINT32                                        NumberOfPml5EntriesNeeded;
> >    UINT32                                        NumberOfPml4EntriesNeeded;
> >    UINT32                                        NumberOfPdpEntriesNeeded;
> > +  PAGE_MAP_AND_DIRECTORY_POINTER                *PageMapLevel5Entry;
> >    PAGE_MAP_AND_DIRECTORY_POINTER                *PageMapLevel4Entry;
> >    PAGE_MAP_AND_DIRECTORY_POINTER                *PageMap;
> >    PAGE_MAP_AND_DIRECTORY_POINTER                *PageDirectoryPointerEntry;
> > @@ -641,9 +646,11 @@ CreateIdentityMappingPageTables (
> >    UINTN                                         TotalPagesNum;
> >    UINTN                                         BigPageAddress;
> >    VOID                                          *Hob;
> > +  BOOLEAN                                       Page5LevelSupport;
> >    BOOLEAN                                       Page1GSupport;
> >    PAGE_TABLE_1G_ENTRY                           *PageDirectory1GEntry;
> >    UINT64                                        AddressEncMask;
> > +  IA32_CR4                                      Cr4;
> >
> >    //
> >    // Make sure AddressEncMask is contained to smallest supported address field
> > @@ -677,33 +684,66 @@ CreateIdentityMappingPageTables (
> >      }
> >    }
> >
> > +  Page5LevelSupport = FALSE;
> > +  if (PcdGetBool (PcdUse5LevelPageTable)) {
> > +    AsmCpuidEx (0x7, 0, &RegEax, &RegEbx, &RegEcx, &RegEdx);
> > +    DEBUG ((DEBUG_INFO, "Cpuid(7/0): %08x/%08x/%08x/%08x\n", RegEax, RegEbx, RegEcx, RegEdx));
> > +    if ((RegEcx & BIT16) != 0) {
> 
> (1) Would it be possible to use macro names here, for Index and SubIndex
> in AsmCpuidEx(), and a "better" macro name than BIT16, in the RegEcx check?

There are macros which are defined in UefiCpuPkg for the CPUID leaf functions
and structures for the CPUID output data.
But since there is rule that MdeModulePkg cannot depend on UefiCpuPkg so
the code cannot use those macros/structures.

I am thinking of a new rule that UefiCpuPkg and MdeModulePkg can depend on each other.
I haven't talked with Mike on this. Not sure if he is ok on this.
For this case, yes I can temporary define 2 macros: one for 0x7, the other for BIT16.


> 
> > +      Page5LevelSupport = TRUE;
> > +    }
> > +  }
> > +
> > +  DEBUG ((DEBUG_INFO, "AddressBits/5LevelPaging/1GPage = %d/%d/%d\n", PhysicalAddressBits, Page5LevelSupport,
> Page1GSupport));
> > +
> 
> (2) Can we format this log message as:
> 
>   AddressBits=%d 5LevelPaging=%d 1GPage=%d

ok.

> 
> ?
> 
> >    //
> > -  // IA-32e paging translates 48-bit linear addresses to 52-bit physical addresses.
> > +  // IA-32e paging translates 48-bit linear addresses to 52-bit physical addresses
> > +  //  when 5-Level Paging is disabled,
> > +  //  due to either unsupported by HW, or disabled by PCD.
> >    //
> >    ASSERT (PhysicalAddressBits <= 52);
> > -  if (PhysicalAddressBits > 48) {
> > +  if (!Page5LevelSupport && PhysicalAddressBits > 48) {
> >      PhysicalAddressBits = 48;
> >    }
> >
> >    //
> >    // Calculate the table entries needed.
> >    //
> > -  if (PhysicalAddressBits <= 39 ) {
> > -    NumberOfPml4EntriesNeeded = 1;
> > -    NumberOfPdpEntriesNeeded = (UINT32)LShiftU64 (1, (PhysicalAddressBits - 30));
> > -  } else {
> > -    NumberOfPml4EntriesNeeded = (UINT32)LShiftU64 (1, (PhysicalAddressBits - 39));
> > -    NumberOfPdpEntriesNeeded = 512;
> > +  NumberOfPml5EntriesNeeded = 1;
> > +  if (PhysicalAddressBits > 48) {
> > +    NumberOfPml5EntriesNeeded = (UINT32) LShiftU64 (1, PhysicalAddressBits - 48);
> > +    PhysicalAddressBits = 48;
> >    }
> >
> > +  NumberOfPml4EntriesNeeded = 1;
> > +  if (PhysicalAddressBits > 39) {
> > +    NumberOfPml4EntriesNeeded = (UINT32) LShiftU64 (1, PhysicalAddressBits - 39);
> > +    PhysicalAddressBits = 39;
> > +  }
> > +
> > +  NumberOfPdpEntriesNeeded = 1;
> > +  ASSERT (PhysicalAddressBits > 30);
> > +  NumberOfPdpEntriesNeeded = (UINT32) LShiftU64 (1, PhysicalAddressBits - 30);
> > +
> >    //
> >    // Pre-allocate big pages to avoid later allocations.
> >    //
> >    if (!Page1GSupport) {
> > -    TotalPagesNum = (NumberOfPdpEntriesNeeded + 1) * NumberOfPml4EntriesNeeded + 1;
> > +    TotalPagesNum = ((NumberOfPdpEntriesNeeded + 1) * NumberOfPml4EntriesNeeded + 1) * NumberOfPml5EntriesNeeded
> + 1;
> >    } else {
> > -    TotalPagesNum = NumberOfPml4EntriesNeeded + 1;
> > +    TotalPagesNum = (NumberOfPml4EntriesNeeded + 1) * NumberOfPml5EntriesNeeded + 1;
> > +  }
> > +
> > +  //
> > +  // Substract the one page occupied by PML5 entries if 5-Level Paging is disabled.
> > +  //
> > +  if (!Page5LevelSupport) {
> > +    TotalPagesNum--;
> >    }
> > +
> > +  DEBUG ((DEBUG_INFO, "Pml5/Pml4/Pdp/TotalPage = %d/%d/%d/%d\n",
> > +    NumberOfPml5EntriesNeeded, NumberOfPml4EntriesNeeded,
> > +    NumberOfPdpEntriesNeeded, TotalPagesNum));
> > +
> 
> (3) Same comment about log message formatting as in (2).
> 
> (4) Please log UINT32 values with %u, not %d.
> 
> (5) Please log UINTN values with %Lu (and cast them to UINT64 first).


ok to above 3, 4, 5.

> 
> (6) More generally, can we please replace this patch with three patches,
> in the series?
> 
> - the first patch should extract the TotalPagesNum calculation to a
> separate *library* function (it should be a public function in a BASE or
> PEIM library)
> 
> - the second patch should extend the TotalPagesNum calculation in the
> library function to 5-level paging
> 
> - the third patch should be the remaining code from the present patch.
> 
> Here's why: in OvmfPkg/PlatformPei, the GetPeiMemoryCap() function
> duplicates this calculation. In OVMF, PEI-phase memory allocations can
> be dominated by the page tables built for 64-bit DXE, and so
> OvmfPkg/PlatformPei must know, for sizing the permanent PEI RAM, how
> much RAM the DXE IPL PEIM will need for those page tables.
> 
> I would *really* dislike copying this update (for 5-level paging) from
> CreateIdentityMappingPageTables() to GetPeiMemoryCap(). Instead, the
> calculation should be extracted to a library function, and then OVMF
> (and all other platforms affected similarly) could call the new function.
> 
> If this is not feasible or very much out of scope, then I guess we'll
> just keep the PCD as FALSE in OVMF, for the time being.
> 

BZ https://bugzilla.tianocore.org/show_bug.cgi?id=847 may be what
you need.
Extracting a library API to return how many pages are needed for page tables
needs to consider several cases:
1. 4K or 2M or 1G page size is used.
2. range of physical memory
3. page table level

But right now, this is not in the high priority in my to-do list.
I can help to change GetPeiMemoryCap() for short-term needs.
Are you ok with this?

> I'll let others review the rest of this patch.
> 
> Thanks
> Laszlo
> 
> >    BigPageAddress = (UINTN) AllocatePageTableMemory (TotalPagesNum);
> >    ASSERT (BigPageAddress != 0);
> >
> > @@ -711,92 +751,125 @@ CreateIdentityMappingPageTables (
> >    // By architecture only one PageMapLevel4 exists - so lets allocate storage for it.
> >    //
> >    PageMap         = (VOID *) BigPageAddress;
> > -  BigPageAddress += SIZE_4KB;
> > -
> > -  PageMapLevel4Entry = PageMap;
> > -  PageAddress        = 0;
> > -  for (IndexOfPml4Entries = 0; IndexOfPml4Entries < NumberOfPml4EntriesNeeded; IndexOfPml4Entries++,
> PageMapLevel4Entry++) {
> > +  if (Page5LevelSupport) {
> >      //
> > -    // Each PML4 entry points to a page of Page Directory Pointer entires.
> > -    // So lets allocate space for them and fill them in in the IndexOfPdpEntries loop.
> > +    // By architecture only one PageMapLevel5 exists - so lets allocate storage for it.
> >      //
> > -    PageDirectoryPointerEntry = (VOID *) BigPageAddress;
> > -    BigPageAddress += SIZE_4KB;
> > +    PageMapLevel5Entry = PageMap;
> > +    BigPageAddress    += SIZE_4KB;
> > +  }
> > +  PageAddress        = 0;
> >
> > +  for ( IndexOfPml5Entries = 0
> > +      ; IndexOfPml5Entries < NumberOfPml5EntriesNeeded
> > +      ; IndexOfPml5Entries++, PageMapLevel5Entry++) {
> >      //
> > -    // Make a PML4 Entry
> > +    // Each PML5 entry points to a page of PML4 entires.
> > +    // So lets allocate space for them and fill them in in the IndexOfPml4Entries loop.
> > +    // When 5-Level Paging is disabled, below allocation happens only once.
> >      //
> > -    PageMapLevel4Entry->Uint64 = (UINT64)(UINTN)PageDirectoryPointerEntry | AddressEncMask;
> > -    PageMapLevel4Entry->Bits.ReadWrite = 1;
> > -    PageMapLevel4Entry->Bits.Present = 1;
> > +    PageMapLevel4Entry = (VOID *) BigPageAddress;
> > +    BigPageAddress    += SIZE_4KB;
> >
> > -    if (Page1GSupport) {
> > -      PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
> > +    if (Page5LevelSupport) {
> > +      //
> > +      // Make a PML5 Entry
> > +      //
> > +      PageMapLevel5Entry->Uint64 = (UINT64) (UINTN) PageMapLevel4Entry;
> > +      PageMapLevel5Entry->Bits.ReadWrite = 1;
> > +      PageMapLevel5Entry->Bits.Present   = 1;
> > +    }
> >
> > -      for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++,
> PageDirectory1GEntry++, PageAddress += SIZE_1GB) {
> > -        if (ToSplitPageTable (PageAddress, SIZE_1GB, StackBase, StackSize)) {
> > -          Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry, StackBase, StackSize);
> > -        } else {
> > -          //
> > -          // Fill in the Page Directory entries
> > -          //
> > -          PageDirectory1GEntry->Uint64 = (UINT64)PageAddress | AddressEncMask;
> > -          PageDirectory1GEntry->Bits.ReadWrite = 1;
> > -          PageDirectory1GEntry->Bits.Present = 1;
> > -          PageDirectory1GEntry->Bits.MustBe1 = 1;
> > -        }
> > -      }
> > -    } else {
> > -      for (IndexOfPdpEntries = 0; IndexOfPdpEntries < NumberOfPdpEntriesNeeded; IndexOfPdpEntries++,
> PageDirectoryPointerEntry++) {
> > -        //
> > -        // Each Directory Pointer entries points to a page of Page Directory entires.
> > -        // So allocate space for them and fill them in in the IndexOfPageDirectoryEntries loop.
> > -        //
> > -        PageDirectoryEntry = (VOID *) BigPageAddress;
> > -        BigPageAddress += SIZE_4KB;
> > +    for ( IndexOfPml4Entries = 0
> > +        ; IndexOfPml4Entries < (NumberOfPml5EntriesNeeded == 1 ? NumberOfPml4EntriesNeeded : 512)
> > +        ; IndexOfPml4Entries++, PageMapLevel4Entry++) {
> > +      //
> > +      // Each PML4 entry points to a page of Page Directory Pointer entires.
> > +      // So lets allocate space for them and fill them in in the IndexOfPdpEntries loop.
> > +      //
> > +      PageDirectoryPointerEntry = (VOID *) BigPageAddress;
> > +      BigPageAddress += SIZE_4KB;
> >
> > -        //
> > -        // Fill in a Page Directory Pointer Entries
> > -        //
> > -        PageDirectoryPointerEntry->Uint64 = (UINT64)(UINTN)PageDirectoryEntry | AddressEncMask;
> > -        PageDirectoryPointerEntry->Bits.ReadWrite = 1;
> > -        PageDirectoryPointerEntry->Bits.Present = 1;
> > +      //
> > +      // Make a PML4 Entry
> > +      //
> > +      PageMapLevel4Entry->Uint64 = (UINT64)(UINTN)PageDirectoryPointerEntry | AddressEncMask;
> > +      PageMapLevel4Entry->Bits.ReadWrite = 1;
> > +      PageMapLevel4Entry->Bits.Present = 1;
> >
> > -        for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++,
> PageDirectoryEntry++, PageAddress += SIZE_2MB) {
> > -          if (ToSplitPageTable (PageAddress, SIZE_2MB, StackBase, StackSize)) {
> > -            //
> > -            // Need to split this 2M page that covers NULL or stack range.
> > -            //
> > -            Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry, StackBase, StackSize);
> > +      if (Page1GSupport) {
> > +        PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
> > +
> > +        for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++,
> PageDirectory1GEntry++, PageAddress += SIZE_1GB) {
> > +          if (ToSplitPageTable (PageAddress, SIZE_1GB, StackBase, StackSize)) {
> > +            Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry, StackBase, StackSize);
> >            } else {
> >              //
> >              // Fill in the Page Directory entries
> >              //
> > -            PageDirectoryEntry->Uint64 = (UINT64)PageAddress | AddressEncMask;
> > -            PageDirectoryEntry->Bits.ReadWrite = 1;
> > -            PageDirectoryEntry->Bits.Present = 1;
> > -            PageDirectoryEntry->Bits.MustBe1 = 1;
> > +            PageDirectory1GEntry->Uint64 = (UINT64)PageAddress | AddressEncMask;
> > +            PageDirectory1GEntry->Bits.ReadWrite = 1;
> > +            PageDirectory1GEntry->Bits.Present = 1;
> > +            PageDirectory1GEntry->Bits.MustBe1 = 1;
> >            }
> >          }
> > -      }
> > +      } else {
> > +        for ( IndexOfPdpEntries = 0
> > +            ; IndexOfPdpEntries < (NumberOfPml4EntriesNeeded == 1 ? NumberOfPdpEntriesNeeded : 512)
> > +            ; IndexOfPdpEntries++, PageDirectoryPointerEntry++) {
> > +          //
> > +          // Each Directory Pointer entries points to a page of Page Directory entires.
> > +          // So allocate space for them and fill them in in the IndexOfPageDirectoryEntries loop.
> > +          //
> > +          PageDirectoryEntry = (VOID *) BigPageAddress;
> > +          BigPageAddress += SIZE_4KB;
> >
> > -      for (; IndexOfPdpEntries < 512; IndexOfPdpEntries++, PageDirectoryPointerEntry++) {
> > -        ZeroMem (
> > -          PageDirectoryPointerEntry,
> > -          sizeof(PAGE_MAP_AND_DIRECTORY_POINTER)
> > -          );
> > +          //
> > +          // Fill in a Page Directory Pointer Entries
> > +          //
> > +          PageDirectoryPointerEntry->Uint64 = (UINT64)(UINTN)PageDirectoryEntry | AddressEncMask;
> > +          PageDirectoryPointerEntry->Bits.ReadWrite = 1;
> > +          PageDirectoryPointerEntry->Bits.Present = 1;
> > +
> > +          for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++,
> PageDirectoryEntry++, PageAddress += SIZE_2MB) {
> > +            if (ToSplitPageTable (PageAddress, SIZE_2MB, StackBase, StackSize)) {
> > +              //
> > +              // Need to split this 2M page that covers NULL or stack range.
> > +              //
> > +              Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry, StackBase, StackSize);
> > +            } else {
> > +              //
> > +              // Fill in the Page Directory entries
> > +              //
> > +              PageDirectoryEntry->Uint64 = (UINT64)PageAddress | AddressEncMask;
> > +              PageDirectoryEntry->Bits.ReadWrite = 1;
> > +              PageDirectoryEntry->Bits.Present = 1;
> > +              PageDirectoryEntry->Bits.MustBe1 = 1;
> > +            }
> > +          }
> > +        }
> > +
> > +        //
> > +        // Fill with null entry for unused PDPTE
> > +        //
> > +        ZeroMem (PageDirectoryPointerEntry, (512 - IndexOfPdpEntries) * sizeof(PAGE_MAP_AND_DIRECTORY_POINTER));
> >        }
> >      }
> > +
> > +    //
> > +    // For the PML4 entries we are not using fill in a null entry.
> > +    //
> > +    ZeroMem (PageMapLevel4Entry, (512 - IndexOfPml4Entries) * sizeof (PAGE_MAP_AND_DIRECTORY_POINTER));
> >    }
> >
> > -  //
> > -  // For the PML4 entries we are not using fill in a null entry.
> > -  //
> > -  for (; IndexOfPml4Entries < 512; IndexOfPml4Entries++, PageMapLevel4Entry++) {
> > -    ZeroMem (
> > -      PageMapLevel4Entry,
> > -      sizeof (PAGE_MAP_AND_DIRECTORY_POINTER)
> > -      );
> > +  if (Page5LevelSupport) {
> > +    Cr4.UintN = AsmReadCr4 ();
> > +    Cr4.Bits.LA57 = 1;
> > +    AsmWriteCr4 (Cr4.UintN);
> > +    //
> > +    // For the PML5 entries we are not using fill in a null entry.
> > +    //
> > +    ZeroMem (PageMapLevel5Entry, (512 - IndexOfPml5Entries) * sizeof (PAGE_MAP_AND_DIRECTORY_POINTER));
> >    }
> >
> >    //
> >
> 
> 
> 

Re: [edk2-devel] [PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level page table for long mode

[Laszlo Ersek]

On 07/23/19 17:29, Ni, Ray wrote:
> 
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Tuesday, July 23, 2019 5:46 PM
>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
>> Cc: Dong, Eric <eric.dong@intel.com>
>> Subject: Re: [edk2-devel] [PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level page table for long mode
>>
>> On 07/22/19 10:15, Ni, Ray wrote:
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
>>>
>>> DxeIpl is responsible to create page table for DXE phase running
>>> either in long mode or in 32bit mode with certain protection
>>> mechanism enabled (refer to ToBuildPageTable()).
>>>
>>> The patch updates DxeIpl to create 5-level page table for DXE phase
>>> running in long mode when PcdUse5LevelPageTable is TRUE and CPU
>>> supports 5-level page table.
>>>
>>> Signed-off-by: Ray Ni <ray.ni@intel.com>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf       |   1 +
>>>  .../Core/DxeIplPeim/X64/VirtualMemory.c       | 227 ++++++++++++------
>>>  2 files changed, 151 insertions(+), 77 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>>> index abc3217b01..98bc17fc9d 100644
>>> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>>> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>>> @@ -110,6 +110,7 @@ [Pcd.IA32,Pcd.X64]
>>>    gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask    ## CONSUMES
>>>    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask               ## CONSUMES
>>>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                       ## CONSUMES
>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable                  ## SOMETIMES_CONSUMES
>>>
>>>  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>>>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ## SOMETIMES_CONSUMES
>>> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
>>> index edc38e4525..a5bcdcc734 100644
>>> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
>>> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
>>> @@ -15,7 +15,7 @@
>>>      2) IA-32 Intel(R) Architecture Software Developer's Manual Volume 2:Instruction Set Reference, Intel
>>>      3) IA-32 Intel(R) Architecture Software Developer's Manual Volume 3:System Programmer's Guide, Intel
>>>
>>> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>>> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>>>  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
>>>
>>>  SPDX-License-Identifier: BSD-2-Clause-Patent
>>> @@ -626,14 +626,19 @@ CreateIdentityMappingPageTables (
>>>    )
>>>  {
>>>    UINT32                                        RegEax;
>>> +  UINT32                                        RegEbx;
>>> +  UINT32                                        RegEcx;
>>>    UINT32                                        RegEdx;
>>>    UINT8                                         PhysicalAddressBits;
>>>    EFI_PHYSICAL_ADDRESS                          PageAddress;
>>> +  UINTN                                         IndexOfPml5Entries;
>>>    UINTN                                         IndexOfPml4Entries;
>>>    UINTN                                         IndexOfPdpEntries;
>>>    UINTN                                         IndexOfPageDirectoryEntries;
>>> +  UINT32                                        NumberOfPml5EntriesNeeded;
>>>    UINT32                                        NumberOfPml4EntriesNeeded;
>>>    UINT32                                        NumberOfPdpEntriesNeeded;
>>> +  PAGE_MAP_AND_DIRECTORY_POINTER                *PageMapLevel5Entry;
>>>    PAGE_MAP_AND_DIRECTORY_POINTER                *PageMapLevel4Entry;
>>>    PAGE_MAP_AND_DIRECTORY_POINTER                *PageMap;
>>>    PAGE_MAP_AND_DIRECTORY_POINTER                *PageDirectoryPointerEntry;
>>> @@ -641,9 +646,11 @@ CreateIdentityMappingPageTables (
>>>    UINTN                                         TotalPagesNum;
>>>    UINTN                                         BigPageAddress;
>>>    VOID                                          *Hob;
>>> +  BOOLEAN                                       Page5LevelSupport;
>>>    BOOLEAN                                       Page1GSupport;
>>>    PAGE_TABLE_1G_ENTRY                           *PageDirectory1GEntry;
>>>    UINT64                                        AddressEncMask;
>>> +  IA32_CR4                                      Cr4;
>>>
>>>    //
>>>    // Make sure AddressEncMask is contained to smallest supported address field
>>> @@ -677,33 +684,66 @@ CreateIdentityMappingPageTables (
>>>      }
>>>    }
>>>
>>> +  Page5LevelSupport = FALSE;
>>> +  if (PcdGetBool (PcdUse5LevelPageTable)) {
>>> +    AsmCpuidEx (0x7, 0, &RegEax, &RegEbx, &RegEcx, &RegEdx);
>>> +    DEBUG ((DEBUG_INFO, "Cpuid(7/0): %08x/%08x/%08x/%08x\n", RegEax, RegEbx, RegEcx, RegEdx));
>>> +    if ((RegEcx & BIT16) != 0) {
>>
>> (1) Would it be possible to use macro names here, for Index and SubIndex
>> in AsmCpuidEx(), and a "better" macro name than BIT16, in the RegEcx check?
> 
> There are macros which are defined in UefiCpuPkg for the CPUID leaf functions
> and structures for the CPUID output data.
> But since there is rule that MdeModulePkg cannot depend on UefiCpuPkg so
> the code cannot use those macros/structures.
> 
> I am thinking of a new rule that UefiCpuPkg and MdeModulePkg can depend on each other.
> I haven't talked with Mike on this. Not sure if he is ok on this.
> For this case, yes I can temporary define 2 macros: one for 0x7, the other for BIT16.

I'm aware of this restriction. I think it's a good one; personally I
wouldn't like MdeModulePkg to depend on UefiCpuPkg. For example, while
MdeModulePkg is very necessary for AARCH64 platforms, UefiCpuPkg doesn't
appear necessary for AARCH64 platforms.

If both MdeModulePkg and UefiCpuPkg depend on such macros, then the
macros should arguably be defined in
"MdeModulePkg/Include/IndustryStandard", or even
"MdePkg/Include/IndustryStandard". The registers and bit-fields come
from published industry specifications. (Published by Intel :) )

>>> +      Page5LevelSupport = TRUE;
>>> +    }
>>> +  }
>>> +
>>> +  DEBUG ((DEBUG_INFO, "AddressBits/5LevelPaging/1GPage = %d/%d/%d\n", PhysicalAddressBits, Page5LevelSupport,
>> Page1GSupport));
>>> +
>>
>> (2) Can we format this log message as:
>>
>>   AddressBits=%d 5LevelPaging=%d 1GPage=%d
> 
> ok.
> 
>>
>> ?
>>
>>>    //
>>> -  // IA-32e paging translates 48-bit linear addresses to 52-bit physical addresses.
>>> +  // IA-32e paging translates 48-bit linear addresses to 52-bit physical addresses
>>> +  //  when 5-Level Paging is disabled,
>>> +  //  due to either unsupported by HW, or disabled by PCD.
>>>    //
>>>    ASSERT (PhysicalAddressBits <= 52);
>>> -  if (PhysicalAddressBits > 48) {
>>> +  if (!Page5LevelSupport && PhysicalAddressBits > 48) {
>>>      PhysicalAddressBits = 48;
>>>    }
>>>
>>>    //
>>>    // Calculate the table entries needed.
>>>    //
>>> -  if (PhysicalAddressBits <= 39 ) {
>>> -    NumberOfPml4EntriesNeeded = 1;
>>> -    NumberOfPdpEntriesNeeded = (UINT32)LShiftU64 (1, (PhysicalAddressBits - 30));
>>> -  } else {
>>> -    NumberOfPml4EntriesNeeded = (UINT32)LShiftU64 (1, (PhysicalAddressBits - 39));
>>> -    NumberOfPdpEntriesNeeded = 512;
>>> +  NumberOfPml5EntriesNeeded = 1;
>>> +  if (PhysicalAddressBits > 48) {
>>> +    NumberOfPml5EntriesNeeded = (UINT32) LShiftU64 (1, PhysicalAddressBits - 48);
>>> +    PhysicalAddressBits = 48;
>>>    }
>>>
>>> +  NumberOfPml4EntriesNeeded = 1;
>>> +  if (PhysicalAddressBits > 39) {
>>> +    NumberOfPml4EntriesNeeded = (UINT32) LShiftU64 (1, PhysicalAddressBits - 39);
>>> +    PhysicalAddressBits = 39;
>>> +  }
>>> +
>>> +  NumberOfPdpEntriesNeeded = 1;
>>> +  ASSERT (PhysicalAddressBits > 30);
>>> +  NumberOfPdpEntriesNeeded = (UINT32) LShiftU64 (1, PhysicalAddressBits - 30);
>>> +
>>>    //
>>>    // Pre-allocate big pages to avoid later allocations.
>>>    //
>>>    if (!Page1GSupport) {
>>> -    TotalPagesNum = (NumberOfPdpEntriesNeeded + 1) * NumberOfPml4EntriesNeeded + 1;
>>> +    TotalPagesNum = ((NumberOfPdpEntriesNeeded + 1) * NumberOfPml4EntriesNeeded + 1) * NumberOfPml5EntriesNeeded
>> + 1;
>>>    } else {
>>> -    TotalPagesNum = NumberOfPml4EntriesNeeded + 1;
>>> +    TotalPagesNum = (NumberOfPml4EntriesNeeded + 1) * NumberOfPml5EntriesNeeded + 1;
>>> +  }
>>> +
>>> +  //
>>> +  // Substract the one page occupied by PML5 entries if 5-Level Paging is disabled.
>>> +  //
>>> +  if (!Page5LevelSupport) {
>>> +    TotalPagesNum--;
>>>    }
>>> +
>>> +  DEBUG ((DEBUG_INFO, "Pml5/Pml4/Pdp/TotalPage = %d/%d/%d/%d\n",
>>> +    NumberOfPml5EntriesNeeded, NumberOfPml4EntriesNeeded,
>>> +    NumberOfPdpEntriesNeeded, TotalPagesNum));
>>> +
>>
>> (3) Same comment about log message formatting as in (2).
>>
>> (4) Please log UINT32 values with %u, not %d.
>>
>> (5) Please log UINTN values with %Lu (and cast them to UINT64 first).
> 
> 
> ok to above 3, 4, 5.
> 
>>
>> (6) More generally, can we please replace this patch with three patches,
>> in the series?
>>
>> - the first patch should extract the TotalPagesNum calculation to a
>> separate *library* function (it should be a public function in a BASE or
>> PEIM library)
>>
>> - the second patch should extend the TotalPagesNum calculation in the
>> library function to 5-level paging
>>
>> - the third patch should be the remaining code from the present patch.
>>
>> Here's why: in OvmfPkg/PlatformPei, the GetPeiMemoryCap() function
>> duplicates this calculation. In OVMF, PEI-phase memory allocations can
>> be dominated by the page tables built for 64-bit DXE, and so
>> OvmfPkg/PlatformPei must know, for sizing the permanent PEI RAM, how
>> much RAM the DXE IPL PEIM will need for those page tables.
>>
>> I would *really* dislike copying this update (for 5-level paging) from
>> CreateIdentityMappingPageTables() to GetPeiMemoryCap(). Instead, the
>> calculation should be extracted to a library function, and then OVMF
>> (and all other platforms affected similarly) could call the new function.
>>
>> If this is not feasible or very much out of scope, then I guess we'll
>> just keep the PCD as FALSE in OVMF, for the time being.
>>
> 
> BZ https://bugzilla.tianocore.org/show_bug.cgi?id=847 may be what
> you need.

Oh, indeed. I'm subscribed to this BZ, but the last update was in Jan
2018 :)

> Extracting a library API to return how many pages are needed for page tables
> needs to consider several cases:
> 1. 4K or 2M or 1G page size is used.
> 2. range of physical memory
> 3. page table level
> 
> But right now, this is not in the high priority in my to-do list.
> I can help to change GetPeiMemoryCap() for short-term needs.
> Are you ok with this?

No, I'm not.

Obviously, I'm not trying to force you to abstract out the library in
question, just for OVMF's sake. What I'm saying is that I strongly
prefer delaying 5-level paging support in OVMF until this library
becomes available, over duplicating yet more code (and complex code, at
that) from other Packages to OvmfPkg. From that perspective, I
appreciate the new PCD very much, because it should make this postponing
easy.

In other words, there is no short-term need.

Thank you!
Laszlo

Re: [edk2-devel] [PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level page table for long mode

[Michael D Kinney]

Laszlo,

There already a few examples in MdePkg/Include/Library/BaseLib.h.
For example, the bit field structures for CR0, CR4, EFLAGS,
and a segment descriptor are in that .h file.  These are all
within:

#if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64)
. . . 
#endif

We have since used a standard way to provide .h files for
registers.  Best example is in UefiCpuPkg/Include/Register.

It may make sense to put the register definitions required
by MdePkg and MdeModulePkg in MdePkg/Include/Register, and
files that use those register types can include the
required register definition include files.

Best regards,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io
> [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
> Sent: Tuesday, July 23, 2019 12:20 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH 4/4]
> MdeModulePkg/DxeIpl: Create 5-level page table for long
> mode
> 
> On 07/23/19 17:29, Ni, Ray wrote:
> >
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Laszlo
> >> Ersek
> >> Sent: Tuesday, July 23, 2019 5:46 PM
> >> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> >> Cc: Dong, Eric <eric.dong@intel.com>
> >> Subject: Re: [edk2-devel] [PATCH 4/4]
> MdeModulePkg/DxeIpl: Create
> >> 5-level page table for long mode
> >>
> >> On 07/22/19 10:15, Ni, Ray wrote:
> >>> REF:
> https://bugzilla.tianocore.org/show_bug.cgi?id=2008
> >>>
> >>> DxeIpl is responsible to create page table for DXE
> phase running
> >>> either in long mode or in 32bit mode with certain
> protection
> >>> mechanism enabled (refer to ToBuildPageTable()).
> >>>
> >>> The patch updates DxeIpl to create 5-level page
> table for DXE phase
> >>> running in long mode when PcdUse5LevelPageTable is
> TRUE and CPU
> >>> supports 5-level page table.
> >>>
> >>> Signed-off-by: Ray Ni <ray.ni@intel.com>
> >>> Cc: Eric Dong <eric.dong@intel.com>
> >>> Cc: Laszlo Ersek <lersek@redhat.com>
> >>> ---
> >>>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf       |
> 1 +
> >>>  .../Core/DxeIplPeim/X64/VirtualMemory.c       |
> 227 ++++++++++++------
> >>>  2 files changed, 151 insertions(+), 77 deletions(-
> )
> >>>
> >>> diff --git
> a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> >>> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> >>> index abc3217b01..98bc17fc9d 100644
> >>> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> >>> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> >>> @@ -110,6 +110,7 @@ [Pcd.IA32,Pcd.X64]
> >>>
> gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionP
> ropertyMask    ## CONSUMES
> >>>
> gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
> ## CONSUMES
> >>>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
> ## CONSUMES
> >>> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable
> ## SOMETIMES_CONSUMES
> >>>
> >>>  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
> >>>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack
> ## SOMETIMES_CONSUMES
> >>> diff --git
> a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> >>> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> >>> index edc38e4525..a5bcdcc734 100644
> >>> ---
> a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> >>> +++
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> >>> @@ -15,7 +15,7 @@
> >>>      2) IA-32 Intel(R) Architecture Software
> Developer's Manual Volume 2:Instruction Set Reference,
> Intel
> >>>      3) IA-32 Intel(R) Architecture Software
> Developer's Manual
> >>> Volume 3:System Programmer's Guide, Intel
> >>>
> >>> -Copyright (c) 2006 - 2018, Intel Corporation. All
> rights
> >>> reserved.<BR>
> >>> +Copyright (c) 2006 - 2019, Intel Corporation. All
> rights
> >>> +reserved.<BR>
> >>>  Copyright (c) 2017, AMD Incorporated. All rights
> reserved.<BR>
> >>>
> >>>  SPDX-License-Identifier: BSD-2-Clause-Patent @@ -
> 626,14 +626,19 @@
> >>> CreateIdentityMappingPageTables (
> >>>    )
> >>>  {
> >>>    UINT32
> RegEax;
> >>> +  UINT32
> RegEbx;
> >>> +  UINT32
> RegEcx;
> >>>    UINT32
> RegEdx;
> >>>    UINT8
> PhysicalAddressBits;
> >>>    EFI_PHYSICAL_ADDRESS
> PageAddress;
> >>> +  UINTN
> IndexOfPml5Entries;
> >>>    UINTN
> IndexOfPml4Entries;
> >>>    UINTN
> IndexOfPdpEntries;
> >>>    UINTN
> IndexOfPageDirectoryEntries;
> >>> +  UINT32
> NumberOfPml5EntriesNeeded;
> >>>    UINT32
> NumberOfPml4EntriesNeeded;
> >>>    UINT32
> NumberOfPdpEntriesNeeded;
> >>> +  PAGE_MAP_AND_DIRECTORY_POINTER
> *PageMapLevel5Entry;
> >>>    PAGE_MAP_AND_DIRECTORY_POINTER
> *PageMapLevel4Entry;
> >>>    PAGE_MAP_AND_DIRECTORY_POINTER
> *PageMap;
> >>>    PAGE_MAP_AND_DIRECTORY_POINTER
> *PageDirectoryPointerEntry;
> >>> @@ -641,9 +646,11 @@
> CreateIdentityMappingPageTables (
> >>>    UINTN
> TotalPagesNum;
> >>>    UINTN
> BigPageAddress;
> >>>    VOID
> *Hob;
> >>> +  BOOLEAN
> Page5LevelSupport;
> >>>    BOOLEAN
> Page1GSupport;
> >>>    PAGE_TABLE_1G_ENTRY
> *PageDirectory1GEntry;
> >>>    UINT64
> AddressEncMask;
> >>> +  IA32_CR4
> Cr4;
> >>>
> >>>    //
> >>>    // Make sure AddressEncMask is contained to
> smallest supported
> >>> address field @@ -677,33 +684,66 @@
> CreateIdentityMappingPageTables (
> >>>      }
> >>>    }
> >>>
> >>> +  Page5LevelSupport = FALSE;
> >>> +  if (PcdGetBool (PcdUse5LevelPageTable)) {
> >>> +    AsmCpuidEx (0x7, 0, &RegEax, &RegEbx, &RegEcx,
> &RegEdx);
> >>> +    DEBUG ((DEBUG_INFO, "Cpuid(7/0):
> %08x/%08x/%08x/%08x\n", RegEax, RegEbx, RegEcx,
> RegEdx));
> >>> +    if ((RegEcx & BIT16) != 0) {
> >>
> >> (1) Would it be possible to use macro names here,
> for Index and
> >> SubIndex in AsmCpuidEx(), and a "better" macro name
> than BIT16, in the RegEcx check?
> >
> > There are macros which are defined in UefiCpuPkg for
> the CPUID leaf
> > functions and structures for the CPUID output data.
> > But since there is rule that MdeModulePkg cannot
> depend on UefiCpuPkg
> > so the code cannot use those macros/structures.
> >
> > I am thinking of a new rule that UefiCpuPkg and
> MdeModulePkg can depend on each other.
> > I haven't talked with Mike on this. Not sure if he is
> ok on this.
> > For this case, yes I can temporary define 2 macros:
> one for 0x7, the other for BIT16.
> 
> I'm aware of this restriction. I think it's a good one;
> personally I wouldn't like MdeModulePkg to depend on
> UefiCpuPkg. For example, while MdeModulePkg is very
> necessary for AARCH64 platforms, UefiCpuPkg doesn't
> appear necessary for AARCH64 platforms.
> 
> If both MdeModulePkg and UefiCpuPkg depend on such
> macros, then the macros should arguably be defined in
> "MdeModulePkg/Include/IndustryStandard", or even
> "MdePkg/Include/IndustryStandard". The registers and
> bit-fields come from published industry specifications.
> (Published by Intel :) )
> 
> >>> +      Page5LevelSupport = TRUE;
> >>> +    }
> >>> +  }
> >>> +
> >>> +  DEBUG ((DEBUG_INFO,
> "AddressBits/5LevelPaging/1GPage =
> >>> + %d/%d/%d\n", PhysicalAddressBits,
> Page5LevelSupport,
> >> Page1GSupport));
> >>> +
> >>
> >> (2) Can we format this log message as:
> >>
> >>   AddressBits=%d 5LevelPaging=%d 1GPage=%d
> >
> > ok.
> >
> >>
> >> ?
> >>
> >>>    //
> >>> -  // IA-32e paging translates 48-bit linear
> addresses to 52-bit physical addresses.
> >>> +  // IA-32e paging translates 48-bit linear
> addresses to 52-bit
> >>> + physical addresses  //  when 5-Level Paging is
> disabled,  //  due
> >>> + to either unsupported by HW, or disabled by PCD.
> >>>    //
> >>>    ASSERT (PhysicalAddressBits <= 52);
> >>> -  if (PhysicalAddressBits > 48) {
> >>> +  if (!Page5LevelSupport && PhysicalAddressBits >
> 48) {
> >>>      PhysicalAddressBits = 48;
> >>>    }
> >>>
> >>>    //
> >>>    // Calculate the table entries needed.
> >>>    //
> >>> -  if (PhysicalAddressBits <= 39 ) {
> >>> -    NumberOfPml4EntriesNeeded = 1;
> >>> -    NumberOfPdpEntriesNeeded = (UINT32)LShiftU64
> (1, (PhysicalAddressBits - 30));
> >>> -  } else {
> >>> -    NumberOfPml4EntriesNeeded = (UINT32)LShiftU64
> (1, (PhysicalAddressBits - 39));
> >>> -    NumberOfPdpEntriesNeeded = 512;
> >>> +  NumberOfPml5EntriesNeeded = 1;
> >>> +  if (PhysicalAddressBits > 48) {
> >>> +    NumberOfPml5EntriesNeeded = (UINT32) LShiftU64
> (1, PhysicalAddressBits - 48);
> >>> +    PhysicalAddressBits = 48;
> >>>    }
> >>>
> >>> +  NumberOfPml4EntriesNeeded = 1;
> >>> +  if (PhysicalAddressBits > 39) {
> >>> +    NumberOfPml4EntriesNeeded = (UINT32) LShiftU64
> (1, PhysicalAddressBits - 39);
> >>> +    PhysicalAddressBits = 39;
> >>> +  }
> >>> +
> >>> +  NumberOfPdpEntriesNeeded = 1;
> >>> +  ASSERT (PhysicalAddressBits > 30);
> NumberOfPdpEntriesNeeded =
> >>> + (UINT32) LShiftU64 (1, PhysicalAddressBits - 30);
> >>> +
> >>>    //
> >>>    // Pre-allocate big pages to avoid later
> allocations.
> >>>    //
> >>>    if (!Page1GSupport) {
> >>> -    TotalPagesNum = (NumberOfPdpEntriesNeeded + 1)
> * NumberOfPml4EntriesNeeded + 1;
> >>> +    TotalPagesNum = ((NumberOfPdpEntriesNeeded +
> 1) *
> >>> + NumberOfPml4EntriesNeeded + 1) *
> NumberOfPml5EntriesNeeded
> >> + 1;
> >>>    } else {
> >>> -    TotalPagesNum = NumberOfPml4EntriesNeeded + 1;
> >>> +    TotalPagesNum = (NumberOfPml4EntriesNeeded +
> 1) *
> >>> + NumberOfPml5EntriesNeeded + 1;  }
> >>> +
> >>> +  //
> >>> +  // Substract the one page occupied by PML5
> entries if 5-Level Paging is disabled.
> >>> +  //
> >>> +  if (!Page5LevelSupport) {
> >>> +    TotalPagesNum--;
> >>>    }
> >>> +
> >>> +  DEBUG ((DEBUG_INFO, "Pml5/Pml4/Pdp/TotalPage =
> %d/%d/%d/%d\n",
> >>> +    NumberOfPml5EntriesNeeded,
> NumberOfPml4EntriesNeeded,
> >>> +    NumberOfPdpEntriesNeeded, TotalPagesNum));
> >>> +
> >>
> >> (3) Same comment about log message formatting as in
> (2).
> >>
> >> (4) Please log UINT32 values with %u, not %d.
> >>
> >> (5) Please log UINTN values with %Lu (and cast them
> to UINT64 first).
> >
> >
> > ok to above 3, 4, 5.
> >
> >>
> >> (6) More generally, can we please replace this patch
> with three
> >> patches, in the series?
> >>
> >> - the first patch should extract the TotalPagesNum
> calculation to a
> >> separate *library* function (it should be a public
> function in a BASE
> >> or PEIM library)
> >>
> >> - the second patch should extend the TotalPagesNum
> calculation in the
> >> library function to 5-level paging
> >>
> >> - the third patch should be the remaining code from
> the present patch.
> >>
> >> Here's why: in OvmfPkg/PlatformPei, the
> GetPeiMemoryCap() function
> >> duplicates this calculation. In OVMF, PEI-phase
> memory allocations
> >> can be dominated by the page tables built for 64-bit
> DXE, and so
> >> OvmfPkg/PlatformPei must know, for sizing the
> permanent PEI RAM, how
> >> much RAM the DXE IPL PEIM will need for those page
> tables.
> >>
> >> I would *really* dislike copying this update (for 5-
> level paging)
> >> from
> >> CreateIdentityMappingPageTables() to
> GetPeiMemoryCap(). Instead, the
> >> calculation should be extracted to a library
> function, and then OVMF
> >> (and all other platforms affected similarly) could
> call the new function.
> >>
> >> If this is not feasible or very much out of scope,
> then I guess we'll
> >> just keep the PCD as FALSE in OVMF, for the time
> being.
> >>
> >
> > BZ https://bugzilla.tianocore.org/show_bug.cgi?id=847
> may be what you
> > need.
> 
> Oh, indeed. I'm subscribed to this BZ, but the last
> update was in Jan
> 2018 :)
> 
> > Extracting a library API to return how many pages are
> needed for page
> > tables needs to consider several cases:
> > 1. 4K or 2M or 1G page size is used.
> > 2. range of physical memory
> > 3. page table level
> >
> > But right now, this is not in the high priority in my
> to-do list.
> > I can help to change GetPeiMemoryCap() for short-term
> needs.
> > Are you ok with this?
> 
> No, I'm not.
> 
> Obviously, I'm not trying to force you to abstract out
> the library in question, just for OVMF's sake. What I'm
> saying is that I strongly prefer delaying 5-level
> paging support in OVMF until this library becomes
> available, over duplicating yet more code (and complex
> code, at
> that) from other Packages to OvmfPkg. From that
> perspective, I appreciate the new PCD very much,
> because it should make this postponing easy.
> 
> In other words, there is no short-term need.
> 
> Thank you!
> Laszlo
> 
> 

Re: [edk2-devel] [PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level page table for long mode

[Ni, Ray]

Mike,
Thanks for the suggestion.
I will try to move Cpuid.h to MdePkg/Include/Register directory in V2 patch.

Thanks,
Ray

> -----Original Message-----
> From: Kinney, Michael D
> Sent: Wednesday, July 24, 2019 7:54 AM
> To: devel@edk2.groups.io; lersek@redhat.com; Ni, Ray <ray.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2-devel] [PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level page table for long mode
> 
> Laszlo,
> 
> There already a few examples in MdePkg/Include/Library/BaseLib.h.
> For example, the bit field structures for CR0, CR4, EFLAGS,
> and a segment descriptor are in that .h file.  These are all
> within:
> 
> #if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64)
> . . .
> #endif
> 
> We have since used a standard way to provide .h files for
> registers.  Best example is in UefiCpuPkg/Include/Register.
> 
> It may make sense to put the register definitions required
> by MdePkg and MdeModulePkg in MdePkg/Include/Register, and
> files that use those register types can include the
> required register definition include files.
> 
> Best regards,
> 
> Mike
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io
> > [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
> > Sent: Tuesday, July 23, 2019 12:20 PM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> > Cc: Dong, Eric <eric.dong@intel.com>
> > Subject: Re: [edk2-devel] [PATCH 4/4]
> > MdeModulePkg/DxeIpl: Create 5-level page table for long
> > mode
> >
> > On 07/23/19 17:29, Ni, Ray wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> > Behalf Of Laszlo
> > >> Ersek
> > >> Sent: Tuesday, July 23, 2019 5:46 PM
> > >> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> > >> Cc: Dong, Eric <eric.dong@intel.com>
> > >> Subject: Re: [edk2-devel] [PATCH 4/4]
> > MdeModulePkg/DxeIpl: Create
> > >> 5-level page table for long mode
> > >>
> > >> On 07/22/19 10:15, Ni, Ray wrote:
> > >>> REF:
> > https://bugzilla.tianocore.org/show_bug.cgi?id=2008
> > >>>
> > >>> DxeIpl is responsible to create page table for DXE
> > phase running
> > >>> either in long mode or in 32bit mode with certain
> > protection
> > >>> mechanism enabled (refer to ToBuildPageTable()).
> > >>>
> > >>> The patch updates DxeIpl to create 5-level page
> > table for DXE phase
> > >>> running in long mode when PcdUse5LevelPageTable is
> > TRUE and CPU
> > >>> supports 5-level page table.
> > >>>
> > >>> Signed-off-by: Ray Ni <ray.ni@intel.com>
> > >>> Cc: Eric Dong <eric.dong@intel.com>
> > >>> Cc: Laszlo Ersek <lersek@redhat.com>
> > >>> ---
> > >>>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf       |
> > 1 +
> > >>>  .../Core/DxeIplPeim/X64/VirtualMemory.c       |
> > 227 ++++++++++++------
> > >>>  2 files changed, 151 insertions(+), 77 deletions(-
> > )
> > >>>
> > >>> diff --git
> > a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > >>> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > >>> index abc3217b01..98bc17fc9d 100644
> > >>> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > >>> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > >>> @@ -110,6 +110,7 @@ [Pcd.IA32,Pcd.X64]
> > >>>
> > gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionP
> > ropertyMask    ## CONSUMES
> > >>>
> > gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
> > ## CONSUMES
> > >>>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
> > ## CONSUMES
> > >>> +
> > gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable
> > ## SOMETIMES_CONSUMES
> > >>>
> > >>>  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
> > >>>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack
> > ## SOMETIMES_CONSUMES
> > >>> diff --git
> > a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> > >>> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> > >>> index edc38e4525..a5bcdcc734 100644
> > >>> ---
> > a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> > >>> +++
> > b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> > >>> @@ -15,7 +15,7 @@
> > >>>      2) IA-32 Intel(R) Architecture Software
> > Developer's Manual Volume 2:Instruction Set Reference,
> > Intel
> > >>>      3) IA-32 Intel(R) Architecture Software
> > Developer's Manual
> > >>> Volume 3:System Programmer's Guide, Intel
> > >>>
> > >>> -Copyright (c) 2006 - 2018, Intel Corporation. All
> > rights
> > >>> reserved.<BR>
> > >>> +Copyright (c) 2006 - 2019, Intel Corporation. All
> > rights
> > >>> +reserved.<BR>
> > >>>  Copyright (c) 2017, AMD Incorporated. All rights
> > reserved.<BR>
> > >>>
> > >>>  SPDX-License-Identifier: BSD-2-Clause-Patent @@ -
> > 626,14 +626,19 @@
> > >>> CreateIdentityMappingPageTables (
> > >>>    )
> > >>>  {
> > >>>    UINT32
> > RegEax;
> > >>> +  UINT32
> > RegEbx;
> > >>> +  UINT32
> > RegEcx;
> > >>>    UINT32
> > RegEdx;
> > >>>    UINT8
> > PhysicalAddressBits;
> > >>>    EFI_PHYSICAL_ADDRESS
> > PageAddress;
> > >>> +  UINTN
> > IndexOfPml5Entries;
> > >>>    UINTN
> > IndexOfPml4Entries;
> > >>>    UINTN
> > IndexOfPdpEntries;
> > >>>    UINTN
> > IndexOfPageDirectoryEntries;
> > >>> +  UINT32
> > NumberOfPml5EntriesNeeded;
> > >>>    UINT32
> > NumberOfPml4EntriesNeeded;
> > >>>    UINT32
> > NumberOfPdpEntriesNeeded;
> > >>> +  PAGE_MAP_AND_DIRECTORY_POINTER
> > *PageMapLevel5Entry;
> > >>>    PAGE_MAP_AND_DIRECTORY_POINTER
> > *PageMapLevel4Entry;
> > >>>    PAGE_MAP_AND_DIRECTORY_POINTER
> > *PageMap;
> > >>>    PAGE_MAP_AND_DIRECTORY_POINTER
> > *PageDirectoryPointerEntry;
> > >>> @@ -641,9 +646,11 @@
> > CreateIdentityMappingPageTables (
> > >>>    UINTN
> > TotalPagesNum;
> > >>>    UINTN
> > BigPageAddress;
> > >>>    VOID
> > *Hob;
> > >>> +  BOOLEAN
> > Page5LevelSupport;
> > >>>    BOOLEAN
> > Page1GSupport;
> > >>>    PAGE_TABLE_1G_ENTRY
> > *PageDirectory1GEntry;
> > >>>    UINT64
> > AddressEncMask;
> > >>> +  IA32_CR4
> > Cr4;
> > >>>
> > >>>    //
> > >>>    // Make sure AddressEncMask is contained to
> > smallest supported
> > >>> address field @@ -677,33 +684,66 @@
> > CreateIdentityMappingPageTables (
> > >>>      }
> > >>>    }
> > >>>
> > >>> +  Page5LevelSupport = FALSE;
> > >>> +  if (PcdGetBool (PcdUse5LevelPageTable)) {
> > >>> +    AsmCpuidEx (0x7, 0, &RegEax, &RegEbx, &RegEcx,
> > &RegEdx);
> > >>> +    DEBUG ((DEBUG_INFO, "Cpuid(7/0):
> > %08x/%08x/%08x/%08x\n", RegEax, RegEbx, RegEcx,
> > RegEdx));
> > >>> +    if ((RegEcx & BIT16) != 0) {
> > >>
> > >> (1) Would it be possible to use macro names here,
> > for Index and
> > >> SubIndex in AsmCpuidEx(), and a "better" macro name
> > than BIT16, in the RegEcx check?
> > >
> > > There are macros which are defined in UefiCpuPkg for
> > the CPUID leaf
> > > functions and structures for the CPUID output data.
> > > But since there is rule that MdeModulePkg cannot
> > depend on UefiCpuPkg
> > > so the code cannot use those macros/structures.
> > >
> > > I am thinking of a new rule that UefiCpuPkg and
> > MdeModulePkg can depend on each other.
> > > I haven't talked with Mike on this. Not sure if he is
> > ok on this.
> > > For this case, yes I can temporary define 2 macros:
> > one for 0x7, the other for BIT16.
> >
> > I'm aware of this restriction. I think it's a good one;
> > personally I wouldn't like MdeModulePkg to depend on
> > UefiCpuPkg. For example, while MdeModulePkg is very
> > necessary for AARCH64 platforms, UefiCpuPkg doesn't
> > appear necessary for AARCH64 platforms.
> >
> > If both MdeModulePkg and UefiCpuPkg depend on such
> > macros, then the macros should arguably be defined in
> > "MdeModulePkg/Include/IndustryStandard", or even
> > "MdePkg/Include/IndustryStandard". The registers and
> > bit-fields come from published industry specifications.
> > (Published by Intel :) )
> >
> > >>> +      Page5LevelSupport = TRUE;
> > >>> +    }
> > >>> +  }
> > >>> +
> > >>> +  DEBUG ((DEBUG_INFO,
> > "AddressBits/5LevelPaging/1GPage =
> > >>> + %d/%d/%d\n", PhysicalAddressBits,
> > Page5LevelSupport,
> > >> Page1GSupport));
> > >>> +
> > >>
> > >> (2) Can we format this log message as:
> > >>
> > >>   AddressBits=%d 5LevelPaging=%d 1GPage=%d
> > >
> > > ok.
> > >
> > >>
> > >> ?
> > >>
> > >>>    //
> > >>> -  // IA-32e paging translates 48-bit linear
> > addresses to 52-bit physical addresses.
> > >>> +  // IA-32e paging translates 48-bit linear
> > addresses to 52-bit
> > >>> + physical addresses  //  when 5-Level Paging is
> > disabled,  //  due
> > >>> + to either unsupported by HW, or disabled by PCD.
> > >>>    //
> > >>>    ASSERT (PhysicalAddressBits <= 52);
> > >>> -  if (PhysicalAddressBits > 48) {
> > >>> +  if (!Page5LevelSupport && PhysicalAddressBits >
> > 48) {
> > >>>      PhysicalAddressBits = 48;
> > >>>    }
> > >>>
> > >>>    //
> > >>>    // Calculate the table entries needed.
> > >>>    //
> > >>> -  if (PhysicalAddressBits <= 39 ) {
> > >>> -    NumberOfPml4EntriesNeeded = 1;
> > >>> -    NumberOfPdpEntriesNeeded = (UINT32)LShiftU64
> > (1, (PhysicalAddressBits - 30));
> > >>> -  } else {
> > >>> -    NumberOfPml4EntriesNeeded = (UINT32)LShiftU64
> > (1, (PhysicalAddressBits - 39));
> > >>> -    NumberOfPdpEntriesNeeded = 512;
> > >>> +  NumberOfPml5EntriesNeeded = 1;
> > >>> +  if (PhysicalAddressBits > 48) {
> > >>> +    NumberOfPml5EntriesNeeded = (UINT32) LShiftU64
> > (1, PhysicalAddressBits - 48);
> > >>> +    PhysicalAddressBits = 48;
> > >>>    }
> > >>>
> > >>> +  NumberOfPml4EntriesNeeded = 1;
> > >>> +  if (PhysicalAddressBits > 39) {
> > >>> +    NumberOfPml4EntriesNeeded = (UINT32) LShiftU64
> > (1, PhysicalAddressBits - 39);
> > >>> +    PhysicalAddressBits = 39;
> > >>> +  }
> > >>> +
> > >>> +  NumberOfPdpEntriesNeeded = 1;
> > >>> +  ASSERT (PhysicalAddressBits > 30);
> > NumberOfPdpEntriesNeeded =
> > >>> + (UINT32) LShiftU64 (1, PhysicalAddressBits - 30);
> > >>> +
> > >>>    //
> > >>>    // Pre-allocate big pages to avoid later
> > allocations.
> > >>>    //
> > >>>    if (!Page1GSupport) {
> > >>> -    TotalPagesNum = (NumberOfPdpEntriesNeeded + 1)
> > * NumberOfPml4EntriesNeeded + 1;
> > >>> +    TotalPagesNum = ((NumberOfPdpEntriesNeeded +
> > 1) *
> > >>> + NumberOfPml4EntriesNeeded + 1) *
> > NumberOfPml5EntriesNeeded
> > >> + 1;
> > >>>    } else {
> > >>> -    TotalPagesNum = NumberOfPml4EntriesNeeded + 1;
> > >>> +    TotalPagesNum = (NumberOfPml4EntriesNeeded +
> > 1) *
> > >>> + NumberOfPml5EntriesNeeded + 1;  }
> > >>> +
> > >>> +  //
> > >>> +  // Substract the one page occupied by PML5
> > entries if 5-Level Paging is disabled.
> > >>> +  //
> > >>> +  if (!Page5LevelSupport) {
> > >>> +    TotalPagesNum--;
> > >>>    }
> > >>> +
> > >>> +  DEBUG ((DEBUG_INFO, "Pml5/Pml4/Pdp/TotalPage =
> > %d/%d/%d/%d\n",
> > >>> +    NumberOfPml5EntriesNeeded,
> > NumberOfPml4EntriesNeeded,
> > >>> +    NumberOfPdpEntriesNeeded, TotalPagesNum));
> > >>> +
> > >>
> > >> (3) Same comment about log message formatting as in
> > (2).
> > >>
> > >> (4) Please log UINT32 values with %u, not %d.
> > >>
> > >> (5) Please log UINTN values with %Lu (and cast them
> > to UINT64 first).
> > >
> > >
> > > ok to above 3, 4, 5.
> > >
> > >>
> > >> (6) More generally, can we please replace this patch
> > with three
> > >> patches, in the series?
> > >>
> > >> - the first patch should extract the TotalPagesNum
> > calculation to a
> > >> separate *library* function (it should be a public
> > function in a BASE
> > >> or PEIM library)
> > >>
> > >> - the second patch should extend the TotalPagesNum
> > calculation in the
> > >> library function to 5-level paging
> > >>
> > >> - the third patch should be the remaining code from
> > the present patch.
> > >>
> > >> Here's why: in OvmfPkg/PlatformPei, the
> > GetPeiMemoryCap() function
> > >> duplicates this calculation. In OVMF, PEI-phase
> > memory allocations
> > >> can be dominated by the page tables built for 64-bit
> > DXE, and so
> > >> OvmfPkg/PlatformPei must know, for sizing the
> > permanent PEI RAM, how
> > >> much RAM the DXE IPL PEIM will need for those page
> > tables.
> > >>
> > >> I would *really* dislike copying this update (for 5-
> > level paging)
> > >> from
> > >> CreateIdentityMappingPageTables() to
> > GetPeiMemoryCap(). Instead, the
> > >> calculation should be extracted to a library
> > function, and then OVMF
> > >> (and all other platforms affected similarly) could
> > call the new function.
> > >>
> > >> If this is not feasible or very much out of scope,
> > then I guess we'll
> > >> just keep the PCD as FALSE in OVMF, for the time
> > being.
> > >>
> > >
> > > BZ https://bugzilla.tianocore.org/show_bug.cgi?id=847
> > may be what you
> > > need.
> >
> > Oh, indeed. I'm subscribed to this BZ, but the last
> > update was in Jan
> > 2018 :)
> >
> > > Extracting a library API to return how many pages are
> > needed for page
> > > tables needs to consider several cases:
> > > 1. 4K or 2M or 1G page size is used.
> > > 2. range of physical memory
> > > 3. page table level
> > >
> > > But right now, this is not in the high priority in my
> > to-do list.
> > > I can help to change GetPeiMemoryCap() for short-term
> > needs.
> > > Are you ok with this?
> >
> > No, I'm not.
> >
> > Obviously, I'm not trying to force you to abstract out
> > the library in question, just for OVMF's sake. What I'm
> > saying is that I strongly prefer delaying 5-level
> > paging support in OVMF until this library becomes
> > available, over duplicating yet more code (and complex
> > code, at
> > that) from other Packages to OvmfPkg. From that
> > perspective, I appreciate the new PCD very much,
> > because it should make this postponing easy.
> >
> > In other words, there is no short-term need.
> >
> > Thank you!
> > Laszlo
> >
> > 

Re: [edk2-devel] [PATCH 3/4] MdeModulePkg/DxeIpl: Introduce PCD PcdUse5LevelPageTable

[Ni, Ray]

Forgot to include MdeModulePkg maintainers for review.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Monday, July 22, 2019 4:16 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2-devel] [PATCH 3/4] MdeModulePkg/DxeIpl: Introduce PCD
> PcdUse5LevelPageTable
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
> 
> The PCD indicates if 5-Level Paging will be enabled in long mode.
> 5-Level Paging will not be enabled when the PCD is TRUE but CPU doesn't
> support 5-Level Paging.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  MdeModulePkg/MdeModulePkg.dec | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec index 12e0bbf579..21388595a9
> 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -1991,6 +1991,13 @@ [PcdsFixedAtBuild, PcdsPatchableInModule,
> PcdsDynamic, PcdsDynamicEx]
>    # @Prompt The address mask when memory encryption is enabled.
> 
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
> ask|0x0|UINT64|0x30001047
> 
> +  ## Indicates if 5-Level Paging will be enabled in long mode. 5-Level
> + Paging will not be enabled  #  when the PCD is TRUE but CPU doesn't
> support 5-Level Paging.
> +  #   TRUE  - 5-Level Paging will be enabled.<BR>
> +  #   FALSE - 5-Level Paging will not be enabled.<BR>
> +  # @Prompt Enable 5-Level Paging support in long mode.
> +
> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable|FALSE|BOOLE
> AN|0x0
> + 001105F
> +
>    ## Capsule In Ram is to use memory to deliver the capsules that will be
> processed after system
>    #  reset.<BR><BR>
>    #  This PCD indicates if the Capsule In Ram is supported.<BR>
> --
> 2.21.0.windows.1
> 
> 
> 

Re: [edk2-devel] [PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level page table for long mode

[Ni, Ray]

Forgot to include MdeModulePkg maintainers for review.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Monday, July 22, 2019 4:16 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2-devel] [PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level
> page table for long mode
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2008
> 
> DxeIpl is responsible to create page table for DXE phase running either in
> long mode or in 32bit mode with certain protection mechanism enabled
> (refer to ToBuildPageTable()).
> 
> The patch updates DxeIpl to create 5-level page table for DXE phase running
> in long mode when PcdUse5LevelPageTable is TRUE and CPU supports 5-
> level page table.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf       |   1 +
>  .../Core/DxeIplPeim/X64/VirtualMemory.c       | 227 ++++++++++++------
>  2 files changed, 151 insertions(+), 77 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index abc3217b01..98bc17fc9d 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -110,6 +110,7 @@ [Pcd.IA32,Pcd.X64]
> 
> gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
> ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
> ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                       ##
> CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable                  ##
> SOMETIMES_CONSUMES
> 
>  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ##
> SOMETIMES_CONSUMES
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index edc38e4525..a5bcdcc734 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -15,7 +15,7 @@
>      2) IA-32 Intel(R) Architecture Software Developer's Manual Volume
> 2:Instruction Set Reference, Intel
>      3) IA-32 Intel(R) Architecture Software Developer's Manual Volume
> 3:System Programmer's Guide, Intel
> 
> -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent @@ -626,14 +626,19 @@
> CreateIdentityMappingPageTables (
>    )
>  {
>    UINT32                                        RegEax;
> +  UINT32                                        RegEbx;
> +  UINT32                                        RegEcx;
>    UINT32                                        RegEdx;
>    UINT8                                         PhysicalAddressBits;
>    EFI_PHYSICAL_ADDRESS                          PageAddress;
> +  UINTN                                         IndexOfPml5Entries;
>    UINTN                                         IndexOfPml4Entries;
>    UINTN                                         IndexOfPdpEntries;
>    UINTN                                         IndexOfPageDirectoryEntries;
> +  UINT32                                        NumberOfPml5EntriesNeeded;
>    UINT32                                        NumberOfPml4EntriesNeeded;
>    UINT32                                        NumberOfPdpEntriesNeeded;
> +  PAGE_MAP_AND_DIRECTORY_POINTER                *PageMapLevel5Entry;
>    PAGE_MAP_AND_DIRECTORY_POINTER                *PageMapLevel4Entry;
>    PAGE_MAP_AND_DIRECTORY_POINTER                *PageMap;
>    PAGE_MAP_AND_DIRECTORY_POINTER
> *PageDirectoryPointerEntry;
> @@ -641,9 +646,11 @@ CreateIdentityMappingPageTables (
>    UINTN                                         TotalPagesNum;
>    UINTN                                         BigPageAddress;
>    VOID                                          *Hob;
> +  BOOLEAN                                       Page5LevelSupport;
>    BOOLEAN                                       Page1GSupport;
>    PAGE_TABLE_1G_ENTRY                           *PageDirectory1GEntry;
>    UINT64                                        AddressEncMask;
> +  IA32_CR4                                      Cr4;
> 
>    //
>    // Make sure AddressEncMask is contained to smallest supported address
> field @@ -677,33 +684,66 @@ CreateIdentityMappingPageTables (
>      }
>    }
> 
> +  Page5LevelSupport = FALSE;
> +  if (PcdGetBool (PcdUse5LevelPageTable)) {
> +    AsmCpuidEx (0x7, 0, &RegEax, &RegEbx, &RegEcx, &RegEdx);
> +    DEBUG ((DEBUG_INFO, "Cpuid(7/0): %08x/%08x/%08x/%08x\n", RegEax,
> RegEbx, RegEcx, RegEdx));
> +    if ((RegEcx & BIT16) != 0) {
> +      Page5LevelSupport = TRUE;
> +    }
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "AddressBits/5LevelPaging/1GPage
> = %d/%d/%d\n",
> + PhysicalAddressBits, Page5LevelSupport, Page1GSupport));
> +
>    //
> -  // IA-32e paging translates 48-bit linear addresses to 52-bit physical
> addresses.
> +  // IA-32e paging translates 48-bit linear addresses to 52-bit
> + physical addresses  //  when 5-Level Paging is disabled,  //  due to
> + either unsupported by HW, or disabled by PCD.
>    //
>    ASSERT (PhysicalAddressBits <= 52);
> -  if (PhysicalAddressBits > 48) {
> +  if (!Page5LevelSupport && PhysicalAddressBits > 48) {
>      PhysicalAddressBits = 48;
>    }
> 
>    //
>    // Calculate the table entries needed.
>    //
> -  if (PhysicalAddressBits <= 39 ) {
> -    NumberOfPml4EntriesNeeded = 1;
> -    NumberOfPdpEntriesNeeded = (UINT32)LShiftU64 (1,
> (PhysicalAddressBits - 30));
> -  } else {
> -    NumberOfPml4EntriesNeeded = (UINT32)LShiftU64 (1,
> (PhysicalAddressBits - 39));
> -    NumberOfPdpEntriesNeeded = 512;
> +  NumberOfPml5EntriesNeeded = 1;
> +  if (PhysicalAddressBits > 48) {
> +    NumberOfPml5EntriesNeeded = (UINT32) LShiftU64 (1,
> PhysicalAddressBits - 48);
> +    PhysicalAddressBits = 48;
>    }
> 
> +  NumberOfPml4EntriesNeeded = 1;
> +  if (PhysicalAddressBits > 39) {
> +    NumberOfPml4EntriesNeeded = (UINT32) LShiftU64 (1,
> PhysicalAddressBits - 39);
> +    PhysicalAddressBits = 39;
> +  }
> +
> +  NumberOfPdpEntriesNeeded = 1;
> +  ASSERT (PhysicalAddressBits > 30);
> +  NumberOfPdpEntriesNeeded = (UINT32) LShiftU64 (1, PhysicalAddressBits
> + - 30);
> +
>    //
>    // Pre-allocate big pages to avoid later allocations.
>    //
>    if (!Page1GSupport) {
> -    TotalPagesNum = (NumberOfPdpEntriesNeeded + 1) *
> NumberOfPml4EntriesNeeded + 1;
> +    TotalPagesNum = ((NumberOfPdpEntriesNeeded + 1) *
> + NumberOfPml4EntriesNeeded + 1) * NumberOfPml5EntriesNeeded + 1;
>    } else {
> -    TotalPagesNum = NumberOfPml4EntriesNeeded + 1;
> +    TotalPagesNum = (NumberOfPml4EntriesNeeded + 1) *
> + NumberOfPml5EntriesNeeded + 1;  }
> +
> +  //
> +  // Substract the one page occupied by PML5 entries if 5-Level Paging is
> disabled.
> +  //
> +  if (!Page5LevelSupport) {
> +    TotalPagesNum--;
>    }
> +
> +  DEBUG ((DEBUG_INFO, "Pml5/Pml4/Pdp/TotalPage = %d/%d/%d/%d\n",
> +    NumberOfPml5EntriesNeeded, NumberOfPml4EntriesNeeded,
> +    NumberOfPdpEntriesNeeded, TotalPagesNum));
> +
>    BigPageAddress = (UINTN) AllocatePageTableMemory (TotalPagesNum);
>    ASSERT (BigPageAddress != 0);
> 
> @@ -711,92 +751,125 @@ CreateIdentityMappingPageTables (
>    // By architecture only one PageMapLevel4 exists - so lets allocate storage
> for it.
>    //
>    PageMap         = (VOID *) BigPageAddress;
> -  BigPageAddress += SIZE_4KB;
> -
> -  PageMapLevel4Entry = PageMap;
> -  PageAddress        = 0;
> -  for (IndexOfPml4Entries = 0; IndexOfPml4Entries <
> NumberOfPml4EntriesNeeded; IndexOfPml4Entries++,
> PageMapLevel4Entry++) {
> +  if (Page5LevelSupport) {
>      //
> -    // Each PML4 entry points to a page of Page Directory Pointer entires.
> -    // So lets allocate space for them and fill them in in the IndexOfPdpEntries
> loop.
> +    // By architecture only one PageMapLevel5 exists - so lets allocate storage
> for it.
>      //
> -    PageDirectoryPointerEntry = (VOID *) BigPageAddress;
> -    BigPageAddress += SIZE_4KB;
> +    PageMapLevel5Entry = PageMap;
> +    BigPageAddress    += SIZE_4KB;
> +  }
> +  PageAddress        = 0;
> 
> +  for ( IndexOfPml5Entries = 0
> +      ; IndexOfPml5Entries < NumberOfPml5EntriesNeeded
> +      ; IndexOfPml5Entries++, PageMapLevel5Entry++) {
>      //
> -    // Make a PML4 Entry
> +    // Each PML5 entry points to a page of PML4 entires.
> +    // So lets allocate space for them and fill them in in the
> IndexOfPml4Entries loop.
> +    // When 5-Level Paging is disabled, below allocation happens only once.
>      //
> -    PageMapLevel4Entry->Uint64 =
> (UINT64)(UINTN)PageDirectoryPointerEntry | AddressEncMask;
> -    PageMapLevel4Entry->Bits.ReadWrite = 1;
> -    PageMapLevel4Entry->Bits.Present = 1;
> +    PageMapLevel4Entry = (VOID *) BigPageAddress;
> +    BigPageAddress    += SIZE_4KB;
> 
> -    if (Page1GSupport) {
> -      PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
> +    if (Page5LevelSupport) {
> +      //
> +      // Make a PML5 Entry
> +      //
> +      PageMapLevel5Entry->Uint64 = (UINT64) (UINTN) PageMapLevel4Entry;
> +      PageMapLevel5Entry->Bits.ReadWrite = 1;
> +      PageMapLevel5Entry->Bits.Present   = 1;
> +    }
> 
> -      for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512;
> IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress +=
> SIZE_1GB) {
> -        if (ToSplitPageTable (PageAddress, SIZE_1GB, StackBase, StackSize)) {
> -          Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry,
> StackBase, StackSize);
> -        } else {
> -          //
> -          // Fill in the Page Directory entries
> -          //
> -          PageDirectory1GEntry->Uint64 = (UINT64)PageAddress |
> AddressEncMask;
> -          PageDirectory1GEntry->Bits.ReadWrite = 1;
> -          PageDirectory1GEntry->Bits.Present = 1;
> -          PageDirectory1GEntry->Bits.MustBe1 = 1;
> -        }
> -      }
> -    } else {
> -      for (IndexOfPdpEntries = 0; IndexOfPdpEntries <
> NumberOfPdpEntriesNeeded; IndexOfPdpEntries++,
> PageDirectoryPointerEntry++) {
> -        //
> -        // Each Directory Pointer entries points to a page of Page Directory
> entires.
> -        // So allocate space for them and fill them in in the
> IndexOfPageDirectoryEntries loop.
> -        //
> -        PageDirectoryEntry = (VOID *) BigPageAddress;
> -        BigPageAddress += SIZE_4KB;
> +    for ( IndexOfPml4Entries = 0
> +        ; IndexOfPml4Entries < (NumberOfPml5EntriesNeeded == 1 ?
> NumberOfPml4EntriesNeeded : 512)
> +        ; IndexOfPml4Entries++, PageMapLevel4Entry++) {
> +      //
> +      // Each PML4 entry points to a page of Page Directory Pointer entires.
> +      // So lets allocate space for them and fill them in in the
> IndexOfPdpEntries loop.
> +      //
> +      PageDirectoryPointerEntry = (VOID *) BigPageAddress;
> +      BigPageAddress += SIZE_4KB;
> 
> -        //
> -        // Fill in a Page Directory Pointer Entries
> -        //
> -        PageDirectoryPointerEntry->Uint64 =
> (UINT64)(UINTN)PageDirectoryEntry | AddressEncMask;
> -        PageDirectoryPointerEntry->Bits.ReadWrite = 1;
> -        PageDirectoryPointerEntry->Bits.Present = 1;
> +      //
> +      // Make a PML4 Entry
> +      //
> +      PageMapLevel4Entry->Uint64 =
> (UINT64)(UINTN)PageDirectoryPointerEntry | AddressEncMask;
> +      PageMapLevel4Entry->Bits.ReadWrite = 1;
> +      PageMapLevel4Entry->Bits.Present = 1;
> 
> -        for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries <
> 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress +=
> SIZE_2MB) {
> -          if (ToSplitPageTable (PageAddress, SIZE_2MB, StackBase, StackSize)) {
> -            //
> -            // Need to split this 2M page that covers NULL or stack range.
> -            //
> -            Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry,
> StackBase, StackSize);
> +      if (Page1GSupport) {
> +        PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
> +
> +        for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries <
> 512; IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress
> += SIZE_1GB) {
> +          if (ToSplitPageTable (PageAddress, SIZE_1GB, StackBase, StackSize)) {
> +            Split1GPageTo2M (PageAddress, (UINT64 *)
> + PageDirectory1GEntry, StackBase, StackSize);
>            } else {
>              //
>              // Fill in the Page Directory entries
>              //
> -            PageDirectoryEntry->Uint64 = (UINT64)PageAddress |
> AddressEncMask;
> -            PageDirectoryEntry->Bits.ReadWrite = 1;
> -            PageDirectoryEntry->Bits.Present = 1;
> -            PageDirectoryEntry->Bits.MustBe1 = 1;
> +            PageDirectory1GEntry->Uint64 = (UINT64)PageAddress |
> AddressEncMask;
> +            PageDirectory1GEntry->Bits.ReadWrite = 1;
> +            PageDirectory1GEntry->Bits.Present = 1;
> +            PageDirectory1GEntry->Bits.MustBe1 = 1;
>            }
>          }
> -      }
> +      } else {
> +        for ( IndexOfPdpEntries = 0
> +            ; IndexOfPdpEntries < (NumberOfPml4EntriesNeeded == 1 ?
> NumberOfPdpEntriesNeeded : 512)
> +            ; IndexOfPdpEntries++, PageDirectoryPointerEntry++) {
> +          //
> +          // Each Directory Pointer entries points to a page of Page Directory
> entires.
> +          // So allocate space for them and fill them in in the
> IndexOfPageDirectoryEntries loop.
> +          //
> +          PageDirectoryEntry = (VOID *) BigPageAddress;
> +          BigPageAddress += SIZE_4KB;
> 
> -      for (; IndexOfPdpEntries < 512; IndexOfPdpEntries++,
> PageDirectoryPointerEntry++) {
> -        ZeroMem (
> -          PageDirectoryPointerEntry,
> -          sizeof(PAGE_MAP_AND_DIRECTORY_POINTER)
> -          );
> +          //
> +          // Fill in a Page Directory Pointer Entries
> +          //
> +          PageDirectoryPointerEntry->Uint64 =
> (UINT64)(UINTN)PageDirectoryEntry | AddressEncMask;
> +          PageDirectoryPointerEntry->Bits.ReadWrite = 1;
> +          PageDirectoryPointerEntry->Bits.Present = 1;
> +
> +          for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries <
> 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress +=
> SIZE_2MB) {
> +            if (ToSplitPageTable (PageAddress, SIZE_2MB, StackBase, StackSize)) {
> +              //
> +              // Need to split this 2M page that covers NULL or stack range.
> +              //
> +              Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry,
> StackBase, StackSize);
> +            } else {
> +              //
> +              // Fill in the Page Directory entries
> +              //
> +              PageDirectoryEntry->Uint64 = (UINT64)PageAddress |
> AddressEncMask;
> +              PageDirectoryEntry->Bits.ReadWrite = 1;
> +              PageDirectoryEntry->Bits.Present = 1;
> +              PageDirectoryEntry->Bits.MustBe1 = 1;
> +            }
> +          }
> +        }
> +
> +        //
> +        // Fill with null entry for unused PDPTE
> +        //
> +        ZeroMem (PageDirectoryPointerEntry, (512 - IndexOfPdpEntries) *
> + sizeof(PAGE_MAP_AND_DIRECTORY_POINTER));
>        }
>      }
> +
> +    //
> +    // For the PML4 entries we are not using fill in a null entry.
> +    //
> +    ZeroMem (PageMapLevel4Entry, (512 - IndexOfPml4Entries) * sizeof
> + (PAGE_MAP_AND_DIRECTORY_POINTER));
>    }
> 
> -  //
> -  // For the PML4 entries we are not using fill in a null entry.
> -  //
> -  for (; IndexOfPml4Entries < 512; IndexOfPml4Entries++,
> PageMapLevel4Entry++) {
> -    ZeroMem (
> -      PageMapLevel4Entry,
> -      sizeof (PAGE_MAP_AND_DIRECTORY_POINTER)
> -      );
> +  if (Page5LevelSupport) {
> +    Cr4.UintN = AsmReadCr4 ();
> +    Cr4.Bits.LA57 = 1;
> +    AsmWriteCr4 (Cr4.UintN);
> +    //
> +    // For the PML5 entries we are not using fill in a null entry.
> +    //
> +    ZeroMem (PageMapLevel5Entry, (512 - IndexOfPml5Entries) * sizeof
> + (PAGE_MAP_AND_DIRECTORY_POINTER));
>    }
> 
>    //
> --
> 2.21.0.windows.1
> 
> 
>