[PATCH v2 0/2] Add new PCD PcdCpuSmmAccessOut to control SMM access out

[PATCH v2 0/2] Add new PCD PcdCpuSmmAccessOut to control SMM access out

[Ni, Ray]

Ray Ni (2):
  UefiCpuPkg: Add PCD PcdCpuSmmAccessOut to control SMM access out
  UefiCpuPkg/PiSmmCpu: PcdCpuSmmAccessOut controls SMM access-out policy

 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 21 +++++++++++++++++----
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c    |  2 +-
 UefiCpuPkg/UefiCpuPkg.dec                  |  7 +++++++
 3 files changed, 25 insertions(+), 5 deletions(-)

-- 
2.21.0.windows.1

[PATCH v2 1/2] UefiCpuPkg: Add PCD PcdCpuSmmAccessOut to control SMM access out

[Ni, Ray]

There is a requirement to allow SMM code access non-SMRAM memory
after ReadyToLock.
The requirement was expected to be satisfied by commit:
c60d36b4d1ee1f69b7cca897d3621dfa951895c2
* UefiCpuPkg/SmmCpu: Block access-out only when static paging is used

Commit c60d36b4 re-interpreted the PcdCpuSmmStaticPageTable as
a way to control whether SMM module can access non-SMRAM memory
after ReadyToLock.
It brought confusion because "static page table" means the page table
is created in advance and there is no dynamic page table modification
at runtime. It only applies to 64bit environment because page table
for memory below 4GB is always created in advance. But the control
of whether allowing SMM module access non-SMRAM memory can also be
applied to 32bit environment.
It makes more sense to have a separate PCD as proposed in this
patch to control the policy.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
---
 UefiCpuPkg/UefiCpuPkg.dec | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 6ddf0cd224..24b44bae39 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -246,6 +246,13 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   # @Prompt Use static page table for all memory in SMM.
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable|TRUE|BOOLEAN|0x3213210D
 
+  ## Controls whether SMM modules can access all non-SMRAM memory after SmmReadyToLock.
+  #   TRUE  - SMM modules can access all non-SMRAM memory after SmmReadyToLock.<BR>
+  #   FALSE - SMM modules can only access reserved, runtime and ACPI NVS type of non-SMRAM memory
+  #           after SmmReadyToLock.<BR>
+  # @Prompt SMM modules can access all non-SMRAM memory after SmmReadyToLock.
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmAccessOut|FALSE|BOOLEAN|0x3213210F
+
   ## Specifies timeout value in microseconds for the BSP in SMM to wait for all APs to come into SMM.
   # @Prompt AP synchronization timeout value in SMM.
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|1000000|UINT64|0x32132104
-- 
2.21.0.windows.1

Re: [edk2-devel] [PATCH v2 1/2] UefiCpuPkg: Add PCD PcdCpuSmmAccessOut to control SMM access out

[Laszlo Ersek]

On 07/31/19 18:38, Ni, Ray wrote:
> There is a requirement to allow SMM code access non-SMRAM memory
> after ReadyToLock.
> The requirement was expected to be satisfied by commit:
> c60d36b4d1ee1f69b7cca897d3621dfa951895c2
> * UefiCpuPkg/SmmCpu: Block access-out only when static paging is used
> 
> Commit c60d36b4 re-interpreted the PcdCpuSmmStaticPageTable as
> a way to control whether SMM module can access non-SMRAM memory
> after ReadyToLock.
> It brought confusion because "static page table" means the page table
> is created in advance and there is no dynamic page table modification
> at runtime. It only applies to 64bit environment because page table
> for memory below 4GB is always created in advance. But the control
> of whether allowing SMM module access non-SMRAM memory can also be
> applied to 32bit environment.
> It makes more sense to have a separate PCD as proposed in this
> patch to control the policy.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  UefiCpuPkg/UefiCpuPkg.dec | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index 6ddf0cd224..24b44bae39 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -246,6 +246,13 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>    # @Prompt Use static page table for all memory in SMM.
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable|TRUE|BOOLEAN|0x3213210D
>  
> +  ## Controls whether SMM modules can access all non-SMRAM memory after SmmReadyToLock.
> +  #   TRUE  - SMM modules can access all non-SMRAM memory after SmmReadyToLock.<BR>
> +  #   FALSE - SMM modules can only access reserved, runtime and ACPI NVS type of non-SMRAM memory
> +  #           after SmmReadyToLock.<BR>
> +  # @Prompt SMM modules can access all non-SMRAM memory after SmmReadyToLock.
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmAccessOut|FALSE|BOOLEAN|0x3213210F
> +
>    ## Specifies timeout value in microseconds for the BSP in SMM to wait for all APs to come into SMM.
>    # @Prompt AP synchronization timeout value in SMM.
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|1000000|UINT64|0x32132104
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [edk2-devel] [PATCH v2 1/2] UefiCpuPkg: Add PCD PcdCpuSmmAccessOut to control SMM access out

[Ni, Ray]

Laszlo,
I appreciate your Reviewed-by on the new PCD creation.

