Re: [PATCH v2 1/5] UefiCpuPkg/SecCore: Migrate page table to permanent memory

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

> -----Original Message-----
> From: Wu, Jiaxin <jiaxin.wu@intel.com>
> 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: [PATCH v2 1/5] UefiCpuPkg/SecCore: Migrate page table to permanent
> memory
> 
> Background:
> For arch X64, system will enable the page table in SPI to cover 0-512G range
> via CR4.PAE & MSR.LME & CR0.PG & CR3 setting (see ResetVector code).
> Existing
> code doesn't cover the higher address access above 512G before memory-
> discovered
> callback. That will be potential problem if system access the higher address
> after the transition from temporary RAM to permanent MEM RAM.
> 
> Solution:
> This patch is to migrate page table to permanent memory to map entire physical
> address space if CR0.PG is set during temporary RAM Done.
> 
> 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/SecCore/SecCore.inf       |   1 +
>  UefiCpuPkg/SecCore/SecCoreNative.inf |   1 +
>  UefiCpuPkg/SecCore/SecMain.c         | 152
> +++++++++++++++++++++++++++++++++++
>  UefiCpuPkg/SecCore/SecMain.h         |   4 +
>  4 files changed, 158 insertions(+)
> 
> diff --git a/UefiCpuPkg/SecCore/SecCore.inf b/UefiCpuPkg/SecCore/SecCore.inf
> index 3758aded3b..cab69b8b97 100644
> --- a/UefiCpuPkg/SecCore/SecCore.inf
> +++ b/UefiCpuPkg/SecCore/SecCore.inf
> @@ -53,10 +53,11 @@
>    CpuExceptionHandlerLib
>    ReportStatusCodeLib
>    PeiServicesLib
>    PeiServicesTablePointerLib
>    HobLib
> +  CpuPageTableLib
> 
>  [Ppis]
>    ## SOMETIMES_CONSUMES
>    ## PRODUCES
>    gEfiSecPlatformInformationPpiGuid
> diff --git a/UefiCpuPkg/SecCore/SecCoreNative.inf
> b/UefiCpuPkg/SecCore/SecCoreNative.inf
> index 1ee6ff7d88..fa241cca94 100644
> --- a/UefiCpuPkg/SecCore/SecCoreNative.inf
> +++ b/UefiCpuPkg/SecCore/SecCoreNative.inf
> @@ -50,10 +50,11 @@
>    CpuExceptionHandlerLib
>    ReportStatusCodeLib
>    PeiServicesLib
>    PeiServicesTablePointerLib
>    HobLib
> +  CpuPageTableLib
> 
>  [Ppis]
>    ## SOMETIMES_CONSUMES
>    ## PRODUCES
>    gEfiSecPlatformInformationPpiGuid
> diff --git a/UefiCpuPkg/SecCore/SecMain.c b/UefiCpuPkg/SecCore/SecMain.c
> index 95375850ec..8ec0b654fb 100644
> --- a/UefiCpuPkg/SecCore/SecMain.c
> +++ b/UefiCpuPkg/SecCore/SecMain.c
> @@ -70,10 +70,139 @@ MigrateGdt (
>    AsmWriteGdtr (&Gdtr);
> 
>    return EFI_SUCCESS;
>  }
> 
> +/**
> +  Migrate page table to permanent memory mapping entire physical address
> space.
> +
> +  @retval   EFI_SUCCESS           The PageTable was migrated successfully.
> +  @retval   EFI_UNSUPPORTED       Unsupport to migrate page table to
> permanent memory if IA-32e Mode not actived.
> +  @retval   EFI_OUT_OF_RESOURCES  The PageTable could not be migrated due
> to lack of available memory.
> +
> +**/
> +EFI_STATUS
> +MigratePageTable (
> +  VOID
> +  )
> +{
> +  EFI_STATUS                      Status;
> +  IA32_CR4                        Cr4;
> +  BOOLEAN                         Page5LevelSupport;
> +  UINT32                          RegEax;
> +  CPUID_EXTENDED_CPU_SIG_EDX      RegEdx;
> +  BOOLEAN                         Page1GSupport;
> +  PAGING_MODE                     PagingMode;
> +  CPUID_VIR_PHY_ADDRESS_SIZE_EAX  VirPhyAddressSize;
> +  UINT32                          MaxExtendedFunctionId;
> +  UINTN                           PageTable;
> +  EFI_PHYSICAL_ADDRESS            Buffer;
> +  UINTN                           BufferSize;
> +  IA32_MAP_ATTRIBUTE              MapAttribute;
> +  IA32_MAP_ATTRIBUTE              MapMask;
> +
> +  VirPhyAddressSize.Uint32    = 0;
> +  PageTable                   = 0;
> +  BufferSize                  = 0;
> +  MapAttribute.Uint64         = 0;
> +  MapMask.Uint64              = MAX_UINT64;
> +  MapAttribute.Bits.Present   = 1;
> +  MapAttribute.Bits.ReadWrite = 1;
> +
> +  //
> +  // Check Page5Level Support or not.
> +  //
> +  Cr4.UintN         = AsmReadCr4 ();
> +  Page5LevelSupport = (Cr4.Bits.LA57 ? TRUE : FALSE);

1. "Cr4.Bits.LA57 != 0", to be consistent with other code in your patch.

> +
> +  //
> +  // Check Page1G Support or not.
> +  //
> +  Page1GSupport = FALSE;
> +  AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
> +  if (RegEax >= CPUID_EXTENDED_CPU_SIG) {
> +    AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL,
> &(RegEdx.Uint32));

2. &RegEdx.Uint32. No need for the bracket.

> +    if ((RegEdx.Bits.Page1GB) != 0) {
3. No need for the bracket.

> +  //
> +  Cr0.UintN = AsmReadCr0 ();
> +  if (Cr0.Bits.PG != 0) {
> +    //
> +    // CR4.PAE must be enabled.
> +    //
> +    ASSERT ((AsmReadCr4 () & BIT5) != 0);

4. No need to check PAE bit because 64bit long mode should set PAE bit.

> +
> +    //
> +    // Assume CPU runs in 64bit mode if paging is enabled.
> +    //
> +    ASSERT (sizeof (UINTN) == sizeof (UINT64));
> +
> +    Status = MigratePageTable ();
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_WARN, "SecTemporaryRamDone: Failed to migrate page
> table to permanent memory: %r.\n", Status));
> +      ASSERT_EFI_ERROR (Status);

5. Can you add CpuDeadLoop ()? Failure of page table migration is unacceptable.