From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.115, mailfrom: jiewen.yao@intel.com) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by groups.io with SMTP; Fri, 02 Aug 2019 19:23:14 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 02 Aug 2019 19:23:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,340,1559545200"; d="scan'208";a="373134851" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga006.fm.intel.com with ESMTP; 02 Aug 2019 19:23:13 -0700 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Fri, 2 Aug 2019 19:23:13 -0700 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.439.0; Fri, 2 Aug 2019 19:23:13 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.19]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.163]) with mapi id 14.03.0439.000; Sat, 3 Aug 2019 10:23:11 +0800 From: "Yao, Jiewen" To: Laszlo Ersek CC: "devel@edk2.groups.io" , "Ni, Ray" , "Dong, Eric" , "Wang, Jian J" Subject: Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu: PcdCpuSmmAccessOut controls SMM access-out policy Thread-Topic: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu: PcdCpuSmmAccessOut controls SMM access-out policy Thread-Index: AQHVR76OWnkpDVK960WtOFbChOOq/Kbk1Y8AgACT3OCAAS48gIAAjWzAgADCUQCAAM3oQA== Date: Sat, 3 Aug 2019 02:23:10 +0000 Message-ID: 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>,<39df5f13-547c-4d16-39d1-82ae9edaa419@redhat.com> In-Reply-To: <39df5f13-547c-4d16-39d1-82ae9edaa419@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: MIME-Version: 1.0 Return-Path: jiewen.yao@intel.com Content-Language: zh-CN Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: quoted-printable Thanks and agree.=20 Comment inlined. thank you! Yao, Jiewen > =1B$B:_=1B(B 2019=1B$BG/=1B(B8=1B$B7n=1B(B3=1B$BF|!$>e8a=1B(B6:06=1B$B!$= =1B(BLaszlo Ersek =1B$B=20 >> On 08/02/19 04:46, Yao, Jiewen wrote: >> Thanks Laszlo. Comment below: >>=20 >>> -----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 >>>=20 >>> On 08/01/19 02:02, Yao, Jiewen wrote: >=20 >>>> 1) StaticPaging=3Dfalse, AccessOut=3Dfalse: it seems invalid. If we >>>> don't support access out, why we need dynamic paging? >>>=20 >>> 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. >>>=20 >>> The first patch that distinguished "static page tables" from "dynamic >>> page tables" was 28b020b5de1e ("UefiCpuPkg/dec: Add >>> PcdCpuSmmStaticPageTable.", 2016-11-17) >>>=20 >>> 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? >>=20 >> [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. >=20 > OK. I don't mind the RAS requirement as long as option #3 remains viable. >=20 >>>> 2) StaticPaging=3Dfalse, AccessOut=3Dtrue: it seems valid. We need acc= ess >>>> out, but we only want a small paging in the beginning. As such we use >>>> dynamic paging. This is to support Hotplug memory. >>>=20 >>> This scenario doesn't look supportable on IA32. (Due to >>> StaticPaging=3Dfalse.) I don't mean that it's impossible to implement, >>> just that the IA32 code today doesn't extend the page tables in respons= e >>> to page faults. >>=20 >> [Jiewen] You are right. >>=20 >> 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 pagin= g inside of SMRAM for X64. >> So we introduce dynamic paging to support this configuration. >>=20 >> For IA32, 5 pages are enough to cover 4G, in 2M paging. As such, I do no= t see the value to provide dynamic paging in Ia32. >>=20 >> Or I could say: this #2 is only valid for X64. No need for IA32. >=20 > That's fine, it's just whatever solution we choose, should not break > compilation or behavior on IA32. >=20 > 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"?) [jiewen] make sense. Agree.=20 >=20 >=20 >>>> 3) StaticPaging=3Dtrue, AccessOut=3Dfalse: it seems valid. The is secu= re >>>> configuration. >>>=20 >>> Agreed. >>>=20 >>>=20 >>>> 4) StaticPaging=3Dtrue, AccessOut=3Dtrue: 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? >>>=20 >>> - the IA32 binary constructs all tables in advance, and it might want t= o >>> interact with the RAS controller in question. >>>=20 >>> - 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. >>>=20 >>> Apologies if I'm missing something obvious that invalidates the above >>> use cases. >>=20 >> [Jiewen] >> Our first focus is for X64 when we design this feature, because RAS is o= nly required by server. >> And all RAS servers use X64, I believe. >>=20 >> I don't think we need consider this case. >>=20 >>=20 >> 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. >=20 > I guess that works for me. How about the following: >=20 > (1) rename PcdCpuSmmStaticPageTable to PcdCpuSmmRestrictedMemoryAccess; > default value TRUE. [jiewen] agree.=20 I also recommend add some comments say this is the rename to old PcdCpuSmmS= taticPageTable.=20 >=20 > (2) move the PCD from >=20 > [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] >=20 > to >=20 > [PcdsFixedAtBuild.X64, PcdsPatchableInModule.X64, PcdsDynamic.X64, > PcdsDynamicEx.X64] >=20 > in the DEC file [jiewen] make sense. agree.=20 >=20 >=20 > (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 [jiewen] agree. > (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. >=20 > 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. [jiewen] agree. I think we can add more description on that. PcdCpuSmmProfileEnable should a debug feature only and never be used for pr= oduction.=20 While PcdCpuSmmRestrictedMemoryAccess is for production.=20 >=20 >=20 > (5) rename mCpuSmmStaticPageTable in the code to > mCpuSmmRestrictedMemoryAccess.=20 [jiewen] agree > (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. [jiewen] agree.=20 >=20 >=20 > Thanks > Laszlo