But let's continue discussing on the other mail thread:
[edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu: PcdCpuSmmAccessOut controls SMM access-out policy

Thanks,
Ray

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, August 1, 2019 6:22 AM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2 1/2] UefiCpuPkg: Add PCD
> PcdCpuSmmAccessOut to control SMM access out
> 
> On 07/31/19 18:38, Ni, Ray wrote:
> > There is a requirement to allow SMM code access non-SMRAM memory
> after
> > ReadyToLock.
> > The requirement was expected to be satisfied by commit:
> > c60d36b4d1ee1f69b7cca897d3621dfa951895c2
> > * UefiCpuPkg/SmmCpu: Block access-out only when static paging is used
> >
> > Commit c60d36b4 re-interpreted the PcdCpuSmmStaticPageTable as a way
> > to control whether SMM module can access non-SMRAM memory after
> > ReadyToLock.
> > It brought confusion because "static page table" means the page table
> > is created in advance and there is no dynamic page table modification
> > at runtime. It only applies to 64bit environment because page table
> > for memory below 4GB is always created in advance. But the control of
> > whether allowing SMM module access non-SMRAM memory can also be
> > applied to 32bit environment.
> > It makes more sense to have a separate PCD as proposed in this patch
> > to control the policy.
> >
> > Signed-off-by: Ray Ni <ray.ni@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > ---
> >  UefiCpuPkg/UefiCpuPkg.dec | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> > index 6ddf0cd224..24b44bae39 100644
> > --- a/UefiCpuPkg/UefiCpuPkg.dec
> > +++ b/UefiCpuPkg/UefiCpuPkg.dec
> > @@ -246,6 +246,13 @@ [PcdsFixedAtBuild, PcdsPatchableInModule,
> PcdsDynamic, PcdsDynamicEx]
> >    # @Prompt Use static page table for all memory in SMM.
> >
> >
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable|TRUE|BOOLEAN
> |0x3213
> > 210D
> >
> > +  ## Controls whether SMM modules can access all non-SMRAM memory
> after SmmReadyToLock.
> > +  #   TRUE  - SMM modules can access all non-SMRAM memory after
> SmmReadyToLock.<BR>
> > +  #   FALSE - SMM modules can only access reserved, runtime and ACPI
> NVS type of non-SMRAM memory
> > +  #           after SmmReadyToLock.<BR>
> > +  # @Prompt SMM modules can access all non-SMRAM memory after
> SmmReadyToLock.
> > +
> > +
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmAccessOut|FALSE|BOOLEAN|0x3
> 213210
> > + F
> > +
> >    ## Specifies timeout value in microseconds for the BSP in SMM to wait for
> all APs to come into SMM.
> >    # @Prompt AP synchronization timeout value in SMM.
> >
> >
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|1000000|UINT64
> |0x3213
> > 2104
> >
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

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

