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.88, mailfrom: jiewen.yao@intel.com) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by groups.io with SMTP; Wed, 31 Jul 2019 20:10:46 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 31 Jul 2019 20:10:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,332,1559545200"; d="scan'208";a="184031751" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga002.jf.intel.com with ESMTP; 31 Jul 2019 20:10:45 -0700 Received: from fmsmsx115.amr.corp.intel.com (10.18.116.19) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 31 Jul 2019 20:10:44 -0700 Received: from shsmsx105.ccr.corp.intel.com (10.239.4.158) by fmsmsx115.amr.corp.intel.com (10.18.116.19) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 31 Jul 2019 20:10:44 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.19]) by SHSMSX105.ccr.corp.intel.com ([169.254.11.15]) with mapi id 14.03.0439.000; Thu, 1 Aug 2019 11:10:42 +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//+IowCAAJLnEA== Date: Thu, 1 Aug 2019 03:10:41 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503F739126@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> <74D8A39837DF1E4DA445A8C0B3885C503F738DCC@shsmsx102.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C266CC5@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C266CC5@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 SmmAccessOut =3D SMM access memory outside SMRAM. So, do we want to treat SMM access ACPI NVS, RSVD, Runtime, MMIO, to be Sm= mAccessOut?=20 Thank you Yao Jiewen > -----Original Message----- > From: Ni, Ray > Sent: Thursday, August 1, 2019 10:24 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 > Hi Jiewen, >=20 > > > > 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 al= lows SMM > access RSVD, ACPINVS, Runtime, as comm buffer, > > plus MMIO for device access. That is secure configuration. > > 2) it is FALSE, it allows SMM to access OS memory. It could be static = or > dynamic paging. > > > > > > PcdCpuSmmAccessOut seems also confusing. > > =09What "Out" means ??? > > =09What Out=3DFalse means? Only allow inside SMRAM access? > > Anyway, I am open for the naming proposal. >=20 > SmmAccessOut =3D SMM access memory outside SMRAM. > My experience is sometimes someone may think up a personally preferred > name but may be very confusing to others. I also want to avoid that. >=20 > Do you have a name proposal? >=20 > > > > > > > > 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 > > > > > > > Below is my thought, please correct if I am wrong. > > > > 1) StaticPaging=3Dfalse, AccessOut=3Dfalse: it seems invalid. If w= e 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 dynamic paging. This is to support Hotplug memory. > > > > 3) StaticPaging=3Dtrue, AccessOut=3Dfalse: it seems valid. The is = secure > > > 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 > static > > > or dynamic? > > > > > > > > > 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. > > > > > > > > > > > As such I recommend we only support #2 and #3. > > > > > > Supporting #2 and #3 is based on real requirement, not from above > > > discussion of 4 combinations. > > > > > > > > > > > Again, if the naming is confusing, I agree we should clarify or ev= en > rename. > > > > > > I also agree that having fewer PCDs to provide fewer configurations.= It > also > > > reduces validation effort. > > > > > > Jiewen & Laszlo, > > > Do you agree that with only #2 and #3 supported, the existing PCD ca= n be > > > renamed to PcdCpuSmmAccessOut? > > > If AccessOut=3Dtrue, it implies static paging is not used. > > > If AccessOut=3Dfalse, it implies static paging is used. > > > > > > 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 ad= d > > > confusing/unnecessary-complexity due to this goal. > > > > > > > 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:1= 3=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 al= l. > > > > >> > > > > >> 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 supporte= d > > > address field. > > > > > > > > > > The above looks correct; however, the PcdGetBool() call depends = on > the > > > > > INF file listing PcdCpuSmmAccessOut. > > > > > > > > > > (1) Ray, did you forget to stage the INF file update for this pa= tch > 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 > MMIO? > > > > > > > > > > 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= still > > > > > conditionalize SetPageTableAttributes() on the *other* (static p= aging) > > > 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 they > > > > > 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 pa= tch > > > > > 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-o= ut > > > > > permitted" case, so there is no need to consider "access-out" in= the > > > > > fault handler at all; > > > > > > > > > > - the "mSmmAccessOut" logic in PerformRemainingTasks() applies t= o > > > both > > > > > IA32 and X64 (commonly), and the SmiPFHandler() function is > > > implemented > > > > > for both IA32 and X64 (separately), and after this patch, both f= ault > > > > > handlers check IsSmmCommBufferForbiddenAddress() identically. So > this > > > 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 = pages > > > > > not yet mapped at all, and the X64 variant of SmiDefaultPFHandle= r() > > > > > 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 t= he > > > > > 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 rela= x > > > > > access-out by eliminating *such* page faults, rather than fixing= them > up > > > > > in the fault handler? > > > > > > > > > > (I certainly agree with this approach: the fixup in the fault ha= ndler, > > > > > namely SmiDefaultPFHandler(), only covers the X64 case; in the I= A32 > > > > > case, it just hangs. That shows that the fault fixup is inherent= ly tied > > > > > to on-demand page table building, whereas the access-out feature > > > should > > > > > be possible to permit on IA32 too.) > > > > > > > > > > Thanks, > > > > > Laszlo > > > > > > > > > >=20 > > > > >