[edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu: PcdCpuSmmAccessOut controls SMM access-out policy
Ni, Ray
ray.ni at intel.com
Thu Aug 1 01:27:56 UTC 2019
> 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 <lersek at redhat.com> 写道：
> >
> > 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 <ray.ni at intel.com>
> >> Cc: Eric Dong <eric.dong at intel.com>
> >> Cc: Jiewen Yao <jiewen.yao at intel.com>
> >> Cc: Jian J Wang <jian.j.wang at intel.com>
> >> Cc: Laszlo Ersek <lersek at redhat.com>
> >> ---
> >> 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
> >
> > 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 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list