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

["duntan" <dun.tan@intel.com>]

Thanks Ray for the comments. I'll modify the code in V2 patch set.
For comments 3, I'll replace the code by LinkedList library APIs from BaseLib in future patches.

Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Monday, December 19, 2022 3:53 PM
To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
Subject: RE: [PATCH 3/3] UefiCpuPkg: Simplify the code to set smm page table as RO

> +  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