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.115; helo=mga14.intel.com; envelope-from=michael.d.kinney@intel.com; receiver=edk2-devel@lists.01.org Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (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 AFA942193DFA8 for ; Thu, 7 Mar 2019 10:10:21 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 Mar 2019 10:10:19 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,451,1544515200"; d="scan'208";a="140019297" Received: from orsmsx108.amr.corp.intel.com ([10.22.240.6]) by orsmga002.jf.intel.com with ESMTP; 07 Mar 2019 10:10:19 -0800 Received: from orsmsx157.amr.corp.intel.com (10.22.240.23) by ORSMSX108.amr.corp.intel.com (10.22.240.6) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 7 Mar 2019 10:10:19 -0800 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.97]) by ORSMSX157.amr.corp.intel.com ([169.254.9.183]) with mapi id 14.03.0415.000; Thu, 7 Mar 2019 10:10:14 -0800 From: "Kinney, Michael D" To: Laszlo Ersek , "Vanguput, Narendra K" , "edk2-devel@lists.01.org" , "Kinney, Michael D" CC: "Yao, Jiewen" , "Dong, Eric" Thread-Topic: [edk2] [PATCH v2] UefiCpuPkg\CpuSmm: Save & restore CR2 on-demand paging in SMM Thread-Index: AQHU1NgeCHkGfw6M7E6jUPg3DxfEMaYA+u2A//98wyA= Date: Thu, 7 Mar 2019 18:10:14 +0000 Message-ID: References: <20190307111439.32344-1-narendra.k.vanguput@intel.com> <428b8706-59f8-fcfc-e06c-57ab4f13c328@redhat.com> In-Reply-To: <428b8706-59f8-fcfc-e06c-57ab4f13c328@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.22.254.138] MIME-Version: 1.0 Subject: Re: [PATCH v2] 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: Thu, 07 Mar 2019 18:10:22 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 outs= ide of SMRAM will be logged. # It could not be enabled at the same time with SMM static page table fe= ature (PcdCpuSmmStaticPageTable). # This PCD is only for validation purpose. It should be set to false in = production.

# TRUE - SMM Profile will be enabled.
# FALSE - SMM Profile will be disabled.
# @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 > ; edk2- > devel@lists.01.org > Cc: Yao, Jiewen ; Dong, Eric > > Subject: Re: [edk2] [PATCH v2] UefiCpuPkg\CpuSmm: Save > & restore CR2 on-demand paging in SMM >=20 > On 03/07/19 12:14, 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. > > > > 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/MpService.c | 20 > ++++++++++++-------- > > 1 file changed, 12 insertions(+), 8 deletions(-) >=20 > (1) There is an open question about the usefulness of > this patch in > >. It should be > answered in the BZ, or the same description should be > included in the > commit message. >=20 > (2) Also, the commit message should refer to the BZ. >=20 >=20 > > 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 =3D AsmReadCr2 (); > > + if ((sizeof (UINTN) =3D=3D sizeof (UINT64)) && > (!PcdGetBool (PcdCpuSmmStaticPageTable))) { >=20 > (3) It doesn't look like a good idea to me to call > PcdGetBool() in the > SmiRendezvous() function. >=20 > 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. >=20 > "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. >=20 > 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. >=20 > 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? >=20 > Thanks > Laszlo >=20 > > + // > > + // Save Cr2 because Page Fault exception in SMM > may override its value > > + // > > + Cr2 =3D AsmReadCr2 (); > > + } > > > > // > > // Perform CPU specific entry hooks > > @@ -1253,10 +1255,12 @@ SmiRendezvous ( > > > > Exit: > > SmmCpuFeaturesRendezvousExit (CpuIndex); > > - // > > - // Restore Cr2 > > - // > > - AsmWriteCr2 (Cr2); > > + if ((sizeof (UINTN) =3D=3D sizeof (UINT64)) && > (!PcdGetBool (PcdCpuSmmStaticPageTable))) { > > + // > > + // Restore Cr2 > > + // > > + AsmWriteCr2 (Cr2); > > + } > > } > > > > /** > > >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel