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:33:18 -0700 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 225263084212; Mon, 29 Jul 2019 11:33:18 +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 503EA19C58; Mon, 29 Jul 2019 11:33:17 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg/PiSmmCpu: Add Internal function IsStaticPageTableEnabled To: devel@edk2.groups.io, ray.ni@intel.com References: <20190727032850.337840-1-ray.ni@intel.com> <20190727032850.337840-2-ray.ni@intel.com> From: "Laszlo Ersek" Message-ID: <1812e3c6-f388-6faa-71d9-8e878b8e9b6c@redhat.com> Date: Mon, 29 Jul 2019 13:33:16 +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-2-ray.ni@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Mon, 29 Jul 2019 11:33:18 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Ray, On 07/27/19 05:28, Ni, Ray wrote: > The internal function reflects the status whether static page table > is enabled. > > Signed-off-by: Ray Ni > Cc: Laszlo Ersek > Cc: Eric Dong --- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 16 ++++++++++++++++ > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 15 +++++++++++++++ > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 16 ++++++++++++++++ > 3 files changed, 47 insertions(+) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > index 05fb455936..2a9af4b77d 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > @@ -28,6 +28,22 @@ EnableCet ( > VOID > ); > > +/** > + Return whether Static Page Table is enabled. > + > + Note: Static Page Table is always disabled for IA32 build. > + > + @retval TRUE Static Page Table is enabled. > + @retval FALSE Static Page Table is disabled. > +**/ > +BOOLEAN > +IsStaticPageTableEnabled ( > + VOID > + ) > +{ > + return FALSE; > +} > + > /** > Create PageTable for SMM use. > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index 186809f431..14b7676c16 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -1267,6 +1267,21 @@ EFIAPI > PiSmmCpuSmiEntryFixupAddress ( > ); > > + > +/** > + Return whether Static Page Table is enabled. > + > + Note: Static Page Table is always disabled for IA32 build. > + > + @retval TRUE Static Page Table is enabled. > + @retval FALSE Static Page Table is disabled. > +**/ > +BOOLEAN > +IsStaticPageTableEnabled ( > + VOID > + ) > +; > + > /** > This function reads CR2 register when on-demand paging is enabled > for 64 bit and no action for 32 bit. > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > index a3b62f7787..18e3f9e08d 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -37,6 +37,22 @@ EnableCet ( > VOID > ); > > +/** > + Return whether Static Page Table is enabled. > + > + Note: Static Page Table is always disabled for IA32 build. > + > + @retval TRUE Static Page Table is enabled. > + @retval FALSE Static Page Table is disabled. > +**/ > +BOOLEAN > +IsStaticPageTableEnabled ( > + VOID > + ) > +{ > + return mCpuSmmStaticPageTable; > +} > + > /** > Check if 1-GByte pages is supported by processor or not. > > I like the approach in this patch; however I think the IA32 implementation (and comments) are wrong. The function should return constant TRUE on IA32. "static paging" means that all page tables used in SMM are built in advance. When "static paging" is off (= "dynamic paging" is enabled), then page tables are built in SMM on-demand, based on individual page faults. (This is my understanding anyway.) And in the IA32 build, the page tables are always built in advance, to my understanding. Hence "static paging" should be reported as "on". The mistake in this patch (patch#1) is apparent in patch#2. Patch#2 seems good (I'll comment on it separately), but because of the incorrect IA32 implementation in patch#1, patch#2 ends up changing the behavior on IA32. Thanks Laszlo