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.43, mailfrom: jiewen.yao@intel.com) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by groups.io with SMTP; Thu, 01 Aug 2019 19:46:29 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 01 Aug 2019 19:46:28 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,336,1559545200"; d="scan'208";a="173101488" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga008.fm.intel.com with ESMTP; 01 Aug 2019 19:46:28 -0700 Received: from fmsmsx606.amr.corp.intel.com (10.18.126.86) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 1 Aug 2019 19:46:28 -0700 Received: from fmsmsx606.amr.corp.intel.com (10.18.126.86) by fmsmsx606.amr.corp.intel.com (10.18.126.86) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Thu, 1 Aug 2019 19:46:28 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx606.amr.corp.intel.com (10.18.126.86) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Thu, 1 Aug 2019 19:46:27 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.19]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.62]) with mapi id 14.03.0439.000; Fri, 2 Aug 2019 10:46:26 +0800 From: "Yao, Jiewen" To: "devel@edk2.groups.io" , "lersek@redhat.com" CC: "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/Kbk1Y8AgACT3OCAAS48gIAAjWzA Date: Fri, 2 Aug 2019 02:46:26 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503F73CFD5@shsmsx102.ccr.corp.intel.com> References: <20190731163852.191708-1-ray.ni@intel.com> <20190731163852.191708-3-ray.ni@intel.com> <50b073a1-bf04-4b8a-e267-2af933d34c5c@redhat.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMTY2N2UyM2EtMWNlYi00YTY0LWJlNGQtNzE0YTEwYTM1YTYzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoieHVvUU1NT2hjNmlNZnJtZFE3dllYaENkKzNPY1g2SVwvR3BxN29CYmQ1ZDRWbGJEYk5FV2J6MmN5RkFtdXQwd3EifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: jiewen.yao@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 >=20 > On 08/01/19 02:02, Yao, Jiewen wrote: > > thanks laszlo, for the detail review > > > > I have not gone through every line of code in detail. Some comment in > > general. > > > > To answer Laszlo's question on Mmio. > > No, the Mmio cannot be used as communication buffer. But the smm must > > setup page table for it because the smm device driver may need access > > it. >=20 > Makes sense. In that case, the comment on the "access-out disabled" case > should mention MMIO. >=20 >=20 > > I am not sure the difference between Mmio and runtime Mmio. Runtime is > > only useful concept for Uefi, but not for Smm. > > > > Back to this patch itself. > > I feel *guilty* when I see a new pcd introduced to control the code > > flow. > > That means the possible number of code path is doubled. Sigh... > > > > The first question is: what unit test has been run? > > Does the unit test cover all possible true/false combination with > > other PCD? > > We have a big table to describe all legal or illegal combination for > > the pcd to support memory protection. I think we have to update that > > table if we decide to add the new one. > > > > Maybe we can think of what is the supported case. Maybe we use an > > *enum* to indicate the supported cases to reduce the number. > > If the number is 4, no difference. > > If the number is 3, I recommend to use enum type instead of a new > > Boolean. > > If the number is 2, I don't recommend we add new Boolean at all. We > > can reinterpret the existing one. > > > > > > > > Below is my thought, please correct if I am wrong. > > 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? [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 >=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 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 p= aging, 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. >=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 to > 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. [Jiewen] Our first focus is for X64 when we design this feature, because RAS is onl= y 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 be= low: A) For IA32, we only consider #3. B) For X64, we consider #3 as default. Only back to #2 for RAS platform. >=20 > > As such I recommend we only support #2 and #3. > > > > Again, if the naming is confusing, I agree we should clarify or even > > rename. > > What I am trying to achieve is to limit the number of supported > > combination to reduce the effort of validation and maintenance. >=20 > I agree. IMO: >=20 > - we should represent separate concepts separately, > - exclude those combinations that make no sense > (with an ASSERT + appropriate comment) > - exclude those combinations that make sense but are > unimportant or too complex to support > (with a different ASSERT + proper comment). >=20 > Thanks > Laszlo >=20 >=20