public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: Eric Dong <eric.dong@intel.com>, devel@edk2.groups.io
Cc: Ray Ni <ray.ni@intel.com>
Subject: Re: [Patch v2] UefiCpuPkg/CpuDxe: clean up PAGE_TABLE_LIB_PAGING_CONTEXT usage.
Date: Fri, 13 Sep 2019 13:24:44 +0200	[thread overview]
Message-ID: <ba50e4c8-7084-7363-9439-74fc974022b6@redhat.com> (raw)
In-Reply-To: <20190912052218.20500-1-eric.dong@intel.com>

On 09/12/19 07:22, Eric Dong wrote:
> V2 changes:
> Avoid use type case, use different file implementation.
> 
> V1 changes:
> 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/CpuDxe.inf             |  4 ++-
>  UefiCpuPkg/CpuDxe/CpuPageTable.c         | 41 +++++++++++++++---------
>  UefiCpuPkg/CpuDxe/CpuPageTable.h         | 15 +++++++++
>  UefiCpuPkg/CpuDxe/Ia32/PagingAttribute.c | 34 ++++++++++++++++++++
>  UefiCpuPkg/CpuDxe/X64/PagingAttribute.c  | 34 ++++++++++++++++++++
>  5 files changed, 112 insertions(+), 16 deletions(-)
>  create mode 100644 UefiCpuPkg/CpuDxe/Ia32/PagingAttribute.c
>  create mode 100644 UefiCpuPkg/CpuDxe/X64/PagingAttribute.c
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> index 57381dbc85..d87fe503d1 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> @@ -1,7 +1,7 @@
>  ## @file
>  #  CPU driver installs CPU Architecture Protocol and CPU MP protocol.
>  #
> -#  Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2008 - 2019, Intel Corporation. All rights reserved.<BR>
>  #  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -54,9 +54,11 @@
>  
>  [Sources.IA32]
>    Ia32/CpuAsm.nasm
> +  Ia32/PagingAttribute.c
>  
>  [Sources.X64]
>    X64/CpuAsm.nasm
> +  X64/PagingAttribute.c
>  
>  [Protocols]
>    gEfiCpuArchProtocolGuid                       ## PRODUCES
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index ec5cd424fc..cb12177142 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.
> @@ -167,25 +169,27 @@ GetCurrentPagingContext (
>        mPagingContext.MachineType = IMAGE_FILE_MACHINE_I386;
>      }
>  
> +    GetPagingDetails (&mPagingContext.ContextData, &PageTableBase, &Attributes);
> +
>      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 +201,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 +399,7 @@ ConvertPageEntryAttribute (
>  {
>    UINT64  CurrentPageEntry;
>    UINT64  NewPageEntry;
> +  UINT32  *PageAttributes;
>  
>    CurrentPageEntry = *PageEntry;
>    NewPageEntry = CurrentPageEntry;
> @@ -438,7 +443,10 @@ ConvertPageEntryAttribute (
>        break;
>      }
>    }
> -  if ((PagingContext->ContextData.Ia32.Attributes & PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED) != 0) {
> +
> +  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 +1346,18 @@ InitializePageTableLib (
>    )
>  {
>    PAGE_TABLE_LIB_PAGING_CONTEXT     CurrentPagingContext;
> +  UINT32                            *Attributes;
> +  UINTN                             *PageTableBase;
>  
>    GetCurrentPagingContext (&CurrentPagingContext);
>  
> +  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 +1372,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%Lx\n", (UINT64)*PageTableBase));
> +  DEBUG ((DEBUG_INFO, "  Attributes    - 0x%x\n", *Attributes));
>  
>    return ;
>  }
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.h b/UefiCpuPkg/CpuDxe/CpuPageTable.h
> index f845956f73..0234d1f96a 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.h
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.h
> @@ -139,4 +139,19 @@ AllocatePageTableMemory (
>    IN UINTN           Pages
>    );
>  
> +/**
> +  Get paging details.
> +
> +  @param  PagingContextData      The paging context. NULL means get page table from current CPU context.
> +  @param  PageTableBase          Return PageTableBase field.
> +  @param  Attributes             Return Attributes field.
> +
> +**/
> +VOID
> +GetPagingDetails (
> +  IN  PAGE_TABLE_LIB_PAGING_CONTEXT_DATA *PagingContextData,
> +  OUT UINTN                              **PageTableBase     OPTIONAL,
> +  OUT UINT32                             **Attributes        OPTIONAL
> +  );
> +
>  #endif
> diff --git a/UefiCpuPkg/CpuDxe/Ia32/PagingAttribute.c b/UefiCpuPkg/CpuDxe/Ia32/PagingAttribute.c
> new file mode 100644
> index 0000000000..120c0f101a
> --- /dev/null
> +++ b/UefiCpuPkg/CpuDxe/Ia32/PagingAttribute.c
> @@ -0,0 +1,34 @@
> +/** @file
> +  Return Paging attribute.
> +
> +  Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include "CpuPageTable.h"
> +
> +
> +/**
> +  Get paging details.
> +
> +  @param  PagingContextData      The paging context. NULL means get page table from current CPU context.
> +  @param  PageTableBase          Return PageTableBase field.
> +  @param  Attributes             Return Attributes field.
> +
> +**/
> +VOID
> +GetPagingDetails (
> +  IN  PAGE_TABLE_LIB_PAGING_CONTEXT_DATA *PagingContextData,
> +  OUT UINTN                              **PageTableBase     OPTIONAL,
> +  OUT UINT32                             **Attributes        OPTIONAL
> +  )
> +{
> +  if (PageTableBase != NULL) {
> +    *PageTableBase = &PagingContextData->Ia32.PageTableBase;
> +  }
> +  if (Attributes != NULL) {
> +    *Attributes = &PagingContextData->Ia32.Attributes;
> +  }
> +}
> +
> diff --git a/UefiCpuPkg/CpuDxe/X64/PagingAttribute.c b/UefiCpuPkg/CpuDxe/X64/PagingAttribute.c
> new file mode 100644
> index 0000000000..fe35d33ed9
> --- /dev/null
> +++ b/UefiCpuPkg/CpuDxe/X64/PagingAttribute.c
> @@ -0,0 +1,34 @@
> +/** @file
> +  Return Paging attribute.
> +
> +  Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include "CpuPageTable.h"
> +
> +
> +/**
> +  Get paging details.
> +
> +  @param  PagingContextData      The paging context. NULL means get page table from current CPU context.
> +  @param  PageTableBase          Return PageTableBase field.
> +  @param  Attributes             Return Attributes field.
> +
> +**/
> +VOID
> +GetPagingDetails (
> +  IN  PAGE_TABLE_LIB_PAGING_CONTEXT_DATA *PagingContextData,
> +  OUT UINTN                              **PageTableBase     OPTIONAL,
> +  OUT UINT32                             **Attributes        OPTIONAL
> +  )
> +{
> +  if (PageTableBase != NULL) {
> +    *PageTableBase = &PagingContextData->X64.PageTableBase;
> +  }
> +  if (Attributes != NULL) {
> +    *Attributes = &PagingContextData->X64.Attributes;
> +  }
> +}
> +
> 

Please remove all three instances of the following comment:

"NULL means get page table from current CPU context."

With that:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

      reply	other threads:[~2019-09-13 11:24 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-12  5:22 [Patch v2] UefiCpuPkg/CpuDxe: clean up PAGE_TABLE_LIB_PAGING_CONTEXT usage Dong, Eric
2019-09-13 11:24 ` Laszlo Ersek [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ba50e4c8-7084-7363-9439-74fc974022b6@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox