Hi Naren, I agree with Laszlo's comment for #1. I think separate functions for IA32/X64 are much clear than the current one. I think in current EDK2 codebase, many similar cases already exits.
Thanks, Eric > -----Original Message----- > From: Vanguput, Narendra K > Sent: Thursday, March 21, 2019 1:28 AM > To: Laszlo Ersek <ler...@redhat.com>; edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen....@intel.com>; Dong, Eric <eric.d...@intel.com>; > Chinnusamy, Rajkumar K <rajkumar.k.chinnus...@intel.com>; Ni, Ray > <ray...@intel.com> > Subject: RE: [edk2] [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on- > demand paging in SMM > > Hi Laszlo, > > Thanks for your comments. > > For your comment #1, My thoughts are when we add two functions (SaveCr2 > & RestoreCr2). For IA32, it actually don't save and restore, simply returns. > Later, it might be confusing unless if we know the background and gone > through 64 bit supported code. And also its kind of adding more code while > we have alternate solution. > In the proposed changes, I felt its straight forward and light changes needed. > Yes, I would like to hear from other reviewers too to take the right option. > > For comments #2 & #4, Yes, I notified it, waiting to update along with other > comments. > > For comments #3 & #5, will consider them. Will adjust the no. characters and > will move extern of mCpuSmmStaticPageTable to PiSmmCpuDxeSmm.h file. > > Thanks, > Naren > > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Wednesday, March 20, 2019 10:01 PM > 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 v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on- > demand paging in SMM > > On 03/18/19 15:38, 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 | 22 ++++++++++++++----- > --- > > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 2 +- > > 2 files changed, 15 insertions(+), 9 deletions(-) > > > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > index 3b0b3b52ac..0c07b31c4f 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > @@ -28,6 +28,7 @@ UINTN > > mSemaphoreSize; > > SPIN_LOCK *mPFLock = NULL; > > SMM_CPU_SYNC_MODE mCpuSmmSyncMode; > > BOOLEAN mMachineCheckSupported = FALSE; > > +BOOLEAN mCpuSmmStaticPageTable = TRUE; > > Hmmm. This change is a bit daring, but I think it could be valid. > > - In the IA32 build, mCpuSmmStaticPageTable would never be modified, or > read, by *preexistent* code (because all that code is in X64/PageTbl.c). > And the new code, added by this patch, would (presumably) work fine, with > the initial TRUE value. > > - In the X64 build, the preexistent code would never read the initial value > (which we now set to TRUE here), i.e. before overwriting the variable from > the PCD -- because that would mean a bug in the preexistent code. (Well, > unless that code relied on the zero initial value of the variable). > > (1) I think I'd like to defer on this to other UefiCpuPkg reviewers. > Honestly I find this style questionable. It makes me feel uncomfortable. > I'd prefer the new APIs with the separate IA32/X64 implementations that I > suggested in my v2 review. But if other reviewers like this one better, I > won't > mind. > > (After hearing their opinions, I'd attempt to find the time to regression test > the patch (or maybe v5), too.) > > Assuming other reviewers prefer this approach over my suggestion, I have > some other comments: > > > > > /** > > Performs an atomic compare exchange operation to get semaphore. > > @@ -1111,10 +1112,13 @@ SmiRendezvous ( > > > > ASSERT(CpuIndex < mMaxNumberOfCpus); > > > > - // > > - // Save Cr2 because Page Fault exception in SMM may override its > > value > > - // > > - Cr2 = AsmReadCr2 (); > > + if (!mCpuSmmStaticPageTable) { > > + // > > + // Save and restore Cr2 when using on-demand paging for above 4G > memory because Page Fault > > + // exception in SMM may override its value > > + // > > + Cr2 = AsmReadCr2 (); > > + } > > (2) The indentation of the "if" is broken. > > (3) Given that we're already using two comment lines, I'd suggest not > exceeding 80 characters per line. > > > > > // > > // Perform CPU specific entry hooks @@ -1253,10 +1257,12 @@ > > SmiRendezvous ( > > > > Exit: > > SmmCpuFeaturesRendezvousExit (CpuIndex); > > - // > > - // Restore Cr2 > > - // > > - AsmWriteCr2 (Cr2); > > + if (!mCpuSmmStaticPageTable) { > > (4) same as (2). > > > + // > > + // Restore Cr2 > > + // > > + AsmWriteCr2 (Cr2); > > + } > > } > > > > /** > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > > index 2c77cb47a4..e444b8a031 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > > @@ -21,7 +21,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; > > +extern BOOLEAN mCpuSmmStaticPageTable; > > (5) This is generally not great style, and it conflicts with the existent > code of > this driver. Namely, declarations of variables with file scope, static storage > duration, and external linkage, should go into "PiSmmCpuDxeSmm.h"-- we > already got a bunch of them there. > > Thanks > Laszlo > > > > > /** > > Disable CET. > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
