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; Wed, 11 Sep 2019 08:25:23 -0700 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3FDFB3083394; Wed, 11 Sep 2019 15:25:23 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-127.ams2.redhat.com [10.36.116.127]) by smtp.corp.redhat.com (Postfix) with ESMTP id 34A865C207; Wed, 11 Sep 2019 15:25:22 +0000 (UTC) Subject: Re: [Patch] UefiCpuPkg/CpuDxe: clean up PAGE_TABLE_LIB_PAGING_CONTEXT usage. To: Eric Dong , devel@edk2.groups.io Cc: Ray Ni References: <20190911014511.9356-1-eric.dong@intel.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 11 Sep 2019 17:25:21 +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: <20190911014511.9356-1-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.44]); Wed, 11 Sep 2019 15:25:23 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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_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 ; > } >