> On Apr 2, 2019, at 8:18 PM, Dong, Eric <eric.d...@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: af...@apple.com [mailto:af...@apple.com]
> Sent: Tuesday, April 2, 2019 1:01 AM
> To: Laszlo Ersek <ler...@redhat.com>
> Cc: Vanguput, Narendra K <narendra.k.vangu...@intel.com>; edk2-devel
> <edk2-devel@lists.01.org>; Yao, Jiewen <jiewen....@intel.com>; Dong, Eric
> <eric.d...@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 <ler...@redhat.com
> <mailto:ler...@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.vangu...@intel.com
> <mailto:narendra.k.vangu...@intel.com>>
> Cc: Eric Dong <eric.d...@intel.com <mailto:eric.d...@intel.com>>
> Cc: Ray Ni <ray...@intel.com <mailto:ray...@intel.com>>
> Cc: Laszlo Ersek <ler...@redhat.com <mailto:ler...@redhat.com>>
> Cc: Yao Jiewen <jiewen....@intel.com <mailto:jiewen....@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 <ler...@redhat.com
> <mailto:ler...@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>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
