On 03/07/19 12:14, nkvangup wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1593 > > For every SMI occurrence, save and restore CR2 register only when SMM > on-demand paging support is enabled in 64 bit operation mode. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Vanguput Narendra K <narendra.k.vangu...@intel.com> > Cc: Eric Dong <eric.d...@intel.com> > Cc: Ray Ni <ray...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Cc: Yao Jiewen <jiewen....@intel.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-)
(1) There is an open question about the usefulness of this patch in <https://bugzilla.tianocore.org/show_bug.cgi?id=1593#c1>. It should be answered in the BZ, or the same description should be included in the commit message. (2) Also, the commit message should refer to the BZ. > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > index 3b0b3b52ac..5be4a2b020 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -1111,10 +1111,12 @@ SmiRendezvous ( > > ASSERT(CpuIndex < mMaxNumberOfCpus); > > - // > - // Save Cr2 because Page Fault exception in SMM may override its value > - // > - Cr2 = AsmReadCr2 (); > + if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool > (PcdCpuSmmStaticPageTable))) { (3) It doesn't look like a good idea to me to call PcdGetBool() in the SmiRendezvous() function. If the PCD is not fixed-at-build (but dynamic), then we'll end up calling a PI protocol member from a function that is by definition executed by multiple processors at the same time. "X64/PageTbl.c" already defines the global variable "mCpuSmmStaticPageTable", setting it from the PCD on the call stack of the entry point function of the driver. That is safe -- we can call PI / UEFI protocols in the entry point functions of a DXE_SMM_DRIVER. Now, the fact that "mCpuSmmStaticPageTable" is only defined in the X64 build (that is, in "X64/PageTbl.c"), is actually quite informative. It means that, instead of this conditional code in "MpService.c", we should introduce two new helper functions, "SaveCr2" and "RestoreCr2". And we should provide separate implementations for IA32 and X64. For IA32, the function should do nothing. For X64, the function should depend on "mCpuSmmStaticPageTable", and massage CR2 as necessary. However: that *still* depends on whether this change is useful. I realize the CR2 manipulation may not be overly useful on IA32 (we can't address >4GB memory, so demand paging for >4GB makes no sense), but its performance hit should be negligible. Again, back to point (1): what is the actual issue with the current code? Thanks Laszlo > + // > + // Save Cr2 because Page Fault exception in SMM may override its value > + // > + Cr2 = AsmReadCr2 (); > + } > > // > // Perform CPU specific entry hooks > @@ -1253,10 +1255,12 @@ SmiRendezvous ( > > Exit: > SmmCpuFeaturesRendezvousExit (CpuIndex); > - // > - // Restore Cr2 > - // > - AsmWriteCr2 (Cr2); > + if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool > (PcdCpuSmmStaticPageTable))) { > + // > + // Restore Cr2 > + // > + AsmWriteCr2 (Cr2); > + } > } > > /** > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