[Ni, Ray]

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@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Laszlo Ersek <lersek@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.
@@ -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 ();
+    }
 
     //
     // Set page table itself to be read-only
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 (
-- 
2.21.0.windows.1

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

[Laszlo Ersek]

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@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Laszlo Ersek <lersek@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

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

[Laszlo Ersek]

On 08/01/19 01:13, Laszlo Ersek wrote:
> 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@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Laszlo Ersek <lersek@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?

Hmmm wait a bit!

Just because we skip SetUefiMemMapAttributes() in
PerformRemainingTasks(), that doesn't mean we cannot end up in
SmiPFHandler()! Because other faults are possible too.

Whenever we skip SetUefiMemMapAttributes() in PerformRemainingTasks(),
we should *also* skip the IsSmmCommBufferForbiddenAddress() check in
SmiPFHandler().

Both SetUefiMemMapAttributes() and IsSmmCommBufferForbiddenAddress()
come from commit d2fc7711136a -- they are two parts of the same
protection feature. If we disable the protection for the sake of
access-out, we should disable all parts.

Furthermore, if we don't call either of
IsSmmCommBufferForbiddenAddress() and SetUefiMemMapAttributes(), then we
shouldn't call GetUefiMemoryMap() either!


So ultimately, I would argue for the following patch series:

- patch#1: Revert commit c60d36b4, and explain why, in the commit
  message.

- patch#2: Introduce the new PCD, also mentioning MMIO.

- Patch#3: modify *all* of the following functions, internally, to
  return immediately, if "mSmmAccessOut" is TRUE:

  - GetUefiMemoryMap()
  - SetUefiMemMapAttributes()
  - IsSmmCommBufferForbiddenAddress()

  Basically, the new PCD should short-circuit all three functions
  declared in "PiSmmCpuDxeSmm.h" by commit d2fc7711136a.

Thanks!
Laszlo



> (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
> 

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

[Laszlo Ersek]

On 08/01/19 01:46, Laszlo Ersek wrote:

> So ultimately, I would argue for the following patch series:
> 
> - patch#1: Revert commit c60d36b4, and explain why, in the commit
>   message.
> 
> - patch#2: Introduce the new PCD, also mentioning MMIO.
> 
> - Patch#3: modify *all* of the following functions, internally, to
>   return immediately, if "mSmmAccessOut" is TRUE:
> 
>   - GetUefiMemoryMap()
>   - SetUefiMemMapAttributes()
>   - IsSmmCommBufferForbiddenAddress()
> 
>   Basically, the new PCD should short-circuit all three functions
>   declared in "PiSmmCpuDxeSmm.h" by commit d2fc7711136a.

Sigh, why is this so friggin complicated.

The functions added by commit d2fc7711136a were extended with the
following commits, later:

* ac6613db4697 ("UefiCpuPkg/PiSmmCpu: Check for untested memory in GCD",
2018-07-26)

The commit message says, "It treats GCD untested memory as invalid SMM
communication buffer".

* 8a2e1a9d54ee ("UefiCpuPkg/PiSmmCpu: Check EFI_RUNTIME_RO in UEFI mem
attrib table.", 2018-07-26)

The commit message says, "It treats the UEFI runtime page with
EFI_MEMORY_RO attribute as invalid SMM communication buffer.


Both of these commits come from the patch series

  [edk2] [PATCH 0/6] Check untested memory and EFI_MEMORY_RO

  http://mid.mail-archive.com/20180720052626.24932-1-hao.a.wu@intel.com
  https://lists.01.org/pipermail/edk2-devel/2018-July/027326.html

And now the question becomes: when a platform permits "access-out", does
it also enable access to untested memory, and EFI_MEMORY_RO?

(a) If the answer is yes, then short-circuiting the three functions that
I listed, on (mSmmAccessOut == TRUE), *immediately* after entering them,
is correct.

(b) If the answer is no -- that is, access-out should *continue*
preventing access to untested memory, and EFI_MEMORY_RO -- then we
cannot completely short-circuit the functions that I listed. Instead, we
can only short-circuit those parts that work with the "mUefiMemoryMap"
variable.

The current description of the PCD suggests that (b) is the right approach.

Thanks,
Laszlo

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

[Yao, Jiewen]

thanks laszlo, for the detail review

I have not gone through every line of code in detail. Some comment in general. 

To answer Laszlo’s question on Mmio. 
No, the Mmio cannot be used as communication buffer. But the smm must setup page table for it because the smm device driver may need access it. 
I am not sure the difference between Mmio and runtime Mmio. Runtime is only useful concept for Uefi, but not for Smm. 

Back to this patch itself.
I feel *guilty* when I see a new pcd introduced to control the code flow. 
That means the possible number of code path is doubled. Sigh...

The first question is: what unit test has been run? 
Does the unit test cover all possible true/false combination with other PCD?
We have a big table to describe all legal or illegal combination for the pcd to support memory protection. I think we have to update that table if we decide to add the new one. 

Maybe we can think of what is the supported  case. Maybe we use an *enum* to indicate the supported cases to reduce the number.
If the number is 4, no difference. 
If the number is 3, I recommend to use enum type instead of a new Boolean. 
If the number is 2, I don’t recommend we add new Boolean at all.  We can reinterpret the existing one. 



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?

As such I recommend we only support #2 and #3.

Again, if the naming is confusing, I agree we should clarify or even rename. 
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@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@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Laszlo Ersek <lersek@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
> 
> 
> 

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

[Ni, Ray]

> 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@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@intel.com>
> >> Cc: Eric Dong <eric.dong@intel.com>
> >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >> Cc: Jian J Wang <jian.j.wang@intel.com>
> >> Cc: Laszlo Ersek <lersek@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
> >
> > 
> >

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

[Yao, 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.



Thank you
Yao Jiewen

> -----Original Message-----
> From: Ni, Ray
> Sent: Thursday, August 1, 2019 9:28 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
> lersek@redhat.com
> Cc: Dong, Eric <eric.dong@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
> 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 <lersek@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@intel.com>
> > >> Cc: Eric Dong <eric.dong@intel.com>
> > >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> > >> Cc: Jian J Wang <jian.j.wang@intel.com>
> > >> Cc: Laszlo Ersek <lersek@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
> > >
> > > 
> > >

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

[Ni, Ray]

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 <jiewen.yao@intel.com>; devel@edk2.groups.io;
> > lersek@redhat.com
> > Cc: Dong, Eric <eric.dong@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
> > 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 <lersek@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@intel.com>
> > > >> Cc: Eric Dong <eric.dong@intel.com>
> > > >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > >> Cc: Jian J Wang <jian.j.wang@intel.com>
> > > >> Cc: Laszlo Ersek <lersek@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
> > > >
> > > > 
> > > >

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

[Yao, Jiewen]

SmmAccessOut = SMM access memory outside SMRAM.

So, do we want to treat SMM access ACPI NVS, RSVD, Runtime, MMIO, to be SmmAccessOut? 

Thank you
Yao Jiewen

> -----Original Message-----
> From: Ni, Ray
> Sent: Thursday, August 1, 2019 10:24 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
> lersek@redhat.com
> Cc: Dong, Eric <eric.dong@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
> Subject: RE: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu:
> PcdCpuSmmAccessOut controls SMM access-out policy
> 
> 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 <jiewen.yao@intel.com>; devel@edk2.groups.io;
> > > lersek@redhat.com
> > > Cc: Dong, Eric <eric.dong@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>
> > > 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 <lersek@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@intel.com>
> > > > >> Cc: Eric Dong <eric.dong@intel.com>
> > > > >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > > >> Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > >> Cc: Laszlo Ersek <lersek@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
> > > > >
> > > > > 
> > > > >

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

[Ni, Ray]

Jiewen, Laszlo,
Firstly let's assume we are aligned to use single/one PCD to control both static page table creation and SMM access out.

With this assumption, we need more discussion about what proper PCD name to choose.

For now there are two candidates: PcdCpuSmmStaticPageTable and PcdCpuSmmAccessOut.
  Laszlo doesn't like PcdCpuSmmStaticPageTable (Jiewen proposed. I don't like this name either).
  Jiewen doesn't like PcdCpuSmmAccessOut (I proposed. Not sure if Laszlo likes it or not).

When using PcdCpuSmmStaticPageTable, the straightforward meaning is whether the page table
to cover all memory that needs access/forbidden is created in advance. When page table is created
in advance, it's a bit hard to deduce that access out is forbidden. When page table is dynamically
created in PF handler, it's also a bit hard to deduce that access out is allowed.

When using PcdCpuSmmAccessOut, the straightforward meaning is whether to allow SMM code
to access non-SMRAM memory. Exception is ACPI NVS/ RSVD, Runtime and MMIO can
always be accessible to SMM code. When PcdCpuSmmAccessOut is TRUE, whether to use static
page table is PiSmmCpu's implementation choice, but since dynamic page table creation in
PF handler saves the SMRAM, the PiSmmCpu can choose to use the optimal solution which means
to create page table in PF handler (Identical to Jiewen's case #2). When PcdCpuSmmAccessOut is
FALSE, it's more secure to have a static page table (Identical to Jiewen's case #3).
What I want to say here is SmmAccessOut is the policy that PiSmmCpu driver wants to get from
platform side, whether to use static page table is its own implementation choice to meet platform
SmmAccessOut requirement.

So I think PcdCpuSmmAccessOut is better than PcdCpuSmmStaticPageTable.

I agree with Jiewen's concern this name is not very precise to describe the behavior.

Laszlo, Jiewen,
Do you have some proposals?

Thanks,
Ray


> -----Original Message-----
> From: Yao, Jiewen
> Sent: Thursday, August 1, 2019 11:11 AM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; lersek@redhat.com
> Cc: Dong, Eric <eric.dong@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
> Subject: RE: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu:
> PcdCpuSmmAccessOut controls SMM access-out policy
> 
> SmmAccessOut = SMM access memory outside SMRAM.
> 
> So, do we want to treat SMM access ACPI NVS, RSVD, Runtime, MMIO, to be
> SmmAccessOut?
> 
> Thank you
> Yao Jiewen
> 
> > -----Original Message-----
> > From: Ni, Ray
> > Sent: Thursday, August 1, 2019 10:24 AM
> > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
> > lersek@redhat.com
> > Cc: Dong, Eric <eric.dong@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>
> > Subject: RE: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu:
> > PcdCpuSmmAccessOut controls SMM access-out policy
> >
> > 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 <jiewen.yao@intel.com>; devel@edk2.groups.io;
> > > > lersek@redhat.com
> > > > Cc: Dong, Eric <eric.dong@intel.com>; Wang, Jian J
> > <jian.j.wang@intel.com>
> > > > 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 <lersek@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@intel.com>
> > > > > >> Cc: Eric Dong <eric.dong@intel.com>
> > > > > >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > > > >> Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > > >> Cc: Laszlo Ersek <lersek@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
> > > > > >
> > > > > > 
> > > > > >

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

[Laszlo Ersek]

On 08/01/19 08:25, Ni, Ray wrote:
> Jiewen, Laszlo,
> Firstly let's assume we are aligned to use single/one PCD to control
> both static page table creation and SMM access out.
>
> With this assumption, we need more discussion about what proper PCD
> name to choose.
>
> For now there are two candidates: PcdCpuSmmStaticPageTable and
> PcdCpuSmmAccessOut.
>   Laszlo doesn't like PcdCpuSmmStaticPageTable (Jiewen proposed. I
>   don't like this name either).
>   Jiewen doesn't like PcdCpuSmmAccessOut (I proposed. Not sure if
>   Laszlo likes it or not).
>
> When using PcdCpuSmmStaticPageTable, the straightforward meaning is
> whether the page table to cover all memory that needs access/forbidden
> is created in advance. When page table is created in advance, it's a
> bit hard to deduce that access out is forbidden. When page table is
> dynamically created in PF handler, it's also a bit hard to deduce that
> access out is allowed.
>
> When using PcdCpuSmmAccessOut, the straightforward meaning is whether
> to allow SMM code to access non-SMRAM memory. Exception is ACPI NVS/
> RSVD, Runtime and MMIO can always be accessible to SMM code. When
> PcdCpuSmmAccessOut is TRUE, whether to use static page table is
> PiSmmCpu's implementation choice, but since dynamic page table
> creation in PF handler saves the SMRAM, the PiSmmCpu can choose to use
> the optimal solution which means to create page table in PF handler
> (Identical to Jiewen's case #2). When PcdCpuSmmAccessOut is FALSE,
> it's more secure to have a static page table (Identical to Jiewen's
> case #3). What I want to say here is SmmAccessOut is the policy that
> PiSmmCpu driver wants to get from platform side, whether to use static
> page table is its own implementation choice to meet platform
> SmmAccessOut requirement.
>
> So I think PcdCpuSmmAccessOut is better than PcdCpuSmmStaticPageTable.
>
> I agree with Jiewen's concern this name is not very precise to
> describe the behavior.
>
> Laszlo, Jiewen,
> Do you have some proposals?

This is not how I remember Jiewen's original explanation of
PcdCpuSmmStaticPageTable.

Whenever I've been in doubt about PcdCpuSmmStaticPageTable in the last
few years, the following has been the answer that I've gone back to,
repeatedly:

  http://mid.mail-archive.com/74D8A39837DF1E4DA445A8C0B3885C50386BD98A@shsmsx102.ccr.corp.intel.com

Alternative link:

  https://lists.01.org/pipermail/edk2-devel/2016-November/004106.html

Key part being

> If we use dynamic paging, we can still provide *partial* protection.
> And hope page table is not modified by other component.

For me that decided the question; I'd *always* want static paging
enabled. It's not an implementation choice for PiSmmCpuDxeSmm, but
something I'd like to dictate from the platform side.

I also strongly dislike the idea of dynamically allocating SMRAM at OS
runtime. (For whatever purpose -- for example, for page tables.) If you
run out of SMRAM before SMM-ready-to-lock, that's one thing; you don't
lose OS data. If you run out of SMRAM at OS runtime, you lose OS data in
the crash. I don't want PiSmmCpuDxeSmm to save SMRAM on page tables;
instead I want the SMRAM footprint to be predictable and allocated as
early as possible during boot.

--*--

In one of my earlier emails, I did consider the case when a single
boolean PCD sufficed. I suggested that we pick that approach only if the
two aspects (access-out and static page table building) are inherently
related: basically, equivalent. By equivalence I don't mean that our
"table of variations" decays from 2*2=4 to just 2 elements, after we
figure out what we want to support -- because "what we like to support"
is not "inherent relationship". In my opinion, the non-equivalence of
both features is shown by our struggle to come up with *one* name for
the PCD such that it describe and control both features. The
non-equivalence is also shown by IA32 supporting access-out but not
supporting dynamic tables, but X64 supporting both.

I also considered the case (earlier) that not all 2*2=4 combinations
might be sensible or desirable. For that, I suggested that we add
ASSERT()s to the entry point of PiSmmCpuDxeSmm, to catch invalid
combinations of both PCDs early. That slims down the testing matrix, and
it still keeps the concepts separate.

Ultimately, what I care about is OVMF:
- keep static paging enabled,
- disable access-out,
- build for both IA32 and X64,
- behave identically (wrt. static paging, and access-out), on IA32 and
  X64.

Thanks
Laszlo

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

[Laszlo Ersek]

On 08/01/19 02:02, Yao, Jiewen wrote:
> thanks laszlo, for the detail review
>
> I have not gone through every line of code in detail. Some comment in
> general.
>
> To answer Laszlo's question on Mmio.
> No, the Mmio cannot be used as communication buffer. But the smm must
> setup page table for it because the smm device driver may need access
> it.

Makes sense. In that case, the comment on the "access-out disabled" case
should mention MMIO.


> I am not sure the difference between Mmio and runtime Mmio. Runtime is
> only useful concept for Uefi, but not for Smm.
>
> Back to this patch itself.
> I feel *guilty* when I see a new pcd introduced to control the code
> flow.
> That means the possible number of code path is doubled. Sigh...
>
> The first question is: what unit test has been run?
> Does the unit test cover all possible true/false combination with
> other PCD?
> We have a big table to describe all legal or illegal combination for
> the pcd to support memory protection. I think we have to update that
> table if we decide to add the new one.
>
> Maybe we can think of what is the supported  case. Maybe we use an
> *enum* to indicate the supported cases to reduce the number.
> If the number is 4, no difference.
> If the number is 3, I recommend to use enum type instead of a new
> Boolean.
> If the number is 2, I don't recommend we add new Boolean at all. We
> can reinterpret the existing one.
>
>
>
> 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?

The first patch that introduced "access-out" (as a use case) was commit
c60d36b4d1ee ("UefiCpuPkg/SmmCpu: Block access-out only when static
paging is used", 2018-11-08). Before that, there was no access-out.

The first patch that distinguished "static page tables" from "dynamic
page tables" was 28b020b5de1e ("UefiCpuPkg/dec: Add
PcdCpuSmmStaticPageTable.", 2016-11-17)

During those two years, we didn't support access-out, but allowed
platforms to choose between static/dynamic paging. Are we calling that
invalid in retrospect?


> 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.

This scenario doesn't look supportable on IA32. (Due to
StaticPaging=false.) I don't mean that it's impossible to implement,
just that the IA32 code today doesn't extend the page tables in response
to page faults.


> 3) StaticPaging=true, AccessOut=false: it seems valid. The is secure
> configuration.

Agreed.


> 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?

- the IA32 binary constructs all tables in advance, and it might want to
interact with the RAS controller in question.

- the X64 binary wants to allocate the SMRAM for page tables in advance
during boot (and not in the page fault handler), and protect the SMM
page tables, but still interact with the RAS controller through normal
RAM.

Apologies if I'm missing something obvious that invalidates the above
use cases.


> As such I recommend we only support #2 and #3.
>
> Again, if the naming is confusing, I agree we should clarify or even
> rename.
> What I am trying to achieve is to limit the number of supported
> combination to reduce the effort of validation and maintenance.

I agree. IMO:

- we should represent separate concepts separately,
- exclude those combinations that make no sense
  (with an ASSERT + appropriate comment)
- exclude those combinations that make sense but are
  unimportant or too complex to support
  (with a different ASSERT + proper comment).

Thanks
Laszlo

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

[Yao, Jiewen]

Thanks Laszlo. Comment below:

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Friday, August 2, 2019 10:05 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Wang,
> Jian J <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu:
> PcdCpuSmmAccessOut controls SMM access-out policy
> 
> On 08/01/19 02:02, Yao, Jiewen wrote:
> > thanks laszlo, for the detail review
> >
> > I have not gone through every line of code in detail. Some comment in
> > general.
> >
> > To answer Laszlo's question on Mmio.
> > No, the Mmio cannot be used as communication buffer. But the smm must
> > setup page table for it because the smm device driver may need access
> > it.
> 
> Makes sense. In that case, the comment on the "access-out disabled" case
> should mention MMIO.
> 
> 
> > I am not sure the difference between Mmio and runtime Mmio. Runtime is
> > only useful concept for Uefi, but not for Smm.
> >
> > Back to this patch itself.
> > I feel *guilty* when I see a new pcd introduced to control the code
> > flow.
> > That means the possible number of code path is doubled. Sigh...
> >
> > The first question is: what unit test has been run?
> > Does the unit test cover all possible true/false combination with
> > other PCD?
> > We have a big table to describe all legal or illegal combination for
> > the pcd to support memory protection. I think we have to update that
> > table if we decide to add the new one.
> >
> > Maybe we can think of what is the supported  case. Maybe we use an
> > *enum* to indicate the supported cases to reduce the number.
> > If the number is 4, no difference.
> > If the number is 3, I recommend to use enum type instead of a new
> > Boolean.
> > If the number is 2, I don't recommend we add new Boolean at all. We
> > can reinterpret the existing one.
> >
> >
> >
> > 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?
> 
> The first patch that introduced "access-out" (as a use case) was commit
> c60d36b4d1ee ("UefiCpuPkg/SmmCpu: Block access-out only when static
> paging is used", 2018-11-08). Before that, there was no access-out.
> 
> The first patch that distinguished "static page tables" from "dynamic
> page tables" was 28b020b5de1e ("UefiCpuPkg/dec: Add
> PcdCpuSmmStaticPageTable.", 2016-11-17)
> 
> During those two years, we didn't support access-out, but allowed
> platforms to choose between static/dynamic paging. Are we calling that
> invalid in retrospect?

[Jiewen] Right. Agree with you.
Our original intension is to provide a secure configuration, as #2.
But we notice it is not possible, due to some special RAS requirement.
Then we have to move to #2, and make #1 invalid.

> 
> 
> > 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.
> 
> This scenario doesn't look supportable on IA32. (Due to
> StaticPaging=false.) I don't mean that it's impossible to implement,
> just that the IA32 code today doesn't extend the page tables in response
> to page faults.

[Jiewen] You are right.

Some background on dynamic paging.
It is introduced to resolve X64 problem, where Intel CPU only support 2M paging, but large physical memory. We cannot support build static 2M paging inside of SMRAM for X64.
So we introduce dynamic paging to support this configuration.

For IA32, 5 pages are enough to cover 4G, in 2M paging. As such, I do not see the value to provide dynamic paging in Ia32.

Or I could say: this #2 is only valid for X64. No need for IA32.


> 
> 
> > 3) StaticPaging=true, AccessOut=false: it seems valid. The is secure
> > configuration.
> 
> Agreed.
> 
> 
> > 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?
> 
> - the IA32 binary constructs all tables in advance, and it might want to
> interact with the RAS controller in question.
> 
> - the X64 binary wants to allocate the SMRAM for page tables in advance
> during boot (and not in the page fault handler), and protect the SMM
> page tables, but still interact with the RAS controller through normal
> RAM.
> 
> Apologies if I'm missing something obvious that invalidates the above
> use cases.

[Jiewen]
Our first focus is for X64 when we design this feature, because RAS is only required by server.
And all RAS servers use X64, I believe.

I don't think we need consider this case.


All in all, to make it easy, I would suggest to limit the configuration below:
A) For IA32, we only consider #3.
B) For X64, we consider #3 as default. Only back to #2 for RAS platform.



