UefiCpuPkg/CpuDxe/CpuPageTable.c | 52 +++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 15 deletions(-)
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1039
Current implementation not checks system mode before using
PAGE_TABLE_LIB_PAGING_CONTEXT.ContextData.X64 or
PAGE_TABLE_LIB_PAGING_CONTEXT.ContextData.Ia32. This patch check
the mode before using the correct one.
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
UefiCpuPkg/CpuDxe/CpuPageTable.c | 52 +++++++++++++++++++++++---------
1 file changed, 37 insertions(+), 15 deletions(-)
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index ec5cd424fc..308f93b1cd 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -155,6 +155,8 @@ GetCurrentPagingContext (
MSR_IA32_EFER_REGISTER MsrEfer;
IA32_CR4 Cr4;
IA32_CR0 Cr0;
+ UINT32 *Attributes;
+ UINTN *PageTableBase;
//
// Don't retrieve current paging context from processor if in SMM mode.
@@ -163,29 +165,33 @@ GetCurrentPagingContext (
ZeroMem (&mPagingContext, sizeof(mPagingContext));
if (sizeof(UINTN) == sizeof(UINT64)) {
mPagingContext.MachineType = IMAGE_FILE_MACHINE_X64;
+ Attributes = &mPagingContext.ContextData.X64.Attributes;
+ PageTableBase = &mPagingContext.ContextData.X64.PageTableBase;
} else {
mPagingContext.MachineType = IMAGE_FILE_MACHINE_I386;
+ Attributes = &mPagingContext.ContextData.Ia32.Attributes;
+ PageTableBase = (UINTN *)&mPagingContext.ContextData.Ia32.PageTableBase;
}
Cr0.UintN = AsmReadCr0 ();
Cr4.UintN = AsmReadCr4 ();
if (Cr0.Bits.PG != 0) {
- mPagingContext.ContextData.X64.PageTableBase = (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
+ *PageTableBase = (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
} else {
- mPagingContext.ContextData.X64.PageTableBase = 0;
+ *PageTableBase = 0;
}
if (Cr0.Bits.WP != 0) {
- mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE;
+ *Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE;
}
if (Cr4.Bits.PSE != 0) {
- mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE;
+ *Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE;
}
if (Cr4.Bits.PAE != 0) {
- mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE;
+ *Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE;
}
if (Cr4.Bits.LA57 != 0) {
- mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_5_LEVEL;
+ *Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_5_LEVEL;
}
AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
@@ -197,12 +203,12 @@ GetCurrentPagingContext (
MsrEfer.Uint64 = AsmReadMsr64(MSR_CORE_IA32_EFER);
if (MsrEfer.Bits.NXE != 0) {
// XD activated
- mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED;
+ *Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED;
}
}
if (RegEdx.Bits.Page1GB != 0) {
- mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT;
+ *Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT;
}
}
}
@@ -395,6 +401,7 @@ ConvertPageEntryAttribute (
{
UINT64 CurrentPageEntry;
UINT64 NewPageEntry;
+ UINT32 *PageAttributes;
CurrentPageEntry = *PageEntry;
NewPageEntry = CurrentPageEntry;
@@ -438,7 +445,13 @@ ConvertPageEntryAttribute (
break;
}
}
- if ((PagingContext->ContextData.Ia32.Attributes & PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED) != 0) {
+
+ if (PagingContext->MachineType == IMAGE_FILE_MACHINE_X64) {
+ PageAttributes = &PagingContext->ContextData.X64.Attributes;
+ } else {
+ PageAttributes = &PagingContext->ContextData.Ia32.Attributes;
+ }
+ if ((*PageAttributes & PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED) != 0) {
if ((Attributes & EFI_MEMORY_XP) != 0) {
switch (PageAction) {
case PageActionAssign:
@@ -1338,15 +1351,24 @@ InitializePageTableLib (
)
{
PAGE_TABLE_LIB_PAGING_CONTEXT CurrentPagingContext;
+ UINT32 *Attributes;
+ UINTN *PageTableBase;
GetCurrentPagingContext (&CurrentPagingContext);
+ if (CurrentPagingContext.MachineType == IMAGE_FILE_MACHINE_X64) {
+ Attributes = &CurrentPagingContext.ContextData.X64.Attributes;
+ PageTableBase = &CurrentPagingContext.ContextData.X64.PageTableBase;
+ } else {
+ Attributes = &CurrentPagingContext.ContextData.Ia32.Attributes;
+ PageTableBase = (UINTN *)&CurrentPagingContext.ContextData.Ia32.PageTableBase;
+ }
+
//
// Reserve memory of page tables for future uses, if paging is enabled.
//
- if (CurrentPagingContext.ContextData.X64.PageTableBase != 0 &&
- (CurrentPagingContext.ContextData.Ia32.Attributes &
- PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE) != 0) {
+ if ((*PageTableBase != 0) &&
+ (*Attributes & PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE) != 0) {
DisableReadOnlyPageWriteProtect ();
InitializePageTablePool (1);
EnableReadOnlyPageWriteProtect ();
@@ -1361,10 +1383,10 @@ InitializePageTableLib (
ASSERT (mLastPFEntryPointer != NULL);
}
- DEBUG ((DEBUG_INFO, "CurrentPagingContext:\n", CurrentPagingContext.MachineType));
+ DEBUG ((DEBUG_INFO, "CurrentPagingContext:\n"));
DEBUG ((DEBUG_INFO, " MachineType - 0x%x\n", CurrentPagingContext.MachineType));
- DEBUG ((DEBUG_INFO, " PageTableBase - 0x%x\n", CurrentPagingContext.ContextData.X64.PageTableBase));
- DEBUG ((DEBUG_INFO, " Attributes - 0x%x\n", CurrentPagingContext.ContextData.X64.Attributes));
+ DEBUG ((DEBUG_INFO, " PageTableBase - 0x%x\n", *PageTableBase));
+ DEBUG ((DEBUG_INFO, " Attributes - 0x%x\n", *Attributes));
return ;
}
--
2.21.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#47093): https://edk2.groups.io/g/devel/message/47093
Mute This Topic: https://groups.io/mt/34099078/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 09/11/19 03:45, Eric Dong wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1039
>
> Current implementation not checks system mode before using
> PAGE_TABLE_LIB_PAGING_CONTEXT.ContextData.X64 or
> PAGE_TABLE_LIB_PAGING_CONTEXT.ContextData.Ia32. This patch check
> the mode before using the correct one.
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> UefiCpuPkg/CpuDxe/CpuPageTable.c | 52 +++++++++++++++++++++++---------
> 1 file changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index ec5cd424fc..308f93b1cd 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -155,6 +155,8 @@ GetCurrentPagingContext (
> MSR_IA32_EFER_REGISTER MsrEfer;
> IA32_CR4 Cr4;
> IA32_CR0 Cr0;
> + UINT32 *Attributes;
> + UINTN *PageTableBase;
>
> //
> // Don't retrieve current paging context from processor if in SMM mode.
> @@ -163,29 +165,33 @@ GetCurrentPagingContext (
> ZeroMem (&mPagingContext, sizeof(mPagingContext));
> if (sizeof(UINTN) == sizeof(UINT64)) {
> mPagingContext.MachineType = IMAGE_FILE_MACHINE_X64;
> + Attributes = &mPagingContext.ContextData.X64.Attributes;
> + PageTableBase = &mPagingContext.ContextData.X64.PageTableBase;
> } else {
> mPagingContext.MachineType = IMAGE_FILE_MACHINE_I386;
> + Attributes = &mPagingContext.ContextData.Ia32.Attributes;
> + PageTableBase = (UINTN *)&mPagingContext.ContextData.Ia32.PageTableBase;
(1) This is quite unfortunate. I don't like the cast.
I understand why it is used here -- when we build for X64, then UINTN is
UINT64, and so PageTableBase points to a UINT64. But
"mPagingContext.ContextData.Ia32.PageTableBase" is a UINT32, so the
compiler will complain about the incompatible pointer assignment.
Therefore you use an explicit cast -- which would be wrong, if the code
ever ran, but then again, that code will never run.
I don't like this trick.
Furthermore, the same issue will exist on the *other* branch, if you
build the present code for IA32 -- you didn't use a cast there. When
building for IA32, PageTableBase points to a UINT32. But, on the first
branch, "mPagingContext.ContextData.X64.PageTableBase" is a UINT64.
Therefore, the compiler will warn the same (although the code will never
run).
Did you build the patch for IA32?
I think there are two ways to fix this.
(1a) The ugly (but correct) fix is to use "#ifdef MDE_CPU_IA32" and
"#ifdef MDE_CPU_X64" directives, and *no* casts. In other words, we
should never compile the code that we won't run.
(1b) The nice (and also correct) fix is to introduce a helper function:
VOID
GetPagingDetails (
IN PAGE_TABLE_LIB_PAGING_CONTEXT_DATA *PagingContextData,
OUT UINTN **PageTableBase OPTIONAL,
OUT UINT32 **Attributes OPTIONAL
);
There would be two implementations:
- IA32 subdirectory:
if (PageTableBase != NULL) {
*PageTableBase = &PagingContextData->Ia32.PageTableBase;
}
if (Attributes != NULL) {
*Attributes = &PagingContextData->Ia32.Attributes;
}
- X64 subdirectory:
if (PageTableBase != NULL) {
*PageTableBase = &PagingContextData->X64.PageTableBase;
}
if (Attributes != NULL) {
*Attributes = &PagingContextData->X64.Attributes;
}
And here we'd do
GetPagingDetails (
&mPagingContext.ContextData,
&PageTableBase,
&Attributes
);
and the rest of the GetCurrentPagingContext() updates would be the same.
> }
>
> Cr0.UintN = AsmReadCr0 ();
> Cr4.UintN = AsmReadCr4 ();
>
> if (Cr0.Bits.PG != 0) {
> - mPagingContext.ContextData.X64.PageTableBase = (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
> + *PageTableBase = (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
> } else {
> - mPagingContext.ContextData.X64.PageTableBase = 0;
> + *PageTableBase = 0;
> }
> if (Cr0.Bits.WP != 0) {
> - mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE;
> + *Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE;
> }
> if (Cr4.Bits.PSE != 0) {
> - mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE;
> + *Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE;
> }
> if (Cr4.Bits.PAE != 0) {
> - mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE;
> + *Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE;
> }
> if (Cr4.Bits.LA57 != 0) {
> - mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_5_LEVEL;
> + *Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_5_LEVEL;
> }
>
> AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
> @@ -197,12 +203,12 @@ GetCurrentPagingContext (
> MsrEfer.Uint64 = AsmReadMsr64(MSR_CORE_IA32_EFER);
> if (MsrEfer.Bits.NXE != 0) {
> // XD activated
> - mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED;
> + *Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED;
> }
> }
>
> if (RegEdx.Bits.Page1GB != 0) {
> - mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT;
> + *Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT;
> }
> }
> }
> @@ -395,6 +401,7 @@ ConvertPageEntryAttribute (
> {
> UINT64 CurrentPageEntry;
> UINT64 NewPageEntry;
> + UINT32 *PageAttributes;
>
> CurrentPageEntry = *PageEntry;
> NewPageEntry = CurrentPageEntry;
> @@ -438,7 +445,13 @@ ConvertPageEntryAttribute (
> break;
> }
> }
> - if ((PagingContext->ContextData.Ia32.Attributes & PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED) != 0) {
> +
> + if (PagingContext->MachineType == IMAGE_FILE_MACHINE_X64) {
> + PageAttributes = &PagingContext->ContextData.X64.Attributes;
> + } else {
> + PageAttributes = &PagingContext->ContextData.Ia32.Attributes;
> + }
(2) So here we'd write
GetPagingDetails (
&PagingContext->ContextData,
NULL,
&PageAttributes
);
> + if ((*PageAttributes & PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED) != 0) {
> if ((Attributes & EFI_MEMORY_XP) != 0) {
> switch (PageAction) {
> case PageActionAssign:
> @@ -1338,15 +1351,24 @@ InitializePageTableLib (
> )
> {
> PAGE_TABLE_LIB_PAGING_CONTEXT CurrentPagingContext;
> + UINT32 *Attributes;
> + UINTN *PageTableBase;
>
> GetCurrentPagingContext (&CurrentPagingContext);
>
> + if (CurrentPagingContext.MachineType == IMAGE_FILE_MACHINE_X64) {
> + Attributes = &CurrentPagingContext.ContextData.X64.Attributes;
> + PageTableBase = &CurrentPagingContext.ContextData.X64.PageTableBase;
> + } else {
> + Attributes = &CurrentPagingContext.ContextData.Ia32.Attributes;
> + PageTableBase = (UINTN *)&CurrentPagingContext.ContextData.Ia32.PageTableBase;
> + }
> +
(3) This would become
GetPagingDetails (
&CurrentPagingContext.ContextData,
&PageTableBase,
&Attributes
);
> //
> // Reserve memory of page tables for future uses, if paging is enabled.
> //
> - if (CurrentPagingContext.ContextData.X64.PageTableBase != 0 &&
> - (CurrentPagingContext.ContextData.Ia32.Attributes &
> - PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE) != 0) {
> + if ((*PageTableBase != 0) &&
> + (*Attributes & PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE) != 0) {
> DisableReadOnlyPageWriteProtect ();
> InitializePageTablePool (1);
> EnableReadOnlyPageWriteProtect ();
> @@ -1361,10 +1383,10 @@ InitializePageTableLib (
> ASSERT (mLastPFEntryPointer != NULL);
> }
>
> - DEBUG ((DEBUG_INFO, "CurrentPagingContext:\n", CurrentPagingContext.MachineType));
> + DEBUG ((DEBUG_INFO, "CurrentPagingContext:\n"));
> DEBUG ((DEBUG_INFO, " MachineType - 0x%x\n", CurrentPagingContext.MachineType));
> - DEBUG ((DEBUG_INFO, " PageTableBase - 0x%x\n", CurrentPagingContext.ContextData.X64.PageTableBase));
> - DEBUG ((DEBUG_INFO, " Attributes - 0x%x\n", CurrentPagingContext.ContextData.X64.Attributes));
> + DEBUG ((DEBUG_INFO, " PageTableBase - 0x%x\n", *PageTableBase));
(4) UINTN values should be cast to UINT64 explicitly, and printed with
"%Lx":
DEBUG ((
DEBUG_INFO,
" PageTableBase - 0x%Lx\n",
(UINT64)*PageTableBase
));
Thanks
Laszlo
> + DEBUG ((DEBUG_INFO, " Attributes - 0x%x\n", *Attributes));
>
> return ;
> }
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#47126): https://edk2.groups.io/g/devel/message/47126
Mute This Topic: https://groups.io/mt/34099078/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Laszlo,
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, September 11, 2019 11:25 PM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: Re: [Patch] UefiCpuPkg/CpuDxe: clean up
> PAGE_TABLE_LIB_PAGING_CONTEXT usage.
>
> On 09/11/19 03:45, Eric Dong wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1039
> >
> > Current implementation not checks system mode before using
> > PAGE_TABLE_LIB_PAGING_CONTEXT.ContextData.X64 or
> > PAGE_TABLE_LIB_PAGING_CONTEXT.ContextData.Ia32. This patch check
> the
> > mode before using the correct one.
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> > UefiCpuPkg/CpuDxe/CpuPageTable.c | 52
> > +++++++++++++++++++++++---------
> > 1 file changed, 37 insertions(+), 15 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > index ec5cd424fc..308f93b1cd 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > @@ -155,6 +155,8 @@ GetCurrentPagingContext (
> > MSR_IA32_EFER_REGISTER MsrEfer;
> > IA32_CR4 Cr4;
> > IA32_CR0 Cr0;
> > + UINT32 *Attributes;
> > + UINTN *PageTableBase;
> >
> > //
> > // Don't retrieve current paging context from processor if in SMM mode.
> > @@ -163,29 +165,33 @@ GetCurrentPagingContext (
> > ZeroMem (&mPagingContext, sizeof(mPagingContext));
> > if (sizeof(UINTN) == sizeof(UINT64)) {
> > mPagingContext.MachineType = IMAGE_FILE_MACHINE_X64;
> > + Attributes = &mPagingContext.ContextData.X64.Attributes;
> > + PageTableBase = &mPagingContext.ContextData.X64.PageTableBase;
> > } else {
> > mPagingContext.MachineType = IMAGE_FILE_MACHINE_I386;
> > + Attributes = &mPagingContext.ContextData.Ia32.Attributes;
> > + PageTableBase = (UINTN
> > + *)&mPagingContext.ContextData.Ia32.PageTableBase;
>
> (1) This is quite unfortunate. I don't like the cast.
>
> I understand why it is used here -- when we build for X64, then UINTN is
> UINT64, and so PageTableBase points to a UINT64. But
> "mPagingContext.ContextData.Ia32.PageTableBase" is a UINT32, so the
> compiler will complain about the incompatible pointer assignment.
> Therefore you use an explicit cast -- which would be wrong, if the code ever ran,
> but then again, that code will never run.
>
> I don't like this trick.
>
> Furthermore, the same issue will exist on the *other* branch, if you build the
> present code for IA32 -- you didn't use a cast there. When building for IA32,
> PageTableBase points to a UINT32. But, on the first branch,
> "mPagingContext.ContextData.X64.PageTableBase" is a UINT64.
> Therefore, the compiler will warn the same (although the code will never run).
>
> Did you build the patch for IA32?
Yes, I have got a report said IA32 and GCC5 build failure. I have prepared V2 change to fix this issue. I don't want to send too much version of code change, so I wait for some feedback from this mail list before I send v2 change.
>
>
> I think there are two ways to fix this.
>
> (1a) The ugly (but correct) fix is to use "#ifdef MDE_CPU_IA32" and "#ifdef
> MDE_CPU_X64" directives, and *no* casts. In other words, we should never
> compile the code that we won't run.
>
> (1b) The nice (and also correct) fix is to introduce a helper function:
>
> VOID
> GetPagingDetails (
> IN PAGE_TABLE_LIB_PAGING_CONTEXT_DATA *PagingContextData,
> OUT UINTN **PageTableBase OPTIONAL,
> OUT UINT32 **Attributes OPTIONAL
> );
>
> There would be two implementations:
>
> - IA32 subdirectory:
>
> if (PageTableBase != NULL) {
> *PageTableBase = &PagingContextData->Ia32.PageTableBase;
> }
> if (Attributes != NULL) {
> *Attributes = &PagingContextData->Ia32.Attributes;
> }
>
> - X64 subdirectory:
>
> if (PageTableBase != NULL) {
> *PageTableBase = &PagingContextData->X64.PageTableBase;
> }
> if (Attributes != NULL) {
> *Attributes = &PagingContextData->X64.Attributes;
> }
>
> And here we'd do
>
> GetPagingDetails (
> &mPagingContext.ContextData,
> &PageTableBase,
> &Attributes
> );
>
> and the rest of the GetCurrentPagingContext() updates would be the same.
>
I will use this option to update my code.
>
> > }
> >
> > Cr0.UintN = AsmReadCr0 ();
> > Cr4.UintN = AsmReadCr4 ();
> >
> > if (Cr0.Bits.PG != 0) {
> > - mPagingContext.ContextData.X64.PageTableBase = (AsmReadCr3 () &
> PAGING_4K_ADDRESS_MASK_64);
> > + *PageTableBase = (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
> > } else {
> > - mPagingContext.ContextData.X64.PageTableBase = 0;
> > + *PageTableBase = 0;
> > }
> > if (Cr0.Bits.WP != 0) {
> > - mPagingContext.ContextData.Ia32.Attributes |=
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE;
> > + *Attributes |=
> > +
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE;
> > }
> > if (Cr4.Bits.PSE != 0) {
> > - mPagingContext.ContextData.Ia32.Attributes |=
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE;
> > + *Attributes |=
> > + PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE;
> > }
> > if (Cr4.Bits.PAE != 0) {
> > - mPagingContext.ContextData.Ia32.Attributes |=
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE;
> > + *Attributes |=
> > + PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE;
> > }
> > if (Cr4.Bits.LA57 != 0) {
> > - mPagingContext.ContextData.Ia32.Attributes |=
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_5_LEVEL;
> > + *Attributes |=
> > + PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_5_LEVEL;
> > }
> >
> > AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
> @@
> > -197,12 +203,12 @@ GetCurrentPagingContext (
> > MsrEfer.Uint64 = AsmReadMsr64(MSR_CORE_IA32_EFER);
> > if (MsrEfer.Bits.NXE != 0) {
> > // XD activated
> > - mPagingContext.ContextData.Ia32.Attributes |=
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATE
> D;
> > + *Attributes |=
> > +
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATE
> D;
> > }
> > }
> >
> > if (RegEdx.Bits.Page1GB != 0) {
> > - mPagingContext.ContextData.Ia32.Attributes |=
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUP
> PORT;
> > + *Attributes |=
> > +
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUP
> PORT;
> > }
> > }
> > }
> > @@ -395,6 +401,7 @@ ConvertPageEntryAttribute ( {
> > UINT64 CurrentPageEntry;
> > UINT64 NewPageEntry;
> > + UINT32 *PageAttributes;
> >
> > CurrentPageEntry = *PageEntry;
> > NewPageEntry = CurrentPageEntry;
> > @@ -438,7 +445,13 @@ ConvertPageEntryAttribute (
> > break;
> > }
> > }
> > - if ((PagingContext->ContextData.Ia32.Attributes &
> >
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATE
> D) != 0)
> > {
> > +
> > + if (PagingContext->MachineType == IMAGE_FILE_MACHINE_X64) {
> > + PageAttributes = &PagingContext->ContextData.X64.Attributes;
> > + } else {
> > + PageAttributes = &PagingContext->ContextData.Ia32.Attributes;
> > + }
>
> (2) So here we'd write
>
> GetPagingDetails (
> &PagingContext->ContextData,
> NULL,
> &PageAttributes
> );
>
> > + if ((*PageAttributes &
> > +
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATE
> D) !=
> > + 0) {
> > if ((Attributes & EFI_MEMORY_XP) != 0) {
> > switch (PageAction) {
> > case PageActionAssign:
> > @@ -1338,15 +1351,24 @@ InitializePageTableLib (
> > )
> > {
> > PAGE_TABLE_LIB_PAGING_CONTEXT CurrentPagingContext;
> > + UINT32 *Attributes;
> > + UINTN *PageTableBase;
> >
> > GetCurrentPagingContext (&CurrentPagingContext);
> >
> > + if (CurrentPagingContext.MachineType == IMAGE_FILE_MACHINE_X64) {
> > + Attributes = &CurrentPagingContext.ContextData.X64.Attributes;
> > + PageTableBase =
> > + &CurrentPagingContext.ContextData.X64.PageTableBase;
> > + } else {
> > + Attributes = &CurrentPagingContext.ContextData.Ia32.Attributes;
> > + PageTableBase = (UINTN
> > + *)&CurrentPagingContext.ContextData.Ia32.PageTableBase;
> > + }
> > +
>
> (3) This would become
>
> GetPagingDetails (
> &CurrentPagingContext.ContextData,
> &PageTableBase,
> &Attributes
> );
>
> > //
> > // Reserve memory of page tables for future uses, if paging is enabled.
> > //
> > - if (CurrentPagingContext.ContextData.X64.PageTableBase != 0 &&
> > - (CurrentPagingContext.ContextData.Ia32.Attributes &
> > - PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE) != 0)
> {
> > + if ((*PageTableBase != 0) &&
> > + (*Attributes &
> > + PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE) != 0) {
> > DisableReadOnlyPageWriteProtect ();
> > InitializePageTablePool (1);
> > EnableReadOnlyPageWriteProtect (); @@ -1361,10 +1383,10 @@
> > InitializePageTableLib (
> > ASSERT (mLastPFEntryPointer != NULL);
> > }
> >
> > - DEBUG ((DEBUG_INFO, "CurrentPagingContext:\n",
> > CurrentPagingContext.MachineType));
> > + DEBUG ((DEBUG_INFO, "CurrentPagingContext:\n"));
> > DEBUG ((DEBUG_INFO, " MachineType - 0x%x\n",
> CurrentPagingContext.MachineType));
> > - DEBUG ((DEBUG_INFO, " PageTableBase - 0x%x\n",
> CurrentPagingContext.ContextData.X64.PageTableBase));
> > - DEBUG ((DEBUG_INFO, " Attributes - 0x%x\n",
> CurrentPagingContext.ContextData.X64.Attributes));
> > + DEBUG ((DEBUG_INFO, " PageTableBase - 0x%x\n", *PageTableBase));
>
> (4) UINTN values should be cast to UINT64 explicitly, and printed with
> "%Lx":
>
> DEBUG ((
> DEBUG_INFO,
> " PageTableBase - 0x%Lx\n",
> (UINT64)*PageTableBase
> ));
Will update it in my next version change.
Thanks,
Eric
>
> Thanks
> Laszlo
>
> > + DEBUG ((DEBUG_INFO, " Attributes - 0x%x\n", *Attributes));
> >
> > return ;
> > }
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#47156): https://edk2.groups.io/g/devel/message/47156
Mute This Topic: https://groups.io/mt/34099078/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.