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; Fri, 02 Aug 2019 15:06:16 -0700 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5739383F3F; Fri, 2 Aug 2019 22:06:15 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-36.ams2.redhat.com [10.36.116.36]) by smtp.corp.redhat.com (Postfix) with ESMTP id D7C081001B03; Fri, 2 Aug 2019 22:06:13 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu: PcdCpuSmmAccessOut controls SMM access-out policy To: "Yao, Jiewen" , "devel@edk2.groups.io" Cc: "Ni, Ray" , "Dong, Eric" , "Wang, Jian J" References: <20190731163852.191708-1-ray.ni@intel.com> <20190731163852.191708-3-ray.ni@intel.com> <50b073a1-bf04-4b8a-e267-2af933d34c5c@redhat.com> <74D8A39837DF1E4DA445A8C0B3885C503F73CFD5@shsmsx102.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: <39df5f13-547c-4d16-39d1-82ae9edaa419@redhat.com> Date: Sat, 3 Aug 2019 00:06:12 +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: <74D8A39837DF1E4DA445A8C0B3885C503F73CFD5@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Fri, 02 Aug 2019 22:06:15 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/02/19 04:46, Yao, Jiewen wrote: > Thanks Laszlo. Comment below: > >> -----Original Message----- >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >> Laszlo Ersek >> Sent: Friday, August 2, 2019 10:05 AM >> To: Yao, Jiewen ; devel@edk2.groups.io >> Cc: Ni, Ray ; Dong, Eric ; Wang, >> Jian J >> Subject: Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu: >> PcdCpuSmmAccessOut controls SMM access-out policy >> >> On 08/01/19 02:02, Yao, Jiewen wrote: >>> 1) StaticPaging=false, AccessOut=false: it seems invalid. If we >>> don't support access out, why we need dynamic paging? >> >> The first patch that introduced "access-out" (as a use case) was commit >> c60d36b4d1ee ("UefiCpuPkg/SmmCpu: Block access-out only when static >> paging is used", 2018-11-08). Before that, there was no access-out. >> >> The first patch that distinguished "static page tables" from "dynamic >> page tables" was 28b020b5de1e ("UefiCpuPkg/dec: Add >> PcdCpuSmmStaticPageTable.", 2016-11-17) >> >> During those two years, we didn't support access-out, but allowed >> platforms to choose between static/dynamic paging. Are we calling that >> invalid in retrospect? > > [Jiewen] Right. Agree with you. > Our original intension is to provide a secure configuration, as #2. > But we notice it is not possible, due to some special RAS requirement. > Then we have to move to #2, and make #1 invalid. OK. I don't mind the RAS requirement as long as option #3 remains viable. >>> 2) StaticPaging=false, AccessOut=true: it seems valid. We need access >>> out, but we only want a small paging in the beginning. As such we use >>> dynamic paging. This is to support Hotplug memory. >> >> This scenario doesn't look supportable on IA32. (Due to >> StaticPaging=false.) I don't mean that it's impossible to implement, >> just that the IA32 code today doesn't extend the page tables in response >> to page faults. > > [Jiewen] You are right. > > Some background on dynamic paging. > It is introduced to resolve X64 problem, where Intel CPU only support 2M paging, but large physical memory. We cannot support build static 2M paging inside of SMRAM for X64. > So we introduce dynamic paging to support this configuration. > > For IA32, 5 pages are enough to cover 4G, in 2M paging. As such, I do not see the value to provide dynamic paging in Ia32. > > Or I could say: this #2 is only valid for X64. No need for IA32. That's fine, it's just whatever solution we choose, should not break compilation or behavior on IA32. Also, preferably, if a PCD only makes sense on X64, then it should be declared in the DEC file as such, so that IA32-only code referencing the PCD not even compile. Otherwise confusion will result (people looking at the IA32-only code will ask themselves, "what should I do about this PCD here"?) >>> 3) StaticPaging=true, AccessOut=false: it seems valid. The is secure >>> configuration. >> >> Agreed. >> >> >>> 4) StaticPaging=true, AccessOut=true: it seems valid, but I do not see >>> the value to support this. If we always allow access out, what is the >>> value to set static paging. Or why we care the paging is static or >>> dynamic? >> >> - the IA32 binary constructs all tables in advance, and it might want to >> interact with the RAS controller in question. >> >> - the X64 binary wants to allocate the SMRAM for page tables in advance >> during boot (and not in the page fault handler), and protect the SMM >> page tables, but still interact with the RAS controller through normal >> RAM. >> >> Apologies if I'm missing something obvious that invalidates the above >> use cases. > > [Jiewen] > Our first focus is for X64 when we design this feature, because RAS is only required by server. > And all RAS servers use X64, I believe. > > I don't think we need consider this case. > > > All in all, to make it easy, I would suggest to limit the configuration below: > A) For IA32, we only consider #3. > B) For X64, we consider #3 as default. Only back to #2 for RAS platform. I guess that works for me. How about the following: (1) rename PcdCpuSmmStaticPageTable to PcdCpuSmmRestrictedMemoryAccess; default value TRUE. (2) move the PCD from [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] to [PcdsFixedAtBuild.X64, PcdsPatchableInModule.X64, PcdsDynamic.X64, PcdsDynamicEx.X64] in the DEC file (3) update the documentation on the PCD precisely, in the DEC file -- describe all aspects and consequences. Also describe that on IA32, the PCD should be considered constant TRUE (4) OK, so this is another interesting issue: in the documentation of "PcdCpuSmmProfileEnable", it is stated that "PcdCpuSmmStaticPageTable" must be disabled for enabling "PcdCpuSmmProfileEnable". That is *already* contradictory (without the feature being added), because IA32 always uses statically built page tables, but the IA32 *code* actually honors PcdCpuSmmProfileEnable. Therefore, the documentation on "PcdCpuSmmProfileEnable" has to be updated first, separately, to reflect reality. This should be a separate patch. And then it can be updated for PcdCpuSmmRestrictedMemoryAccess. (5) rename mCpuSmmStaticPageTable in the code to mCpuSmmRestrictedMemoryAccess. (6) If some common code (built for both IA32 and X64) needs to depend on this variable, then a helper function is need; returning constant TRUE on IA32 and returning mCpuSmmRestrictedMemoryAccess on X64. Thanks Laszlo