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

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

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