Thanks Mike for the comments! I updated and send as PATCH v6. Please review.
Thanks, Narendra > -----Original Message----- > From: Kinney, Michael D > Sent: Saturday, March 23, 2019 12:44 AM > To: 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>; > Laszlo Ersek <ler...@redhat.com> > Subject: RE: [edk2] [PATCH v5] UefiCpuPkg\CpuSmm: Save & restore CR2 on- > demand paging in SMM > > Hi Narendra, > > With this implementation, you have moved the save/restore location to a > module global variable. The name should be prefixed with 'm' to make that > clear. > > mCr2 > > I do not think using a module global is MP safe. > > The current implementation uses a local on the stack that is MP safe because > each CPU has its own stack. > > Mike > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] > > On Behalf Of nkvangup > > Sent: Friday, March 22, 2019 11:50 AM > > To: edk2-devel@lists.01.org > > Cc: Yao, Jiewen <jiewen....@intel.com>; Dong, Eric > > <eric.d...@intel.com>; Laszlo Ersek <ler...@redhat.com> > > Subject: [edk2] [PATCH v5] 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. > > > > 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/Ia32/PageTbl.c | 22 > > ++++++++++++++++++++++ > > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 9 +++++--- > > - > > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 16 > > ++++++++++++++++ > > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 28 > > ++++++++++++++++++++++++++++ > > 4 files changed, 71 insertions(+), 4 deletions(-) > > > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > > b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > > index b734a1ea8c..3750332ca8 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > > @@ -316,3 +316,25 @@ SetPageTableAttributes ( > > > > return ; > > } > > + > > +/** > > + This function returns with no action for 32 bit. > > +**/ > > +VOID > > +SaveCr2 ( > > + VOID > > + ) > > +{ > > +// Do Nothing > > +} > > + > > +/** > > + This function returns with no action for 32 bit. > > +**/ > > +VOID > > +RestoreCr2 ( > > + VOID > > + ) > > +{ > > +// Do Nothing > > +} > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > index 3b0b3b52ac..6a5736a3eb 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > @@ -1107,14 +1107,14 @@ SmiRendezvous ( > > BOOLEAN IsBsp; > > BOOLEAN BspInProgress; > > UINTN Index; > > - UINTN Cr2; > > > > 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 (); > > + SaveCr2 (); > > > > // > > // Perform CPU specific entry hooks @@ -1253,10 +1253,11 @@ > > SmiRendezvous ( > > > > Exit: > > SmmCpuFeaturesRendezvousExit (CpuIndex); > > + > > // > > // Restore Cr2 > > // > > - AsmWriteCr2 (Cr2); > > + RestoreCr2 (); > > } > > > > /** > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > > index 84efb22981..71a8c13960 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > > @@ -1243,4 +1243,20 @@ EFIAPI > > PiSmmCpuSmiEntryFixupAddress ( > > ); > > > > +/** > > + This function saves CR2 register for 64 bit and no > > action for 32 bit. > > +**/ > > +VOID > > +SaveCr2 ( > > + VOID > > + ); > > + > > +/** > > + This function restores CR2 register for 64 bit and no > > action for 32 bit. > > +**/ > > +VOID > > +RestoreCr2 ( > > + VOID > > + ); > > + > > #endif > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > > index 2c77cb47a4..76a30de171 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > > @@ -22,6 +22,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY > KIND, > > EITHER EXPRESS OR IMPLIED. > > LIST_ENTRY mPagePool = > > INITIALIZE_LIST_HEAD_VARIABLE (mPagePool); > > BOOLEAN m1GPageTableSupport > > = FALSE; > > BOOLEAN > > mCpuSmmStaticPageTable; > > +UINTN Cr2 = 0; > > > > /** > > Disable CET. > > @@ -1053,3 +1054,30 @@ SetPageTableAttributes ( > > > > return ; > > } > > + > > +/** > > + This function saves CR2 register. > > +**/ > > +VOID > > +SaveCr2 ( > > + VOID > > + ) > > +{ > > + if (!mCpuSmmStaticPageTable) { > > + Cr2 = AsmReadCr2 (); > > + } > > +} > > + > > +/** > > + This function restores CR2 register. > > +**/ > > +VOID > > +RestoreCr2 ( > > + VOID > > + ) > > +{ > > + if ((!mCpuSmmStaticPageTable) && (Cr2 != 0)) { > > + AsmWriteCr2 (Cr2); > > + Cr2 = 0; > > + } > > +} > > -- > > 2.16.2.windows.1 > > > > _______________________________________________ > > 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
