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.136, mailfrom: jiewen.yao@intel.com) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by groups.io with SMTP; Wed, 31 Jul 2019 17:02:53 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 31 Jul 2019 17:02:52 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,332,1559545200"; d="scan'208";a="163348578" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga007.jf.intel.com with ESMTP; 31 Jul 2019 17:02:52 -0700 Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 31 Jul 2019 17:02:50 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX154.amr.corp.intel.com (10.18.116.70) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 31 Jul 2019 17:02:50 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.19]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.139]) with mapi id 14.03.0439.000; Thu, 1 Aug 2019 08:02:49 +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/Kbk1Y8AgACT3OA= Date: Thu, 1 Aug 2019 00:02:48 +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> In-Reply-To: <50b073a1-bf04-4b8a-e267-2af933d34c5c@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 laszlo, for the detail review I have not gone through every line of code in detail. Some comment in gene= ral.=20 To answer Laszlo=1B$B!G=1B(Bs question on Mmio.=20 No, the Mmio cannot be used as communication buffer. But the smm must setu= p page table for it because the smm device driver may need access it.=20 I am not sure the difference between Mmio and runtime Mmio. Runtime is onl= y useful concept for Uefi, but not for Smm.=20 Back to this patch itself. I feel *guilty* when I see a new pcd introduced to control the code flow.= =20 That means the possible number of code path is doubled. Sigh... The first question is: what unit test has been run?=20 Does the unit test cover all possible true/false combination with other PC= D? We have a big table to describe all legal or illegal combination for the p= cd to support memory protection. I think we have to update that table if we= decide to add the new one.=20 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.=20 If the number is 3, I recommend to use enum type instead of a new Boolean.= = =20 If the number is 2, I don=1B$B!G=1B(Bt recommend we add new Boolean at all= . We can reinterpret the existing one.=20 Below is my thought, please correct if I am wrong.=20 1) StaticPaging=3Dfalse, AccessOut=3Dfalse: it seems invalid. If we don=1B= $B!G=1B(Bt support access out, why we need dynamic paging? 2) StaticPaging=3Dfalse, AccessOut=3Dtrue: it seems valid. We need access = out, but we only want a small paging in the beginning. As such we use dynam= ic paging. This is to support Hotplug memory. 3) StaticPaging=3Dtrue, AccessOut=3Dfalse: it seems valid. The is secure c= onfiguration. 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 valu= e to set static paging. Or why we care the paging is static or dynamic? As such I recommend we only support #2 and #3. Again, if the naming is confusing, I agree we should clarify or even renam= e.=20 What I am trying to achieve is to limit the number of supported combinatio= n to reduce the effort of validation and maintenance.=20 thank you! Yao, Jiewen > =1B$B:_=1B(B 2019=1B$BG/=1B(B8=1B$B7n=1B(B1=1B$BF|!$>e8a=1B(B7:13=1B$B!$= = =1B(BLaszlo Ersek =1B$B=20 > Hi Ray, Jiewen, >=20 > I've got several comments / questions: >=20 >> On 07/31/19 18:38, Ni, Ray wrote: >> This patch skips to update page table for non-SMRAM memory when >> PcdCpuSmmAccessOut is TRUE. >> So that when SMM accesses out, no page fault is triggered at all. >>=20 >> Signed-off-by: Ray Ni >> Cc: Eric Dong >> Cc: Jiewen Yao >> Cc: Jian J Wang >> Cc: Laszlo Ersek >> --- >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 21 +++++++++++++++++---- >> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 2 +- >> 2 files changed, 18 insertions(+), 5 deletions(-) >>=20 >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/Pi= SmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> index 69a04dfb23..427c33fb01 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c >> @@ -130,6 +130,11 @@ UINT8 mPhysicalAddressBits; >> UINT32 mSmmCr0; >> UINT32 mSmmCr4; >>=20 >> +// >> +// Cache of PcdCpuSmmAccessOut >> +// >> +BOOLEAN mSmmAccessOut; >> + >> /** >> Initialize IDT to setup exception handlers for SMM. >>=20 >> @@ -610,6 +615,12 @@ PiCpuSmmEntry ( >> mSmmCodeAccessCheckEnable =3D PcdGetBool (PcdCpuSmmCodeAccessCheckEna= ble); >> DEBUG ((EFI_D_INFO, "PcdCpuSmmCodeAccessCheckEnable =3D %d\n", mSmmCo= deAccessCheckEnable)); >>=20 >> + // >> + // Save the PcdCpuSmmAccessOut value into a global variable. >> + // >> + mSmmAccessOut =3D PcdGetBool (PcdCpuSmmAccessOut); >> + DEBUG ((DEBUG_INFO, "PcdCpuSmmAccessOut =3D %d\n", mSmmAccessOut)); >> + >> // >> // Save the PcdPteMemoryEncryptionAddressOrMask value into a global v= ariable. >> // Make sure AddressEncMask is contained to smallest supported addres= s field. >=20 > The above looks correct; however, the PcdGetBool() call depends on the > INF file listing PcdCpuSmmAccessOut. >=20 > (1) Ray, did you forget to stage the INF file update for this patch comm= it? >=20 >=20 >> @@ -1431,10 +1442,12 @@ PerformRemainingTasks ( >> // >> SetMemMapAttributes (); >>=20 >> - // >> - // For outside SMRAM, we only map SMM communication buffer or MMIO= . >> - // >> - SetUefiMemMapAttributes (); >> + if (!mSmmAccessOut) { >> + // >> + // For outside SMRAM, we only map SMM communication buffer or MM= IO. >> + // >> + SetUefiMemMapAttributes (); >> + } >=20 > This change looks OK. It seems to conditionalize the logic added in > commit d2fc7711136a ("UefiCpuPkg/PiSmmCpu: Add SMM Comm Buffer Paging > Protection.", 2016-12-19). >=20 > However, the comment confuses me a bit; it says "SMM communication > buffer *or MMIO*". >=20 > I assume "SMM communication buffer" can be in "reserved, runtime and > ACPI NVS" RAM, so that part likely matches the new PCD's explanation > from patch#1. But MMIO is not mentioned in patch#1. >=20 > (2) Should we extend the description of the PCD in patch#1, with MMIO? >=20 > Or is MMIO considered *runtime* MMIO (and so covered by "runtime")? >=20 >>=20 >> // >> // Set page table itself to be read-only >=20 > In the previous version of the patch series, namely >=20 > [edk2-devel] [PATCH 3/3] > UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is OFF >=20 > http://mid.mail-archive.com/20190727032850.337840-4-ray.ni@intel.com > https://edk2.groups.io/g/devel/message/44470 >=20 > the next operation (namely SetPageTableAttributes()) was conditionalized > too. Is that no longer necessary, with the new PCD? Shouldn't we still > conditionalize SetPageTableAttributes() on the *other* (static paging) P= CD? >=20 > Ah wait, we already do it! SetPageTableAttributes() has two > implementations, one for IA32 and another for X64. >=20 > - The X64 variant checks for static page tables internally, and if they > are disabled, then SetPageTableAttributes() does nothing. >=20 > - The IA32 variant does not contain that check, because on IA32 the page > tables are always built statically. >=20 > So SetPageTableAttributes() already depends on static paging > *internally*, hence gating the SetPageTableAttributes() call > *externally*, like it was done in the previous version of the patch > series, is superfluous in the first place. >=20 > Good! >=20 >=20 >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmm= CpuDxeSmm/X64/PageTbl.c >> index a3b62f7787..6699aac65d 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c >> @@ -1029,7 +1029,7 @@ SmiPFHandler ( >> goto Exit; >> } >>=20 >> - if (mCpuSmmStaticPageTable && IsSmmCommBufferForbiddenAddress (PFA= ddress)) { >> + if (IsSmmCommBufferForbiddenAddress (PFAddress)) { >> DumpCpuContext (InterruptType, SystemContext); >> DEBUG ((DEBUG_ERROR, "Access SMM communication forbidden address = (0x%lx)!\n", PFAddress)); >> DEBUG_CODE ( >>=20 >=20 > This hunk looks good. Because: >=20 > - we ensure that there is no page fault at all, in the "access-out > permitted" case, so there is no need to consider "access-out" in the > fault handler at all; >=20 > - the "mSmmAccessOut" logic in PerformRemainingTasks() applies to both > IA32 and X64 (commonly), and the SmiPFHandler() function is implemented > for both IA32 and X64 (separately), and after this patch, both fault > handlers check IsSmmCommBufferForbiddenAddress() identically. So this is > nice and symmetric; >=20 > - we don't interfere with on-demand page table building (when it is > enabled on X64) -- page faults should still be triggered by RAM pages > not yet mapped at all, and the X64 variant of SmiDefaultPFHandler() > should fix up *those* faults like before. >=20 >=20 > However: given that this hunk practically undes commit c60d36b4, I would > suggest that we revert commit c60d36b4 in a separate patch. So the > series would go like: >=20 > - patch#1: commit c60d36b4 is not good enough, we're going to implement > a separate approach, so revert commit c60d36b4, at first. > - patch#2: introduce the new PCD > - patch#3: disable page fault generation for non-SMRAM addresses (=3D ke= ep > them mapped as normal) to which access-out is permitted. >=20 > (3) Ray, do you agree to structure the patch series like that? >=20 > (4) Jiewen, are you OK with the general approach, namely to relax > access-out by eliminating *such* page faults, rather than fixing them up > in the fault handler? >=20 > (I certainly agree with this approach: the fixup in the fault handler, > namely SmiDefaultPFHandler(), only covers the X64 case; in the IA32 > case, it just hangs. That shows that the fault fixup is inherently tied > to on-demand page table building, whereas the access-out feature should > be possible to permit on IA32 too.) >=20 > Thanks, > Laszlo >=20 >=20 >=20