[edk2-devel] [PATCH v7 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address
Laszlo Ersek
lersek at redhat.com
Sat Nov 14 01:24:53 UTC 2020
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67569): https://edk2.groups.io/g/devel/message/67569
Mute This Topic: https://groups.io/mt/78222299/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list