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; Thu, 01 Aug 2019 19:04:36 -0700 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DCB5CA3B6E; Fri, 2 Aug 2019 02:04:35 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-67.ams2.redhat.com [10.36.116.67]) by smtp.corp.redhat.com (Postfix) with ESMTP id 517E0600C6; Fri, 2 Aug 2019 02:04:34 +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> From: "Laszlo Ersek" Message-ID: Date: Fri, 2 Aug 2019 04:04:33 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Fri, 02 Aug 2019 02:04:35 +0000 (UTC) Content-Type: text/plain; charset=iso-2022-jp Content-Language: en-US Content-Transfer-Encoding: 7bit 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. Makes sense. In that case, the comment on the "access-out disabled" case should mention MMIO. > 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=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? > 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. > 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. > 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. I agree. IMO: - 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). Thanks Laszlo