Good catch Laszo!!! I found PcdCpuSmmStaticPageTable is [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]. I think it should only be static, but I am wrong. Thanks to point it out. Then I think we need get the PCD value at the entrypoint.
Another option is just to move the CR2 access from C code to ASM code, to isolate the CR2 access in C code. Thank you Yao Jiewen > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Thursday, March 7, 2019 9:58 AM > To: Vanguput, Narendra K <narendra.k.vangu...@intel.com>; > edk2-devel@lists.01.org > Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Yao, > Jiewen <jiewen....@intel.com> > Subject: Re: [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 > on-demand paging in SMM > > 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
