> 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 (#44709): https://edk2.groups.io/g/devel/message/44709 Mute This Topic: https://groups.io/mt/32668874/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
