Re: [PATCH v1 2/3] UefiCpuPkg/CpuMpPei: Enable PAE page table if CR0.PG is not set

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

> -----Original Message-----
> From: Wu, Jiaxin <jiaxin.wu@intel.com>
> Sent: Tuesday, May 9, 2023 6:23 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar, Rahul R
> <rahul.r.kumar@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.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zeng Star <star.zeng@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@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