[edk2-devel] [PATCH v6 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address
Laszlo Ersek
lersek at redhat.com
Tue Nov 10 20:25:55 UTC 2020
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67251): https://edk2.groups.io/g/devel/message/67251
Mute This Topic: https://groups.io/mt/78152032/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