> 
> > As such I recommend we only support #2 and #3.
> >
> > Again, if the naming is confusing, I agree we should clarify or even
> > rename.
> > What I am trying to achieve is to limit the number of supported
> > combination to reduce the effort of validation and maintenance.
> 
> I agree. IMO:
> 
> - we should represent separate concepts separately,
> - exclude those combinations that make no sense
>   (with an ASSERT + appropriate comment)
> - exclude those combinations that make sense but are
>   unimportant or too complex to support
>   (with a different ASSERT + proper comment).
> 
> Thanks
> Laszlo
> 
> 

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

[Laszlo Ersek]

On 08/02/19 04:46, Yao, Jiewen wrote:
> Thanks Laszlo. Comment below:
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Laszlo Ersek
>> Sent: Friday, August 2, 2019 10:05 AM
>> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
>> Cc: Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Wang,
>> Jian J <jian.j.wang@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu:
>> PcdCpuSmmAccessOut controls SMM access-out policy
>>
>> On 08/01/19 02:02, Yao, Jiewen wrote:

>>> 1) StaticPaging=false, AccessOut=false: it seems invalid. If we
>>> don't support access out, why we need dynamic paging?
>>
>> The first patch that introduced "access-out" (as a use case) was commit
>> c60d36b4d1ee ("UefiCpuPkg/SmmCpu: Block access-out only when static
>> paging is used", 2018-11-08). Before that, there was no access-out.
>>
>> The first patch that distinguished "static page tables" from "dynamic
>> page tables" was 28b020b5de1e ("UefiCpuPkg/dec: Add
>> PcdCpuSmmStaticPageTable.", 2016-11-17)
>>
>> During those two years, we didn't support access-out, but allowed
>> platforms to choose between static/dynamic paging. Are we calling that
>> invalid in retrospect?
> 
> [Jiewen] Right. Agree with you.
> Our original intension is to provide a secure configuration, as #2.
> But we notice it is not possible, due to some special RAS requirement.
> Then we have to move to #2, and make #1 invalid.

OK. I don't mind the RAS requirement as long as option #3 remains viable.

>>> 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.
>>
>> This scenario doesn't look supportable on IA32. (Due to
>> StaticPaging=false.) I don't mean that it's impossible to implement,
>> just that the IA32 code today doesn't extend the page tables in response
>> to page faults.
> 
> [Jiewen] You are right.
> 
> Some background on dynamic paging.
> It is introduced to resolve X64 problem, where Intel CPU only support 2M paging, but large physical memory. We cannot support build static 2M paging inside of SMRAM for X64.
> So we introduce dynamic paging to support this configuration.
> 
> For IA32, 5 pages are enough to cover 4G, in 2M paging. As such, I do not see the value to provide dynamic paging in Ia32.
> 
> Or I could say: this #2 is only valid for X64. No need for IA32.

