Re: [edk2-devel] [PATCH v2 2/5] UefiCpuPkg/CpuMpPei: Enable PAE page table if CR0.PG is not set

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

1. can you please change the commit title as "UefiCpuPkg/CpuMpPei: Conditionally enable PAE paging in 32bit mode"?

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Jiaxin
> Sent: Friday, May 12, 2023 12:16 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: [edk2-devel] [PATCH v2 2/5] 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.

2. No need to emphasize paging is not enabled during temp-ram to permanent-ram migration.
We can just say "Some security features depend on the page table enabling. So, This patch
is to enable paging if it is not enabled (32bit mode)"

3. following commit message might not be needed.

> 
> 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.
> 

> -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;

4. No need of local variable PagingMode.

> +  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.

5. Let's say "1:1 map 4GB in 32bit mode." Because people might say "PAE can support up to 2^36 address, why 2^32 here".

> +  // Create PageTable in permanent memory.
> +  // The Max size of LinearAddress for PAE is 2^32.

6. above comments seem to be redundant.

>    EFI_PEI_HOB_POINTERS    Hob;
> +  IA32_CR0                Cr0;
> 
>    //
>    // 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 +575,29 @@ 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.

7. Maybe simply say " Some security features depend on the page table enabling. So, here
is to enable paging if it is not enabled (only in 32bit mode)"

> +  //
> +  Cr0.UintN = AsmReadCr0 ();
> +  if ((Cr0.Bits.PG == 0) && (InitStackGuard || (Hob.Raw != NULL))) {
> +    ASSERT (sizeof (UINTN) == sizeof (UINT32));
> +
> +    Status = EnablePaePageTable ();
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_WARN, "MemoryDiscoveredPpiNotifyCallback: Failed to
> enable PAE page table: %r.\n", Status));
> +      ASSERT_EFI_ERROR (Status);

8. Shall we use "CpuDeadLoop()" if PAE paging enabling is failed?