[edk2-devel] [PATCH 4/4] MdeModulePkg/DxeIpl: Create 5-level page table for long mode
Michael D Kinney
michael.d.kinney at intel.com
Tue Jul 23 23:54:02 UTC 2019
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 at edk2.groups.io
> [mailto:devel at edk2.groups.io] On Behalf Of Laszlo Ersek
> Sent: Tuesday, July 23, 2019 12:20 PM
> To: Ni, Ray <ray.ni at intel.com>; devel at edk2.groups.io
> Cc: Dong, Eric <eric.dong at 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 at edk2.groups.io <devel at edk2.groups.io> On
> Behalf Of Laszlo
> >> Ersek
> >> Sent: Tuesday, July 23, 2019 5:46 PM
> >> To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>
> >> Cc: Dong, Eric <eric.dong at 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 at intel.com>
> >>> Cc: Eric Dong <eric.dong at intel.com>
> >>> Cc: Laszlo Ersek <lersek at 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
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#44272): https://edk2.groups.io/g/devel/message/44272
Mute This Topic: https://groups.io/mt/32556535/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list