That's fine, it's just whatever solution we choose, should not break
compilation or behavior on IA32.

Also, preferably, if a PCD only makes sense on X64, then it should be
declared in the DEC file as such, so that IA32-only code referencing the
PCD not even compile. Otherwise confusion will result (people looking at
the IA32-only code will ask themselves, "what should I do about this PCD
here"?)


>>> 3) StaticPaging=true, AccessOut=false: it seems valid. The is secure
>>> configuration.
>>
>> Agreed.
>>
>>
>>> 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?
>>
>> - the IA32 binary constructs all tables in advance, and it might want to
>> interact with the RAS controller in question.
>>
>> - the X64 binary wants to allocate the SMRAM for page tables in advance
>> during boot (and not in the page fault handler), and protect the SMM
>> page tables, but still interact with the RAS controller through normal
>> RAM.
>>
>> Apologies if I'm missing something obvious that invalidates the above
>> use cases.
> 
> [Jiewen]
> Our first focus is for X64 when we design this feature, because RAS is only required by server.
> And all RAS servers use X64, I believe.
> 
> I don't think we need consider this case.
> 
> 
> All in all, to make it easy, I would suggest to limit the configuration below:
> A) For IA32, we only consider #3.
> B) For X64, we consider #3 as default. Only back to #2 for RAS platform.

