Re: [edk2-devel] [PATCH v7 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address

["Laszlo Ersek" <lersek@redhat.com>]

On 11/13/20 03:50, Sheng Wei wrote:

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 810985df20..1c6075e332 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -1111,14 +1140,13 @@ SetPageTableAttributes (
>    UINT64                *L3PageTable;
>    UINT64                *L4PageTable;
>    UINT64                *L5PageTable;
> +  UINTN                 PageTableBase;
>    BOOLEAN               IsSplitted;
>    BOOLEAN               PageTableSplitted;
>    BOOLEAN               CetEnabled;
> -  IA32_CR4              Cr4;
>    BOOLEAN               Enable5LevelPaging;
>
> -  Cr4.UintN = AsmReadCr4 ();
> -  Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
> +  GetPageTable (&PageTableBase, &Enable5LevelPaging);
>
>    //
>    // Don't mark page table memory as read-only if

(1) You didn't address point (14) from my v6 review:

  https://edk2.groups.io/g/devel/message/67251
  https://www.redhat.com/archives/edk2-devel-archive/2020-November/msg00410.html

To summarize it here: in v8, please move the GetPageTable() call into
the outermost loop, as follows:

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 1c6075e3326a..cdc1fcefc524 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -1127,89 +1127,90 @@ SmiPFHandler (
>    This function sets memory attribute for page table.
>  **/
>  VOID
>  SetPageTableAttributes (
>    VOID
>    )
>  {
>    UINTN                 Index2;
>    UINTN                 Index3;
>    UINTN                 Index4;
>    UINTN                 Index5;
>    UINT64                *L1PageTable;
>    UINT64                *L2PageTable;
>    UINT64                *L3PageTable;
>    UINT64                *L4PageTable;
>    UINT64                *L5PageTable;
>    UINTN                 PageTableBase;
>    BOOLEAN               IsSplitted;
>    BOOLEAN               PageTableSplitted;
>    BOOLEAN               CetEnabled;
>    BOOLEAN               Enable5LevelPaging;
>
> -  GetPageTable (&PageTableBase, &Enable5LevelPaging);
> -
>    //
>    // Don't mark page table memory as read-only if
>    //  - no restriction on access to non-SMRAM memory; or
>    //  - SMM heap guard feature enabled; or
>    //      BIT2: SMM page guard enabled
>    //      BIT3: SMM pool guard enabled
>    //  - SMM profile feature enabled
>    //
>    if (!mCpuSmmRestrictedMemoryAccess ||
>        ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT3 | BIT2)) != 0) ||
>        FeaturePcdGet (PcdCpuSmmProfileEnable)) {
>      //
>      // Restriction on access to non-SMRAM memory and heap guard could not be enabled at the same time.
>      //
>      ASSERT (!(mCpuSmmRestrictedMemoryAccess &&
>                (PcdGet8 (PcdHeapGuardPropertyMask) & (BIT3 | BIT2)) != 0));
>
>      //
>      // Restriction on access to non-SMRAM memory and SMM profile could not be enabled at the same time.
>      //
>      ASSERT (!(mCpuSmmRestrictedMemoryAccess && FeaturePcdGet (PcdCpuSmmProfileEnable)));
>      return ;
>    }
>
>    DEBUG ((DEBUG_INFO, "SetPageTableAttributes\n"));
>
>    //
>    // Disable write protection, because we need mark page table to be write protected.
>    // We need *write* page table memory, to mark itself to be *read only*.
>    //
>    CetEnabled = ((AsmReadCr4() & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
>    if (CetEnabled) {
>      //
>      // CET must be disabled if WP is disabled.
>      //
>      DisableCet();
>    }
>    AsmWriteCr0 (AsmReadCr0() & ~CR0_WP);
>
>    do {
>      DEBUG ((DEBUG_INFO, "Start...\n"));
>      PageTableSplitted = FALSE;
>      L5PageTable = NULL;
> +
> +    GetPageTable (&PageTableBase, &Enable5LevelPaging);
> +
>      if (Enable5LevelPaging) {
>        L5PageTable = (UINT64 *)PageTableBase;
>        SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)PageTableBase, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted);
>        PageTableSplitted = (PageTableSplitted || IsSplitted);
>      }
>
>      for (Index5 = 0; Index5 < (Enable5LevelPaging ? SIZE_4KB/sizeof(UINT64) : 1); Index5++) {
>        if (Enable5LevelPaging) {
>          L4PageTable = (UINT64 *)(UINTN)(L5PageTable[Index5] & ~mAddressEncMask & PAGING_4K_ADDRESS_MASK_64);
>          if (L4PageTable == NULL) {
>            continue;
>          }
>        } else {
>          L4PageTable = (UINT64 *)PageTableBase;
>        }
>        SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L4PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted);
>        PageTableSplitted = (PageTableSplitted || IsSplitted);
>
>        for (Index4 = 0; Index4 < SIZE_4KB/sizeof(UINT64); Index4++) {
>          L3PageTable = (UINT64 *)(UINTN)(L4PageTable[Index4] & ~mAddressEncMask & PAGING_4K_ADDRESS_MASK_64);
>          if (L3PageTable == NULL) {
>            continue;

This incremental patch should be squashed into your v8 2/2.

Then I'll give my R-b.

Thanks,
Laszlo