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

["Vanguput, Narendra K" <narendra.k.vanguput@intel.com>]

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