I guess that works for me. How about the following:

(1) rename PcdCpuSmmStaticPageTable to PcdCpuSmmRestrictedMemoryAccess;
default value TRUE.


(2) move the PCD from

[PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]

to

[PcdsFixedAtBuild.X64, PcdsPatchableInModule.X64, PcdsDynamic.X64,
PcdsDynamicEx.X64]

in the DEC file


(3) update the documentation on the PCD precisely, in the DEC file --
describe all aspects and consequences. Also describe that on IA32, the
PCD should be considered constant TRUE


(4) OK, so this is another interesting issue: in the documentation of
"PcdCpuSmmProfileEnable", it is stated that "PcdCpuSmmStaticPageTable"
must be disabled for enabling "PcdCpuSmmProfileEnable". That is
*already* contradictory (without the feature being added), because IA32
always uses statically built page tables, but the IA32 *code* actually
honors PcdCpuSmmProfileEnable.

Therefore, the documentation on "PcdCpuSmmProfileEnable" has to be
updated first, separately, to reflect reality. This should be a separate
patch. And then it can be updated for PcdCpuSmmRestrictedMemoryAccess.


(5) rename mCpuSmmStaticPageTable in the code to
mCpuSmmRestrictedMemoryAccess.


(6) If some common code (built for both IA32 and X64) needs to depend on
this variable, then a helper function is need; returning constant TRUE
on IA32 and returning mCpuSmmRestrictedMemoryAccess on X64.


