Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Store PageTablePool in TdxWorkArea
On September 27, 2022 11:01 AM, Jiewen Yao wrote: > > My feeling is that this mPageTablePool is an driver *internal* data. > It is not the best idea to expose an internal data structure to *public* > space. > > Can we have a way to move mPageTablePool to stack instead of global data > area? such as input as function parameter? > Yes, we can do in that way. I will submit a new version of the patch so that the community can have a review. Thanks Min -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#94385): https://edk2.groups.io/g/devel/message/94385 Mute This Topic: https://groups.io/mt/93517163/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Store PageTablePool in TdxWorkArea
My feeling is that this mPageTablePool is an driver *internal* data. It is not the best idea to expose an internal data structure to *public* space. Can we have a way to move mPageTablePool to stack instead of global data area? such as input as function parameter? Thank you Yao Jiewen > -Original Message- > From: Gerd Hoffmann > Sent: Friday, September 23, 2022 1:44 PM > To: Xu, Min M > Cc: devel@edk2.groups.io; Aktas, Erdem ; > James Bottomley ; Yao, Jiewen > ; Tom Lendacky > Subject: Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Store PageTablePool in > TdxWorkArea > > On Thu, Sep 22, 2022 at 04:43:04AM +, Xu, Min M wrote: > > Hi, Gerd > > Do you have any comments on this patch? > > Looks ok to me. > > But I'm increasingly wondering whenever it actually was that a smart > move to ditch the PEI phase for the IntelTdx builds. Anything which > can't be handled in DXE must be done in SEC, and the SEC restrictions > seem to be a constant struggle. > > We had that when adding tdx measurement support which required sha* > hash support for SEC. And this looks like another case ... > > take care, > Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#94384): https://edk2.groups.io/g/devel/message/94384 Mute This Topic: https://groups.io/mt/93517163/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Store PageTablePool in TdxWorkArea
On Thu, Sep 22, 2022 at 04:43:04AM +, Xu, Min M wrote: > Hi, Gerd > Do you have any comments on this patch? Looks ok to me. But I'm increasingly wondering whenever it actually was that a smart move to ditch the PEI phase for the IntelTdx builds. Anything which can't be handled in DXE must be done in SEC, and the SEC restrictions seem to be a constant struggle. We had that when adding tdx measurement support which required sha* hash support for SEC. And this looks like another case ... take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#94166): https://edk2.groups.io/g/devel/message/94166 Mute This Topic: https://groups.io/mt/93517163/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/1] OvmfPkg: Store PageTablePool in TdxWorkArea
Hi, Gerd
Do you have any comments on this patch?
> -Original Message-
> From: devel@edk2.groups.io On Behalf Of Min Xu
> Sent: Wednesday, September 7, 2022 10:18 AM
> To: devel@edk2.groups.io
> Cc: Xu, Min M ; Aktas, Erdem
> ; Gerd Hoffmann ; James
> Bottomley ; Yao, Jiewen ; Tom
> Lendacky
> Subject: [edk2-devel] [PATCH 1/1] OvmfPkg: Store PageTablePool in
> TdxWorkArea
>
> From: Min M Xu
>
> PeilessStartupLib is running in SEC phase. In that phase global variable is
> not
> allowed. So the mPageTablePool cannot be used. In stead the PageTablePool
> is stored in TdxWorkArea.
>
> Cc: Erdem Aktas
> Cc: Gerd Hoffmann
> Cc: James Bottomley
> Cc: Jiewen Yao
> Cc: Tom Lendacky
> Signed-off-by: Min M Xu
> ---
> OvmfPkg/Include/WorkArea.h| 1 +
> .../PeilessStartupLib/PeilessStartupLib.inf | 1 +
> .../PeilessStartupLib/X64/VirtualMemory.c | 82 +--
> 3 files changed, 60 insertions(+), 24 deletions(-)
>
> diff --git a/OvmfPkg/Include/WorkArea.h b/OvmfPkg/Include/WorkArea.h
> index bf56fc4a6f65..0b2fb2f8c573 100644
> --- a/OvmfPkg/Include/WorkArea.h
> +++ b/OvmfPkg/Include/WorkArea.h
> @@ -71,6 +71,7 @@ typedef struct _SEC_TDX_WORK_AREA {
>UINT32PageTableReady;
>UINT32Gpaw;
>UINT64HobList;
> + UINT64PageTablePool;
> } SEC_TDX_WORK_AREA;
>
> typedef struct _TDX_WORK_AREA {
> diff --git a/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf
> b/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf
> index def50b4b019e..eed9f27d3d01 100644
> --- a/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf
> +++ b/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf
> @@ -88,3 +88,4 @@
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
> ## CONSUMES
>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase
>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
> diff --git a/OvmfPkg/Library/PeilessStartupLib/X64/VirtualMemory.c
> b/OvmfPkg/Library/PeilessStartupLib/X64/VirtualMemory.c
> index 6877e521e485..7e33f044993a 100644
> --- a/OvmfPkg/Library/PeilessStartupLib/X64/VirtualMemory.c
> +++ b/OvmfPkg/Library/PeilessStartupLib/X64/VirtualMemory.c
> @@ -19,13 +19,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include #include
> #include
> +#include
> #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,
> @@ -50,6 +46,32 @@ UINT64 mLevelSize[5] = {
>SIZE_512GB
> };
>
> +PAGE_TABLE_POOL *
> +GetPageTablePool (
> + VOID
> + )
> +{
> + TDX_WORK_AREA *TdxWorkArea;
> +
> + TdxWorkArea = (TDX_WORK_AREA *)(UINTN)FixedPcdGet32
> + (PcdOvmfWorkAreaBase); ASSERT (TdxWorkArea != NULL);
> +
> + return (PAGE_TABLE_POOL *)TdxWorkArea-
> >SecTdxWorkArea.PageTablePool;
> +}
> +
> +VOID
> +SetPageTablePool (
> + VOID *PageTablePool
> + )
> +{
> + TDX_WORK_AREA *TdxWorkArea;
> +
> + TdxWorkArea = (TDX_WORK_AREA *)(UINTN)FixedPcdGet32
> + (PcdOvmfWorkAreaBase); ASSERT (TdxWorkArea != NULL);
> +
> + TdxWorkArea->SecTdxWorkArea.PageTablePool =
> +(UINT64)(UINTN)PageTablePool; }
> +
> BOOLEAN
> IsSetNxForStack (
>VOID
> @@ -283,7 +305,10 @@ InitializePageTablePool (
>IN UINTN PoolPages
>)
> {
> - VOID *Buffer;
> + VOID *Buffer;
> + PAGE_TABLE_POOL *PageTablePool;
> +
> + PageTablePool = GetPageTablePool ();
>
>DEBUG ((DEBUG_INFO, "InitializePageTablePool PoolPages=%d\n",
> PoolPages));
>
> @@ -303,20 +328,21 @@ InitializePageTablePool (
>//
>// Link all pools into a list for easier track later.
>//
> - if (mPageTablePool == NULL) {
> -mPageTablePool = Buffer;
> -mPageTablePool->NextPool = mPageTablePool;
> + if (PageTablePool == NULL) {
> +PageTablePool = Buffer;
> +PageTablePool->NextPool = PageTablePool;
> +SetPageTablePool (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.
>//
>
[edk2-devel] [PATCH 1/1] OvmfPkg: Store PageTablePool in TdxWorkArea
From: Min M Xu
PeilessStartupLib is running in SEC phase. In that phase global variable
is not allowed. So the mPageTablePool cannot be used. In stead the
PageTablePool is stored in TdxWorkArea.
Cc: Erdem Aktas
Cc: Gerd Hoffmann
Cc: James Bottomley
Cc: Jiewen Yao
Cc: Tom Lendacky
Signed-off-by: Min M Xu
---
OvmfPkg/Include/WorkArea.h| 1 +
.../PeilessStartupLib/PeilessStartupLib.inf | 1 +
.../PeilessStartupLib/X64/VirtualMemory.c | 82 +--
3 files changed, 60 insertions(+), 24 deletions(-)
diff --git a/OvmfPkg/Include/WorkArea.h b/OvmfPkg/Include/WorkArea.h
index bf56fc4a6f65..0b2fb2f8c573 100644
--- a/OvmfPkg/Include/WorkArea.h
+++ b/OvmfPkg/Include/WorkArea.h
@@ -71,6 +71,7 @@ typedef struct _SEC_TDX_WORK_AREA {
UINT32PageTableReady;
UINT32Gpaw;
UINT64HobList;
+ UINT64PageTablePool;
} SEC_TDX_WORK_AREA;
typedef struct _TDX_WORK_AREA {
diff --git a/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf
b/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf
index def50b4b019e..eed9f27d3d01 100644
--- a/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf
+++ b/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf
@@ -88,3 +88,4 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask##
CONSUMES
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
diff --git a/OvmfPkg/Library/PeilessStartupLib/X64/VirtualMemory.c
b/OvmfPkg/Library/PeilessStartupLib/X64/VirtualMemory.c
index 6877e521e485..7e33f044993a 100644
--- a/OvmfPkg/Library/PeilessStartupLib/X64/VirtualMemory.c
+++ b/OvmfPkg/Library/PeilessStartupLib/X64/VirtualMemory.c
@@ -19,13 +19,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include
#include
#include
+#include
#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,
@@ -50,6 +46,32 @@ UINT64 mLevelSize[5] = {
SIZE_512GB
};
+PAGE_TABLE_POOL *
+GetPageTablePool (
+ VOID
+ )
+{
+ TDX_WORK_AREA *TdxWorkArea;
+
+ TdxWorkArea = (TDX_WORK_AREA *)(UINTN)FixedPcdGet32 (PcdOvmfWorkAreaBase);
+ ASSERT (TdxWorkArea != NULL);
+
+ return (PAGE_TABLE_POOL *)TdxWorkArea->SecTdxWorkArea.PageTablePool;
+}
+
+VOID
+SetPageTablePool (
+ VOID *PageTablePool
+ )
+{
+ TDX_WORK_AREA *TdxWorkArea;
+
+ TdxWorkArea = (TDX_WORK_AREA *)(UINTN)FixedPcdGet32 (PcdOvmfWorkAreaBase);
+ ASSERT (TdxWorkArea != NULL);
+
+ TdxWorkArea->SecTdxWorkArea.PageTablePool = (UINT64)(UINTN)PageTablePool;
+}
+
BOOLEAN
IsSetNxForStack (
VOID
@@ -283,7 +305,10 @@ InitializePageTablePool (
IN UINTN PoolPages
)
{
- VOID *Buffer;
+ VOID *Buffer;
+ PAGE_TABLE_POOL *PageTablePool;
+
+ PageTablePool = GetPageTablePool ();
DEBUG ((DEBUG_INFO, "InitializePageTablePool PoolPages=%d\n", PoolPages));
@@ -303,20 +328,21 @@ InitializePageTablePool (
//
// Link all pools into a list for easier track later.
//
- if (mPageTablePool == NULL) {
-mPageTablePool = Buffer;
-mPageTablePool->NextPool = mPageTablePool;
+ if (PageTablePool == NULL) {
+PageTablePool = Buffer;
+PageTablePool->NextPool = PageTablePool;
+SetPageTablePool (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;
}
@@ -343,28 +369,33 @@ AllocatePageTableMemory (
IN UINTN Pages
)
{
- VOID *Buffer;
+ VOID *Buffer;
+ PAGE_TABLE_POOL *PageTablePool;
if (Pages == 0) {
return NULL;
}
- DEBUG ((DEBUG_INFO, "AllocatePageTableMemory. mPageTablePool=%p,
Pages=%d\n", mPageTablePool, Pages));
+ PageTablePool = GetPageTablePool ();
+
+ 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)) {
return NULL;
+} else {
+ PageTablePool = GetPageTablePool ();
}
}
- Buffer = (UINT8 *)mPageTablePool + mPageTablePool->Offset;
+ Buffer = (UINT8 *)PageTablePool + PageTablePool-
