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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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