Thanks
Laszlo

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

[Yao, Jiewen]

Thanks and agree. 

Comment inlined.

thank you!
Yao, Jiewen


> 在 2019年8月3日，上午6:06，Laszlo Ersek <lersek@redhat.com> 写道：
> 
>> On 08/02/19 04:46, Yao, Jiewen wrote:
>> Thanks Laszlo. Comment below:
>> 
>>> -----Original Message-----
>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>>> Laszlo Ersek
>>> Sent: Friday, August 2, 2019 10:05 AM
>>> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
>>> Cc: Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Wang,
>>> Jian J <jian.j.wang@intel.com>
>>> Subject: Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/PiSmmCpu:
>>> PcdCpuSmmAccessOut controls SMM access-out policy
>>> 
>>> On 08/01/19 02:02, Yao, Jiewen wrote:
> 
>>>> 1) StaticPaging=false, AccessOut=false: it seems invalid. If we
>>>> don't support access out, why we need dynamic paging?
>>> 
>>> The first patch that introduced "access-out" (as a use case) was commit
>>> c60d36b4d1ee ("UefiCpuPkg/SmmCpu: Block access-out only when static
>>> paging is used", 2018-11-08). Before that, there was no access-out.
>>> 
>>> The first patch that distinguished "static page tables" from "dynamic
>>> page tables" was 28b020b5de1e ("UefiCpuPkg/dec: Add
>>> PcdCpuSmmStaticPageTable.", 2016-11-17)
>>> 
>>> During those two years, we didn't support access-out, but allowed
>>> platforms to choose between static/dynamic paging. Are we calling that
>>> invalid in retrospect?
>> 
>> [Jiewen] Right. Agree with you.
>> Our original intension is to provide a secure configuration, as #2.
>> But we notice it is not possible, due to some special RAS requirement.
>> Then we have to move to #2, and make #1 invalid.
> 
> OK. I don't mind the RAS requirement as long as option #3 remains viable.
> 
>>>> 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.
>>> 
>>> This scenario doesn't look supportable on IA32. (Due to
>>> StaticPaging=false.) I don't mean that it's impossible to implement,
>>> just that the IA32 code today doesn't extend the page tables in response
>>> to page faults.
>> 
>> [Jiewen] You are right.
>> 
>> Some background on dynamic paging.
>> It is introduced to resolve X64 problem, where Intel CPU only support 2M paging, but large physical memory. We cannot support build static 2M paging inside of SMRAM for X64.
>> So we introduce dynamic paging to support this configuration.
>> 
>> For IA32, 5 pages are enough to cover 4G, in 2M paging. As such, I do not see the value to provide dynamic paging in Ia32.
>> 
>> Or I could say: this #2 is only valid for X64. No need for IA32.
> 
> That's fine, it's just whatever solution we choose, should not break
> compilation or behavior on IA32.
> 
> Also, preferably, if a PCD only makes sense on X64, then it should be
> declared in the DEC file as such, so that IA32-only code referencing the
> PCD not even compile. Otherwise confusion will result (people looking at
> the IA32-only code will ask themselves, "what should I do about this PCD
> here"?)

[jiewen] make sense. Agree. 

> 
> 
>>>> 3) StaticPaging=true, AccessOut=false: it seems valid. The is secure
>>>> configuration.
>>> 
>>> Agreed.
>>> 
>>> 
>>>> 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?
>>> 
>>> - the IA32 binary constructs all tables in advance, and it might want to
>>> interact with the RAS controller in question.
>>> 
>>> - the X64 binary wants to allocate the SMRAM for page tables in advance
>>> during boot (and not in the page fault handler), and protect the SMM
>>> page tables, but still interact with the RAS controller through normal
>>> RAM.
>>> 
>>> Apologies if I'm missing something obvious that invalidates the above
>>> use cases.
>> 
>> [Jiewen]
>> Our first focus is for X64 when we design this feature, because RAS is only required by server.
>> And all RAS servers use X64, I believe.
>> 
>> I don't think we need consider this case.
>> 
>> 
>> All in all, to make it easy, I would suggest to limit the configuration below:
>> A) For IA32, we only consider #3.
>> B) For X64, we consider #3 as default. Only back to #2 for RAS platform.
> 
> I guess that works for me. How about the following:
> 
> (1) rename PcdCpuSmmStaticPageTable to PcdCpuSmmRestrictedMemoryAccess;
> default value TRUE.

