> -----Original Message-----
> From: Wu, Jiaxin <jiaxin...@intel.com>
> Sent: Tuesday, May 9, 2023 6:23 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Zeng, Star
> <star.z...@intel.com>; Gerd Hoffmann <kra...@redhat.com>; Kumar, Rahul R
> <rahul.r.ku...@intel.com>
> Subject: [PATCH v1 2/3] UefiCpuPkg/CpuMpPei: Enable PAE page table if CR0.PG
> is not set
>
> Some security features depends on the page table enabling. So, This patch
> is to enable the page table if page table has not been enabled during the
> transition from Temporary RAM to Permanent RAM.
>
> Note: If page table is not enabled before this point, which means the system
> IA-32e Mode is not activated. Because on Intel 64 processors, IA-32e Mode
> operation requires physical address extensions with 4 or 5 levels of enhanced
> paging structures (see Section 4.5, "4 - Level Paging and 5 -Level Paging"
> and Section 9.8, "Initializing IA-32e Mode"). So, just enable PAE page table
> if CR0.PG is not set.
>
> Change-Id: Ibfbfdace1fe7e29ab94463629d8d2f539a43f1b9
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Zeng Star <star.z...@intel.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> Cc: Rahul Kumar <rahul1.ku...@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin...@intel.com>
> ---
> UefiCpuPkg/CpuMpPei/CpuMpPei.h | 1 +
> UefiCpuPkg/CpuMpPei/CpuMpPei.inf | 1 +
> UefiCpuPkg/CpuMpPei/CpuPaging.c | 228 +++++++++++++++++-------------------
> ---
> 3 files changed, 102 insertions(+), 128 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.h
> b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
> index 0649c48d14..1b9a94e18f 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.h
> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.h
> @@ -26,10 +26,11 @@
> #include <Library/ReportStatusCodeLib.h>
> #include <Library/CpuExceptionHandlerLib.h>
> #include <Library/MpInitLib.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/MemoryAllocationLib.h>
> +#include <Library/CpuPageTableLib.h>
>
> extern EFI_PEI_PPI_DESCRIPTOR mPeiCpuMpPpiDesc;
>
> /**
> This service retrieves the number of logical processor in the platform
> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
> b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
> index 7444bdb968..865be5627e 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
> @@ -44,10 +44,11 @@
> CpuExceptionHandlerLib
> MpInitLib
> BaseMemoryLib
> CpuLib
> MemoryAllocationLib
> + CpuPageTableLib
>
> [Guids]
> gEdkiiMigratedFvInfoGuid ##
> SOMETIMES_CONSUMES
> ## HOB
>
> [Ppis]
> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> index a471f089c8..6c113051fe 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
> @@ -115,42 +115,10 @@ AllocatePageTableMemory (
> }
>
> return Address;
> }
>
> -/**
> - Get the address width supported by current processor.
> -
> - @retval 32 If processor is in 32-bit mode.
> - @retval 36-48 If processor is in 64-bit mode.
> -
> -**/
> -UINTN
> -GetPhysicalAddressWidth (
> - VOID
> - )
> -{
> - UINT32 RegEax;
> -
> - if (sizeof (UINTN) == 4) {
> - return 32;
> - }
> -
> - AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
> - if (RegEax >= CPUID_VIR_PHY_ADDRESS_SIZE) {
> - AsmCpuid (CPUID_VIR_PHY_ADDRESS_SIZE, &RegEax, NULL, NULL, NULL);
> - RegEax &= 0xFF;
> - if (RegEax > 48) {
> - return 48;
> - }
> -
> - return (UINTN)RegEax;
> - }
> -
> - return 36;
> -}
> -
> /**
> Get the type of top level page table.
>
> @retval Page512G PML4 paging.
> @retval Page1G PAE paging.
> @@ -381,120 +349,93 @@ ConvertMemoryPageAttributes (
>
> return RETURN_SUCCESS;
> }
>
> /**
> - Get maximum size of page memory supported by current processor.
> -
> - @param[in] TopLevelType The type of top level page entry.
> + Enable PAE Page Table.
>
> - @retval Page1G If processor supports 1G page and PML4.
> - @retval Page2M For all other situations.
> + @retval EFI_SUCCESS The PAE Page Table was enabled
> successfully.
> + @retval EFI_OUT_OF_RESOURCES The PAE Page Table could not be enabled
> due to lack of available memory.
>
> **/
> -PAGE_ATTRIBUTE
> -GetMaxMemoryPage (
> - IN PAGE_ATTRIBUTE TopLevelType
> - )
> -{
> - UINT32 RegEax;
> - UINT32 RegEdx;
> -
> - if (TopLevelType == Page512G) {
> - AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
> - if (RegEax >= CPUID_EXTENDED_CPU_SIG) {
> - AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, &RegEdx);
> - if ((RegEdx & BIT26) != 0) {
> - return Page1G;
> - }
> - }
> - }
> -
> - return Page2M;
> -}
> -
> -/**
> - Create PML4 or PAE page table.
> -
> - @return The address of page table.
> -
> -**/
> -UINTN
> -CreatePageTable (
> +EFI_STATUS
> +EnablePaePageTable (
> VOID
> )
> {
> - RETURN_STATUS Status;
> - UINTN PhysicalAddressBits;
> - UINTN NumberOfEntries;
> - PAGE_ATTRIBUTE TopLevelPageAttr;
> - UINTN PageTable;
> - PAGE_ATTRIBUTE MaxMemoryPage;
> - UINTN Index;
> - UINT64 AddressEncMask;
> - UINT64 *PageEntry;
> - EFI_PHYSICAL_ADDRESS PhysicalAddress;
> -
> - TopLevelPageAttr = (PAGE_ATTRIBUTE)GetPageTableTopLevelType ();
> - PhysicalAddressBits = GetPhysicalAddressWidth ();
> - NumberOfEntries = (UINTN)1 << (PhysicalAddressBits -
> -
> mPageAttributeTable[TopLevelPageAttr].AddressBitOffset);
> + EFI_STATUS Status;
> + PAGING_MODE PagingMode;
> +
> + UINTN PageTable;
> + VOID *Buffer;
> + UINTN BufferSize;
> + IA32_MAP_ATTRIBUTE MapAttribute;
> + IA32_MAP_ATTRIBUTE MapMask;
> +
> + PagingMode = PagingPae;
> + PageTable = 0;
> + Buffer = NULL;
> + BufferSize = 0;
> + MapAttribute.Uint64 = 0;
> + MapMask.Uint64 = MAX_UINT64;
> + MapAttribute.Bits.Present = 1;
> + MapAttribute.Bits.ReadWrite = 1;
>
> - PageTable = (UINTN)AllocatePageTableMemory (1);
> - if (PageTable == 0) {
> - return 0;
> + //
> + // Get required buffer size for the pagetable that will be created.
> + // The Max size of LinearAddress for PAE is 2^32.
> + //
> + Status = PageTableMap (&PageTable, PagingMode, 0, &BufferSize, 0,
> LShiftU64 (1, 32), &MapAttribute, &MapMask, NULL);
1. how about directly use SIZE_4GB macro in above?
> + ASSERT (Status == EFI_BUFFER_TOO_SMALL);
> + if (Status != EFI_BUFFER_TOO_SMALL) {
> + DEBUG ((DEBUG_ERROR, "EnablePaePageTable: Failed to get PageTable
> required BufferSize, Status = %r\n", Status));
2. no need for debug message.
> + return Status;
> }
>
> - AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask);
> - AddressEncMask &= mPageAttributeTable[TopLevelPageAttr].AddressMask;
3. Good to remove the address encryption mask for 32bit env.
> - MaxMemoryPage = GetMaxMemoryPage (TopLevelPageAttr);
> - PageEntry = (UINT64 *)PageTable;
> + DEBUG ((DEBUG_INFO, "EnablePaePageTable: Get PageTable required
> BufferSize = %x\n", BufferSize));
4. Can you remove this debug log?
> +
> + //
> + // Allocate required Buffer.
> + //
> + Buffer = AllocatePageTableMemory (EFI_SIZE_TO_PAGES (BufferSize));
> + ASSERT (Buffer != NULL);
> + if (Buffer == NULL) {
> + DEBUG ((DEBUG_ERROR, "EnablePaePageTable: Failed to allocate PageTable
> required BufferSize!\n"));
5. can you please dump the failure message but also the BufferSize value?
> + return EFI_OUT_OF_RESOURCES;
> + }
>
> - PhysicalAddress = 0;
> - for (Index = 0; Index < NumberOfEntries; ++Index) {
> - *PageEntry = PhysicalAddress | AddressEncMask | PAGE_ATTRIBUTE_BITS;
> + //
> + // Create PageTable in permanent memory.
> + // The Max size of LinearAddress for PAE is 2^32.
> + //
> + Status = PageTableMap (&PageTable, PagingMode, Buffer, &BufferSize, 0,
> LShiftU64 (1, 32), &MapAttribute, &MapMask, NULL);
> + ASSERT (!EFI_ERROR (Status) && PageTable != 0);
6. ASSERT_EFI_ERROR (Status) is enough.
> + if (EFI_ERROR (Status) || PageTable == 0) {
> + DEBUG ((DEBUG_ERROR, "EnablePaePageTable: Failed to create PageTable,
> Status = %r, PageTable = 0x%lx\n", Status, PageTable));
7. if error happens, no need to dump PageTable value.
> + return EFI_OUT_OF_RESOURCES;
> + }
>
> - //
> - // Split the top page table down to the maximum page size supported
> - //
> - if (MaxMemoryPage < TopLevelPageAttr) {
> - Status = SplitPage (PageEntry, TopLevelPageAttr, MaxMemoryPage, TRUE);
> - ASSERT_EFI_ERROR (Status);
> - }
> + DEBUG ((DEBUG_INFO, "EnablePaePageTable: Create PageTable = 0x%x\n",
> PageTable));
>
> - if (TopLevelPageAttr == Page1G) {
> - //
> - // PDPTE[2:1] (PAE Paging) must be 0. SplitPage() might change them to
> 1.
> - //
> - *PageEntry &= ~(UINT64)(IA32_PG_RW | IA32_PG_U);
> - }
> + //
> + // Write the Pagetable to CR3.
> + //
> + AsmWriteCr3 (PageTable);
>
> - PageEntry += 1;
> - PhysicalAddress += mPageAttributeTable[TopLevelPageAttr].Length;
> - }
> + //
> + // Enable CR4.PAE
> + //
> + AsmWriteCr4 (AsmReadCr4 () | BIT5);
>
> - return PageTable;
> -}
> + //
> + // Enable CR0.PG
> + //
> + AsmWriteCr0 (AsmReadCr0 () | BIT31);
>
> -/**
> - Setup page tables and make them work.
> + DEBUG ((DEBUG_INFO, "EnablePaePageTable: Enabled PAE PageTable
> Sucessfully.\n"));
>
> -**/
> -VOID
> -EnablePaging (
> - VOID
> - )
> -{
> - UINTN PageTable;
> -
> - PageTable = CreatePageTable ();
> - ASSERT (PageTable != 0);
> - if (PageTable != 0) {
> - AsmWriteCr3 (PageTable);
> - AsmWriteCr4 (AsmReadCr4 () | BIT5); // CR4.PAE
> - AsmWriteCr0 (AsmReadCr0 () | BIT31); // CR0.PG
> - }
> + return Status;
> }
>
> /**
> Get the base address of current AP's stack.
>
> @@ -622,10 +563,11 @@ MemoryDiscoveredPpiNotifyCallback (
> {
> EFI_STATUS Status;
> BOOLEAN InitStackGuard;
> EDKII_MIGRATED_FV_INFO *MigratedFvInfo;
> EFI_PEI_HOB_POINTERS Hob;
> + MSR_IA32_EFER_REGISTER MsrEfer;
>
> //
> // Paging must be setup first. Otherwise the exception TSS setup during MP
> // initialization later will not contain paging information and then fail
> // the task switch (for the sake of stack switch).
> @@ -635,12 +577,42 @@ MemoryDiscoveredPpiNotifyCallback (
> if (IsIa32PaeSupported ()) {
> Hob.Raw = GetFirstGuidHob (&gEdkiiMigratedFvInfoGuid);
> InitStackGuard = PcdGetBool (PcdCpuStackGuard);
> }
>
> - if (InitStackGuard || (Hob.Raw != NULL)) {
> - EnablePaging ();
> + //
> + // Some security features depends on the page table enabling.So, here
> + // is to enable the page table if page table has not been enabled yet.
> + // If page table is not enabled before this point, which means the system
> + // IA-32e Mode is not activated.Because on Intel 64 processors, IA-32e Mode
> + // operation requires physical address extensions with 4 or 5 levels of
> + // enhanced paging structures (see Section 4.5, "4 - Level Paging and 5 -
> + // Level Paging" and Section 9.8, "Initializing IA-32e Mode"). So, just
> + // enable PAE page table if CR0.PG is not set.
> + //
> + if (((AsmReadCr0 () & BIT31) == 0) && (InitStackGuard || (Hob.Raw !=
> NULL))) {
8. Can you use IA32_CR0 structure in if-check?
> + //
> + // Check CPU runs in 32bit mode.
> + //
> + MsrEfer.Uint64 = AsmReadMsr64 (MSR_CORE_IA32_EFER);
> + if (MsrEfer.Bits.LMA == 1) {
> + //
> + // On Intel 64 processors, IA-32e Mode operation requires physical -
> address extensions with
> + // 4 or 5 levels of enhanced paging structures (see Section 4.5, "4 -
> Level
> Paging and
> + // 5 - Level Paging" and Section 9.8, "Initializing IA-32e Mode"). So,
> it must
> something wrong
> + // if MsrEfer.Bits.LMA == 1 with no page table enbaled before.
> + //
> + DEBUG ((DEBUG_ERROR, "MemoryDiscoveredPpiNotifyCallback: No page
> table with IA-32e Mode actived!\n"));
> + ASSERT (MsrEfer.Bits.LMA == 0);
> + return EFI_DEVICE_ERROR;
> + }
9. I don't think we need to check if CPU runs in 32bit mode. We should have
confidence that
when paging is disabled, CPU should run in 32bit mode.
I agree adding ASSERT (sizeof (UINTN) == sizeof (UINT32)) can improve the
readability.
> +
> + Status = EnablePaePageTable ();
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_WARN, "MemoryDiscoveredPpiNotifyCallback: Failed to
> enable PAE page table: %r.\n", Status));
> + ASSERT_EFI_ERROR (Status);
> + }
> }
>
> Status = InitializeCpuMpWorker ((CONST EFI_PEI_SERVICES **)PeiServices);
> ASSERT_EFI_ERROR (Status);
>
> --
> 2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104507): https://edk2.groups.io/g/devel/message/104507
Mute This Topic: https://groups.io/mt/98780501/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
