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...@intel.com>
Sent: Monday, December 19, 2022 3:53 PM
To: Tan, Dun <dun....@intel.com>; devel@edk2.groups.io; Wang, Jian J
<jian.j.w...@intel.com>
Cc: Dong, Eric <eric.d...@intel.com>; Kumar, Rahul R <rahul.r.ku...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97656): https://edk2.groups.io/g/devel/message/97656
Mute This Topic: https://groups.io/mt/95703349/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
