public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v5] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM
@ 2019-03-22 18:49 nkvangup
  2019-03-22 19:13 ` Kinney, Michael D
  0 siblings, 1 reply; 3+ messages in thread
From: nkvangup @ 2019-03-22 18:49 UTC (permalink / raw)
  To: edk2-devel
  Cc: Vanguput Narendra K, Eric Dong, Ray Ni, Laszlo Ersek, Yao Jiewen

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.

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   | 22 ++++++++++++++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c      |  9 +++++----
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 16 ++++++++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c    | 28 ++++++++++++++++++++++++++++
 4 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index b734a1ea8c..3750332ca8 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -316,3 +316,25 @@ SetPageTableAttributes (
 
   return ;
 }
+
+/**
+  This function returns with no action for 32 bit.
+**/
+VOID
+SaveCr2 (
+  VOID
+  )
+{
+// Do Nothing
+}
+
+/**
+  This function returns with no action for 32 bit.
+**/
+VOID
+RestoreCr2 (
+  VOID
+  )
+{
+// Do Nothing
+}
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 3b0b3b52ac..6a5736a3eb 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1107,14 +1107,14 @@ SmiRendezvous (
   BOOLEAN                        IsBsp;
   BOOLEAN                        BspInProgress;
   UINTN                          Index;
-  UINTN                          Cr2;
 
   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 ();
+  SaveCr2 ();
 
   //
   // Perform CPU specific entry hooks
@@ -1253,10 +1253,11 @@ SmiRendezvous (
 
 Exit:
   SmmCpuFeaturesRendezvousExit (CpuIndex);
+
   //
   // Restore Cr2
   //
-  AsmWriteCr2 (Cr2);
+  RestoreCr2 ();
 }
 
 /**
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 84efb22981..71a8c13960 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -1243,4 +1243,20 @@ EFIAPI
 PiSmmCpuSmiEntryFixupAddress (
  );
 
+/**
+  This function saves CR2 register for 64 bit and no action for 32 bit.
+**/
+VOID
+SaveCr2 (
+  VOID
+  );
+
+/**
+  This function restores CR2 register for 64 bit and no action for 32 bit.
+**/
+VOID
+RestoreCr2 (
+  VOID
+  );
+
 #endif
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index 2c77cb47a4..76a30de171 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -22,6 +22,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 LIST_ENTRY                          mPagePool = INITIALIZE_LIST_HEAD_VARIABLE (mPagePool);
 BOOLEAN                             m1GPageTableSupport = FALSE;
 BOOLEAN                             mCpuSmmStaticPageTable;
+UINTN                               Cr2 = 0;
 
 /**
   Disable CET.
@@ -1053,3 +1054,30 @@ SetPageTableAttributes (
 
   return ;
 }
+
+/**
+  This function saves CR2 register.
+**/
+VOID
+SaveCr2 (
+  VOID
+  )
+{
+  if (!mCpuSmmStaticPageTable) {
+    Cr2 = AsmReadCr2 ();
+  }
+}
+
+/**
+  This function restores CR2 register.
+**/
+VOID
+RestoreCr2 (
+  VOID
+  )
+{
+  if ((!mCpuSmmStaticPageTable) && (Cr2 != 0)) {
+    AsmWriteCr2 (Cr2);
+    Cr2 = 0;
+  }
+}
-- 
2.16.2.windows.1



^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v5] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM
  2019-03-22 18:49 [PATCH v5] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM nkvangup
@ 2019-03-22 19:13 ` Kinney, Michael D
  2019-03-29  5:03   ` Vanguput, Narendra K
  0 siblings, 1 reply; 3+ messages in thread
From: Kinney, Michael D @ 2019-03-22 19:13 UTC (permalink / raw)
  To: Vanguput, Narendra K, edk2-devel@lists.01.org, Kinney, Michael D
  Cc: Yao, Jiewen, Dong, Eric, Laszlo Ersek

Hi Narendra,

With this implementation, you have moved the save/restore
location to a module global variable.  The name should be
prefixed with 'm' to make that clear.  

	mCr2

I do not think using a module global is MP safe.

The current implementation uses a local on the stack that
is MP safe because each CPU has its own stack.

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
> On Behalf Of nkvangup
> Sent: Friday, March 22, 2019 11:50 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 v5] 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.
> 
> 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   | 22
> ++++++++++++++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c      |  9 +++++---
> -
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 16
> ++++++++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c    | 28
> ++++++++++++++++++++++++++++
>  4 files changed, 71 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index b734a1ea8c..3750332ca8 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -316,3 +316,25 @@ SetPageTableAttributes (
> 
>    return ;
>  }
> +
> +/**
> +  This function returns with no action for 32 bit.
> +**/
> +VOID
> +SaveCr2 (
> +  VOID
> +  )
> +{
> +// Do Nothing
> +}
> +
> +/**
> +  This function returns with no action for 32 bit.
> +**/
> +VOID
> +RestoreCr2 (
> +  VOID
> +  )
> +{
> +// Do Nothing
> +}
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 3b0b3b52ac..6a5736a3eb 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1107,14 +1107,14 @@ SmiRendezvous (
>    BOOLEAN                        IsBsp;
>    BOOLEAN                        BspInProgress;
>    UINTN                          Index;
> -  UINTN                          Cr2;
> 
>    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 ();
> +  SaveCr2 ();
> 
>    //
>    // Perform CPU specific entry hooks
> @@ -1253,10 +1253,11 @@ SmiRendezvous (
> 
>  Exit:
>    SmmCpuFeaturesRendezvousExit (CpuIndex);
> +
>    //
>    // Restore Cr2
>    //
> -  AsmWriteCr2 (Cr2);
> +  RestoreCr2 ();
>  }
> 
>  /**
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 84efb22981..71a8c13960 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1243,4 +1243,20 @@ EFIAPI
>  PiSmmCpuSmiEntryFixupAddress (
>   );
> 
> +/**
> +  This function saves CR2 register for 64 bit and no
> action for 32 bit.
> +**/
> +VOID
> +SaveCr2 (
> +  VOID
> +  );
> +
> +/**
> +  This function restores CR2 register for 64 bit and no
> action for 32 bit.
> +**/
> +VOID
> +RestoreCr2 (
> +  VOID
> +  );
> +
>  #endif
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 2c77cb47a4..76a30de171 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -22,6 +22,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS
> OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  LIST_ENTRY                          mPagePool =
> INITIALIZE_LIST_HEAD_VARIABLE (mPagePool);
>  BOOLEAN                             m1GPageTableSupport
> = FALSE;
>  BOOLEAN
> mCpuSmmStaticPageTable;
> +UINTN                               Cr2 = 0;
> 
>  /**
>    Disable CET.
> @@ -1053,3 +1054,30 @@ SetPageTableAttributes (
> 
>    return ;
>  }
> +
> +/**
> +  This function saves CR2 register.
> +**/
> +VOID
> +SaveCr2 (
> +  VOID
> +  )
> +{
> +  if (!mCpuSmmStaticPageTable) {
> +    Cr2 = AsmReadCr2 ();
> +  }
> +}
> +
> +/**
> +  This function restores CR2 register.
> +**/
> +VOID
> +RestoreCr2 (
> +  VOID
> +  )
> +{
> +  if ((!mCpuSmmStaticPageTable) && (Cr2 != 0)) {
> +    AsmWriteCr2 (Cr2);
> +    Cr2 = 0;
> +  }
> +}
> --
> 2.16.2.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH v5] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM
  2019-03-22 19:13 ` Kinney, Michael D