[jiewen] agree. 
I also recommend add some comments say this is the rename to old PcdCpuSmmStaticPageTable. 

> 
> (2) move the PCD from
> 
> [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
> 
> to
> 
> [PcdsFixedAtBuild.X64, PcdsPatchableInModule.X64, PcdsDynamic.X64,
> PcdsDynamicEx.X64]
> 
> in the DEC file

[jiewen] make sense. agree. 

> 
> 
> (3) update the documentation on the PCD precisely, in the DEC file --
> describe all aspects and consequences. Also describe that on IA32, the
> PCD should be considered constant TRUE

[jiewen] agree.

> (4) OK, so this is another interesting issue: in the documentation of
> "PcdCpuSmmProfileEnable", it is stated that "PcdCpuSmmStaticPageTable"
> must be disabled for enabling "PcdCpuSmmProfileEnable". That is
> *already* contradictory (without the feature being added), because IA32
> always uses statically built page tables, but the IA32 *code* actually
> honors PcdCpuSmmProfileEnable.
> 
> Therefore, the documentation on "PcdCpuSmmProfileEnable" has to be
> updated first, separately, to reflect reality. This should be a separate
> patch. And then it can be updated for PcdCpuSmmRestrictedMemoryAccess.

[jiewen] agree. I think we can add more description on that.
PcdCpuSmmProfileEnable should a debug feature only and never be used for production. 
While PcdCpuSmmRestrictedMemoryAccess is for production. 

> 
> 
> (5) rename mCpuSmmStaticPageTable in the code to
> mCpuSmmRestrictedMemoryAccess. 

[jiewen] agree


> (6) If some common code (built for both IA32 and X64) needs to depend on
> this variable, then a helper function is need; returning constant TRUE
> on IA32 and returning mCpuSmmRestrictedMemoryAccess on X64.

[jiewen] agree. 

> 
> 
> Thanks
> Laszlo