From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Fri, 13 Sep 2019 04:24:47 -0700 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D4C34C00E906; Fri, 13 Sep 2019 11:24:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-100.rdu2.redhat.com [10.10.120.100]) by smtp.corp.redhat.com (Postfix) with ESMTP id B50C7608C2; Fri, 13 Sep 2019 11:24:45 +0000 (UTC) Subject: Re: [Patch v2] UefiCpuPkg/CpuDxe: clean up PAGE_TABLE_LIB_PAGING_CONTEXT usage. To: Eric Dong , devel@edk2.groups.io Cc: Ray Ni References: <20190912052218.20500-1-eric.dong@intel.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 13 Sep 2019 13:24:44 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190912052218.20500-1-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Fri, 13 Sep 2019 11:24:46 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Laszlo Ersek > Signed-off-by: Eric Dong > --- > 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.
> +# Copyright (c) 2008 - 2019, Intel Corporation. All rights reserved.
> # Copyright (c) 2017, AMD Incorporated. All rights reserved.
> # > # 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.
> + 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.
> + 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 Thanks! Laszlo