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

[Laszlo Ersek <lersek@redhat.com>]

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);
> +  }
>  }
>  
>  /**
>