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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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
> 
>