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 18:38:38 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 31 Jul 2019 18:38:38 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,332,1559545200"; d="scan'208";a="324009601" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga004.jf.intel.com with ESMTP; 31 Jul 2019 18:38:37 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 31 Jul 2019 18:38:37 -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 09:38:35 +0800 From: "Yao, Jiewen" To: "Ni, Ray" , "devel@edk2.groups.io" , "lersek@redhat.com" CC: "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/Kbk1Y8AgACT3OD//5GsAIAAhuYg Date: Thu, 1 Aug 2019 01:38:34 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503F738DCC@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> <734D49CCEBEEF84792F5B80ED585239D5C266BC4@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C266BC4@SHSMSX104.ccr.corp.intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYWViNTU5ZDctYzNiNi00OWUxLWJlZjgtZmJkZWViZjU1YzkxIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiSGQ0RHN0ZW44OFZ3Vm1taGdZSEg3M3BJY1FmN2Z3V2ZsejVpaWMyZkx0bVkxNVhuZ1R0Y2FCNnA5WDJaaXJHQyJ9 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="iso-2022-jp" Content-Transfer-Encoding: quoted-printable I agree that we only need support #2 and #3. We need one PCD: 1) if it is set to TRUE, it locks SMM paging (make it static), only allows= SMM access RSVD, ACPINVS, Runtime, as comm buffer, plus MMIO for device ac= cess. That is secure configuration. 2) it is FALSE, it allows SMM to access OS memory. It could be static or d= ynamic paging. PcdCpuSmmAccessOut seems also confusing. What "Out" means ??? What Out=3DFalse means? Only allow inside SMRAM access? Anyway, I am open for the naming proposal. Thank you Yao Jiewen > -----Original Message----- > From: Ni, Ray > Sent: Thursday, August 1, 2019 9:28 AM > To: Yao, Jiewen ; devel@edk2.groups.io; > lersek@redhat.com > Cc: Dong, Eric ; Wang, Jian J > Subject: RE: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu: > PcdCpuSmmAccessOut controls SMM access-out policy >=20 > > Below is my thought, please correct if I am wrong. > > 1) StaticPaging=3Dfalse, AccessOut=3Dfalse: it seems invalid. If we do= n=1B$B!G=1B(Bt > support access out, why we need dynamic paging? > > 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. > > 3) StaticPaging=3Dtrue, AccessOut=3Dfalse: it seems valid. The is secu= re > configuration. > > 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 s= tatic > or dynamic? >=20 >=20 > Jiewen, > The value of static paging is to reduce page table SMRAM consumption. We > could create the page table in advance and that > page table permits smm access out. >=20 > > > > As such I recommend we only support #2 and #3. >=20 > Supporting #2 and #3 is based on real requirement, not from above > discussion of 4 combinations. >=20 > > > > Again, if the naming is confusing, I agree we should clarify or even r= ename. >=20 > I also agree that having fewer PCDs to provide fewer configurations. It = also > reduces validation effort. >=20 > Jiewen & Laszlo, > Do you agree that with only #2 and #3 supported, the existing PCD can be > renamed to PcdCpuSmmAccessOut? > If AccessOut=3Dtrue, it implies static paging is not used. > If AccessOut=3Dfalse, it implies static paging is used. >=20 > My goal is to have a way to allow RAS code access out from SMM after > ReadyToLock. > Any solution that can meet this goal is ok to me. I don't want to add > confusing/unnecessary-complexity due to this goal. >=20 > > What I am trying to achieve is to limit the number of supported > combination to reduce the effort of validation and maintenance. > > > > 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 =1B$BF;!'=1B(B > > > > > > Hi Ray, Jiewen, > > > > > > I've got several comments / questions: > > > > > >> 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. > > >> > > >> 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(-) > > >> > > >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/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; > > >> > > >> +// > > >> +// Cache of PcdCpuSmmAccessOut > > >> +// > > >> +BOOLEAN mSmmAccessOut; > > >> + > > >> /** > > >> Initialize IDT to setup exception handlers for SMM. > > >> > > >> @@ -610,6 +615,12 @@ PiCpuSmmEntry ( > > >> mSmmCodeAccessCheckEnable =3D PcdGetBool > (PcdCpuSmmCodeAccessCheckEnable); > > >> DEBUG ((EFI_D_INFO, "PcdCpuSmmCodeAccessCheckEnable =3D %d\n", > mSmmCodeAccessCheckEnable)); > > >> > > >> + // > > >> + // 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 variable. > > >> // Make sure AddressEncMask is contained to smallest supported > address field. > > > > > > The above looks correct; however, the PcdGetBool() call depends on t= he > > > INF file listing PcdCpuSmmAccessOut. > > > > > > (1) Ray, did you forget to stage the INF file update for this patch = commit? > > > > > > > > >> @@ -1431,10 +1442,12 @@ PerformRemainingTasks ( > > >> // > > >> SetMemMapAttributes (); > > >> > > >> - // > > >> - // For outside SMRAM, we only map SMM communication buffer > or MMIO. > > >> - // > > >> - SetUefiMemMapAttributes (); > > >> + if (!mSmmAccessOut) { > > >> + // > > >> + // For outside SMRAM, we only map SMM communication > buffer or MMIO. > > >> + // > > >> + SetUefiMemMapAttributes (); > > >> + } > > > > > > 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). > > > > > > However, the comment confuses me a bit; it says "SMM communication > > > buffer *or MMIO*". > > > > > > 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. > > > > > > (2) Should we extend the description of the PCD in patch#1, with MMI= O? > > > > > > Or is MMIO considered *runtime* MMIO (and so covered by "runtime")? > > > > > >> > > >> // > > >> // Set page table itself to be read-only > > > > > > In the previous version of the patch series, namely > > > > > > [edk2-devel] [PATCH 3/3] > > > UefiCpuPkg/PiSmmCpu: Allow SMM access-out when static paging is > OFF > > > > > > > http://mid.mail-archive.com/20190727032850.337840-4-ray.ni@intel.com > > > https://edk2.groups.io/g/devel/message/44470 > > > > > > the next operation (namely SetPageTableAttributes()) was > conditionalized > > > too. Is that no longer necessary, with the new PCD? Shouldn't we sti= ll > > > conditionalize SetPageTableAttributes() on the *other* (static pagin= g) > PCD? > > > > > > Ah wait, we already do it! SetPageTableAttributes() has two > > > implementations, one for IA32 and another for X64. > > > > > > - The X64 variant checks for static page tables internally, and if t= hey > > > are disabled, then SetPageTableAttributes() does nothing. > > > > > > - The IA32 variant does not contain that check, because on IA32 the = page > > > tables are always built statically. > > > > > > 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. > > > > > > Good! > > > > > > > > >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/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; > > >> } > > >> > > >> - if (mCpuSmmStaticPageTable && > IsSmmCommBufferForbiddenAddress (PFAddress)) { > > >> + if (IsSmmCommBufferForbiddenAddress (PFAddress)) { > > >> DumpCpuContext (InterruptType, SystemContext); > > >> DEBUG ((DEBUG_ERROR, "Access SMM communication > forbidden address (0x%lx)!\n", PFAddress)); > > >> DEBUG_CODE ( > > >> > > > > > > This hunk looks good. Because: > > > > > > - 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; > > > > > > - 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 thi= s > is > > > nice and symmetric; > > > > > > - we don't interfere with on-demand page table building (when it is > > > enabled on X64) -- page faults should still be triggered by RAM page= s > > > not yet mapped at all, and the X64 variant of SmiDefaultPFHandler() > > > should fix up *those* faults like before. > > > > > > > > > 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: > > > > > > - 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 > keep > > > them mapped as normal) to which access-out is permitted. > > > > > > (3) Ray, do you agree to structure the patch series like that? > > > > > > (4) Jiewen, are you OK with the general approach, namely to relax > > > access-out by eliminating *such* page faults, rather than fixing the= m up > > > in the fault handler? > > > > > > (I certainly agree with this approach: the fixup in the fault handle= r, > > > namely SmiDefaultPFHandler(), only covers the X64 case; in the IA32 > > > case, it just hangs. That shows that the fault fixup is inherently t= ied > > > to on-demand page table building, whereas the access-out feature > should > > > be possible to permit on IA32 too.) > > > > > > Thanks, > > > Laszlo > > > > > >=20 > > >