* [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM @ 2019-03-07 11:14 nkvangup 2019-03-07 14:38 ` Yao, Jiewen 2019-03-07 17:57 ` Laszlo Ersek 0 siblings, 2 replies; 6+ messages in thread From: nkvangup @ 2019-03-07 11:14 UTC (permalink / raw) To: edk2-devel Cc: Vanguput Narendra K, Eric Dong, Ray Ni, Laszlo Ersek, Yao Jiewen 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.vanguput@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Yao Jiewen <jiewen.yao@intel.com> --- UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) 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))) { + // + // 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); + } } /** -- 2.16.2.windows.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM 2019-03-07 11:14 [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM nkvangup @ 2019-03-07 14:38 ` Yao, Jiewen 2019-03-07 17:57 ` Laszlo Ersek 1 sibling, 0 replies; 6+ messages in thread From: Yao, Jiewen @ 2019-03-07 14:38 UTC (permalink / raw) To: Vanguput, Narendra K, edk2-devel@lists.01.org Cc: Dong, Eric, Ni, Ray, Laszlo Ersek Reviewed-by: Jiewen.yao@intel.com > -----Original Message----- > From: Vanguput, Narendra K > Sent: Thursday, March 7, 2019 3:15 AM > To: edk2-devel@lists.01.org > Cc: Vanguput, Narendra K <narendra.k.vanguput@intel.com>; Dong, Eric > <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek > <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com> > Subject: [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand > paging in SMM > > 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.vanguput@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Yao Jiewen <jiewen.yao@intel.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > 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))) { > + // > + // 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); > + } > } > > /** > -- > 2.16.2.windows.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM 2019-03-07 11:14 [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM nkvangup 2019-03-07 14:38 ` Yao, Jiewen @ 2019-03-07 17:57 ` Laszlo Ersek 2019-03-07 18:10 ` Kinney, Michael D 2019-03-07 18:18 ` Yao, Jiewen 1 sibling, 2 replies; 6+ messages in thread From: Laszlo Ersek @ 2019-03-07 17:57 UTC (permalink / raw) To: nkvangup, edk2-devel; +Cc: Eric Dong, Ray Ni, Yao Jiewen 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.vanguput@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Yao Jiewen <jiewen.yao@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); > + } > } > > /** > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM 2019-03-07 17:57 ` Laszlo Ersek @ 2019-03-07 18:10 ` Kinney, Michael D 2019-03-07 18:24 ` Kinney, Michael D 2019-03-07 18:18 ` Yao, Jiewen 1 sibling, 1 reply; 6+ messages in thread From: Kinney, Michael D @ 2019-03-07 18:10 UTC (permalink / raw) To: Laszlo Ersek, Vanguput, Narendra K, edk2-devel@lists.01.org, Kinney, Michael D Cc: Yao, Jiewen, Dong, Eric 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- > bounces@lists.01.org] On Behalf Of Laszlo Ersek > Sent: Thursday, March 7, 2019 9:58 AM > To: Vanguput, Narendra K > <narendra.k.vanguput@intel.com>; edk2- > devel@lists.01.org > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric > <eric.dong@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.vanguput@intel.com> > > Cc: Eric Dong <eric.dong@intel.com> > > Cc: Ray Ni <ray.ni@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Cc: Yao Jiewen <jiewen.yao@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM 2019-03-07 18:10 ` Kinney, Michael D @ 2019-03-07 18:24 ` Kinney, Michael D 0 siblings, 0 replies; 6+ messages in thread From: Kinney, Michael D @ 2019-03-07 18:24 UTC (permalink / raw) To: Laszlo Ersek, Vanguput, Narendra K, edk2-devel@lists.01.org, Kinney, Michael D Cc: Yao, Jiewen, Dong, Eric 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 <lersek@redhat.com>; Vanguput, > Narendra K <narendra.k.vanguput@intel.com>; edk2- > devel@lists.01.org; Kinney, Michael D > <michael.d.kinney@intel.com> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric > <eric.dong@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- > > bounces@lists.01.org] On Behalf Of Laszlo Ersek > > Sent: Thursday, March 7, 2019 9:58 AM > > To: Vanguput, Narendra K > > <narendra.k.vanguput@intel.com>; edk2- > > devel@lists.01.org > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric > > <eric.dong@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.vanguput@intel.com> > > > Cc: Eric Dong <eric.dong@intel.com> > > > Cc: Ray Ni <ray.ni@intel.com> > > > Cc: Laszlo Ersek <lersek@redhat.com> > > > Cc: Yao Jiewen <jiewen.yao@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM 2019-03-07 17:57 ` Laszlo Ersek 2019-03-07 18:10 ` Kinney, Michael D @ 2019-03-07 18:18 ` Yao, Jiewen 1 sibling, 0 replies; 6+ messages in thread From: Yao, Jiewen @ 2019-03-07 18:18 UTC (permalink / raw) To: Laszlo Ersek, Vanguput, Narendra K, edk2-devel@lists.01.org Cc: Dong, Eric, Ni, Ray 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:lersek@redhat.com] > Sent: Thursday, March 7, 2019 9:58 AM > To: Vanguput, Narendra K <narendra.k.vanguput@intel.com>; > edk2-devel@lists.01.org > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Yao, > Jiewen <jiewen.yao@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.vanguput@intel.com> > > Cc: Eric Dong <eric.dong@intel.com> > > Cc: Ray Ni <ray.ni@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Cc: Yao Jiewen <jiewen.yao@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); > > + } > > } > > > > /** > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-07 18:24 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-03-07 11:14 [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM nkvangup 2019-03-07 14:38 ` Yao, Jiewen 2019-03-07 17:57 ` Laszlo Ersek 2019-03-07 18:10 ` Kinney, Michael D 2019-03-07 18:24 ` Kinney, Michael D 2019-03-07 18:18 ` Yao, Jiewen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox