Hi Jiewen, > > 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 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. > What "Out" means ??? > What Out=False means? Only allow inside SMRAM access? > Anyway, I am open for the naming proposal.
SmmAccessOut = 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. Do you have a name proposal? > > > > Thank you > Yao Jiewen > > > -----Original Message----- > > From: Ni, Ray > > Sent: Thursday, August 1, 2019 9:28 AM > > To: Yao, Jiewen <[email protected]>; [email protected]; > > [email protected] > > Cc: Dong, Eric <[email protected]>; Wang, Jian J <[email protected]> > > 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=false, AccessOut=false: it seems invalid. If we don’t > > support access out, why we need dynamic paging? > > > 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. > > > 3) StaticPaging=true, AccessOut=false: it seems valid. The is secure > > configuration. > > > 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? > > > > > > 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 even > > > 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 can be > > renamed to PcdCpuSmmAccessOut? > > If AccessOut=true, it implies static paging is not used. > > If AccessOut=false, 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 add > > 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 > > > > > > > > > > 在 2019年8月1日，上午7:13，Laszlo Ersek <[email protected]> 写 > > 道： > > > > > > > > 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 <[email protected]> > > > >> Cc: Eric Dong <[email protected]> > > > >> Cc: Jiewen Yao <[email protected]> > > > >> Cc: Jian J Wang <[email protected]> > > > >> Cc: Laszlo Ersek <[email protected]> > > > >> --- > > > >> 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 = PcdGetBool > > (PcdCpuSmmCodeAccessCheckEnable); > > > >> DEBUG ((EFI_D_INFO, "PcdCpuSmmCodeAccessCheckEnable = %d\n", > > mSmmCodeAccessCheckEnable)); > > > >> > > > >> + // > > > >> + // Save the PcdCpuSmmAccessOut value into a global variable. > > > >> + // > > > >> + mSmmAccessOut = PcdGetBool (PcdCpuSmmAccessOut); > > > >> + DEBUG ((DEBUG_INFO, "PcdCpuSmmAccessOut = %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 the > > > > 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 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 > > > > > > > > > > [email protected]">http://mid.mail-archive.com/[email protected] > > > > 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 paging) > > 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 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 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 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 (= > > 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 them up > > > > in the fault handler? > > > > > > > > (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.) > > > > > > > > Thanks, > > > > Laszlo > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#44711): https://edk2.groups.io/g/devel/message/44711 Mute This Topic: https://groups.io/mt/32668874/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