@ 2019-03-29  5:03   ` Vanguput, Narendra K
  0 siblings, 0 replies; 3+ messages in thread
From: Vanguput, Narendra K @ 2019-03-29  5:03 UTC (permalink / raw)
  To: Kinney, Michael D, edk2-devel@lists.01.org
  Cc: Yao, Jiewen, Dong, Eric, Laszlo Ersek, Chinnusamy, Rajkumar K

Thanks Mike for the comments!

I updated and send as PATCH v6.
Please review.

Thanks,
Narendra

> -----Original Message-----
> From: Kinney, Michael D
> Sent: Saturday, March 23, 2019 12:44 AM
> To: Vanguput, Narendra K <narendra.k.vanguput@intel.com>; edk2-
> devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [edk2] [PATCH v5] UefiCpuPkg\CpuSmm: Save & restore CR2 on-
> demand paging in SMM
> 
> Hi Narendra,
> 
> With this implementation, you have moved the save/restore location to a
> module global variable.  The name should be prefixed with 'm' to make that
> clear.
> 
> 	mCr2
> 
> I do not think using a module global is MP safe.
> 
> The current implementation uses a local on the stack that is MP safe because
> each CPU has its own stack.
> 
> Mike
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
> > On Behalf Of nkvangup
> > Sent: Friday, March 22, 2019 11:50 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 v5] 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.
> >
> > 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   | 22
> > ++++++++++++++++++++++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c      |  9 +++++---
> > -
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 16
> > ++++++++++++++++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c    | 28
> > ++++++++++++++++++++++++++++
> >  4 files changed, 71 insertions(+), 4 deletions(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > index b734a1ea8c..3750332ca8 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > @@ -316,3 +316,25 @@ SetPageTableAttributes (
> >
> >    return ;
> >  }
> > +
> > +/**
> > +  This function returns with no action for 32 bit.
> > +**/
> > +VOID
> > +SaveCr2 (
> > +  VOID
> > +  )
> > +{
> > +// Do Nothing
> > +}
> > +
> > +/**
> > +  This function returns with no action for 32 bit.
> > +**/
> > +VOID
> > +RestoreCr2 (
> > +  VOID
> > +  )
> > +{
> > +// Do Nothing
> > +}
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > index 3b0b3b52ac..6a5736a3eb 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > @@ -1107,14 +1107,14 @@ SmiRendezvous (
> >    BOOLEAN                        IsBsp;
> >    BOOLEAN                        BspInProgress;
> >    UINTN                          Index;
> > -  UINTN                          Cr2;
> >
> >    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 ();
> > +  SaveCr2 ();
> >
> >    //
> >    // Perform CPU specific entry hooks @@ -1253,10 +1253,11 @@
> > SmiRendezvous (
> >
> >  Exit:
> >    SmmCpuFeaturesRendezvousExit (CpuIndex);
> > +
> >    //
> >    // Restore Cr2
> >    //
> > -  AsmWriteCr2 (Cr2);
> > +  RestoreCr2 ();
> >  }
> >
> >  /**
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > index 84efb22981..71a8c13960 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > @@ -1243,4 +1243,20 @@ EFIAPI
> >  PiSmmCpuSmiEntryFixupAddress (
> >   );
> >
> > +/**
> > +  This function saves CR2 register for 64 bit and no
> > action for 32 bit.
> > +**/
> > +VOID
> > +SaveCr2 (
> > +  VOID
> > +  );
> > +
> > +/**
> > +  This function restores CR2 register for 64 bit and no
> > action for 32 bit.
> > +**/
> > +VOID
> > +RestoreCr2 (
> > +  VOID
> > +  );
> > +
> >  #endif
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > index 2c77cb47a4..76a30de171 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > @@ -22,6 +22,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND,
> > EITHER EXPRESS OR IMPLIED.
> >  LIST_ENTRY                          mPagePool =
> > INITIALIZE_LIST_HEAD_VARIABLE (mPagePool);
> >  BOOLEAN                             m1GPageTableSupport
> > = FALSE;
> >  BOOLEAN
> > mCpuSmmStaticPageTable;
> > +UINTN                               Cr2 = 0;
> >
> >  /**
> >    Disable CET.
> > @@ -1053,3 +1054,30 @@ SetPageTableAttributes (
> >
> >    return ;
> >  }
> > +
> > +/**
> > +  This function saves CR2 register.
> > +**/
> > +VOID
> > +SaveCr2 (
> > +  VOID
> > +  )
> > +{
> > +  if (!mCpuSmmStaticPageTable) {
> > +    Cr2 = AsmReadCr2 ();
> > +  }
> > +}
> > +
> > +/**
> > +  This function restores CR2 register.
> > +**/
> > +VOID
> > +RestoreCr2 (
> > +  VOID
> > +  )
> > +{
> > +  if ((!mCpuSmmStaticPageTable) && (Cr2 != 0)) {
> > +    AsmWriteCr2 (Cr2);
> > +    Cr2 = 0;
> > +  }
> > +}
> > --
> > 2.16.2.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-03-29  5:03 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-22 18:49 [PATCH v5] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM nkvangup
2019-03-22 19:13 ` Kinney, Michael D
2019-03-29  5:03   ` Vanguput, Narendra K

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox