On 08/25/17 11:53, Zeng, Star wrote: > Laszlo, > > X64 defined mPhysicalAddressBits already before the patch, and has the code > below to assign it. > > mPhysicalAddressBits = CalculateMaximumSupportAddress ();
Thanks. Do you think it would make sense to centralize the definition (i.e., the allocation) of the mPhysicalAddressBits variable in this patch? That is, - instead of adding mPhysicalAddressBits to "Ia32/PageTbl.c", - you could move it from "X64/PageTbl.c" to "SmmCpuMemoryManagement.c" (or another C source file that is built into both Ia32 and X64). Thanks, Laszlo > -----Original Message----- > From: edk2-devel [mailto:[email protected]] On Behalf Of Laszlo > Ersek > Sent: Friday, August 25, 2017 5:50 PM > To: Zeng, Star <[email protected]>; [email protected] > Cc: Yao, Jiewen <[email protected]>; Dong, Eric <[email protected]> > Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection > crash > > Star, > > On 08/24/17 05:20, Star Zeng wrote: >> https://bugzilla.tianocore.org/show_bug.cgi?id=624 reports memory >> protection crash in PiSmmCpuDxeSmm, Ia32 build with RAM above 4GB (of >> which 2GB are placed in 64-bit address). >> It is because UEFI builds identity mapping page tables, >>> 4G address is not supported at Ia32 build. >> >> This patch is to get the PhysicalAddressBits that is used to build in >> PageTbl.c(Ia32/X64), and use it to check whether the address is >> supported or not in ConvertMemoryPageAttributes(). >> >> With this patch, the debug messages will be like below. >> UefiMemory protection: 0x0 - 0x9F000 Success UefiMemory protection: >> 0x100000 - 0x807000 Success UefiMemory protection: 0x808000 - 0x810000 >> Success UefiMemory protection: 0x818000 - 0x820000 Success UefiMemory >> protection: 0x1510000 - 0x7B798000 Success UefiMemory protection: >> 0x7B79B000 - 0x7E538000 Success UefiMemory protection: 0x7E539000 - >> 0x7E545000 Success UefiMemory protection: 0x7E55A000 - 0x7E61F000 >> Success UefiMemory protection: 0x7E62B000 - 0x7F6AB000 Success >> UefiMemory protection: 0x7F703000 - 0x7F70B000 Success UefiMemory >> protection: 0x7F70F000 - 0x7F778000 Success UefiMemory protection: >> 0x100000000 - 0x180000000 Unsupported >> >> Cc: Jiewen Yao <[email protected]> >> Cc: Laszlo Ersek <[email protected]> >> Cc: Eric Dong <[email protected]> >> Originally-suggested-by: Jiewen Yao <[email protected]> >> Reported-by: Laszlo Ersek <[email protected]> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Star Zeng <[email protected]> >> --- >> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 4 +++ >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 1 + >> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 31 >> +++++++++++++++++----- >> 3 files changed, 30 insertions(+), 6 deletions(-) >> >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c >> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c >> index 32ce5958c59c..e88b42d73343 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c >> @@ -16,6 +16,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER >> EXPRESS OR IMPLIED. >> >> #include "PiSmmCpuDxeSmm.h" >> >> +UINT8 mPhysicalAddressBits; >> + >> /** >> Create PageTable for SMM use. >> >> @@ -36,6 +38,8 @@ SmmInitPageTable ( >> // >> InitializeSpinLock (mPFLock); >> >> + mPhysicalAddressBits = 32; >> + >> if (FeaturePcdGet (PcdCpuSmmProfileEnable)) { >> // >> // Set own Page Fault entry instead of the default one, because >> SMM Profile diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h >> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h >> index dbce9ec520fe..1cf85c1481a7 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h >> @@ -419,6 +419,7 @@ extern SPIN_LOCK >> *mConfigSmmCodeAccessCheckLock; >> extern SPIN_LOCK *mMemoryMappedLock; >> extern EFI_SMRAM_DESCRIPTOR *mSmmCpuSmramRanges; >> extern UINTN mSmmCpuSmramRangeCount; >> +extern UINT8 mPhysicalAddressBits; >> >> // >> // Copy of the PcdPteMemoryEncryptionAddressOrMask >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c >> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c >> index a535389c26ce..3ad5256f1e03 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c >> @@ -1,6 +1,6 @@ >> /** @file >> >> -Copyright (c) 2016, Intel Corporation. All rights reserved.<BR> >> +Copyright (c) 2016 - 2017, Intel Corporation. All rights >> +reserved.<BR> >> This program and the accompanying materials are licensed and made >> available under the terms and conditions of the BSD License which >> accompanies this distribution. The full text of the license may be >> found at @@ -380,6 +380,7 @@ ConvertMemoryPageAttributes ( >> PAGE_ATTRIBUTE SplitAttribute; >> RETURN_STATUS Status; >> BOOLEAN IsEntryModified; >> + EFI_PHYSICAL_ADDRESS MaximumSupportMemAddress; >> >> ASSERT (Attributes != 0); >> ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | >> EFI_MEMORY_XP)) == 0); @@ -391,6 +392,17 @@ ConvertMemoryPageAttributes ( >> return RETURN_INVALID_PARAMETER; >> } >> >> + MaximumSupportMemAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)(LShiftU64 >> + (1, mPhysicalAddressBits) - 1); if (BaseAddress > >> MaximumSupportMemAddress) { >> + return RETURN_UNSUPPORTED; >> + } >> + if (Length > MaximumSupportMemAddress) { >> + return RETURN_UNSUPPORTED; >> + } >> + if ((Length != 0) && (BaseAddress > MaximumSupportMemAddress - (Length - >> 1))) { >> + return RETURN_UNSUPPORTED; >> + } >> + >> // DEBUG ((DEBUG_ERROR, "ConvertMemoryPageAttributes(%x) - %016lx, >> %016lx, %02lx\n", IsSet, BaseAddress, Length, Attributes)); >> >> if (IsSplitted != NULL) { >> @@ -1037,6 +1049,7 @@ SetUefiMemMapAttributes ( >> VOID >> ) >> { >> + EFI_STATUS Status; >> EFI_MEMORY_DESCRIPTOR *MemoryMap; >> UINTN MemoryMapEntryCount; >> UINTN Index; >> @@ -1052,12 +1065,18 @@ SetUefiMemMapAttributes ( >> MemoryMap = mUefiMemoryMap; >> for (Index = 0; Index < MemoryMapEntryCount; Index++) { >> if (IsUefiPageNotPresent(MemoryMap)) { >> - DEBUG ((DEBUG_INFO, "UefiMemory protection: 0x%lx - 0x%lx\n", >> MemoryMap->PhysicalStart, MemoryMap->PhysicalStart + >> (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages))); >> - SmmSetMemoryAttributes ( >> + Status = SmmSetMemoryAttributes ( >> + MemoryMap->PhysicalStart, >> + EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages), >> + EFI_MEMORY_RP >> + ); >> + DEBUG (( >> + DEBUG_INFO, >> + "UefiMemory protection: 0x%lx - 0x%lx %r\n", >> MemoryMap->PhysicalStart, >> - EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages), >> - EFI_MEMORY_RP >> - ); >> + MemoryMap->PhysicalStart + >> (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages), >> + Status >> + )); >> } >> MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize); >> } >> > > before applying this patch for local testing, I figured I'd look it over > quickly. > > I think that you missed adding the X64 changes to the commit, with "git add". > Because, the "mPhysicalAddressBits" variable is declared in common code, it > is also consumed in common code, but it is only defined (i.e., > allocated) and set in Ia32 code. I believe that applying this exact patch > would prevent PiSmmCpuDxeSmm even from linking. > > I think for X64 you likely have a change similar to the Ia32 one (defining > the variable and setting it to the actual physical address bits, likely from > the CPU HOB), but it's not part of the patch. > > If I'm right and you decide to post v2, then I suggest another (very > small) improvement: I think the definition (=allocation) of > "mPhysicalAddressBits" could also be moved to common code; only the > assignments have to be architecture-specific. > > Thanks > Laszlo > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
