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; ler...@redhat.com; Ni, Ray <ray...@intel.com>; > Kinney, Michael D <michael.d.kin...@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 > > 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 (#44276): https://edk2.groups.io/g/devel/message/44276 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] -=-=-=-=-=-=-=-=-=-=-=-
