Re: [edk2-devel] [Patch] UefiCpuPkg/CpuDxe: clean up PAGE_TABLE_LIB_PAGING_CONTEXT usage.
Hi Laszlo, > -Original Message- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Wednesday, September 11, 2019 11:25 PM > To: Dong, Eric ; devel@edk2.groups.io > Cc: Ni, Ray > 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 > > Cc: Laszlo Ersek > > Signed-off-by: Eric Dong > > --- > > 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_CR4Cr4; > >IA32_CR0Cr0; > > + UINT32 *Attributes; > > + UINTN *PageTableBase; > > > >// > >// Don't retrieve current paging context from processor if in SMM mode. > > @@ -163,29 +165,33 @@ GetCurrentPagingContext ( > > ZeroMem (, sizeof(mPagingContext)); > > if (sizeof(UINTN) == sizeof(UINT64)) { > >mPagingContext.MachineType = IMAGE_FILE_MACHINE_X64; > > + Attributes = > > + PageTableBase = > > } else { > >mPagingContext.MachineType = IMAGE_FILE_MACHINE_I386; > > + Attributes = > > + PageTableBase = (UINTN > > + *) > > (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 **AttributesOPTIONAL > ); > > There would be two implementations: > > - IA32 subdirectory: > > if (PageTableBase != NULL) { > *PageTableBase = >Ia32.PageTableBase; > } > if (Attributes != NULL) { > *Attributes = >Ia32.Attributes; > } > > - X64 subdirectory: > > if (PageTableBase != NULL) { > *PageTableBase = >X64.PageTableBase; > } > if (Attributes != NULL) { > *Attributes = >X64.Attributes; > } > > And here we'd do > > GetPagingDetails ( > , > , > > ); > > 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; >
Re: [edk2-devel] [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 > Cc: Laszlo Ersek > Signed-off-by: Eric Dong > --- > 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_CR4Cr4; >IA32_CR0Cr0; > + UINT32 *Attributes; > + UINTN *PageTableBase; > >// >// Don't retrieve current paging context from processor if in SMM mode. > @@ -163,29 +165,33 @@ GetCurrentPagingContext ( > ZeroMem (, sizeof(mPagingContext)); > if (sizeof(UINTN) == sizeof(UINT64)) { >mPagingContext.MachineType = IMAGE_FILE_MACHINE_X64; > + Attributes = > + PageTableBase = > } else { >mPagingContext.MachineType = IMAGE_FILE_MACHINE_I386; > + Attributes = > + PageTableBase = (UINTN > *) (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 **AttributesOPTIONAL ); There would be two implementations: - IA32 subdirectory: if (PageTableBase != NULL) { *PageTableBase = >Ia32.PageTableBase; } if (Attributes != NULL) { *Attributes = >Ia32.Attributes; } - X64 subdirectory: if (PageTableBase != NULL) { *PageTableBase = >X64.PageTableBase; } if (Attributes != NULL) { *Attributes = >X64.Attributes; } And here we'd do GetPagingDetails ( , , ); 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, , NULL, NULL, NULL); > @@ -197,12 +203,12 @@ GetCurrentPagingContext ( > MsrEfer.Uint64 = AsmReadMsr64(MSR_CORE_IA32_EFER); > if (MsrEfer.Bits.NXE != 0) { >// XD activated >
[edk2-devel] [Patch] UefiCpuPkg/CpuDxe: clean up PAGE_TABLE_LIB_PAGING_CONTEXT usage.
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 Cc: Laszlo Ersek Signed-off-by: Eric Dong --- 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_CR4Cr4; IA32_CR0Cr0; + UINT32 *Attributes; + UINTN *PageTableBase; // // Don't retrieve current paging context from processor if in SMM mode. @@ -163,29 +165,33 @@ GetCurrentPagingContext ( ZeroMem (, sizeof(mPagingContext)); if (sizeof(UINTN) == sizeof(UINT64)) { mPagingContext.MachineType = IMAGE_FILE_MACHINE_X64; + Attributes = + PageTableBase = } else { mPagingContext.MachineType = IMAGE_FILE_MACHINE_I386; + Attributes = + PageTableBase = (UINTN *) } 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, , 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 = >ContextData.X64.Attributes; + } else { +PageAttributes = >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 (); + if (CurrentPagingContext.MachineType == IMAGE_FILE_MACHINE_X64) { +Attributes = +PageTableBase = + } else { +Attributes = +PageTableBase = (UINTN *) + } + // // 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 &