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

[PATCH v2] 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.

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/MpService.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 3b0b3b52ac..5be4a2b020 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1111,10 +1111,12 @@ SmiRendezvous (
 
   ASSERT(CpuIndex < mMaxNumberOfCpus);
 
-  //
-  // Save Cr2 because Page Fault exception in SMM may override its value
-  //
-  Cr2 = AsmReadCr2 ();
+  if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool (PcdCpuSmmStaticPageTable))) {
+    //
+    // Save Cr2 because Page Fault exception in SMM may override its value
+    //
+    Cr2 = AsmReadCr2 ();
+  }
 
   //
   // Perform CPU specific entry hooks
@@ -1253,10 +1255,12 @@ SmiRendezvous (
 
 Exit:
   SmmCpuFeaturesRendezvousExit (CpuIndex);
-  //
-  // Restore Cr2
-  //
-  AsmWriteCr2 (Cr2);
+  if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool (PcdCpuSmmStaticPageTable))) {
+    //
+    // Restore Cr2
+    //
+    AsmWriteCr2 (Cr2);
+  }
 }
 
 /**
-- 
2.16.2.windows.1

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

[Yao, Jiewen]

Reviewed-by: Jiewen.yao@intel.com

> -----Original Message-----
> From: Vanguput, Narendra K
> Sent: Thursday, March 7, 2019 3:15 AM
> To: edk2-devel@lists.01.org
> Cc: Vanguput, Narendra K <narendra.k.vanguput@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH v2] 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.
> 
> 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/MpService.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 3b0b3b52ac..5be4a2b020 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1111,10 +1111,12 @@ SmiRendezvous (
> 
>    ASSERT(CpuIndex < mMaxNumberOfCpus);
> 
> -  //
> -  // Save Cr2 because Page Fault exception in SMM may override its value
> -  //
> -  Cr2 = AsmReadCr2 ();
> +  if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool
> (PcdCpuSmmStaticPageTable))) {
> +    //
> +    // Save Cr2 because Page Fault exception in SMM may override its
> value
> +    //
> +    Cr2 = AsmReadCr2 ();
> +  }
> 
>    //
>    // Perform CPU specific entry hooks
> @@ -1253,10 +1255,12 @@ SmiRendezvous (
> 
>  Exit:
>    SmmCpuFeaturesRendezvousExit (CpuIndex);
> -  //
> -  // Restore Cr2
> -  //
> -  AsmWriteCr2 (Cr2);
> +  if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool
> (PcdCpuSmmStaticPageTable))) {
> +    //
> +    // Restore Cr2
> +    //
> +    AsmWriteCr2 (Cr2);
> +  }
>  }
> 
>  /**
> --
> 2.16.2.windows.1

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

[Laszlo Ersek]

On 03/07/19 12:14, 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.
> 
> 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/MpService.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)

(1) There is an open question about the usefulness of this patch in
<https://bugzilla.tianocore.org/show_bug.cgi?id=1593#c1>. It should be
answered in the BZ, or the same description should be included in the
commit message.

(2) Also, the commit message should refer to the BZ.


> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 3b0b3b52ac..5be4a2b020 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1111,10 +1111,12 @@ SmiRendezvous (
>  
>    ASSERT(CpuIndex < mMaxNumberOfCpus);
>  
> -  //
> -  // Save Cr2 because Page Fault exception in SMM may override its value
> -  //
> -  Cr2 = AsmReadCr2 ();
> +  if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool (PcdCpuSmmStaticPageTable))) {

(3) It doesn't look like a good idea to me to call PcdGetBool() in the
SmiRendezvous() function.

If the PCD is not fixed-at-build (but dynamic), then we'll end up
calling a PI protocol member from a function that is by definition
executed by multiple processors at the same time.

"X64/PageTbl.c" already defines the global variable
"mCpuSmmStaticPageTable", setting it from the PCD on the call stack of
the entry point function of the driver. That is safe -- we can call PI /
UEFI protocols in the entry point functions of a DXE_SMM_DRIVER.

Now, the fact that "mCpuSmmStaticPageTable" is only defined in the X64
build (that is, in "X64/PageTbl.c"), is actually quite informative. It
means that, instead of this conditional code in "MpService.c", we should
introduce two new helper functions, "SaveCr2" and "RestoreCr2". And we
should provide separate implementations for IA32 and X64. For IA32, the
function should do nothing. For X64, the function should depend on
"mCpuSmmStaticPageTable", and massage CR2 as necessary.

However: that *still* depends on whether this change is useful. I
realize the CR2 manipulation may not be overly useful on IA32 (we can't
address >4GB memory, so demand paging for >4GB makes no sense), but its
performance hit should be negligible. Again, back to point (1): what is
the actual issue with the current code?

Thanks
Laszlo

> +    //
> +    // Save Cr2 because Page Fault exception in SMM may override its value
> +    //
> +    Cr2 = AsmReadCr2 ();
> +  }
>  
>    //
>    // Perform CPU specific entry hooks
> @@ -1253,10 +1255,12 @@ SmiRendezvous (
>  
>  Exit:
>    SmmCpuFeaturesRendezvousExit (CpuIndex);
> -  //
> -  // Restore Cr2
> -  //
> -  AsmWriteCr2 (Cr2);
> +  if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool (PcdCpuSmmStaticPageTable))) {
> +    //
> +    // Restore Cr2
> +    //
> +    AsmWriteCr2 (Cr2);
> +  }
>  }
>  
>  /**
> 

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

[Kinney, Michael D]

Laszlo,

Good news is that the PCD being used is a Feature Flag.

[PcdsFeatureFlag]
  ## Indicates if SMM Profile will be enabled.
  #  If enabled, instruction executions in and data accesses to memory outside of SMRAM will be logged.
  #  It could not be enabled at the same time with SMM static page table feature (PcdCpuSmmStaticPageTable).
  #  This PCD is only for validation purpose. It should be set to false in production.<BR><BR>
  #   TRUE  - SMM Profile will be enabled.<BR>
  #   FALSE - SMM Profile will be disabled.<BR>
  # @Prompt Enable SMM Profile.
  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable|FALSE|BOOLEAN|0x32132109

That means a different PcdLib function should be used to look up
the value so it is clear that it is safe at SMM runtime.

    FeaturePcdGet(TokenName)

Best regards,

Mike


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-
> bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Thursday, March 7, 2019 9:58 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>
> Subject: Re: [edk2] [PATCH v2] UefiCpuPkg\CpuSmm: Save
> & restore CR2 on-demand paging in SMM
> 
> On 03/07/19 12:14, 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.
> >
> > 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/MpService.c | 20
> ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> (1) There is an open question about the usefulness of
> this patch in
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1593#c1
> >. It should be
> answered in the BZ, or the same description should be
> included in the
> commit message.
> 
> (2) Also, the commit message should refer to the BZ.
> 
> 
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > index 3b0b3b52ac..5be4a2b020 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > @@ -1111,10 +1111,12 @@ SmiRendezvous (
> >
> >    ASSERT(CpuIndex < mMaxNumberOfCpus);
> >
> > -  //
> > -  // Save Cr2 because Page Fault exception in SMM
> may override its value
> > -  //
> > -  Cr2 = AsmReadCr2 ();
> > +  if ((sizeof (UINTN) == sizeof (UINT64)) &&
> (!PcdGetBool (PcdCpuSmmStaticPageTable))) {
> 
> (3) It doesn't look like a good idea to me to call
> PcdGetBool() in the
> SmiRendezvous() function.
> 
> If the PCD is not fixed-at-build (but dynamic), then
> we'll end up
> calling a PI protocol member from a function that is by
> definition
> executed by multiple processors at the same time.
> 
> "X64/PageTbl.c" already defines the global variable
> "mCpuSmmStaticPageTable", setting it from the PCD on
> the call stack of
> the entry point function of the driver. That is safe --
> we can call PI /
> UEFI protocols in the entry point functions of a
> DXE_SMM_DRIVER.
> 
> Now, the fact that "mCpuSmmStaticPageTable" is only
> defined in the X64
> build (that is, in "X64/PageTbl.c"), is actually quite
> informative. It
> means that, instead of this conditional code in
> "MpService.c", we should
> introduce two new helper functions, "SaveCr2" and
> "RestoreCr2". And we
> should provide separate implementations for IA32 and
> X64. For IA32, the
> function should do nothing. For X64, the function
> should depend on
> "mCpuSmmStaticPageTable", and massage CR2 as necessary.
> 
> However: that *still* depends on whether this change is
> useful. I
> realize the CR2 manipulation may not be overly useful
> on IA32 (we can't
> address >4GB memory, so demand paging for >4GB makes no
> sense), but its
> performance hit should be negligible. Again, back to
> point (1): what is
> the actual issue with the current code?
> 
> Thanks
> Laszlo
> 
> > +    //
> > +    // Save Cr2 because Page Fault exception in SMM
> may override its value
> > +    //
> > +    Cr2 = AsmReadCr2 ();
> > +  }
> >
> >    //
> >    // Perform CPU specific entry hooks
> > @@ -1253,10 +1255,12 @@ SmiRendezvous (
> >
> >  Exit:
> >    SmmCpuFeaturesRendezvousExit (CpuIndex);
> > -  //
> > -  // Restore Cr2
> > -  //
> > -  AsmWriteCr2 (Cr2);
> > +  if ((sizeof (UINTN) == sizeof (UINT64)) &&
> (!PcdGetBool (PcdCpuSmmStaticPageTable))) {
> > +    //
> > +    // Restore Cr2
> > +    //
> > +    AsmWriteCr2 (Cr2);
> > +  }
> >  }
> >
> >  /**
> >
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Yao, Jiewen]

Good catch Laszo!!!

I found PcdCpuSmmStaticPageTable is [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx].
I think it should only be static, but I am wrong. Thanks to point it out.
Then I think we need get the PCD value at the entrypoint.


Another option is just to move the CR2 access from C code to ASM code, to isolate the CR2 access in C code.


Thank you
Yao Jiewen


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, March 7, 2019 9:58 AM
> To: Vanguput, Narendra K <narendra.k.vanguput@intel.com>;
> edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>
> Subject: Re: [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2
> on-demand paging in SMM
> 
> On 03/07/19 12:14, 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.
> >
> > 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/MpService.c | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> (1) There is an open question about the usefulness of this patch in
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1593#c1>. It should be
> answered in the BZ, or the same description should be included in the
> commit message.
> 
> (2) Also, the commit message should refer to the BZ.
> 
> 
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > index 3b0b3b52ac..5be4a2b020 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > @@ -1111,10 +1111,12 @@ SmiRendezvous (
> >
> >    ASSERT(CpuIndex < mMaxNumberOfCpus);
> >
> > -  //
> > -  // Save Cr2 because Page Fault exception in SMM may override its value
> > -  //
> > -  Cr2 = AsmReadCr2 ();
> > +  if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool
> (PcdCpuSmmStaticPageTable))) {
> 
> (3) It doesn't look like a good idea to me to call PcdGetBool() in the
> SmiRendezvous() function.
> 
> If the PCD is not fixed-at-build (but dynamic), then we'll end up
> calling a PI protocol member from a function that is by definition
> executed by multiple processors at the same time.
> 
> "X64/PageTbl.c" already defines the global variable
> "mCpuSmmStaticPageTable", setting it from the PCD on the call stack of
> the entry point function of the driver. That is safe -- we can call PI /
> UEFI protocols in the entry point functions of a DXE_SMM_DRIVER.
> 
> Now, the fact that "mCpuSmmStaticPageTable" is only defined in the X64
> build (that is, in "X64/PageTbl.c"), is actually quite informative. It
> means that, instead of this conditional code in "MpService.c", we should
> introduce two new helper functions, "SaveCr2" and "RestoreCr2". And we
> should provide separate implementations for IA32 and X64. For IA32, the
> function should do nothing. For X64, the function should depend on
> "mCpuSmmStaticPageTable", and massage CR2 as necessary.
> 
> However: that *still* depends on whether this change is useful. I
> realize the CR2 manipulation may not be overly useful on IA32 (we can't
> address >4GB memory, so demand paging for >4GB makes no sense), but its
> performance hit should be negligible. Again, back to point (1): what is
> the actual issue with the current code?
> 
> Thanks
> Laszlo
> 
> > +    //
> > +    // Save Cr2 because Page Fault exception in SMM may override its
> value
> > +    //
> > +    Cr2 = AsmReadCr2 ();
> > +  }
> >
> >    //
> >    // Perform CPU specific entry hooks
> > @@ -1253,10 +1255,12 @@ SmiRendezvous (
> >
> >  Exit:
> >    SmmCpuFeaturesRendezvousExit (CpuIndex);
> > -  //
> > -  // Restore Cr2
> > -  //
> > -  AsmWriteCr2 (Cr2);
> > +  if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool
> (PcdCpuSmmStaticPageTable))) {
> > +    //
> > +    // Restore Cr2
> > +    //
> > +    AsmWriteCr2 (Cr2);
> > +  }
> >  }
> >
> >  /**
> >

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

[Kinney, Michael D]

Laszlo,

The information I provided below is incorrect.  The PCD referenced
does support all PCD types as Jiewen noted.

Mike

> -----Original Message-----
> From: Kinney, Michael D
> Sent: Thursday, March 7, 2019 10:10 AM
> To: Laszlo Ersek <lersek@redhat.com>; 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>
> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg\CpuSmm: Save
> & restore CR2 on-demand paging in SMM
> 
> Laszlo,
> 
> Good news is that the PCD being used is a Feature Flag.
> 
> [PcdsFeatureFlag]
>   ## Indicates if SMM Profile will be enabled.
>   #  If enabled, instruction executions in and data
> accesses to memory outside of SMRAM will be logged.
>   #  It could not be enabled at the same time with SMM
> static page table feature (PcdCpuSmmStaticPageTable).
>   #  This PCD is only for validation purpose. It should
> be set to false in production.<BR><BR>
>   #   TRUE  - SMM Profile will be enabled.<BR>
>   #   FALSE - SMM Profile will be disabled.<BR>
>   # @Prompt Enable SMM Profile.
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable|FALSE|
> BOOLEAN|0x32132109
> 
> That means a different PcdLib function should be used
> to look up
> the value so it is clear that it is safe at SMM
> runtime.
> 
>     FeaturePcdGet(TokenName)
> 
> Best regards,
> 
> Mike
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-
> > bounces@lists.01.org] On Behalf Of Laszlo Ersek
> > Sent: Thursday, March 7, 2019 9:58 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>
> > Subject: Re: [edk2] [PATCH v2] UefiCpuPkg\CpuSmm:
> Save
> > & restore CR2 on-demand paging in SMM
> >
> > On 03/07/19 12:14, 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.
> > >
> > > 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/MpService.c | 20
> > ++++++++++++--------
> > >  1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > (1) There is an open question about the usefulness of
> > this patch in
> >
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1593#c1
> > >. It should be
> > answered in the BZ, or the same description should be
> > included in the
> > commit message.
> >
> > (2) Also, the commit message should refer to the BZ.
> >
> >
> > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > > index 3b0b3b52ac..5be4a2b020 100644
> > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > > @@ -1111,10 +1111,12 @@ SmiRendezvous (
> > >
> > >    ASSERT(CpuIndex < mMaxNumberOfCpus);
> > >
> > > -  //
> > > -  // Save Cr2 because Page Fault exception in SMM
> > may override its value
> > > -  //
> > > -  Cr2 = AsmReadCr2 ();
> > > +  if ((sizeof (UINTN) == sizeof (UINT64)) &&
> > (!PcdGetBool (PcdCpuSmmStaticPageTable))) {
> >
> > (3) It doesn't look like a good idea to me to call
> > PcdGetBool() in the
> > SmiRendezvous() function.
> >
> > If the PCD is not fixed-at-build (but dynamic), then
> > we'll end up
> > calling a PI protocol member from a function that is
> by
> > definition
> > executed by multiple processors at the same time.
> >
> > "X64/PageTbl.c" already defines the global variable
> > "mCpuSmmStaticPageTable", setting it from the PCD on
> > the call stack of
> > the entry point function of the driver. That is safe
> --
> > we can call PI /
> > UEFI protocols in the entry point functions of a
> > DXE_SMM_DRIVER.
> >
> > Now, the fact that "mCpuSmmStaticPageTable" is only
> > defined in the X64
> > build (that is, in "X64/PageTbl.c"), is actually
> quite
> > informative. It
> > means that, instead of this conditional code in
> > "MpService.c", we should
> > introduce two new helper functions, "SaveCr2" and
> > "RestoreCr2". And we
> > should provide separate implementations for IA32 and
> > X64. For IA32, the
> > function should do nothing. For X64, the function
> > should depend on
> > "mCpuSmmStaticPageTable", and massage CR2 as
> necessary.
> >
> > However: that *still* depends on whether this change
> is
> > useful. I
> > realize the CR2 manipulation may not be overly useful
> > on IA32 (we can't
> > address >4GB memory, so demand paging for >4GB makes
> no
> > sense), but its
> > performance hit should be negligible. Again, back to
> > point (1): what is
> > the actual issue with the current code?
> >
> > Thanks
> > Laszlo
> >
> > > +    //
> > > +    // Save Cr2 because Page Fault exception in
> SMM
> > may override its value
> > > +    //
> > > +    Cr2 = AsmReadCr2 ();
> > > +  }
> > >
> > >    //
> > >    // Perform CPU specific entry hooks
> > > @@ -1253,10 +1255,12 @@ SmiRendezvous (
> > >
> > >  Exit:
> > >    SmmCpuFeaturesRendezvousExit (CpuIndex);
> > > -  //
> > > -  // Restore Cr2
> > > -  //
> > > -  AsmWriteCr2 (Cr2);
> > > +  if ((sizeof (UINTN) == sizeof (UINT64)) &&
> > (!PcdGetBool (PcdCpuSmmStaticPageTable))) {
> > > +    //
> > > +    // Restore Cr2
> > > +    //
> > > +    AsmWriteCr2 (Cr2);
> > > +  }
> > >  }
> > >
> > >  /**
> > >
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel