From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.136; helo=mga12.intel.com; envelope-from=eric.dong@intel.com; receiver=edk2-devel@lists.01.org Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 32224211DF235 for ; Tue, 2 Apr 2019 20:18:08 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 02 Apr 2019 20:18:07 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,303,1549958400"; d="scan'208,217";a="128162638" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga007.jf.intel.com with ESMTP; 02 Apr 2019 20:18:07 -0700 Received: from fmsmsx151.amr.corp.intel.com (10.18.125.4) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.408.0; Tue, 2 Apr 2019 20:18:06 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX151.amr.corp.intel.com (10.18.125.4) with Microsoft SMTP Server (TLS) id 14.3.408.0; Tue, 2 Apr 2019 20:18:06 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.206]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.164]) with mapi id 14.03.0415.000; Wed, 3 Apr 2019 11:18:04 +0800 From: "Dong, Eric" To: "afish@apple.com" , Laszlo Ersek CC: "Vanguput, Narendra K" , edk2-devel , "Yao, Jiewen" , "Dong, Eric" Thread-Topic: [edk2] [PATCH v9] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM Thread-Index: AQHU6GMsqHYOgl0iM0KOrE4X4QJGi6Ym/kiAgAAD3ACAAsD+MA== Date: Wed, 3 Apr 2019 03:18:03 +0000 Message-ID: References: <20190401081601.22388-1-narendra.k.vanguput@intel.com> <5345695C-14DF-4D3E-B8D8-30914252EF10@apple.com> In-Reply-To: <5345695C-14DF-4D3E-B8D8-30914252EF10@apple.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [PATCH v9] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Apr 2019 03:18:09 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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? Thanks Eric From: afish@apple.com [mailto:afish@apple.com] Sent: Tuesday, April 2, 2019 1:01 AM To: Laszlo Ersek Cc: Vanguput, Narendra K ; edk2-devel ; Yao, Jiewen ; Dong, Eric Subject: Re: [edk2] [PATCH v9] UefiCpuPkg\CpuSmm: Save & restore CR2 on-dem= and paging in SMM On Apr 1, 2019, at 9:47 AM, Laszlo Ersek > wrote: On 04/01/19 10:16, nkvangup wrote: BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1593 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 > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Yao Jiewen > --- 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/PiSmmCpu= DxeSmm/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/PiSmmCpuDxe= Smm/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 =3D AsmReadCr2 (); + Cr2 =3D 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/PiSmmC= puDxeSmm/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/PiSmmCpuD= xeSmm/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 =3D 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 t= his 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 fee= ls 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 > Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel