1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH 0/3] Allow SMM access-out when static paging is OFF
  4. View patch

[edk2-devel] [PATCH 2/3] UefiCpuPkg/PiSmmCpu: Refine CR2 save/restore logic

Ni, Ray posted 3 patches 6 years, 6 months ago
[edk2-devel] [PATCH 2/3] UefiCpuPkg/PiSmmCpu: Refine CR2 save/restore logic
Posted by Ni, Ray 6 years, 6 months ago 
Because IsStaticPageTableEnabled() is added for both IA32 and x64
build, the CR2 save/restore logic can be refined:
1. Remove arch specific SaveCr2() /  RestoreCr2() implementation;
2. Conditionally save and restore CR2 in SmiRendezvous().

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Vanguput Narendra K <narendra.k.vanguput@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c   | 25 -------------------
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c      |  8 ++++--
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22 ----------------
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c    | 29 ----------------------
 4 files changed, 6 insertions(+), 78 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index 2a9af4b77d..cae23d6d1d 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -327,28 +327,3 @@ 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 ef16997547..5d0124b368 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1575,7 +1575,9 @@ SmiRendezvous (
   // when using on-demand paging for above 4G memory.
   //
   Cr2 = 0;
-  SaveCr2 (&Cr2);
+  if (!IsStaticPageTableEnabled ()) {
+    Cr2 = AsmReadCr2 ();
+  }
 
   //
   // Call the user register Startup function first.
@@ -1725,7 +1727,9 @@ Exit:
   //
   // Restore Cr2
   //
-  RestoreCr2 (Cr2);
+  if (!IsStaticPageTableEnabled ()) {
+    AsmWriteCr2 (Cr2);
+  }
 }
 
 /**
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 14b7676c16..5a97733def 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -1282,28 +1282,6 @@ IsStaticPageTableEnabled (
   )
 ;
 
-/**
-  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
-  );
-
 /**
   Schedule a procedure to run on the specified CPU.
 
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index 18e3f9e08d..8259b01a95 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -1209,32 +1209,3 @@ 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.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44469): https://edk2.groups.io/g/devel/message/44469
Mute This Topic: https://groups.io/mt/32616002/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/PiSmmCpu: Refine CR2 save/restore logic
Posted by Dong, Eric 6 years, 6 months ago 
Reviewed-by: Eric Dong <eric.dong@intel.com>

> -----Original Message-----
> From: Ni, Ray
> Sent: Saturday, July 27, 2019 11:29 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Vanguput, Narendra K <narendra.k.vanguput@intel.com>
> Subject: [PATCH 2/3] UefiCpuPkg/PiSmmCpu: Refine CR2 save/restore logic
> 
> Because IsStaticPageTableEnabled() is added for both IA32 and x64 build, the
> CR2 save/restore logic can be refined:
> 1. Remove arch specific SaveCr2() /  RestoreCr2() implementation; 2.
> Conditionally save and restore CR2 in SmiRendezvous().
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Vanguput Narendra K <narendra.k.vanguput@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c   | 25 -------------------
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c      |  8 ++++--
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22 ----------------
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c    | 29 ----------------------
>  4 files changed, 6 insertions(+), 78 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index 2a9af4b77d..cae23d6d1d 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -327,28 +327,3 @@ 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 ef16997547..5d0124b368 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1575,7 +1575,9 @@ SmiRendezvous (
>    // when using on-demand paging for above 4G memory.
>    //
>    Cr2 = 0;
> -  SaveCr2 (&Cr2);
> +  if (!IsStaticPageTableEnabled ()) {
> +    Cr2 = AsmReadCr2 ();
> +  }
> 
>    //
>    // Call the user register Startup function first.
> @@ -1725,7 +1727,9 @@ Exit:
>    //
>    // Restore Cr2
>    //
> -  RestoreCr2 (Cr2);
> +  if (!IsStaticPageTableEnabled ()) {
> +    AsmWriteCr2 (Cr2);
> +  }
>  }
> 
>  /**
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 14b7676c16..5a97733def 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1282,28 +1282,6 @@ IsStaticPageTableEnabled (
>    )
>  ;
> 
> -/**
> -  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
> -  );
> -
>  /**
>    Schedule a procedure to run on the specified CPU.
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 18e3f9e08d..8259b01a95 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -1209,32 +1209,3 @@ 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.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44520): https://edk2.groups.io/g/devel/message/44520
Mute This Topic: https://groups.io/mt/32616002/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/PiSmmCpu: Refine CR2 save/restore logic
Posted by Laszlo Ersek 6 years, 6 months ago 
On 07/27/19 05:28, Ni, Ray wrote:
> Because IsStaticPageTableEnabled() is added for both IA32 and x64
> build, the CR2 save/restore logic can be refined:
> 1. Remove arch specific SaveCr2() /  RestoreCr2() implementation;
> 2. Conditionally save and restore CR2 in SmiRendezvous().
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Vanguput Narendra K <narendra.k.vanguput@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c   | 25 -------------------
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c      |  8 ++++--
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22 ----------------
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c    | 29 ----------------------
>  4 files changed, 6 insertions(+), 78 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index 2a9af4b77d..cae23d6d1d 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -327,28 +327,3 @@ 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 ef16997547..5d0124b368 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1575,7 +1575,9 @@ SmiRendezvous (
>    // when using on-demand paging for above 4G memory.
>    //
>    Cr2 = 0;
> -  SaveCr2 (&Cr2);
> +  if (!IsStaticPageTableEnabled ()) {
> +    Cr2 = AsmReadCr2 ();
> +  }
>  
>    //
>    // Call the user register Startup function first.

So, because this patch is supposed to only refactor / simplify the code,
it should not change behavior.

But, because in patch#1 we return FALSE for IA32, the condition above
will evaluate to TRUE. And so we will massage CR2 (= fault address),
even though the IA32 build shouldn't do that (and doesn't do it, at the
moment).

This should be fixed by returning constant TRUE from
IsStaticPageTableEnabled(), in patch#1, on IA32.

(Note: in the message of commit d47b85a621ad ("Revert
"UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF"",
2019-07-26), I wrote that "The IA32 implementation should return a
constant value". I didn't say either "constant TRUE" or "constant
FALSE". And that's because I couldn't know the right value, without
actually looking at the code. Determining the correct IA32 value was out
of scope for the revert.)

More below:

> @@ -1725,7 +1727,9 @@ Exit:
>    //
>    // Restore Cr2
>    //
> -  RestoreCr2 (Cr2);
> +  if (!IsStaticPageTableEnabled ()) {
> +    AsmWriteCr2 (Cr2);
> +  }
>  }
>  
>  /**
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 14b7676c16..5a97733def 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1282,28 +1282,6 @@ IsStaticPageTableEnabled (
>    )
>  ;
>  
> -/**
> -  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
> -  );
> -
>  /**
>    Schedule a procedure to run on the specified CPU.
>  
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 18e3f9e08d..8259b01a95 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -1209,32 +1209,3 @@ 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);
> -  }
> -}
> 

For this patch:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44516): https://edk2.groups.io/g/devel/message/44516
Mute This Topic: https://groups.io/mt/32616002/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.