From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 29 Jul 2019 04:42:08 -0700 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 4BD6E3082E6E; Mon, 29 Jul 2019 11:42:08 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-240.ams2.redhat.com [10.36.117.240]) by smtp.corp.redhat.com (Postfix) with ESMTP id 181F71DD; Mon, 29 Jul 2019 11:42:06 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/PiSmmCpu: Refine CR2 save/restore logic To: devel@edk2.groups.io, ray.ni@intel.com Cc: Eric Dong , Vanguput Narendra K References: <20190727032850.337840-1-ray.ni@intel.com> <20190727032850.337840-3-ray.ni@intel.com> From: "Laszlo Ersek" Message-ID: <37b7800b-589b-9ac8-5758-cf9deb5029bf@redhat.com> Date: Mon, 29 Jul 2019 13:42:06 +0200 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: <20190727032850.337840-3-ray.ni@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]); Mon, 29 Jul 2019 11:42:08 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 07/27/19 05:28, Ni, Ray wrote: > Because IsStaticPageTableEnabled() is added for both IA32 and x64 > build, the CR2 save/restore logic can be refined: > 1. Remove arch specific SaveCr2() / RestoreCr2() implementation; > 2. Conditionally save and restore CR2 in SmiRendezvous(). > > Signed-off-by: Ray Ni > Cc: Eric Dong > Cc: Laszlo Ersek > Cc: Vanguput Narendra K > --- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 25 ------------------- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 8 ++++-- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 22 ---------------- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 29 ---------------------- > 4 files changed, 6 insertions(+), 78 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > index 2a9af4b77d..cae23d6d1d 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > @@ -327,28 +327,3 @@ 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 ef16997547..5d0124b368 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -1575,7 +1575,9 @@ SmiRendezvous ( > // when using on-demand paging for above 4G memory. > // > Cr2 = 0; > - SaveCr2 (&Cr2); > + if (!IsStaticPageTableEnabled ()) { > + Cr2 = AsmReadCr2 (); > + } > > // > // Call the user register Startup function first. So, because this patch is supposed to only refactor / simplify the code, it should not change behavior. But, because in patch#1 we return FALSE for IA32, the condition above will evaluate to TRUE. And so we will massage CR2 (= fault address), even though the IA32 build shouldn't do that (and doesn't do it, at the moment). This should be fixed by returning constant TRUE from IsStaticPageTableEnabled(), in patch#1, on IA32. (Note: in the message of commit d47b85a621ad ("Revert "UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF"", 2019-07-26), I wrote that "The IA32 implementation should return a constant value". I didn't say either "constant TRUE" or "constant FALSE". And that's because I couldn't know the right value, without actually looking at the code. Determining the correct IA32 value was out of scope for the revert.) More below: > @@ -1725,7 +1727,9 @@ Exit: > // > // Restore Cr2 > // > - RestoreCr2 (Cr2); > + if (!IsStaticPageTableEnabled ()) { > + AsmWriteCr2 (Cr2); > + } > } > > /** > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index 14b7676c16..5a97733def 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -1282,28 +1282,6 @@ IsStaticPageTableEnabled ( > ) > ; > > -/** > - 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 > - ); > - > /** > Schedule a procedure to run on the specified CPU. > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > index 18e3f9e08d..8259b01a95 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -1209,32 +1209,3 @@ 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); > - } > -} > For this patch: Reviewed-by: Laszlo Ersek Thanks Laszlo