Re: [PATCH v1 1/3] UefiCpuPkg/SecCore: Migrate page table to permanent memory

["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 1/3] 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.
> 
> Change-Id: I29bdb078ef567ed9455d1328cb007f4f60a617a2

1. please remove Change-Id.

> +
> +  //
> +  // Check CPU runs in 64bit mode or 32bit mode
> +  //
> +  if (sizeof (UINTN) == sizeof (UINT32)) {
> +    DEBUG ((DEBUG_WARN, "MigratePageTable: CPU runs in 32bit mode,
> unsupport to migrate page table to permanent memory.\n"));

2. I am ok to assume that this function should not be called when CPU runs in 32bit mode because we
assume 32bit cpu doesn't enable paging in PEI at this moment.
Can you just add assert such as ASSERT (sizeof (UINTN) == sizeof (UINT64))?
The if check and debug messages can be removed.


> +  //
> +  // 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);
> +    if ((RegEdx & BIT26) != 0) {

3. can you use the bitfield in above if-check instead of checking BIT26?

> +
> +  //
> +  // Decide Paging Mode according Page5LevelSupport & Page1GSupport.
> +  //
> +  PagingMode = Paging5Level1GB;
> +  if (Page5LevelSupport && !Page1GSupport) {
> +    PagingMode = Paging5Level;
> +  } else if (!Page5LevelSupport && Page1GSupport) {
> +    PagingMode = Paging4Level1GB;
> +  } else if (!Page5LevelSupport && !Page1GSupport) {
> +    PagingMode = Paging4Level;
> +  }

4. how about?
   if (Page5LevelSupport) {
      PagingMode = Page1GSupport? Paging5Level1GB: Paging5Level;
    } else {
      PagingMode = Page1GSupport? Paging4Level1GB: Paging4Level;
   }


> +
> +  DEBUG ((DEBUG_INFO, "MigratePageTable: PagingMode = 0x%lx\n", (UINTN)
> PagingMode));

5. There are lots of DEBUG() macro call in this patch. Can you think about how to merge them?
Basically if code between two DEBUG() calls is impossible to cause hang, you can combine the 1st
DEBUG() call into the 2nd.

> +
> +  //
> +  // Get Maximum Physical Address Bits
> +  // Get the number of address lines; Maximum Physical Address is
> 2^PhysicalAddressBits - 1.
> +  // If CPUID does not supported, then use a max value of 36 as per SDM 3A,
> 4.1.4.
> +  //
> +  AsmCpuid (CPUID_EXTENDED_FUNCTION, &MaxExtendedFunctionId, NULL,
> NULL, NULL);
> +  if (MaxExtendedFunctionId >= CPUID_VIR_PHY_ADDRESS_SIZE) {
> +    AsmCpuid (CPUID_VIR_PHY_ADDRESS_SIZE, &VirPhyAddressSize.Uint32,
> NULL, NULL, NULL);
> +  } else {
> +    VirPhyAddressSize.Bits.PhysicalAddressBits = 36;
> +  }
> +
> +  if (PagingMode == Paging4Level1GB || PagingMode == Paging4Level) {
> +    //
> +    // The max lineaddress bits is 48 for 4 level page table.
> +    //
> +    VirPhyAddressSize.Bits.PhysicalAddressBits = MIN
> (VirPhyAddressSize.Bits.PhysicalAddressBits, 48);
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "MigratePageTable: Maximum Physical Address Bits
> = %d\n", VirPhyAddressSize.Bits.PhysicalAddressBits));
> +
> +  //
> +  // Get required buffer size for the pagetable that will be created.
> +  //
> +  Status = PageTableMap (&PageTable, PagingMode, 0, &BufferSize, 0,
> LShiftU64 (1, VirPhyAddressSize.Bits.PhysicalAddressBits), &MapAttribute,
> &MapMask, NULL);
> +  ASSERT (Status == EFI_BUFFER_TOO_SMALL);
> +  if (Status != EFI_BUFFER_TOO_SMALL) {
> +    DEBUG ((DEBUG_ERROR, "MigratePageTable: Failed to get PageTable
> required BufferSize, Status = %r\n", Status));

6. I think ASSERT() is enough. No need for a debug message.

> +    return Status;
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "MigratePageTable: Get PageTable required
> BufferSize = %x\n", BufferSize));
> +
> +  //
> +  // Allocate required Buffer.
> +  //
> +  Status = PeiServicesAllocatePages (
> +             EfiBootServicesData,
> +             EFI_SIZE_TO_PAGES (BufferSize),
> +             &((EFI_PHYSICAL_ADDRESS) Buffer)
> +             );
> +  ASSERT (Buffer != NULL);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "MigratePageTable: Failed to allocate PageTable
> required BufferSize, Status = %r\n", Status));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  //
> +  // Create PageTable in permanent memory.
> +  //
> +  Status = PageTableMap (&PageTable, PagingMode, Buffer, &BufferSize, 0,
> LShiftU64 (1, VirPhyAddressSize.Bits.PhysicalAddressBits), &MapAttribute,
> &MapMask, NULL);
> +  ASSERT (!EFI_ERROR (Status) && PageTable != 0);
> +  if (EFI_ERROR (Status) || PageTable == 0) {
> +    DEBUG ((DEBUG_ERROR, "MigratePageTable: Failed to create PageTable,
> Status = %r, PageTable = 0x%lx\n", Status, PageTable));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "MigratePageTable: Create PageTable = 0x%lx\n",
> PageTable));
> +
> +  //
> +  // Write the Pagetable to CR3.
> +  //
> +  AsmWriteCr3 (PageTable);
> +
> +  DEBUG ((DEBUG_INFO, "MigratePageTable: Write the Pagetable to CR3
> Sucessfully.\n"));
> +
> +  return Status;
> +}
> +
>  //
>  // These are IDT entries pointing to 10:FFFFFFE4h.
>  //
>  UINT64  mIdtEntryTemplate = 0xffff8e000010ffe4ULL;
> 
> @@ -492,10 +641,25 @@ SecTemporaryRamDone (
>    if (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)) {
>      Status = MigrateGdt ();
>      ASSERT_EFI_ERROR (Status);
>    }
> 
> +  //
> +  // Migrate page table to permanent memory mapping entire physical address
> space if CR0.PG is set.
> +  //
> +  if ((AsmReadCr0 () & BIT31) != 0) {

7. Can you use IA32_CR0 structure in if-check?

8. can you please add ASSERT (sizeof (UINTN) == sizeof (UINT32)) before calling MigratePageTable()?

> +    //
> +    // CR4.PAE must be enabled.
> +    //
> +    ASSERT ((AsmReadCr4 () & BIT5) != 0);
> +    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);
> +    }
> +  }
> +
>    //
>    // Disable Temporary RAM after Stack and Heap have been migrated at this
> point.
>    //
>    SecPlatformDisableTemporaryMemory ();
> 
> diff --git a/UefiCpuPkg/SecCore/SecMain.h b/UefiCpuPkg/SecCore/SecMain.h
> index 880e6cd1b8..b50d96e45b 100644
> --- a/UefiCpuPkg/SecCore/SecMain.h
> +++ b/UefiCpuPkg/SecCore/SecMain.h
> @@ -17,10 +17,11 @@
>  #include <Ppi/PeiCoreFvLocation.h>
>  #include <Ppi/RepublishSecPpi.h>
> 
>  #include <Guid/FirmwarePerformance.h>
> 
> +#include <Library/BaseLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/PcdLib.h>
>  #include <Library/BaseMemoryLib.h>
>  #include <Library/PlatformSecLib.h>
>  #include <Library/CpuLib.h>
> @@ -30,10 +31,13 @@
>  #include <Library/CpuExceptionHandlerLib.h>
>  #include <Library/ReportStatusCodeLib.h>
>  #include <Library/PeiServicesTablePointerLib.h>
>  #include <Library/HobLib.h>
>  #include <Library/PeiServicesLib.h>
> +#include <Library/CpuPageTableLib.h>
> +#include <Register/Intel/Cpuid.h>
> +#include <Register/Intel/Msr.h>
> 
>  #define SEC_IDT_ENTRY_COUNT  34
> 
>  typedef struct _SEC_IDT_TABLE {
>    //
> --
> 2.16.2.windows.1