> On Apr 1, 2019, at 9:47 AM, Laszlo Ersek <ler...@redhat.com> wrote:
>
> On 04/01/19 10:16, 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.
>> 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>
>> Cc: Eric Dong <eric.d...@intel.com>
>> Cc: Ray Ni <ray...@intel.com>
>> Cc: Laszlo Ersek <ler...@redhat.com>
>> Cc: Yao Jiewen <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
