Hi Laszlo, > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Wednesday, September 11, 2019 11:25 PM > To: Dong, Eric <eric.d...@intel.com>; devel@edk2.groups.io > Cc: Ni, Ray <ray...@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...@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Signed-off-by: Eric Dong <eric.d...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-