On 07/26/19 11:58, Laszlo Ersek wrote: > Ray, > > On 07/18/19 08:58, Ni, Ray wrote: >> Commit c60d36b4d1ee1f69b7cca897d3621dfa951895c2 >> * UefiCpuPkg/SmmCpu: Block access-out only when static paging is used >> >> updated page fault handler to treat SMM access-out as allowed >> address when static paging is not used. >> >> But that commit is not complete because the page table is still >> updated in SetUefiMemMapAttributes() for non-SMRAM memory. When SMM >> code accesses non-SMRAM memory, page fault is still generated. >> >> This patch skips to update page table for non-SMRAM memory and >> page table itself. >> >> Signed-off-by: Ray Ni <ray...@intel.com> >> Cc: Eric Dong <eric.d...@intel.com> >> Cc: Jiewen Yao <jiewen....@intel.com> >> Cc: Jian J Wang <jian.j.w...@intel.com> >> --- >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 21 +++++++++++++++------ >> 1 file changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> index 2f7d777ee7..f75e75f55c 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> @@ -1103,6 +1103,9 @@ FindSmramInfo ( >> *SmrrBase = (UINT32)CurrentSmramRange->CpuStart; >> *SmrrSize = (UINT32)CurrentSmramRange->PhysicalSize; >> >> + // >> + // Extend *SmrrBase/*SmrrSize to include adjacent SMRAM ranges >> + // >> do { >> Found = FALSE; >> for (Index = 0; Index < mSmmCpuSmramRangeCount; Index++) { >> @@ -1414,14 +1417,20 @@ PerformRemainingTasks ( >> SetMemMapAttributes (); >> >> // >> - // For outside SMRAM, we only map SMM communication buffer or MMIO. >> + // Do not protect memory outside SMRAM when SMM static page table is >> not enabled. >> // >> - SetUefiMemMapAttributes (); >> + if (mCpuSmmStaticPageTable) { >> >> - // >> - // Set page table itself to be read-only >> - // >> - SetPageTableAttributes (); >> + // >> + // For outside SMRAM, we only map SMM communication buffer or MMIO. >> + // >> + SetUefiMemMapAttributes (); >> + >> + // >> + // Set page table itself to be read-only >> + // >> + SetPageTableAttributes (); >> + } >> >> // >> // Configure SMM Code Access Check feature if available. >> > > Commit 30f6148546c7 causes a build failure, when building for IA32: > >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c: In function >> 'PerformRemainingTasks': >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c:1440:9: error: >> 'mCpuSmmStaticPageTable' undeclared (first use in this function) >> if (mCpuSmmStaticPageTable) { > > "mCpuSmmStaticPageTable" is an X64-only variable. It is defined in > "X64/PageTbl.c", which is not linked into the IA32 binary. We must not > reference the variable in such code that is linked into both IA32 and > X64 builds, such as "PiSmmCpuDxeSmm.c". > > We have encountered the same challenge at least once in the past: > > - https://bugzilla.tianocore.org/show_bug.cgi?id=1593 > - commit 37f9fea5b88d ("UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand > paging in SMM", 2019-04-04) > > The right approach is to declare a new function in "PiSmmCpuDxeSmm.h", > and to provide two definitions for the function, one in > "Ia32/PageTbl.c", and another in "X64/PageTbl.c". The IA32 > implementation should return a constant value. The X64 implementation > should return "mCpuSmmStaticPageTable". (In the example named above, the > functions were SaveCr2() and RestoreCr2().) > > --*-- > > I'm going to revert commit 30f6148546c7 immediately, because it breaks > the build, and because catching this issue in advance would have been > trivial, if you had attempted to build for IA32. (To be sure, the > reviewers on this patch are not responsible; reviewers are welcome, but > not required, to catch such errors that the compiler/linker is supposed > to catch.) With this build breakage, I wouldn't be able to test Eric's > series > > [edk2-devel] [Patch v3 0/6] UefiCpuPkg: Enable Edkii Mp Services2 Ppi > > and I'd like to proceed with that.
The revert has commit hash d47b85a621ad. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#44452): https://edk2.groups.io/g/devel/message/44452 Mute This Topic: https://groups.io/mt/32512605/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-