* [PATCH v8] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM @ 2019-03-29 15:44 nkvangup 2019-03-29 21:22 ` Desimone, Nathaniel L 2019-03-29 22:03 ` Ni, Ray 0 siblings, 2 replies; 5+ messages in thread From: nkvangup @ 2019-03-29 15:44 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. This is not a bug but to have better improvement of code. Patch5 is updated with separate functions for Save and Restore of CR2 based on review feedback. Patch6 - Removed Global Cr2 instead used function parameter Patch7 - Removed checking Cr2 with 0 as per feedback 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/Ia32/PageTbl.c | 26 ++++++++++++++++++++++++++ UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 9 ++++++--- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22 ++++++++++++++++++++++ UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 30 ++++++++++++++++++++++++++++++ 4 files changed, 84 insertions(+), 3 deletions(-) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c index b734a1ea8c..d3f62ed806 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c @@ -316,3 +316,29 @@ SetPageTableAttributes ( return ; } + +/** + This function returns with no action for 32 bit. + + @param[out] *Cr2 Pointer to variable to hold CR2 register value +**/ +VOID +SaveCr2 ( + UINTN *Cr2 + ) +{ + return ; +} + +/** + This function returns with no action for 32 bit. + + @param[in] Cr2 Value to write into CR2 register +**/ +VOID +RestoreCr2 ( + UINTN Cr2 + ) +{ + return ; +} diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c index 3b0b3b52ac..ce70f77709 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c @@ -1112,9 +1112,11 @@ SmiRendezvous ( ASSERT(CpuIndex < mMaxNumberOfCpus); // - // Save Cr2 because Page Fault exception in SMM may override its value + // Save Cr2 because Page Fault exception in SMM may override its value, + // when using on-demand paging for above 4G memory. // - Cr2 = AsmReadCr2 (); + Cr2 = 0; + SaveCr2 (&Cr2); // // Perform CPU specific entry hooks @@ -1253,10 +1255,11 @@ SmiRendezvous ( Exit: SmmCpuFeaturesRendezvousExit (CpuIndex); + // // Restore Cr2 // - AsmWriteCr2 (Cr2); + RestoreCr2 (Cr2); } /** diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h index 84efb22981..05e1b54ed2 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h @@ -1243,4 +1243,26 @@ EFIAPI PiSmmCpuSmiEntryFixupAddress ( ); +/** + This function reads CR2 register when on-demand paging is enabled + for 64 bit and no action for 32 bit. + + @param[out] *Cr2 Pointer to variable to hold CR2 register value +**/ +VOID +SaveCr2 ( + UINTN *Cr2 + ); + +/** + This function writes into CR2 register when on-demand paging is enabled + for 64 bit and no action for 32 bit. + + @param[in] Cr2 Value to write into CR2 register +**/ +VOID +RestoreCr2 ( + UINTN Cr2 + ); + #endif diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c index 2c77cb47a4..e60628c080 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c @@ -1053,3 +1053,33 @@ SetPageTableAttributes ( return ; } + +/** + This function reads CR2 register when on-demand paging is enabled + + @param[out] *Cr2 Pointer to variable to hold CR2 register value +**/ +VOID +SaveCr2 ( + UINTN *Cr2 + ) +{ + if (!mCpuSmmStaticPageTable) { + *Cr2 = AsmReadCr2 (); + } +} + +/** + This function restores CR2 register when on-demand paging is enabled + + @param[in] Cr2 Value to write into CR2 register +**/ +VOID +RestoreCr2 ( + UINTN Cr2 + ) +{ + if (!mCpuSmmStaticPageTable) { + AsmWriteCr2 (Cr2); + } +} -- 2.16.2.windows.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v8] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM 2019-03-29 15:44 [PATCH v8] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM nkvangup @ 2019-03-29 21:22 ` Desimone, Nathaniel L 2019-03-29 21:37 ` Andrew Fish 2019-03-29 22:03 ` Ni, Ray 1 sibling, 1 reply; 5+ messages in thread From: Desimone, Nathaniel L @ 2019-03-29 21:22 UTC (permalink / raw) To: Vanguput, Narendra K, edk2-devel@lists.01.org Cc: Yao, Jiewen, Dong, Eric, Laszlo Ersek 1. Why would you do this for 64 bit but not 32 bit? 2. Why don't you add the if statement to MpService.c instead of spreading it to PageTbl.c? 3. What is the reason for this anyway? Adding the conditional is probably more execution time than just reading CR2 always. Thanks, Nate -----Original Message----- From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of nkvangup Sent: Friday, March 29, 2019 8:45 AM To: edk2-devel@lists.01.org Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com> Subject: [edk2] [PATCH v8] 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. This is not a bug but to have better improvement of code. Patch5 is updated with separate functions for Save and Restore of CR2 based on review feedback. Patch6 - Removed Global Cr2 instead used function parameter Patch7 - Removed checking Cr2 with 0 as per feedback Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Vanguput Narendra K <mailto:narendra.k.vanguput@intel.com> Cc: Eric Dong <mailto:eric.dong@intel.com> Cc: Ray Ni <mailto:ray.ni@intel.com> Cc: Laszlo Ersek <mailto:lersek@redhat.com> Cc: Yao Jiewen <mailto:jiewen.yao@intel.com> --- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 26 ++++++++++++++++++++++++++ UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 9 ++++++--- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22 ++++++++++++++++++++++ UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 30 ++++++++++++++++++++++++++++++ 4 files changed, 84 insertions(+), 3 deletions(-) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c index b734a1ea8c..d3f62ed806 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c @@ -316,3 +316,29 @@ SetPageTableAttributes ( return ; } + +/** + This function returns with no action for 32 bit. + + @param[out] *Cr2 Pointer to variable to hold CR2 register value **/ +VOID +SaveCr2 ( + UINTN *Cr2 + ) +{ + return ; +} + +/** + This function returns with no action for 32 bit. + + @param[in] Cr2 Value to write into CR2 register **/ VOID +RestoreCr2 ( + UINTN Cr2 + ) +{ + return ; +} diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c index 3b0b3b52ac..ce70f77709 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c @@ -1112,9 +1112,11 @@ SmiRendezvous ( ASSERT(CpuIndex < mMaxNumberOfCpus); // - // Save Cr2 because Page Fault exception in SMM may override its value + // Save Cr2 because Page Fault exception in SMM may override its + value, // when using on-demand paging for above 4G memory. // - Cr2 = AsmReadCr2 (); + Cr2 = 0; + SaveCr2 (&Cr2); // // Perform CPU specific entry hooks @@ -1253,10 +1255,11 @@ SmiRendezvous ( Exit: SmmCpuFeaturesRendezvousExit (CpuIndex); + // // Restore Cr2 // - AsmWriteCr2 (Cr2); + RestoreCr2 (Cr2); } /** diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h index 84efb22981..05e1b54ed2 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h @@ -1243,4 +1243,26 @@ EFIAPI PiSmmCpuSmiEntryFixupAddress ( ); +/** + This function reads CR2 register when on-demand paging is enabled + for 64 bit and no action for 32 bit. + + @param[out] *Cr2 Pointer to variable to hold CR2 register value **/ +VOID +SaveCr2 ( + UINTN *Cr2 + ); + +/** + This function writes into CR2 register when on-demand paging is +enabled + for 64 bit and no action for 32 bit. + + @param[in] Cr2 Value to write into CR2 register **/ VOID +RestoreCr2 ( + UINTN Cr2 + ); + #endif diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c index 2c77cb47a4..e60628c080 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c @@ -1053,3 +1053,33 @@ SetPageTableAttributes ( return ; } + +/** + This function reads CR2 register when on-demand paging is enabled + + @param[out] *Cr2 Pointer to variable to hold CR2 register value **/ +VOID +SaveCr2 ( + UINTN *Cr2 + ) +{ + if (!mCpuSmmStaticPageTable) { + *Cr2 = AsmReadCr2 (); + } +} + +/** + This function restores CR2 register when on-demand paging is enabled + + @param[in] Cr2 Value to write into CR2 register **/ VOID +RestoreCr2 ( + UINTN Cr2 + ) +{ + if (!mCpuSmmStaticPageTable) { + AsmWriteCr2 (Cr2); + } +} -- 2.16.2.windows.1 _______________________________________________ edk2-devel mailing list mailto:edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v8] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM 2019-03-29 21:22 ` Desimone, Nathaniel L @ 2019-03-29 21:37 ` Andrew Fish 2019-04-01 6:46 ` Vanguput, Narendra K 0 siblings, 1 reply; 5+ messages in thread From: Andrew Fish @ 2019-03-29 21:37 UTC (permalink / raw) To: Desimone, Nathaniel L Cc: Vanguput, Narendra K, edk2-devel@lists.01.org, Laszlo Ersek, Yao, Jiewen, Dong, Eric > On Mar 29, 2019, at 2:22 PM, Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> wrote: > > 1. Why would you do this for 64 bit but not 32 bit? Is paging enabled on 32-bit, it is required for Long mode? Also I'm not clear why it is an enhancement given you could take a periodic SMM in the kernels page fault handler and trashing CR2 seems bad. Maybe there is some behavior I'm missing? I'm not sure how big an issue this is but if SMM is modifying CR2 it is leaking information about SMM operations outside of SMM. Thanks, Andrew Fish > 2. Why don't you add the if statement to MpService.c instead of spreading it to PageTbl.c? > 3. What is the reason for this anyway? Adding the conditional is probably more execution time than just reading CR2 always. > > Thanks, > Nate > > -----Original Message----- > From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of nkvangup > Sent: Friday, March 29, 2019 8:45 AM > To: edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com> > Subject: [edk2] [PATCH v8] 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. > This is not a bug but to have better improvement of code. > > Patch5 is updated with separate functions for Save and Restore of CR2 based on review feedback. > > Patch6 - Removed Global Cr2 instead used function parameter > > Patch7 - Removed checking Cr2 with 0 as per feedback > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Vanguput Narendra K <mailto:narendra.k.vanguput@intel.com> > Cc: Eric Dong <mailto:eric.dong@intel.com> > Cc: Ray Ni <mailto:ray.ni@intel.com> > Cc: Laszlo Ersek <mailto:lersek@redhat.com> > Cc: Yao Jiewen <mailto:jiewen.yao@intel.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 26 ++++++++++++++++++++++++++ > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 9 ++++++--- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22 ++++++++++++++++++++++ > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 30 ++++++++++++++++++++++++++++++ > 4 files changed, 84 insertions(+), 3 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > index b734a1ea8c..d3f62ed806 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > @@ -316,3 +316,29 @@ SetPageTableAttributes ( > > return ; > } > + > +/** > + This function returns with no action for 32 bit. > + > + @param[out] *Cr2 Pointer to variable to hold CR2 register value **/ > +VOID > +SaveCr2 ( > + UINTN *Cr2 > + ) > +{ > + return ; > +} > + > +/** > + This function returns with no action for 32 bit. > + > + @param[in] Cr2 Value to write into CR2 register **/ VOID > +RestoreCr2 ( > + UINTN Cr2 > + ) > +{ > + return ; > +} > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > index 3b0b3b52ac..ce70f77709 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -1112,9 +1112,11 @@ SmiRendezvous ( > ASSERT(CpuIndex < mMaxNumberOfCpus); > > // > - // Save Cr2 because Page Fault exception in SMM may override its value > + // Save Cr2 because Page Fault exception in SMM may override its > + value, // when using on-demand paging for above 4G memory. > // > - Cr2 = AsmReadCr2 (); > + Cr2 = 0; > + SaveCr2 (&Cr2); > > // > // Perform CPU specific entry hooks > @@ -1253,10 +1255,11 @@ SmiRendezvous ( > > Exit: > SmmCpuFeaturesRendezvousExit (CpuIndex); > + > // > // Restore Cr2 > // > - AsmWriteCr2 (Cr2); > + RestoreCr2 (Cr2); > } > > /** > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index 84efb22981..05e1b54ed2 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -1243,4 +1243,26 @@ EFIAPI > PiSmmCpuSmiEntryFixupAddress ( > ); > > +/** > + This function reads CR2 register when on-demand paging is enabled > + for 64 bit and no action for 32 bit. > + > + @param[out] *Cr2 Pointer to variable to hold CR2 register value **/ > +VOID > +SaveCr2 ( > + UINTN *Cr2 > + ); > + > +/** > + This function writes into CR2 register when on-demand paging is > +enabled > + for 64 bit and no action for 32 bit. > + > + @param[in] Cr2 Value to write into CR2 register **/ VOID > +RestoreCr2 ( > + UINTN Cr2 > + ); > + > #endif > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > index 2c77cb47a4..e60628c080 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -1053,3 +1053,33 @@ SetPageTableAttributes ( > > return ; > } > + > +/** > + This function reads CR2 register when on-demand paging is enabled > + > + @param[out] *Cr2 Pointer to variable to hold CR2 register value **/ > +VOID > +SaveCr2 ( > + UINTN *Cr2 > + ) > +{ > + if (!mCpuSmmStaticPageTable) { > + *Cr2 = AsmReadCr2 (); > + } > +} > + > +/** > + This function restores CR2 register when on-demand paging is enabled > + > + @param[in] Cr2 Value to write into CR2 register **/ VOID > +RestoreCr2 ( > + UINTN Cr2 > + ) > +{ > + if (!mCpuSmmStaticPageTable) { > + AsmWriteCr2 (Cr2); > + } > +} > -- > 2.16.2.windows.1 > > _______________________________________________ > edk2-devel mailing list > mailto: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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v8] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM 2019-03-29 21:37 ` Andrew Fish @ 2019-04-01 6:46 ` Vanguput, Narendra K 0 siblings, 0 replies; 5+ messages in thread From: Vanguput, Narendra K @ 2019-04-01 6:46 UTC (permalink / raw) To: afish@apple.com, Desimone, Nathaniel L Cc: edk2-devel@lists.01.org, Laszlo Ersek, Yao, Jiewen, Dong, Eric Hi Nate, Andrew Fish, Paging is enabled. Here the point is SMM using Static page table Vs On-Demand Paging. SMM always builds static page table for 32 bit. Only for 64 bit, there is a PCD flag to control whether to use Static Page Table or On-Demand Paging. Below is the PCD flag details copied from 'UefiCpuPkg.dec' file [ ## Indicates if SMM uses static page table. # If enabled, SMM will not use on-demand paging. SMM will build static page table for all memory. # This flag only impacts X64 build, because SMM always builds static page table for IA32. # It could not be enabled at the same time with SMM profile feature (PcdCpuSmmProfileEnable). # It could not be enabled also at the same time with heap guard feature for SMM # (PcdHeapGuardPropertyMask in MdeModulePkg).<BR><BR> # TRUE - SMM uses static page table for all memory.<BR> # FALSE - SMM uses static page table for below 4G memory and use on-demand paging for above 4G memory.<BR> # @Prompt Use static page table for all memory in SMM. gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable|TRUE|BOOLEAN|0x3213210D ] Storing and restoring CR2 register is only required for On-Demand Paging when its in 64 bit. So the changes are related to that. For the Comment #2, Initially it was like that (I think in Patch 4), but based on review comments and to make clear implementation changed to use APIs. Hope I have answered your questions. Thanks, Naren > -----Original Message----- > From: afish@apple.com [mailto:afish@apple.com] > Sent: Saturday, March 30, 2019 3:08 AM > To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> > Cc: Vanguput, Narendra K <narendra.k.vanguput@intel.com>; edk2- > devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen > <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com> > Subject: Re: [edk2] [PATCH v8] UefiCpuPkg\CpuSmm: Save & restore CR2 on- > demand paging in SMM > > > > > On Mar 29, 2019, at 2:22 PM, Desimone, Nathaniel L > <nathaniel.l.desimone@intel.com> wrote: > > > > 1. Why would you do this for 64 bit but not 32 bit? > > Is paging enabled on 32-bit, it is required for Long mode? > > Also I'm not clear why it is an enhancement given you could take a periodic > SMM in the kernels page fault handler and trashing CR2 seems bad. Maybe > there is some behavior I'm missing? > > I'm not sure how big an issue this is but if SMM is modifying CR2 it is leaking > information about SMM operations outside of SMM. > > Thanks, > > Andrew Fish > > > 2. Why don't you add the if statement to MpService.c instead of spreading it > to PageTbl.c? > > 3. What is the reason for this anyway? Adding the conditional is probably > more execution time than just reading CR2 always. > > > > Thanks, > > Nate > > > > -----Original Message----- > > From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of > > nkvangup > > Sent: Friday, March 29, 2019 8:45 AM > > To: edk2-devel@lists.01.org > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric > > <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com> > > Subject: [edk2] [PATCH v8] 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. > > This is not a bug but to have better improvement of code. > > > > Patch5 is updated with separate functions for Save and Restore of CR2 > based on review feedback. > > > > Patch6 - Removed Global Cr2 instead used function parameter > > > > Patch7 - Removed checking Cr2 with 0 as per feedback > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Vanguput Narendra K > > <mailto:narendra.k.vanguput@intel.com> > > Cc: Eric Dong <mailto:eric.dong@intel.com> > > Cc: Ray Ni <mailto:ray.ni@intel.com> > > Cc: Laszlo Ersek <mailto:lersek@redhat.com> > > Cc: Yao Jiewen <mailto:jiewen.yao@intel.com> > > --- > > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 26 > ++++++++++++++++++++++++++ > > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 9 ++++++--- > > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22 > ++++++++++++++++++++++ > > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 30 > ++++++++++++++++++++++++++++++ > > 4 files changed, 84 insertions(+), 3 deletions(-) > > > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > > b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > > index b734a1ea8c..d3f62ed806 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > > @@ -316,3 +316,29 @@ SetPageTableAttributes ( > > > > return ; > > } > > + > > +/** > > + This function returns with no action for 32 bit. > > + > > + @param[out] *Cr2 Pointer to variable to hold CR2 register value > > +**/ VOID > > +SaveCr2 ( > > + UINTN *Cr2 > > + ) > > +{ > > + return ; > > +} > > + > > +/** > > + This function returns with no action for 32 bit. > > + > > + @param[in] Cr2 Value to write into CR2 register **/ VOID > > +RestoreCr2 ( > > + UINTN Cr2 > > + ) > > +{ > > + return ; > > +} > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > index 3b0b3b52ac..ce70f77709 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > @@ -1112,9 +1112,11 @@ SmiRendezvous ( > > ASSERT(CpuIndex < mMaxNumberOfCpus); > > > > // > > - // Save Cr2 because Page Fault exception in SMM may override its > > value > > + // Save Cr2 because Page Fault exception in SMM may override its > > + value, // when using on-demand paging for above 4G memory. > > // > > - Cr2 = AsmReadCr2 (); > > + Cr2 = 0; > > + SaveCr2 (&Cr2); > > > > // > > // Perform CPU specific entry hooks > > @@ -1253,10 +1255,11 @@ SmiRendezvous ( > > > > Exit: > > SmmCpuFeaturesRendezvousExit (CpuIndex); > > + > > // > > // Restore Cr2 > > // > > - AsmWriteCr2 (Cr2); > > + RestoreCr2 (Cr2); > > } > > > > /** > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > > index 84efb22981..05e1b54ed2 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > > @@ -1243,4 +1243,26 @@ EFIAPI > > PiSmmCpuSmiEntryFixupAddress ( > > ); > > > > +/** > > + This function reads CR2 register when on-demand paging is enabled > > + for 64 bit and no action for 32 bit. > > + > > + @param[out] *Cr2 Pointer to variable to hold CR2 register value > > +**/ VOID > > +SaveCr2 ( > > + UINTN *Cr2 > > + ); > > + > > +/** > > + This function writes into CR2 register when on-demand paging is > > +enabled > > + for 64 bit and no action for 32 bit. > > + > > + @param[in] Cr2 Value to write into CR2 register **/ VOID > > +RestoreCr2 ( > > + UINTN Cr2 > > + ); > > + > > #endif > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > > index 2c77cb47a4..e60628c080 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > > @@ -1053,3 +1053,33 @@ SetPageTableAttributes ( > > > > return ; > > } > > + > > +/** > > + This function reads CR2 register when on-demand paging is enabled > > + > > + @param[out] *Cr2 Pointer to variable to hold CR2 register value > > +**/ VOID > > +SaveCr2 ( > > + UINTN *Cr2 > > + ) > > +{ > > + if (!mCpuSmmStaticPageTable) { > > + *Cr2 = AsmReadCr2 (); > > + } > > +} > > + > > +/** > > + This function restores CR2 register when on-demand paging is > > +enabled > > + > > + @param[in] Cr2 Value to write into CR2 register **/ VOID > > +RestoreCr2 ( > > + UINTN Cr2 > > + ) > > +{ > > + if (!mCpuSmmStaticPageTable) { > > + AsmWriteCr2 (Cr2); > > + } > > +} > > -- > > 2.16.2.windows.1 > > > > _______________________________________________ > > edk2-devel mailing list > > mailto: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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v8] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM 2019-03-29 15:44 [PATCH v8] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM nkvangup 2019-03-29 21:22 ` Desimone, Nathaniel L @ 2019-03-29 22:03 ` Ni, Ray 1 sibling, 0 replies; 5+ messages in thread From: Ni, Ray @ 2019-03-29 22:03 UTC (permalink / raw) To: Vanguput, Narendra K Cc: edk2-devel@lists.01.org, Dong, Eric, Laszlo Ersek, Yao, Jiewen Reviewed-by: Ray Ni <Ray.ni@intel.com> Sent from small device > 在 2019年3月29日，下午11:45，Vanguput, Narendra K <narendra.k.vanguput@intel.com> 写道： > > 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. > This is not a bug but to have better improvement of code. > > Patch5 is updated with separate functions for Save and Restore of CR2 > based on review feedback. > > Patch6 - Removed Global Cr2 instead used function parameter > > Patch7 - Removed checking Cr2 with 0 as per feedback > > 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/Ia32/PageTbl.c | 26 ++++++++++++++++++++++++++ > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 9 ++++++--- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22 ++++++++++++++++++++++ > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 30 ++++++++++++++++++++++++++++++ > 4 files changed, 84 insertions(+), 3 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > index b734a1ea8c..d3f62ed806 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > @@ -316,3 +316,29 @@ SetPageTableAttributes ( > > return ; > } > + > +/** > + This function returns with no action for 32 bit. > + > + @param[out] *Cr2 Pointer to variable to hold CR2 register value > +**/ > +VOID > +SaveCr2 ( > + UINTN *Cr2 > + ) > +{ > + return ; > +} > + > +/** > + This function returns with no action for 32 bit. > + > + @param[in] Cr2 Value to write into CR2 register > +**/ > +VOID > +RestoreCr2 ( > + UINTN Cr2 > + ) > +{ > + return ; > +} > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > index 3b0b3b52ac..ce70f77709 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -1112,9 +1112,11 @@ SmiRendezvous ( > ASSERT(CpuIndex < mMaxNumberOfCpus); > > // > - // Save Cr2 because Page Fault exception in SMM may override its value > + // Save Cr2 because Page Fault exception in SMM may override its value, > + // when using on-demand paging for above 4G memory. > // > - Cr2 = AsmReadCr2 (); > + Cr2 = 0; > + SaveCr2 (&Cr2); > > // > // Perform CPU specific entry hooks > @@ -1253,10 +1255,11 @@ SmiRendezvous ( > > Exit: > SmmCpuFeaturesRendezvousExit (CpuIndex); > + > // > // Restore Cr2 > // > - AsmWriteCr2 (Cr2); > + RestoreCr2 (Cr2); > } > > /** > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index 84efb22981..05e1b54ed2 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -1243,4 +1243,26 @@ EFIAPI > PiSmmCpuSmiEntryFixupAddress ( > ); > > +/** > + This function reads CR2 register when on-demand paging is enabled > + for 64 bit and no action for 32 bit. > + > + @param[out] *Cr2 Pointer to variable to hold CR2 register value > +**/ > +VOID > +SaveCr2 ( > + UINTN *Cr2 > + ); > + > +/** > + This function writes into CR2 register when on-demand paging is enabled > + for 64 bit and no action for 32 bit. > + > + @param[in] Cr2 Value to write into CR2 register > +**/ > +VOID > +RestoreCr2 ( > + UINTN Cr2 > + ); > + > #endif > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > index 2c77cb47a4..e60628c080 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -1053,3 +1053,33 @@ SetPageTableAttributes ( > > return ; > } > + > +/** > + This function reads CR2 register when on-demand paging is enabled > + > + @param[out] *Cr2 Pointer to variable to hold CR2 register value > +**/ > +VOID > +SaveCr2 ( > + UINTN *Cr2 > + ) > +{ > + if (!mCpuSmmStaticPageTable) { > + *Cr2 = AsmReadCr2 (); > + } > +} > + > +/** > + This function restores CR2 register when on-demand paging is enabled > + > + @param[in] Cr2 Value to write into CR2 register > +**/ > +VOID > +RestoreCr2 ( > + UINTN Cr2 > + ) > +{ > + if (!mCpuSmmStaticPageTable) { > + AsmWriteCr2 (Cr2); > + } > +} > -- > 2.16.2.windows.1 > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-04-01 6:46 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-03-29 15:44 [PATCH v8] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM nkvangup 2019-03-29 21:22 ` Desimone, Nathaniel L 2019-03-29 21:37 ` Andrew Fish 2019-04-01 6:46 ` Vanguput, Narendra K 2019-03-29 22:03 ` Ni, Ray
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox