[edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu: PcdCpuSmmAccessOut controls SMM access-out policy

Wed Jul 31 23:13:36 UTC 2019

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 (#44705): https://edk2.groups.io/g/devel/message/44705
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]
-=-=-=-=-=-=-=-=-=-=-=-