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

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

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