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

["Ni, Ray" <ray.ni@intel.com>]

Yes. With below "Cr2 !=0" removed from final commit, Reviewed-by: Ray Ni <ray.ni@intel.com>
> > +RestoreCr2 (
> > +  UINTN  Cr2
> > +  )
> > +{
> > +  if ((!mCpuSmmStaticPageTable) && (Cr2 != 0)) {
> > +    AsmWriteCr2 (Cr2);
> > +  }
> > +}

> -----Original Message-----
> From: Vanguput, Narendra K <narendra.k.vanguput@intel.com>
> Sent: Friday, March 29, 2019 5:32 PM
> To: Ni, Ray <ray.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [edk2] [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-
> demand paging in SMM
> 
> Hi Ray,
> 
> While programming, I thought this cannot be 0 as in SDM, it says page-fault
> linear address.
> >> CR2 - Contains the page-fault linear address (the linear address that
> caused a page fault).
> 
> So added a check for 0.
> 
> Now as the function is changed like program into CR2 register based on input
> parameter and checking for 0 is up to caller of this function. And also we
> don't need to require for checking 0 means, will remove it. Please confirm.
> 
> Thanks,
> Naren
> 
> -----Original Message-----
> From: Ni, Ray
> Sent: Friday, March 29, 2019 10:40 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>;
> Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [edk2] [PATCH v6] UefiCpuPkg\CpuSmm: Save & restore CR2 on-
> demand paging in SMM
> 
> > +  if ((!mCpuSmmStaticPageTable) && (Cr2 != 0)) {
> 
> Can the "Cr2 != 0" be removed?
> 
> > -----Original Message-----
> > From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of
> > nkvangup
> > Sent: Friday, March 29, 2019 12:58 PM
> > 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 v6] 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.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..af96e42982 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[in]  *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..c9d147c8a1 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[in]  *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..6cb44fbbe5 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[in]  *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) && (Cr2 != 0)) {
> > +    AsmWriteCr2 (Cr2);
> > +  }
> > +}
> > --
> > 2.16.2.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel