* [Patch] UefiCpuPkg/CpuDxe: clean up PAGE_TABLE_LIB_PAGING_CONTEXT usage. @ 2019-09-11 1:45 Dong, Eric 2019-09-11 15:25 ` Laszlo Ersek 0 siblings, 1 reply; 3+ messages in thread From: Dong, Eric @ 2019-09-11 1:45 UTC (permalink / raw) To: devel; +Cc: Ray Ni, Laszlo Ersek 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Patch] UefiCpuPkg/CpuDxe: clean up PAGE_TABLE_LIB_PAGING_CONTEXT usage. 2019-09-11 1:45 [Patch] UefiCpuPkg/CpuDxe: clean up PAGE_TABLE_LIB_PAGING_CONTEXT usage Dong, Eric @ 2019-09-11 15:25 ` Laszlo Ersek 2019-09-12 2:54 ` Dong, Eric 0 siblings, 1 reply; 3+ messages in thread From: Laszlo Ersek @ 2019-09-11 15:25 UTC (permalink / raw) To: Eric Dong, devel; +Cc: Ray Ni 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 ; > } > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Patch] UefiCpuPkg/CpuDxe: clean up PAGE_TABLE_LIB_PAGING_CONTEXT usage. 2019-09-11 15:25 ` Laszlo Ersek @ 2019-09-12 2:54 ` Dong, Eric 0 siblings, 0 replies; 3+ messages in thread From: Dong, Eric @ 2019-09-12 2:54 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io; +Cc: Ni, Ray 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 ; > > } > > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-09-12 2:54 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-09-11 1:45 [Patch] UefiCpuPkg/CpuDxe: clean up PAGE_TABLE_LIB_PAGING_CONTEXT usage Dong, Eric 2019-09-11 15:25 ` Laszlo Ersek 2019-09-12 2:54 ` Dong, Eric
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox