Re: [PATCH v6 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address

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

On 11/10/20 03:24, Sheng Wei wrote:

> @@ -1114,11 +1145,10 @@ SetPageTableAttributes (
>    BOOLEAN               IsSplitted;
>    BOOLEAN               PageTableSplitted;
>    BOOLEAN               CetEnabled;
> -  IA32_CR4              Cr4;
>    BOOLEAN               Enable5LevelPaging;
> +  UINT64                *PageTableBase = NULL;
>
> -  Cr4.UintN = AsmReadCr4 ();
> -  Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
> +  GetPageTable ((UINTN *)&PageTableBase, &Enable5LevelPaging);
>
>    //
>    // Don't mark page table memory as read-only if
> @@ -1164,7 +1194,7 @@ SetPageTableAttributes (
>      PageTableSplitted = FALSE;
>      L5PageTable = NULL;
>      if (Enable5LevelPaging) {
> -      L5PageTable = (UINT64 *)GetPageTableBase ();
> +      L5PageTable = PageTableBase;
>        SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L5PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted);
>        PageTableSplitted = (PageTableSplitted || IsSplitted);
>      }
> @@ -1176,7 +1206,7 @@ SetPageTableAttributes (
>            continue;
>          }
>        } else {
> -        L4PageTable = (UINT64 *)GetPageTableBase ();
> +        L4PageTable = PageTableBase;
>        }
>        SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L4PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted);
>        PageTableSplitted = (PageTableSplitted || IsSplitted);
>

(14) Are you sure that we never need to re-read the CR3 register inside
the outermost loop?

I would feel safer if you didn't place the GetPageTable() call at the
top of the function (replacing the AsmReadCr4() call). Instead, I
suggest placing GetPageTable() near the top of the outermost loop (the
one that is repeated as long as we split pages).

Here's what I suggest, for "UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c":

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 810985df20ae..52b8eac9cdaf 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -1112,16 +1112,13 @@ SetPageTableAttributes (
>    UINT64                *L4PageTable;
>    UINT64                *L5PageTable;
>    BOOLEAN               IsSplitted;
>    BOOLEAN               PageTableSplitted;
>    BOOLEAN               CetEnabled;
> -  IA32_CR4              Cr4;
> +  UINTN                 PageTableBase;
>    BOOLEAN               Enable5LevelPaging;
>
> -  Cr4.UintN = AsmReadCr4 ();
> -  Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
> -
>    //
>    // 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
> @@ -1161,24 +1158,27 @@ SetPageTableAttributes (
>
>    do {
>      DEBUG ((DEBUG_INFO, "Start...\n"));
>      PageTableSplitted = FALSE;
>      L5PageTable = NULL;
> +
> +    GetPageTableBase (&PageTableBase, &Enable5LevelPaging);
> +
>      if (Enable5LevelPaging) {
> -      L5PageTable = (UINT64 *)GetPageTableBase ();
> -      SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L5PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted);
> +      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 *)GetPageTableBase ();
> +        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++) {

This way, we preserve the pre-patch behavior that the page table base is
re-read *exactly once* per outermost loop iteration.

My proposal matches the IA32 version more closely as well -- in the IA32
version too, GetPageTableBase() is called near the top of the outermost
loop.

It is true that we re-read "Enable5LevelPaging" as well once per
outermost iteration, but (IMO) that's fine. It's a consequence of
binding "PageTableBase" and "Enable5LevelPaging" together (which is a
good thing), and re-reading "Enable5LevelPaging" once per outermost
iteration is not costly.

Thanks
Laszlo