public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [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