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

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

[nkvangup]

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);
+  }
+}
-- 
2.16.2.windows.1

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

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of nkvangup
Sent: Monday, April 1, 2019 4:16 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 v9] 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.

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);
+  }
+}
--
2.16.2.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Laszlo Ersek]

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.

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>

Thanks
Laszlo

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

[Andrew Fish]

> 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>

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

[Desimone, Nathaniel L]

This patch seems to only add the IN/OUT decorators on function parameters, which is a good change. However, it does not address any of my previous comments:

1. Why would you do this for 64 bit but not 32 bit?
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.

I also share Andrew's concern that in the case of a periodic SMI happening during OS runtime, there is nothing preventing the handler of the periodic SMI from clobbering the value of CR2, which could potentially cause kernel panics once we return back from SMM to the OS. I am not aware of any periodic SMIs in OVMF, so I don't believe OVMF testing will catch these type of issues. I consider not doing the save/restore in the 32 bit SMM to be dangerous, especially since all recent platforms that I can think of don't use 32 bit SMM anymore, so any bug(s) introduced may go unnoticed for a long time.

Thanks,
Nate

-----Original Message-----
From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of nkvangup
Sent: Monday, April 1, 2019 1:16 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 v9] 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.

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);
+  }
+}
--
2.16.2.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Vanguput, Narendra K]

Hi Nate,

The 'PcdCpuSmmStaticPageTable' is only used in the X64 version of PageTbl.c  that's why I updated only for 64 bit. SMM always builds static page table for IA32.
Please refer my previous mail for more details.

Thanks,
Narendra

> -----Original Message-----
> From: Desimone, Nathaniel L
> Sent: Tuesday, April 2, 2019 5:57 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 v9] UefiCpuPkg\CpuSmm: Save & restore CR2 on-
> demand paging in SMM
> 
> This patch seems to only add the IN/OUT decorators on function parameters,
> which is a good change. However, it does not address any of my previous
> comments:
> 
> 1. Why would you do this for 64 bit but not 32 bit?
> 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.
> 
> I also share Andrew's concern that in the case of a periodic SMI happening
> during OS runtime, there is nothing preventing the handler of the periodic
> SMI from clobbering the value of CR2, which could potentially cause kernel
> panics once we return back from SMM to the OS. I am not aware of any
> periodic SMIs in OVMF, so I don't believe OVMF testing will catch these type
> of issues. I consider not doing the save/restore in the 32 bit SMM to be
> dangerous, especially since all recent platforms that I can think of don't use 32
> bit SMM anymore, so any bug(s) introduced may go unnoticed for a long time.
> 
> Thanks,
> Nate
> 
> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of nkvangup
> Sent: Monday, April 1, 2019 1:16 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 v9] 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.
> 
> 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);
> +  }
> +}
> --
> 2.16.2.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Desimone, Nathaniel L]

Hi Narendra,

I see that now, thank you. I now understand your patch.

Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>

-----Original Message-----
From: Vanguput, Narendra K 
Sent: Monday, April 1, 2019 7:32 PM
To: Desimone, Nathaniel L <nathaniel.l.desimone@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 v9] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM

Hi Nate,

The 'PcdCpuSmmStaticPageTable' is only used in the X64 version of PageTbl.c  that's why I updated only for 64 bit. SMM always builds static page table for IA32.
Please refer my previous mail for more details.

Thanks,
Narendra

> -----Original Message-----
> From: Desimone, Nathaniel L
> Sent: Tuesday, April 2, 2019 5:57 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 v9] UefiCpuPkg\CpuSmm: Save & restore CR2 
> on- demand paging in SMM
> 
> This patch seems to only add the IN/OUT decorators on function 
> parameters, which is a good change. However, it does not address any 
> of my previous
> comments:
> 
> 1. Why would you do this for 64 bit but not 32 bit?
> 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.
> 
> I also share Andrew's concern that in the case of a periodic SMI 
> happening during OS runtime, there is nothing preventing the handler 
> of the periodic SMI from clobbering the value of CR2, which could 
> potentially cause kernel panics once we return back from SMM to the 
> OS. I am not aware of any periodic SMIs in OVMF, so I don't believe 
> OVMF testing will catch these type of issues. I consider not doing the 
> save/restore in the 32 bit SMM to be dangerous, especially since all 
> recent platforms that I can think of don't use 32 bit SMM anymore, so any bug(s) introduced may go unnoticed for a long time.
> 
> Thanks,
> Nate
> 
> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of 
> nkvangup
> Sent: Monday, April 1, 2019 1:16 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 v9] 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.
> 
> 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);
> +  }
> +}
> --
> 2.16.2.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Dong, Eric]

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?

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

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

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

[Andrew Fish]

> 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>