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...@intel.com>; devel@edk2.groups.io > Cc: Dong, Eric <eric.d...@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...@intel.com> > >> Cc: Dong, Eric <eric.d...@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...@intel.com> > >>> Cc: Eric Dong <eric.d...@intel.com> > >>> Cc: Laszlo Ersek <ler...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-