Re: [PATCH v4] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

["Dong, Eric" <eric.dong@intel.com>]

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 <lersek@redhat.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Chinnusamy, Rajkumar K <rajkumar.k.chinnusamy@intel.com>; Ni, Ray
> <ray.ni@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:lersek@redhat.com]
> Sent: Wednesday, March 20, 2019 10:01 PM
> 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 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.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   | 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.
> >