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

[Andrew Fish <afish@apple.com>]

> On Apr 2, 2019, at 8:18 PM, Dong, Eric <eric.dong@intel.com> wrote:
> 
> Hi Andrew,
>  
> I double confirmed in SDM, CR2 is not included in SMRAM State Save Map. Do you means we should add this info in the commit message?
>  

Eric,

Sorry I was confused by the commit message. I thought the state save was being added, vs. being removed in paths that don't modify CR2. 

I realize now that the fix did not end up in SmiRendezvous() like this. 

if (!mCpuSmmStaticPageTable) {
  Cr2 = AsmReadCr2 ();
}
...
if (!mCpuSmmStaticPageTable) {
  AsmWriteCr2 (Cr2);
}

As mCpuSmmStaticPageTable is local to X64/PageTbl.c

So we would have to do something like this to not have the functions. 
Ia32/PageTbl.c
mCpuSmmStaticPageTable = FALSE;

Thus "This is not a bug but to have better improvement of code." actually means don't save CR2 if it is not modified in SMM context vs. unconditionally saving it. 

Sorry I have a high error rate on text diffs. In my day job I always use a difftool or grab the entire branch. 

Thanks,

Andrew Fish


> Thanks
> Eric
> From: afish@apple.com [mailto:afish@apple.com] 
> Sent: Tuesday, April 2, 2019 1:01 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Vanguput, Narendra K <narendra.k.vanguput@intel.com>; edk2-devel <edk2-devel@lists.01.org>; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH v9] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM
>  
>  
> 
> 
> On Apr 1, 2019, at 9:47 AM, Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>> wrote:
>  
> On 04/01/19 10:16, nkvangup wrote:
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1593 <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.
> 
> Patch8 and 9 - Aligned with EDK2 Coding style.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Vanguput Narendra K <narendra.k.vanguput@intel.com <mailto:narendra.k.vanguput@intel.com>>
> Cc: Eric Dong <eric.dong@intel.com <mailto:eric.dong@intel.com>>
> Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
> Cc: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> Cc: Yao Jiewen <jiewen.yao@intel.com <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..d1e146a70c 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 (
> +  OUT UINTN  *Cr2
> +  )
> +{
> +  return ;
> +}
> +
> +/**
> +  This function returns with no action for 32 bit.
> +
> +  @param[in]  Cr2  Value to write into CR2 register.
> +**/
> +VOID
> +RestoreCr2 (
> +  IN 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..38f9104117 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 (
> +  OUT 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 (
> +  IN UINTN  Cr2
> +  );
> +
> #endif
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 2c77cb47a4..95eaf0b016 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 (
> +  OUT 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 (
> +  IN UINTN  Cr2
> +  )
> +{
> +  if (!mCpuSmmStaticPageTable) {
> +    AsmWriteCr2 (Cr2);
> +  }
> +}
> 
> 
> I agree *how* this patch is implemented is correct, wrt. the IA32 / X64
> split.
> 
> A slight improvement for edk2 coding style would be to replace "*Cr2"
> with just "Cr2" in the @param[out] comments, but there's no need to
> repost the patch just because of that.
> 
> Regarding the "what" and "why", Nate's and Andrew's comments under v8
> make me uncomfortable about the patch. While the pre-patch comments do say
> 
>  Save Cr2 because Page Fault exception in SMM may override its value
> 
> the post-patch comment (and code) are more restricted -- they claim that
> such an exception (from which we return, anyway) may only occur when
> on-demand paging is enabled (which is in turn a pre-requisite to both
> the SMM profile feature and the SMM heap guard feature).
> 
> It is this "narrowing" that concerns me (i.e. the claim that a page
> fault that we consider "expected", and return from, may only occur due
> to enabling on-demand paging). It *seems* like a correct statement, but
> I'd like other reviewers to prove (or disprove) it; so I will not give
> either A-b or R-b.
> 
>  
> Laszlo,
>  
> My understanding for SMM for X64 there are 2 options page tables from 0 - 4 GB + making page table entries on page faults, and a pure identity mapped page table. This behavior is controlled by a PCD setting. So that part of this patch makes sense to me. 
>  
> As I mentioned if the non SMM ring 0 CR2 is getting changed that seems like a bug to me. If the state save of CR2 is some internal state in SMM it feels like that should be better documented in the patch?
>  
> Thanks,
>  
> Andrew Fish
> 
> 
> On the testing front, I confirm the patch doesn't regress OVMF. (OVMF
> has on-demand paging *disabled* -- it uses static page tables in X64 SMM
> --, so there the patch removes the CR2 save/restore, on both IA32 and X64.)
> 
> Regression-tested-by: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>