Re: [PATCH 3/3] UefiCpuPkg: Simplify the code to set smm page table as RO

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

> +  PageTableBase = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64;
> +
> +  //
> +  // ConvertMemoryPageAttributes might update mPageTablePool. It's safer to
> +  // remember original one in advance.
> +  //
> +  HeadPool = mPageTablePool;
> +  Pool     = HeadPool;
> +  do {
> +    Address  = (EFI_PHYSICAL_ADDRESS)(UINTN)Pool & PAGE_TABLE_POOL_ALIGN_MASK;

1. When is the Pool not aligned on 128KB boundary? If it's guaranteed, can we remove the "& PAGE_TABLE_POOL_ALIGN_MASK"?


> +    PoolSize = Pool->Offset + EFI_PAGES_TO_SIZE (Pool->FreePages);
> +
> +    ConvertMemoryPageAttributes (PageTableBase, m5LevelPagingNeeded, Address, PoolSize, EFI_MEMORY_RO, TRUE,
> NULL, NULL);

2. Can you please explain in comments that above call is to make the entire pool including header, used-memory, free-memory
    as read-only?

3. It's better to use LinkedList library APIs from BaseLib. The comments apply to the first patch as well. But I am fine if you decide not to do it in this patch.

> +  {
> +    if (sizeof (UINTN) == sizeof (UINT64)) {
> +      //
> +      // Restriction on access to non-SMRAM memory and heap guard could not be enabled at the same time.
> +      //
> +      ASSERT (
> +        !(IsRestrictedMemoryAccess () &&
> +          (PcdGet8 (PcdHeapGuardPropertyMask) & (BIT3 | BIT2)) != 0)
> +        );
> +
> +      //
> +      // Restriction on access to non-SMRAM memory and SMM profile could not be enabled at the same time.
> +      //
> +      ASSERT (!(IsRestrictedMemoryAccess () && FeaturePcdGet (PcdCpuSmmProfileEnable)));
> +    }

4. I don't think we still need the above two assertions. But let's not clean up the code in your patch. @Wang, Jian J