From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 86934211E0939 for ; Wed, 20 Mar 2019 09:31:22 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id ECD7899D2E; Wed, 20 Mar 2019 16:31:21 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-91.rdu2.redhat.com [10.10.120.91]) by smtp.corp.redhat.com (Postfix) with ESMTP id ADC0A5D9CD; Wed, 20 Mar 2019 16:31:20 +0000 (UTC) To: nkvangup , edk2-devel@lists.01.org Cc: Yao Jiewen , Eric Dong References: <20190318143825.17352-1-narendra.k.vanguput@intel.com> From: Laszlo Ersek Message-ID: Date: Wed, 20 Mar 2019 17:31:19 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190318143825.17352-1-narendra.k.vanguput@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Wed, 20 Mar 2019 16:31:22 +0000 (UTC) Subject: Re: [PATCH v4] 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, 20 Mar 2019 16:31:22 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 03/18/19 15:38, 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 > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Yao Jiewen > --- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 22 ++++++++++++++-------- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 2 +- > 2 files changed, 15 insertions(+), 9 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > index 3b0b3b52ac..0c07b31c4f 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -28,6 +28,7 @@ UINTN mSemaphoreSize; > SPIN_LOCK *mPFLock = NULL; > SMM_CPU_SYNC_MODE mCpuSmmSyncMode; > BOOLEAN mMachineCheckSupported = FALSE; > +BOOLEAN mCpuSmmStaticPageTable = TRUE; Hmmm. This change is a bit daring, but I think it could be valid. - In the IA32 build, mCpuSmmStaticPageTable would never be modified, or read, by *preexistent* code (because all that code is in X64/PageTbl.c). And the new code, added by this patch, would (presumably) work fine, with the initial TRUE value. - In the X64 build, the preexistent code would never read the initial value (which we now set to TRUE here), i.e. before overwriting the variable from the PCD -- because that would mean a bug in the preexistent code. (Well, unless that code relied on the zero initial value of the variable). (1) I think I'd like to defer on this to other UefiCpuPkg reviewers. Honestly I find this style questionable. It makes me feel uncomfortable. I'd prefer the new APIs with the separate IA32/X64 implementations that I suggested in my v2 review. But if other reviewers like this one better, I won't mind. (After hearing their opinions, I'd attempt to find the time to regression test the patch (or maybe v5), too.) Assuming other reviewers prefer this approach over my suggestion, I have some other comments: > > /** > Performs an atomic compare exchange operation to get semaphore. > @@ -1111,10 +1112,13 @@ SmiRendezvous ( > > ASSERT(CpuIndex < mMaxNumberOfCpus); > > - // > - // Save Cr2 because Page Fault exception in SMM may override its value > - // > - Cr2 = AsmReadCr2 (); > + if (!mCpuSmmStaticPageTable) { > + // > + // Save and restore Cr2 when using on-demand paging for above 4G memory because Page Fault > + // exception in SMM may override its value > + // > + Cr2 = AsmReadCr2 (); > + } (2) The indentation of the "if" is broken. (3) Given that we're already using two comment lines, I'd suggest not exceeding 80 characters per line. > > // > // Perform CPU specific entry hooks > @@ -1253,10 +1257,12 @@ SmiRendezvous ( > > Exit: > SmmCpuFeaturesRendezvousExit (CpuIndex); > - // > - // Restore Cr2 > - // > - AsmWriteCr2 (Cr2); > + if (!mCpuSmmStaticPageTable) { (4) same as (2). > + // > + // Restore Cr2 > + // > + AsmWriteCr2 (Cr2); > + } > } > > /** > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > index 2c77cb47a4..e444b8a031 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -21,7 +21,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > > LIST_ENTRY mPagePool = INITIALIZE_LIST_HEAD_VARIABLE (mPagePool); > BOOLEAN m1GPageTableSupport = FALSE; > -BOOLEAN mCpuSmmStaticPageTable; > +extern BOOLEAN mCpuSmmStaticPageTable; (5) This is generally not great style, and it conflicts with the existent code of this driver. Namely, declarations of variables with file scope, static storage duration, and external linkage, should go into "PiSmmCpuDxeSmm.h"-- we already got a bunch of them there. Thanks Laszlo > > /** > Disable CET. >