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

[Andrew Fish <afish@apple.com>]

> On Apr 1, 2019, at 9:47 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 04/01/19 10:16, 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.
>> 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>
>> 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..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>