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 E39FE211D07A6 for ; Thu, 7 Mar 2019 09:57:42 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 350E33082E06; Thu, 7 Mar 2019 17:57:42 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-123-89.rdu2.redhat.com [10.10.123.89]) by smtp.corp.redhat.com (Postfix) with ESMTP id E55556090C; Thu, 7 Mar 2019 17:57:40 +0000 (UTC) To: nkvangup , edk2-devel@lists.01.org Cc: Eric Dong , Ray Ni , Yao Jiewen References: <20190307111439.32344-1-narendra.k.vanguput@intel.com> From: Laszlo Ersek Message-ID: <428b8706-59f8-fcfc-e06c-57ab4f13c328@redhat.com> Date: Thu, 7 Mar 2019 18:57:39 +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: <20190307111439.32344-1-narendra.k.vanguput@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.46]); Thu, 07 Mar 2019 17:57:42 +0000 (UTC) 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 17:57:43 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 03/07/19 12:14, 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 | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) (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. (2) Also, the commit message should refer to the BZ. > 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 = AsmReadCr2 (); > + if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool (PcdCpuSmmStaticPageTable))) { (3) It doesn't look like a good idea to me to call PcdGetBool() in the SmiRendezvous() function. 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. "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. 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. 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? Thanks Laszlo > + // > + // Save Cr2 because Page Fault exception in SMM may override its value > + // > + Cr2 = AsmReadCr2 (); > + } > > // > // Perform CPU specific entry hooks > @@ -1253,10 +1255,12 @@ SmiRendezvous ( > > Exit: > SmmCpuFeaturesRendezvousExit (CpuIndex); > - // > - // Restore Cr2 > - // > - AsmWriteCr2 (Cr2); > + if ((sizeof (UINTN) == sizeof (UINT64)) && (!PcdGetBool (PcdCpuSmmStaticPageTable))) { > + // > + // Restore Cr2 > + // > + AsmWriteCr2 (Cr2); > + } > } > > /** >