From: "Yao, Jiewen" <jiewen.yao@intel.com>
To: "Xu, Min M" <min.m.xu@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Aktas, Erdem" <erdemaktas@google.com>,
Gerd Hoffmann <kraxel@redhat.com>,
James Bottomley <jejb@linux.ibm.com>
Subject: Re: [PATCH V2 1/1] OvmfPkg/PeilessStartupLib: move mPageTablePool to stack
Date: Tue, 27 Sep 2022 08:21:52 +0000 [thread overview]
Message-ID: <MW4PR11MB5872CE18DB7E1F9B677AB1CF8C559@MW4PR11MB5872.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20220927070753.470-1-min.m.xu@intel.com>
Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Tuesday, September 27, 2022 3:08 PM
> To: devel@edk2.groups.io
> Cc: Xu, Min M <min.m.xu@intel.com>; Aktas, Erdem
> <erdemaktas@google.com>; Gerd Hoffmann <kraxel@redhat.com>; James
> Bottomley <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH V2 1/1] OvmfPkg/PeilessStartupLib: move mPageTablePool
> to stack
>
> From: Min M Xu <min.m.xu@intel.com>
>
> PeilessStartupLib is running in SEC phase. In this phase global variable
> is not allowed to be modified. This patch moves mPageTablePool to stack
> and pass it as input parameter between functions.
>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
> .../PeilessStartupLib/X64/VirtualMemory.c | 117 ++++++++++--------
> 1 file changed, 68 insertions(+), 49 deletions(-)
>
> diff --git a/OvmfPkg/Library/PeilessStartupLib/X64/VirtualMemory.c
> b/OvmfPkg/Library/PeilessStartupLib/X64/VirtualMemory.c
> index 6877e521e485..b444c052d1bf 100644
> --- a/OvmfPkg/Library/PeilessStartupLib/X64/VirtualMemory.c
> +++ b/OvmfPkg/Library/PeilessStartupLib/X64/VirtualMemory.c
> @@ -21,11 +21,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <Library/PlatformInitLib.h>
> #include "PageTables.h"
>
> -//
> -// Global variable to keep track current available memory used as page
> table.
> -//
> -PAGE_TABLE_POOL *mPageTablePool = NULL;
> -
> UINTN mLevelShift[5] = {
> 0,
> PAGING_L1_ADDRESS_SHIFT,
> @@ -273,14 +268,17 @@ ToSplitPageTable (
> reserve at least another PAGE_TABLE_POOL_UNIT_PAGES. But usually
> this won't
> happen in practice.
>
> - @param PoolPages The least page number of the pool to be created.
> + @param[in] PoolPages The least page number of the pool to be
> created.
> + @param[in, out] PageTablePool Pointer of Pointer to the current
> available memory
> + used as page table.
>
> @retval TRUE The pool is initialized successfully.
> @retval FALSE The memory is out of resource.
> **/
> BOOLEAN
> InitializePageTablePool (
> - IN UINTN PoolPages
> + IN UINTN PoolPages,
> + IN OUT PAGE_TABLE_POOL **PageTablePool
> )
> {
> VOID *Buffer;
> @@ -303,20 +301,20 @@ InitializePageTablePool (
> //
> // Link all pools into a list for easier track later.
> //
> - if (mPageTablePool == NULL) {
> - mPageTablePool = Buffer;
> - mPageTablePool->NextPool = mPageTablePool;
> + if (*PageTablePool == NULL) {
> + *(UINT64 *)(UINTN)PageTablePool = (UINT64)(UINTN)Buffer;
> + (*PageTablePool)->NextPool = *PageTablePool;
> } else {
> - ((PAGE_TABLE_POOL *)Buffer)->NextPool = mPageTablePool->NextPool;
> - mPageTablePool->NextPool = Buffer;
> - mPageTablePool = Buffer;
> + ((PAGE_TABLE_POOL *)Buffer)->NextPool = (*PageTablePool)->NextPool;
> + (*PageTablePool)->NextPool = Buffer;
> + *PageTablePool = Buffer;
> }
>
> //
> // Reserve one page for pool header.
> //
> - mPageTablePool->FreePages = PoolPages - 1;
> - mPageTablePool->Offset = EFI_PAGES_TO_SIZE (1);
> + (*PageTablePool)->FreePages = PoolPages - 1;
> + (*PageTablePool)->Offset = EFI_PAGES_TO_SIZE (1);
>
> return TRUE;
> }
> @@ -333,14 +331,17 @@ InitializePageTablePool (
> If there is not enough memory remaining to satisfy the request, then
> NULL is
> returned.
>
> - @param Pages The number of 4 KB pages to allocate.
> + @param[in] Pages The number of 4 KB pages to allocate.
> + @param[in, out] PageTablePool Pointer of pointer to the current
> available
> + memory used as page table.
>
> @return A pointer to the allocated buffer or NULL if allocation fails.
>
> **/
> VOID *
> AllocatePageTableMemory (
> - IN UINTN Pages
> + IN UINTN Pages,
> + IN OUT PAGE_TABLE_POOL **PageTablePool
> )
> {
> VOID *Buffer;
> @@ -349,30 +350,31 @@ AllocatePageTableMemory (
> return NULL;
> }
>
> - DEBUG ((DEBUG_INFO, "AllocatePageTableMemory. mPageTablePool=%p,
> Pages=%d\n", mPageTablePool, Pages));
> + DEBUG ((DEBUG_INFO, "AllocatePageTableMemory. PageTablePool=%p,
> Pages=%d\n", *PageTablePool, Pages));
> //
> // Renew the pool if necessary.
> //
> - if ((mPageTablePool == NULL) ||
> - (Pages > mPageTablePool->FreePages))
> + if ((*PageTablePool == NULL) ||
> + (Pages > (*PageTablePool)->FreePages))
> {
> - if (!InitializePageTablePool (Pages)) {
> + if (!InitializePageTablePool (Pages, PageTablePool)) {
> return NULL;
> }
> }
>
> - Buffer = (UINT8 *)mPageTablePool + mPageTablePool->Offset;
> + Buffer = (UINT8 *)(*PageTablePool) + (*PageTablePool)->Offset;
>
> - mPageTablePool->Offset += EFI_PAGES_TO_SIZE (Pages);
> - mPageTablePool->FreePages -= Pages;
> + (*PageTablePool)->Offset += EFI_PAGES_TO_SIZE (Pages);
> + (*PageTablePool)->FreePages -= Pages;
>
> DEBUG ((
> DEBUG_INFO,
> - "%a:%a: Buffer=0x%Lx Pages=%ld\n",
> + "%a:%a: Buffer=0x%Lx Pages=%ld, PageTablePool=%p\n",
> gEfiCallerBaseName,
> __FUNCTION__,
> Buffer,
> - Pages
> + Pages,
> + *PageTablePool
> ));
>
> return Buffer;
> @@ -385,6 +387,8 @@ AllocatePageTableMemory (
> @param[in, out] PageEntry2M Pointer to 2M page entry.
> @param[in] StackBase Stack base address.
> @param[in] StackSize Stack size.
> + @param[in, out] PageTablePool Pointer to the current available
> memory used as
> + page table.
>
> **/
> VOID
> @@ -392,7 +396,8 @@ Split2MPageTo4K (
> IN EFI_PHYSICAL_ADDRESS PhysicalAddress,
> IN OUT UINT64 *PageEntry2M,
> IN EFI_PHYSICAL_ADDRESS StackBase,
> - IN UINTN StackSize
> + IN UINTN StackSize,
> + IN OUT PAGE_TABLE_POOL *PageTablePool
> )
> {
> EFI_PHYSICAL_ADDRESS PhysicalAddress4K;
> @@ -401,7 +406,7 @@ Split2MPageTo4K (
>
> DEBUG ((DEBUG_INFO, "Split2MPageTo4K\n"));
>
> - PageTableEntry = AllocatePageTableMemory (1);
> + PageTableEntry = AllocatePageTableMemory (1, &PageTablePool);
>
> if (PageTableEntry == NULL) {
> ASSERT (FALSE);
> @@ -448,6 +453,8 @@ Split2MPageTo4K (
> @param[in, out] PageEntry1G Pointer to 1G page entry.
> @param[in] StackBase Stack base address.
> @param[in] StackSize Stack size.
> + @param[in, out] PageTablePool Pointer to the current available
> memory used as
> + page table.
>
> **/
> VOID
> @@ -455,14 +462,16 @@ Split1GPageTo2M (
> IN EFI_PHYSICAL_ADDRESS PhysicalAddress,
> IN OUT UINT64 *PageEntry1G,
> IN EFI_PHYSICAL_ADDRESS StackBase,
> - IN UINTN StackSize
> + IN UINTN StackSize,
> + IN OUT PAGE_TABLE_POOL *PageTablePool
> )
> {
> EFI_PHYSICAL_ADDRESS PhysicalAddress2M;
> UINTN IndexOfPageDirectoryEntries;
> PAGE_TABLE_ENTRY *PageDirectoryEntry;
>
> - PageDirectoryEntry = AllocatePageTableMemory (1);
> + DEBUG ((DEBUG_INFO, "Split1GPageTo2M\n"));
> + PageDirectoryEntry = AllocatePageTableMemory (1, &PageTablePool);
>
> if (PageDirectoryEntry == NULL) {
> ASSERT (FALSE);
> @@ -480,7 +489,7 @@ Split1GPageTo2M (
> //
> // Need to split this 2M page that covers NULL or stack range.
> //
> - Split2MPageTo4K (PhysicalAddress2M, (UINT64 *)PageDirectoryEntry,
> StackBase, StackSize);
> + Split2MPageTo4K (PhysicalAddress2M, (UINT64 *)PageDirectoryEntry,
> StackBase, StackSize, PageTablePool);
> } else {
> //
> // Fill in the Page Directory entries
> @@ -496,16 +505,19 @@ Split1GPageTo2M (
> /**
> Set one page of page table pool memory to be read-only.
>
> - @param[in] PageTableBase Base address of page table (CR3).
> - @param[in] Address Start address of a page to be set as read-only.
> - @param[in] Level4Paging Level 4 paging flag.
> + @param[in] PageTableBase Base address of page table (CR3).
> + @param[in] Address Start address of a page to be set as read-
> only.
> + @param[in] Level4Paging Level 4 paging flag.
> + @param[in, out] PageTablePool Pointer to the current available
> memory used as
> + page table.
>
> **/
> VOID
> SetPageTablePoolReadOnly (
> IN UINTN PageTableBase,
> IN EFI_PHYSICAL_ADDRESS Address,
> - IN BOOLEAN Level4Paging
> + IN BOOLEAN Level4Paging,
> + IN OUT PAGE_TABLE_POOL *PageTablePool
> )
> {
> UINTN Index;
> @@ -573,7 +585,8 @@ SetPageTablePoolReadOnly (
> //
> ASSERT (Level > 1);
>
> - NewPageTable = AllocatePageTableMemory (1);
> + DEBUG ((DEBUG_INFO, "SetPageTablePoolReadOnly\n"));
> + NewPageTable = AllocatePageTableMemory (1, &PageTablePool);
>
> if (NewPageTable == NULL) {
> ASSERT (FALSE);
> @@ -604,14 +617,17 @@ SetPageTablePoolReadOnly (
> /**
> Prevent the memory pages used for page table from been overwritten.
>
> - @param[in] PageTableBase Base address of page table (CR3).
> - @param[in] Level4Paging Level 4 paging flag.
> + @param[in] PageTableBase Base address of page table (CR3).
> + @param[in] Level4Paging Level 4 paging flag.
> + @param[in, out] PageTablePool Pointer to the current available
> memory used as
> + page table.
>
> **/
> VOID
> EnablePageTableProtection (
> - IN UINTN PageTableBase,
> - IN BOOLEAN Level4Paging
> + IN UINTN PageTableBase,
> + IN BOOLEAN Level4Paging,
> + IN OUT PAGE_TABLE_POOL *PageTablePool
> )
> {
> PAGE_TABLE_POOL *HeadPool;
> @@ -621,7 +637,7 @@ EnablePageTableProtection (
>
> DEBUG ((DEBUG_INFO, "EnablePageTableProtection\n"));
>
> - if (mPageTablePool == NULL) {
> + if (PageTablePool == NULL) {
> return;
> }
>
> @@ -632,10 +648,10 @@ EnablePageTableProtection (
> AsmWriteCr0 (AsmReadCr0 () & ~CR0_WP);
>
> //
> - // SetPageTablePoolReadOnly might update mPageTablePool. It's safer to
> + // SetPageTablePoolReadOnly might update PageTablePool. It's safer to
> // remember original one in advance.
> //
> - HeadPool = mPageTablePool;
> + HeadPool = PageTablePool;
> Pool = HeadPool;
> do {
> Address = (EFI_PHYSICAL_ADDRESS)(UINTN)Pool;
> @@ -647,7 +663,7 @@ EnablePageTableProtection (
> // protection to them one by one.
> //
> while (PoolSize > 0) {
> - SetPageTablePoolReadOnly (PageTableBase, Address, Level4Paging);
> + SetPageTablePoolReadOnly (PageTableBase, Address, Level4Paging,
> PageTablePool);
> Address += PAGE_TABLE_POOL_UNIT_SIZE;
> PoolSize -= PAGE_TABLE_POOL_UNIT_SIZE;
> }
> @@ -700,6 +716,7 @@ CreateIdentityMappingPageTables (
> BOOLEAN Page1GSupport;
> PAGE_TABLE_1G_ENTRY *PageDirectory1GEntry;
> IA32_CR4 Cr4;
> + PAGE_TABLE_POOL *PageTablePool;
>
> //
> // Set PageMapLevel5Entry to suppress incorrect compiler/analyzer
> warnings
> @@ -785,13 +802,14 @@ CreateIdentityMappingPageTables (
> (UINT64)TotalPagesNum
> ));
>
> - BigPageAddress = (UINTN)AllocatePageTableMemory (TotalPagesNum);
> + PageTablePool = NULL;
> + BigPageAddress = (UINTN)AllocatePageTableMemory (TotalPagesNum,
> &PageTablePool);
> if (BigPageAddress == 0) {
> ASSERT (FALSE);
> return 0;
> }
>
> - DEBUG ((DEBUG_INFO, "BigPageAddress = 0x%llx\n", BigPageAddress));
> + DEBUG ((DEBUG_INFO, "BigPageAddress = 0x%llx, PageTablePool=%p\n",
> BigPageAddress, PageTablePool));
>
> //
> // By architecture only one PageMapLevel4 exists - so lets allocate storage
> for it.
> @@ -856,7 +874,8 @@ CreateIdentityMappingPageTables (
> PageAddress,
> (UINT64 *)PageDirectory1GEntry,
> StackBase,
> - StackSize
> + StackSize,
> + PageTablePool
> );
> } else {
> //
> @@ -892,7 +911,7 @@ CreateIdentityMappingPageTables (
> //
> // Need to split this 2M page that covers NULL or stack range.
> //
> - Split2MPageTo4K (PageAddress, (UINT64 *)PageDirectoryEntry,
> StackBase, StackSize);
> + Split2MPageTo4K (PageAddress, (UINT64 *)PageDirectoryEntry,
> StackBase, StackSize, PageTablePool);
> } else {
> //
> // Fill in the Page Directory entries
> @@ -929,7 +948,7 @@ CreateIdentityMappingPageTables (
> // Protect the page table by marking the memory used for page table to
> be
> // read-only.
> //
> - EnablePageTableProtection ((UINTN)PageMap, TRUE);
> + EnablePageTableProtection ((UINTN)PageMap, TRUE, PageTablePool);
>
> return (UINTN)PageMap;
> }
> --
> 2.29.2.windows.2
next prev parent reply other threads:[~2022-09-27 8:22 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-27 7:07 [PATCH V2 1/1] OvmfPkg/PeilessStartupLib: move mPageTablePool to stack Min Xu
2022-09-27 8:21 ` Yao, Jiewen [this message]
2022-09-28 2:03 ` Yao, Jiewen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MW4PR11MB5872CE18DB7E1F9B677AB1CF8C559@MW4PR11MB5872.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox