Laszlo, The information I provided below is incorrect. The PCD referenced does support all PCD types as Jiewen noted.
Mike > -----Original Message----- > From: Kinney, Michael D > Sent: Thursday, March 7, 2019 10:10 AM > To: Laszlo Ersek <ler...@redhat.com>; Vanguput, > Narendra K <narendra.k.vangu...@intel.com>; edk2- > de...@lists.01.org; Kinney, Michael D > <michael.d.kin...@intel.com> > Cc: Yao, Jiewen <jiewen....@intel.com>; Dong, Eric > <eric.d...@intel.com> > Subject: RE: [edk2] [PATCH v2] UefiCpuPkg\CpuSmm: Save > & restore CR2 on-demand paging in SMM > > Laszlo, > > Good news is that the PCD being used is a Feature Flag. > > [PcdsFeatureFlag] > ## Indicates if SMM Profile will be enabled. > # If enabled, instruction executions in and data > accesses to memory outside of SMRAM will be logged. > # It could not be enabled at the same time with SMM > static page table feature (PcdCpuSmmStaticPageTable). > # This PCD is only for validation purpose. It should > be set to false in production.<BR><BR> > # TRUE - SMM Profile will be enabled.<BR> > # FALSE - SMM Profile will be disabled.<BR> > # @Prompt Enable SMM Profile. > > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable|FALSE| > BOOLEAN|0x32132109 > > That means a different PcdLib function should be used > to look up > the value so it is clear that it is safe at SMM > runtime. > > FeaturePcdGet(TokenName) > > Best regards, > > Mike > > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel- > > boun...@lists.01.org] On Behalf Of Laszlo Ersek > > Sent: Thursday, March 7, 2019 9:58 AM > > To: Vanguput, Narendra K > > <narendra.k.vangu...@intel.com>; edk2- > > de...@lists.01.org > > Cc: Yao, Jiewen <jiewen....@intel.com>; Dong, Eric > > <eric.d...@intel.com> > > Subject: Re: [edk2] [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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
